-
Notifications
You must be signed in to change notification settings - Fork 361
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
feat(cli): Add hyperlane warp apply #4094
Conversation
🦋 Changeset detectedLatest commit: 4ea996e The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 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 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4094 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 1 1
Lines 14 14
=====================================
Misses 14 14
|
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.
Mostly lgtm, just a few comments
// Convert warpCoreConfig.tokens[] into a mapping of { [chainName]: Config } | ||
// This allows O(1) reads within the loop | ||
const warpCoreByChain = Object.fromEntries( | ||
warpCoreConfig.tokens.map((token) => [token.chainName, token]), | ||
); |
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 is fine but realistically if n < 1000-ish if really doesn't matter if it's O(1), O(n), or even O(n^2)
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.
True that
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 mostly did it because i like reasoning with similar data shapes. I thought it would be hard to compare an array with an object
// Update Warp | ||
config.ismFactoryAddresses = addresses[ | ||
chain | ||
] as ProxyFactoryFactoriesAddresses; |
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.
Why is a cast needed here?
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.
Because addresses[chain] returns a Record<string, string> which seems to be no specific enough
Type 'Record<string, string>' is missing the following properties from type '{ staticMerkleRootMultisigIsmFactory: string; staticMessageIdMultisigIsmFactory: string; staticAggregationIsmFactory: string; staticAggregationHookFactory: string; domainRoutingIsmFactory: string; }': staticMerkleRootMultisigIsmFactory, staticMessageIdMultisigIsmFactory, staticAggregationIsmFactory, staticAggregationHookFactory, domainRoutingIsmFactoryts(2739)
} catch (e) { | ||
logRed(`Warp config on ${chain} failed to update.`, e); | ||
} |
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.
Are you certain it's better to continue updating the rest if there's a failure instead of stopping and letting the user debug the issue?
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.
Not certain. But i'm open to changing this as requirements change. As of now, there are none.
The reason I did this was done is, we are only doing ism updates, which are mostly isolated for each chain. When updating an ISM we may also deploy one, which is committed right away. Then after deploying, we set a warp route's ISM as a transaction.
If we revert here because an update failed in another chain, we've potentially deployed an ISM for no reason.
Description
hyperlane warp apply
Related issues
Backward compatibility
Yes
Testing
Manual
To test:
yarn hyperlane warp apply --warp