Conversation
🦋 Changeset detectedLatest commit: 4bfb441 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Hyperlane/mailbox remappings and addresses, updates dev dependencies and pnpm config, refreshes gas snapshots, extends Redeployer with EXA/router deploy/upgrade plus timelock-based role scheduling, adds forked multi-chain HypEXA tests with revert coverage, tweaks a Redeployer test, plus small spellcheck/changeset edits. Changes
Sequence DiagramsequenceDiagram
autonumber
participant User as "User (EOA)"
participant Redeployer as "Redeployer script"
participant ProxyAdmin as "ProxyAdmin"
participant EXA as "EXA Proxy/Impl"
participant Router as "Router (domain)"
participant Timelock as "Timelock"
participant Mailbox as "Hyperlane Mailbox"
User->>Redeployer: deployEXA / deployRouter
Redeployer->>ProxyAdmin: upgradeAndCall(EXA.initialize)
ProxyAdmin->>EXA: call initialize()
Redeployer->>ProxyAdmin: upgradeAndCall(EXA.initialize2(acct))
ProxyAdmin->>EXA: call initialize2(acct)
User->>Redeployer: proposeBridgeRole(token, salt)
Redeployer->>Router: ensure deployed
Redeployer->>Timelock: schedule(grantRole(BRIDGE_ROLE, router))
Timelock-->>Redeployer: operation scheduled
User->>Router: transferRemote(amount, remoteDomain, payload)
Router->>Mailbox: sendMessage(payload)
Mailbox->>Router: deliverMessage(payload) (remote)
Router->>EXA: handle -> mint/credit or revert if no BRIDGE_ROLE
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's interoperability by integrating Hyperlane, a cross-chain communication protocol. The changes enable the EXA token to be bridged and managed across different blockchain domains, expanding its utility and reach. This involved updating core dependencies, modifying deployment processes to include Hyperlane-specific components, and adding comprehensive tests to ensure the robustness of the new cross-chain capabilities. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
✅ All tests passed. |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
contracts/script/Redeployer.s.sol (3)
96-149:⚠️ Potential issue | 🟠 MajorRun
forge fmton this file to unblock CI.The pipeline is currently failing
nx run@exactly/plugin:test:fmtdue to formatting differences in this file.As per coding guidelines
**/*.sol: Follow Solhint rules strictly and use Forge fmt for code formatting.
113-121:⚠️ Potential issue | 🟠 MajorFail fast when EXA implementation is missing before
upgradeEXA.Line 120 upgrades to
address(exa)without checking code presence, which defers failure to a less actionable downstream revert.Proposed fix
function upgradeEXA(address proxy) external { address admin = acct("admin"); + if (address(exa).code.length == 0) revert EXAImplementationNotDeployed(); ProxyAdmin p = ProxyAdmin(address(uint160(uint256( vm.load(proxy, bytes32(uint256(keccak256("eip1967.proxy.admin")) - 1)) )))); vm.broadcast(p.owner()); p.upgradeAndCall( ITransparentUpgradeableProxy(proxy), address(exa), abi.encodeCall(EXA.initialize2, (admin)) ); } @@ error ProxyAdminNotDeployed(); error TargetNonceTooLow(); +error EXAImplementationNotDeployed();
124-130: 🛠️ Refactor suggestion | 🟠 MajorAlign CREATE3 salt derivation with
token(or removetokenfrom the API).Line 129 hardcodes
"HypEXA"even though the function accepts atoken; that creates deterministic-slot collisions for multi-token use, and Line 145 resolves that same fixed slot.Proposed refactor
function deployRouter(address token) external returns (HypXERC20 router) { @@ - keccak256(abi.encode("HypEXA")), + keccak256(abi.encode("HypEXA", token)), @@ function setupRouter(address token, uint32 remoteDomain) external { address admin = acct("admin"); - address router = CREATE3_FACTORY.getDeployed(admin, keccak256(abi.encode("HypEXA"))); + address router = CREATE3_FACTORY.getDeployed(admin, keccak256(abi.encode("HypEXA", token)));Also applies to: 145-145
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
.changeset/beige-sails-worry.mdcontracts/.gas-snapshotcontracts/deploy.jsoncontracts/script/Redeployer.s.solcontracts/test/HypEXA.t.sol
There was a problem hiding this comment.
♻️ Duplicate comments (3)
contracts/script/Redeployer.s.sol (3)
121-127: 🛠️ Refactor suggestion | 🟠 Major
deployRouterignorestokenin the deterministic salt, making reuse collision-prone.The function accepts
tokenbut always uses the fixed"HypEXA"slot. Reusing it for another token collides on the same CREATE3 address.Proposed refactor
- keccak256(abi.encode("HypEXA")), + keccak256(abi.encode("HypEXA", token)), @@ - address router = CREATE3_FACTORY.getDeployed(admin, keccak256(abi.encode("HypEXA"))); + address router = CREATE3_FACTORY.getDeployed(admin, keccak256(abi.encode("HypEXA", token)));Also applies to: 142-142
140-146:⚠️ Potential issue | 🟠 Major
setupRoutershould fail fast if the deterministic router address is not deployed.
getDeployedcan resolve an address before code exists. Without a code-length guard, role/config steps can silently target an undeployed address path.Proposed fix
function setupRouter(address token, uint32 remoteDomain) external { address admin = acct("admin"); address router = CREATE3_FACTORY.getDeployed(admin, keccak256(abi.encode("HypEXA"))); + if (router.code.length == 0) revert RouterNotDeployed(); vm.startBroadcast(admin); EXA(token).grantRole(keccak256("BRIDGE_ROLE"), router); HypXERC20(router).enrollRemoteRouter(remoteDomain, bytes32(uint256(uint160(router)))); vm.stopBroadcast(); } @@ error TargetNonceTooLow(); +error RouterNotDeployed();
113-119:⚠️ Potential issue | 🟠 Major
upgradeEXAshould guard against missing EXA implementation deployment.The upgrade path uses
address(exa)directly; adding an explicit code-length guard gives a clearer, earlier failure mode.Proposed fix
function upgradeEXA(address proxy) external { address admin = acct("admin"); + if (address(exa).code.length == 0) revert EXAImplementationNotDeployed(); ProxyAdmin p = ProxyAdmin(address(uint160(uint256(vm.load(proxy, bytes32(uint256(keccak256("eip1967.proxy.admin")) - 1)))))); vm.broadcast(p.owner()); p.upgradeAndCall(ITransparentUpgradeableProxy(proxy), address(exa), abi.encodeCall(EXA.initialize2, (admin))); } @@ error TargetNonceTooLow(); +error EXAImplementationNotDeployed();
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
.changeset/beige-sails-worry.mdcontracts/.gas-snapshotcontracts/deploy.jsoncontracts/script/Redeployer.s.solcontracts/test/HypEXA.t.sol
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| ] | ||
| }, | ||
| "neverBuiltDependencies": [], | ||
| "onlyBuiltDependencies": ["@exactly/protocol"], |
There was a problem hiding this comment.
🚩 onlyBuiltDependencies restricts install scripts to @exactly/protocol only
The change from "neverBuiltDependencies": [] (empty blocklist — all packages can run scripts) to "onlyBuiltDependencies": ["@exactly/protocol"] (allowlist — only @exactly/protocol runs scripts) is a significant behavioral shift. This blocks install/build scripts for ALL other dependencies including the newly added @hyperlane-xyz/core. This is likely intentional since @exactly/protocol changed from an npm published package (^0.2.22) to a git commit reference (exactly/protocol#5833408) that needs a build step, and @hyperlane-xyz/core is a Solidity library that typically doesn't need post-install scripts. However, if any future dependency requires build scripts, they would silently fail to run.
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
1 similar comment
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
itofarina
force-pushed
the
hyperlane
branch
2 times, most recently
from
476de3b to
ac57e92
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/script/Redeployer.s.sol (1)
195-203: 🧹 Nitpick | 🔵 Trivial
deployEXAdoubleupgradeAndCallworks but is slightly wasteful.The second
upgradeAndCallat line 199–201 re-points the proxy to the same implementation just to invokeinitialize2via the privileged path. It is correct (initialize2 asreinitializer(2)runs cleanly afterinitialize()asinitializer), but the upgrade step itself is redundant. An alternative is to callEXA(proxy).initialize2(acct("exactly"))directly from a non-admin EOA after the first upgrade — the transparent proxy will route it through the fallback to the implementation.Not blocking; only flagging in case you prefer the cheaper path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6659ecaa-8dbf-4b6f-9d9e-f57252b80a7c
📒 Files selected for processing (3)
contracts/.gas-snapshotcontracts/script/Redeployer.s.solcontracts/test/HypEXA.t.sol
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
2 participants
Summary by CodeRabbit
New Features
Tests
Chores
Style/Docs