Ccip-4158 prereq Changeset #15250
Ccip-4158 prereq Changeset #15250
Conversation
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
AnieeG
requested review from
winder,
rstout,
asoliman92,
makramkd,
dimkouv,
mateusz-sekara and
0xAustinWang
deployment/ccip/deploy.go
Outdated
| // This is only required for staging and test environments where the contracts are not already deployed. | ||
| // We are not deploying router and RMNProxy here as RMNProxy has dependency on RMNRemote which is a new contract and | ||
| // router depends on RMNProxy. | ||
| func DeployPrerequisiteContracts(e deployment.Environment, ab deployment.AddressBook, selectors []uint64) error { |
There was a problem hiding this comment.
Can we change 'prerequisite' to something more descriptive?
From the code it looks like we're deploying contracts related to token registry. Then in the comment we can indicate that this is not necessary for live environments
There was a problem hiding this comment.
should be more than registry. This is aiming to deploy basically all the 1.5 contracts which will exist pre 1.6
There was a problem hiding this comment.
Maybe DeployLegacyContracts or DeployRDDContracts, and include Connor's comment that legacy contracts are the ones deployed by RDD which include the 1.5 release and earlier.
| ) | ||
|
|
||
| var ( | ||
| _ deployment.ChangeSet[PrerequisiteConfig] = InitializePrerequisites |
There was a problem hiding this comment.
formatting here is strange?
| return deployment.ChangesetOutput{}, fmt.Errorf("failed to deploy prerequisite contracts: %w", err) | ||
| } | ||
| return deployment.ChangesetOutput{ | ||
| Proposals: []timelock.MCMSWithTimelockProposal{}, |
There was a problem hiding this comment.
for initializing, should this return an 'accept_ownership' proposal?
There was a problem hiding this comment.
I don't think so, We need to test the whole set up before transferring ownership. The transfer ownership should have its own changeset
| for _, ec := range cfg.ExistingContracts { | ||
| err = ab.Save(sel, ec.Address.String(), ec.TypeAndVersion) | ||
| if err != nil { | ||
| env.Logger.Errorw("Failed to deploy prerequisite contracts", "err", err, "addressBook", ab) |
There was a problem hiding this comment.
| env.Logger.Errorw("Failed to deploy prerequisite contracts", "err", err, "addressBook", ab) | |
| env.Logger.Errorw("Failed to save prerequisite contracts", "err", err, "addressBook", ab) |
| return err | ||
| } | ||
| e.Logger.Infow("Deployed nonce manager", "addr", nonceManager.Address) | ||
| nmContract = nonceManager.Contract |
There was a problem hiding this comment.
instead of creating a new variable, let's just update chainState.NonceManager to maintain chainState as single source of truth
Same with the other contracts
There was a problem hiding this comment.
why not new var? seems clean. Also if you are deploying new noncemanager chain state won't be updated right away. only the addressbook is updated after deployment
There was a problem hiding this comment.
For someone updating this file in the future I can see friction for them trying to know which contracts to use chainState.<Contract> or the contract variable.
Alternatively, initialize all these contract variables at the top of the file and initialize them to chainstate, and only use those new variables in the function
deployment/ccip/deploy.go
Outdated
| e.Logger.Infow("assigned registry module on token admin registry") | ||
| } | ||
| if weth9Contract == nil { | ||
| weth9, err := deployContract(lggr, chain, ab, |
There was a problem hiding this comment.
should we also mint some tokens?
| type PrerequisiteConfig struct { | ||
| ChainSelectors []uint64 | ||
| Deploy bool | ||
| // if Deploy is false, ExistingContracts must be set |
There was a problem hiding this comment.
I think cleaner to have a separate changeset like SaveExistingContracts which takes in an arbitrary number of []Contract{selector, address, typeAndVersion} and just saves them to an address book and returns it. That would be more generally reusable and avoid the conditional
deployment/ccip/deploy.go
Outdated
| @@ -125,6 +125,136 @@ func deployContract[C Contracts]( | |||
| return &contractDeploy, nil | |||
| } | |||
|
|
|||
| // DeployPrerequisiteContracts deploys the contracts that can be ported from previous CCIP version to the new one. | |||
| // This is only required for staging and test environments where the contracts are not already deployed. | |||
| // We are not deploying router and RMNProxy here as RMNProxy has dependency on RMNRemote which is a new contract and | |||
There was a problem hiding this comment.
Wait I thought we were going to deploy router + RMNProxy pointing at MockRMN to simulate v1? Then separate RMNProxy for V2 until we adjust RMNRemote
There was a problem hiding this comment.
Yes, I got confused here. Changing
deployment/ccip/deploy.go
Outdated
| } | ||
| lggr := e.Logger | ||
| for _, sel := range selectors { | ||
| chain := e.Chains[sel] |
There was a problem hiding this comment.
definitely should be a function DeployPrerequisiteChainContracts
| }, nil | ||
| } | ||
|
|
||
| type DeployPrerequisiteConfig struct { |
There was a problem hiding this comment.
Missing some comments on exported functions/types
| } | ||
|
|
||
| // DeployPrerequisiteContracts deploys the contracts that can be ported from previous CCIP version to the new one. | ||
| // This is only required for staging and test environments where the contracts are not already deployed. |
There was a problem hiding this comment.
Wouldn't it be required for new chains as well? Or are the old contracts all replaced after the initial migration?
There was a problem hiding this comment.
This is mainly designed thinking migration as an immediate goal. Are you referring to adding a completely newchain in 1.5 which is not existing in 1.6? I think in that case as we will have to call this function.
Creates a changeset for all pre-existing contracts.
Updates smoke workflow to run tests in parallel so that workflow duration is reduced.