feat: core deploy apply admin proxy ownership fixes #4767
Conversation
…min field to allow deploy overrides and checks
…hod to retrive proxy admin data
…e an already existing proxy admi ncontract for deployments
…to xeno/warp-deploy-apply-ownership-fixes
…to xeno/warp-deploy-apply-ownership-fixes
…ents and admin proxy ownership transfer in warp apply
Co-authored-by: Paul Balaji <10051819+paulbalaji@users.noreply.github.com>
… to core init command
…nfig defines owners fro the mailbox proxy admin adn ica router and use those values intead of the signer
…to xeno/warp-deploy-apply-ownership-fixes
…to xeno/warp-deploy-apply-ownership-fixes
…t defined on the type
…to xeno/core-deploy-apply-admin-proxy-ownership-fixes
…yperlane-xyz/hyperlane-monorepo into xeno/core-deploy-apply-admin-proxy-ownership-fixes
…r the mailbox contract
…admin proxy ownership code
…ly for core command testing
…to xeno/core-deploy-apply-admin-proxy-ownership-fixes
…to xeno/warp-deploy-apply-ownership-fixes
…yperlane-xyz/hyperlane-monorepo into xeno/core-deploy-apply-admin-proxy-ownership-fixes
xeno097
requested review from
Mo-Hussain,
jmrossy,
yorhodes and
ltyu
as code owners
🦋 Changeset detectedLatest commit: 7798c00 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…to xeno/core-deploy-apply-admin-proxy-ownership-fixes
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4767 +/- ##
=======================================
Coverage 74.27% 74.27%
=======================================
Files 101 101
Lines 1481 1481
Branches 192 192
=======================================
Hits 1100 1100
Misses 360 360
Partials 21 21
|
| // This field is set as optional because the old core config | ||
| // did not have it and we want to maintain backward compatibility | ||
| proxyAdmin: DeployedOwnableSchema.optional(), |
There was a problem hiding this comment.
define "old core config" - out of 140ish chains with a mailbox in registry, 134 have a proxyAdmin
There was a problem hiding this comment.
For example, if someone uses an old config file where the proxyAdmin field was not defined because it was generated with an old version of the CLI, Example:
- Deploy core contracts with old cli
- Update the CLI to the new version that adds this field
- Try to read the core deployment config. If the field is set as required in the schema this will fail
There was a problem hiding this comment.
I see - wonder if it's ok to just make it a breaking change that we demand the proxyAdmin now and if an old version doesn't have it then that's on the user
wdyt @ltyu?
There was a problem hiding this comment.
Technically it shouldn't be a problem because worst case scenario the user can just core read to 'update' the config to the latest version but it is an inconvenience anyways because the first time the CLI will fail and the fix is not that obvious. deferring to @ltyu for his thoughts. Maybe we should consider versioning configs in the future to avoid breaking changes and allows conversion from an old config file to a new one
| // This field is set as optional because the old core config | ||
| // did not have it and we want to maintain backward compatibility | ||
| proxyAdmin: DeployedOwnableSchema.optional(), |
There was a problem hiding this comment.
I see - wonder if it's ok to just make it a breaking change that we demand the proxyAdmin now and if an old version doesn't have it then that's on the user
wdyt @ltyu?
Description
This PR updates the
hyperlane core init,hyperlane core deployandhyperlane core applycommands to allow a user to change ownership of the mailbox ProxyAdmin contract by setting a value in the config.Drive-by changes
randomAddresstest util implementations across thesdk, 'infra' andclipackageanvil1to therun-e2e-test.shscript to testhyperlane corecommands in isolationproxyAdminOwnershipUpdateTxsto deduplicate proxy admin ownership tx data generationRelated issues
Backward compatibility
Testing
NOTE: