Execute forks separately

[josojo]

Just first initial thoughts...

[edmundedgar]

That looks right to me. I guess this happens next but I think the places that use the onlyAfterForking modifier for split operations etc use the same method where they can be done independently (although you can also call both in one go if you want). If you only call one side to split assets we'll need to track which assets you've migrated and which you haven't.

I think we also need to manage a state where the fork is judged to have happened from the point of view of stopping action on the old version. I'm not sure whether this would be measuring whether one or the other children have been created, or whether it automatically kicks in once the executionTimeForProposal is reached.

[edmundedgar]

For the token splitting I would do an interface something like

splitTokenIntoChildTokens(uint256 amount, bool onlyYesToken, bool onlyNoToken)
...then do anything to do with preparing internally based on that. I think that's simpler to understand and write a UI for than having multiple different functions you have to call in different situations.

Then I think the balances need to be tracked per-user, not globally.

[edited]

[edmundedgar]

Also on the token splitting, I don't think we need to write both to storage? You say which ones you want to move. If you say both, just do the burn and mint on both. If you only say one, mint that and write the other one to storage in unspentTokens[owner][yesOrNo] or something then credit it if the user calls the other one later.

[josojo]

splitTokenIntoChildTokens(uint256 amount, bool onlyYesToken, bool onlyNoToken)

these things I would add later. I hate the fact that I needed to touch the bridge contract anyways, as it was optimized such that it nearly fits into a block limit. Hence, all these "helper function", I can add later, once I have the size again under control.

Then I think the balances need to be tracked per-user, not globally.

Fixed

Also on the token splitting, I don't think we need to write both to storage? You say which ones you want to move. If you say both, just do the burn and mint on both. If you only say one, mint that and write the other one to storage in unspentTokens[owner][yesOrNo] or something then credit it if the user calls the other one later.

Yes, I have that locally, but still need to figure out the contract size with that. the if and else things add some size. Let me try to figure out a good way

[josojo]

Contract	Size (kB)	Margin (kB)
ForkableBridge	25.658	-1.082
ForkableBridgeWrapper	25.923	-1.347

I adopted much of the feedback. Contracts are now too big, but I will find a way to make them smaller again

[josojo]

Finally, I am okaisch with the result:

I needed to revert the separate creation of child1 and child2 for the bridge contract, since this would require two function that I could not fit into the bridge contract - (one consumes less space).

It sounds crazy, but I would there are really no low hanging wins to reduce the contract size.
Now, it barely fits in:

Contract	Size (kB)	Margin (kB)
ForkableBridge	24.573	0.003
ForkableBridgeWrapper	24.821	-0.245

Also, I could not put in any storage variable that would store the tokens that are immediately created during the splitting process( as I did earlier in the PR). Now, the users just have the option to either mint both child-tokens after the forking process - or only one child-fork token, if the second version might have a broken minting functionality.

In the forkonomic token, we have enough contract size left to do it more elegantly and also store the amount, if initially only one child-token was created.

[edmundedgar]

On onlyBeforeForking, it's currently checking for the deployment of the first child as a sign that it should stop bridges and things. Is that the right behaviour, or do we want to shut down on the scheduled fork block? Currently we're shutting down based on whether one of the forks is deployed but not the other, which means if there are several patterns we have to consider that might have subtleties.