-
Notifications
You must be signed in to change notification settings - Fork 23
update changesets to use stored addressRef #1412
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
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.
Pull request overview
This PR refactors v1_6_4 changesets to retrieve deployed contract addresses from the datastore using addressRef lookups rather than requiring users to supply them as input parameters. This simplification reduces the potential for configuration errors during the durable pipeline creation process.
Key Changes:
- Modified changeset input structs to remove address fields that can be retrieved from the datastore
- Updated changeset implementations to use
FindAndFormatRefutility for address resolution - Refactored tests to populate the datastore with required addresses before executing changesets
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| usdc_token_pool_deploy.go | Removed CCTPMessageTransmitterProxy deployment logic; now expects it to be pre-deployed and stored in datastore |
| usdc_token_pool_proxy_deploy.go | Retrieves Router, USDCToken, and pool addresses from datastore instead of input parameters |
| usdc_token_pool_cctp_v2_deploy.go | Added datastore lookups for Router, RMN, and Token addresses with fallback to input parameter for Token |
| siloed_usdc_token_pool_deploy.go | Removed Token, RMNProxy, Router, and LockBox from input struct; retrieves from datastore |
| erc20_lockbox_deploy.go | Removed TokenAdminRegistry from input; retrieves from datastore |
| set_domains.go | Removed single Address field; now uses per-chain address lookups from datastore |
| configure_allowed_callers.go | Renamed Address to AddressesByChain map; retrieves ERC20LockBox addresses from datastore |
| update_lock_or_burn_mechanism.go | Removed Address from input; retrieves USDCTokenPoolProxy address from datastore |
| update_lock_release_pool_addresses.go | Removed Address from input; retrieves USDCTokenPoolProxy address from datastore |
| *_test.go files | Updated tests to pre-populate datastore with required addresses and removed address fields from changeset inputs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
chains/evm/deployment/v1_6_4/sequences/usdc_token_pool_deploy.go
Outdated
Show resolved
Hide resolved
chains/evm/deployment/v1_6_4/changesets/usdc_token_pool_cctp_v2_deploy.go
Outdated
Show resolved
Hide resolved
chains/evm/deployment/v1_6_4/changesets/configure_allowed_callers.go
Outdated
Show resolved
Hide resolved
| for _, perChainInput := range input.ChainInputs { | ||
| addressByChain[perChainInput.ChainSelector] = perChainInput.Address | ||
|
|
||
| erc20_lock_box_address, err := datastore_utils.FindAndFormatRef(e.DataStore, datastore.AddressRef{ |
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.
For this one, you need to allow for the ability to pass in a full ref. Or at least a qualifier string (qualifier is another address ref field). Only because there will be multiple lock boxes for different tokens in the datastore. Qualifier would typically be the token symbol, ultimately depends on what you save to the datastore post-deployment.
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.
Qualifier is a good idea. I think the better way would be to use "IOwnable" as the qualifier since the main identifying feature of the lockbox is that it uses Ownable token pools. That way in the future if we build a lockbox which works on RBAC-based token pools we can make the qualifier "IAccessControl". Good Call out. In terms of 1.6.4 there is only one version but forward thinking is also useful.
chains/evm/deployment/v1_6_4/changesets/erc20_lockbox_deploy.go
Outdated
Show resolved
Hide resolved
chains/evm/deployment/v1_6_4/changesets/erc20_lockbox_deploy.go
Outdated
Show resolved
Hide resolved
| TokenAdminRegistry: perChainInput.TokenAdminRegistry, | ||
| TokenAdminRegistry: tokenAdminRegistryAddress, | ||
| } | ||
| report, err := cldf_ops.ExecuteSequence(e.OperationsBundle, sequences.ERC20LockboxDeploySequence, e.BlockChains, sequenceInput) |
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.
Also, make sure that when you deploy something like a lock box or a token pool, you add the entry to the datastore with the qualifier field populated (i.e. set to the token symbol). Ensures that you can differentiate between different token pools / lock boxes in the datastore and what their purposes are.
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.
so the lockbox is not one-token-one-lockbox, but multiple tokens to one lockbox so I don't think using the token in the qualifier is the best solution, but I do agree with better labeling practices. Also the token pools being deployed here do have "USDC" in the type which should solve the issue.
chains/evm/deployment/v1_6_4/changesets/siloed_usdc_token_pool_deploy.go
Show resolved
Hide resolved
…oolAddresses changeset
b-gopalswami
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.
LGTM! Just left one question.
| erc20LockBoxAddress, err := datastore_utils.FindAndFormatRef(e.DataStore, datastore.AddressRef{ | ||
| Type: datastore.ContractType(erc20_lock_box.ContractType), | ||
| Version: erc20_lock_box.Version, | ||
| }, perChainInput.ChainSelector, evm_datastore_utils.ToEVMAddress) | ||
| if err != nil { | ||
| return cldf.ChangesetOutput{}, err | ||
| } |
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.
How do we ensure this is the right address to act on? Like, do we expect only one such contract to be available for the entire network?
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've added a qualifier to the lookup to ensure there are no conflicts. Currently we don't expect there to be more than one LockBox per chain but that may change in the future so adding this qualifier helps with that. It is also possible that we could just do it based on versioning, as any future version of the lockbox will have a version >1.6.4 but the qualifier is a fine solution as well.
e13bde4#diff-e5ca08ab4642f513279363eb93ed645ab89ab91a2f85b047c3eaac5220fe78f4R52
…hangeset to prevent dataStore conflicts.
AnieeG
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.
Approved with 1 observation -
For all changesets I see only mcms input is verified and no other input is considered for verify function. If you want to come back to it later please add a ticket. Otherwise we are opening up ways for contract reverts and unintended input set-up through operations.
| Type: "TokenAdminRegistry", | ||
| Version: semver.MustParse("1.5.0"), |
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.
nit : better to refer it from constants - https://github.com/smartcontractkit/chainlink-ccip/blob/main/chains/evm/deployment/v1_5_0/operations/token_admin_registry/token_admin_registry.go#L15-L16 - Applicable to all contract types and version usages
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.
| Type: "RBACTimelock", | ||
| Version: semver.MustParse("1.0.0"), |
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.
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.
| erc20LockboxAddress, err := datastore_utils.FindAndFormatRef(e.DataStore, datastore.AddressRef{ | ||
| Type: datastore.ContractType(erc20_lock_box.ContractType), | ||
| Version: erc20_lock_box.Version, | ||
| Qualifier: "IOwnable", |
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.
let's have it as constant
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.
|
This PR modifies the previous v1_6_4 changesets to use the addressRef to acquire necessary deployed contract addresses rather than have them be supplied by the user. This reduces the amount of potential errors in the durable pipeline creation process as there are fewer possible fields for the creator to fill out incorrectly.