-
Notifications
You must be signed in to change notification settings - Fork 9
Port migration tool to rebased branch #138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
8e12a87 to
61ffab4
Compare
| // UseInterop is a flag that indicates if the system is using interop | ||
| UseInterop bool `json:"useInterop,omitempty"` | ||
|
|
||
| // DeployCeloContracts indicates whether to deploy Celo contracts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // DeployCeloContracts indicates whether to deploy Celo contracts. | |
| // DeployCeloContracts indicates whether to deploy Celo contracts, this should be set to true when initialising a new chain, and false in the case of a migration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, do we even need this parameter, isn't this implied by the l2GenesisBlockNumber if it's 0 then we will deploy the contracts, if not then it must be a migration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's nicer to be explicit and add the option, but have no strong opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a valid case where deployCeloContracts is true and l2GenesisBlockNumber is greater than zero, or vi ce versa, then I guess we need it. Otherwise it seems like it's just increasing the amount of required config and opening up a path for misconfiguration which wouldn't exist otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carterqw2 thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me. I can't think of a valid case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@piersy Any idea if this the L2GenesisBlockNumber is actually set somewhere to a non-zero value? I don't see it in any file under packages/contracts-bedrock/deploy-config/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, well I'm actually I did a bit of digging and it seems in our case the L2GenesisBlockNumber is irrelevant.
It's passed in the config to genesis.BuildL2Genesis and used there as the block number of the built L2 genesis and as the BedrockBlock in the ChainConfig included with that genesis.
But then the only thing we use from that genesis is Alloc at main.go 257 and we load the chain config from the datadir that we are migrating and manually set the BedrockBlock to the cel2 block number at main.go 364.
It seems like we don't need theL2GenesisBlockNumber so I don't think we can make any decision based on it. So I think it's fine to keep the explicit DeployCeloContracts flag.
This works by loading the database of a celo node. It then removes all existing blocks and generates a new genesis block including the existing state tree. Migrate to urfave/cli/v2 Update op-chain-ops/cmd/op-migrate/main.go Co-authored-by: Karl Bartel <[email protected]>
Kept in a separate commit to make potential adding them back easier.
9c76617 to
c965009
Compare
piersy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Ports #110 on
celo5branch, which reworked the whole (l2) genesis creation logic.The allocations containing the OP contracts are now generated using the
L2Genesis.s.solfoundry script, stored and passed into the migration tool via the--l2-allocsflag. This also helps to shorten migration time and splits generation/application nicely.Also adds an option to not deploy celo contracts during l2 genesis creation, which will be used when creating the allocs for the migration.
Resolves #134
Resolves #142
Resolves #143