Add changesets for self-serve token pool deployments #15877
Conversation
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
There was a problem hiding this comment.
This is a partial review.
I think we should create sep changeset for new token pool addition and Updating existing pool config otherwise it is cluttering the changeset. Also DeployTokenPoolContractsConfig name does not convey that it can be used for ExistingPoolUpdates.
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
| @@ -74,32 +105,10 @@ func (d *DeployerGroup) GetDeployer(chain uint64) (*bind.TransactOpts, error) { | |||
| Context: txOpts.Context, | |||
| AccessList: txOpts.AccessList, | |||
| NoSend: true, | |||
| // Set the gas limit to avoid simulating the call before appending to transactions. | |||
| // Some calls depend on each other, so we don't want to simulate until we actually start executing. | |||
| GasLimit: 1_000_000, | |||
There was a problem hiding this comment.
Just a thought, would it make sense to allow simulating the changeset before executing on the enact part? Maybe not something for this PR but that could add value overall. I am also a bit sad that we have to remove the simulation from here, that was a nice feature
There was a problem hiding this comment.
I've adjusted it a bit to address your concerns: ed01ffe
If deployer key is used, simulations now happen when transactions are appended. I've split my token admin registry changeset into 3 different proposals to pass simulations. The deployer group now takes opCount offsets into account to enable enacting multiple proposals.
I was wondering though, do you think simulations should happen if MCMS key is used? Right now they are not because gasLimit is hard-set in SimTransactOpts.
There was a problem hiding this comment.
One backlog tickets for deployer group @carte7000
There was a problem hiding this comment.
I think there is value of having simulation even with MCMS, but that could be something we work out after if for now it's blocking this PR
|
|
||
| // To perform these actions, we have to be the admin of the token. There are three ways this can happen: | ||
| // 1. We are already the admin of the token (no action) | ||
| // 2. We are the proposed admin of the token (just have to accept) |
There was a problem hiding this comment.
We fine with doing this "compound" operation (i.e the accept adminship not being a separate CS)?
| defer d.reset() | ||
|
|
There was a problem hiding this comment.
Does this make sense even when mcms is not enabled?
There was a problem hiding this comment.
Also this object is becoming pretty complex and a critical component since its used by a bunch of changesets, can we have some coverage of this multi-Enact logic?
Or enforce a single-Enact behavior if thats safer / makes more sense.
There was a problem hiding this comment.
I wonder if a pattern like this would be more obvious/safe for multi proposal. Move the reason and mcmsOpCount to a DeployerGroupContext object.
Then we call it like that
d := NewDeployerGroup(...., DeployerGroupContext("proposal description"))
d.DoSomethingOnChain()
// Next Proposal
d = d.WithNewContext("proposal2 description") // Internally clone the DeployerGroupContext and increment the offsets
d.DoSomethingElseOnChain()
d.Enact() // Create the 2 proposals
| if c.MCMS != nil { | ||
| // Pre-allocate the proposal slice with the correct capacity | ||
| totalProposals := len(proposeAdminOutput.Proposals) + | ||
| len(acceptAdminOutput.Proposals) + | ||
| len(configurationOutput.Proposals) | ||
| proposals := make([]timelock.MCMSWithTimelockProposal, 0, totalProposals) | ||
| proposals = append(proposals, proposeAdminOutput.Proposals...) | ||
| proposals = append(proposals, acceptAdminOutput.Proposals...) | ||
| proposals = append(proposals, configurationOutput.Proposals...) | ||
|
|
||
| return deployment.ChangesetOutput{ | ||
| Proposals: proposals, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
One thing I have been thinking is that since the deployerGroup was initially made to abstract MCMS vs deployer key. Would it make sense to also move this logic into the deployerGroup?
Perhaps we could have a function called .NewProposal(reason) instead of using Enact in multi proposal scenario. Not sure if this is needed for this PR but interested in your opinion if that would be a good next step
There was a problem hiding this comment.
I think revising the API so that we return a single Enact at the end of the changeset makes the most sense.
| } | ||
|
|
||
| // DeployTokenPoolContractsChangeset deploys new pools for a given token across multiple chains. | ||
| func DeployTokenPoolContractsChangeset(env deployment.Environment, c DeployTokenPoolContractsConfig) (deployment.ChangesetOutput, error) { |
There was a problem hiding this comment.
Do we want to support MCMS for this changeset? Also would be benefit from simulation in the DeployerGroup even if we don't use MCMS?
There was a problem hiding this comment.
MCMS doesn't apply to this one, since it just involves deploying contracts
|
https://smartcontract-it.atlassian.net/browse/CCIP-4508
Supports
#16062