Conversation
🦋 Changeset detectedLatest commit: f7f7600 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 integration and mailbox addresses, new Redeployer router/EXA deploy/upgrade functions, a multi-domain HypEXA test suite, dependency and remapping updates, a gas-snapshot refresh, and minor test expectation adjustment; no public API signatures removed. Changes
Sequence DiagramsequenceDiagram
participant OP as OP Chain
participant OPRouter as OP Router
participant Hyperlane as Hyperlane Mailbox
participant BaseRouter as BASE Router
participant BASE as BASE Chain
participant PolyRouter as POLYGON Router
OP->>OPRouter: transferRemote(amount, remoteDomain, gas)
OPRouter->>Hyperlane: sendMessage(payload)
Hyperlane->>BaseRouter: deliverMessage(payload)
BaseRouter->>BASE: handle -> mint/burn
BaseRouter->>Hyperlane: optionally send response
Hyperlane->>OPRouter: deliverAck/response
OPRouter->>OP: finalize (update balances/supply)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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.
| type(TransparentUpgradeableProxy).creationCode, | ||
| abi.encode( | ||
| address(new HypXERC20(token, 1, acct("mailbox"))), | ||
| protocol("ProxyAdmin"), |
There was a problem hiding this comment.
Bug: The deployRouter function will fail on chains without a pre-existing ProxyAdmin deployment because protocol("ProxyAdmin") reverts instead of using a fallback.
Severity: HIGH
Suggested Fix
Modify deployRouter to provide a fallback ProxyAdmin if one is not found via protocol("ProxyAdmin"). For example, call protocol("ProxyAdmin", false) to prevent a revert, and if the result is address(0), use the proxyAdmin deployed by the Redeployer script itself as the default.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: contracts/script/Redeployer.s.sol#L231
Potential issue: The `deployRouter` function fetches a `ProxyAdmin` address by calling
`protocol("ProxyAdmin")`. This function is configured to revert if the address is not
found in the protocol's deployment files for the target chain. While tests for chains
like Base and Optimism pass because these deployment files exist, the test for Polygon
requires a manual workaround (`set("ProxyAdmin", ...)`), indicating the deployment file
is missing for that chain. In a production deployment on a chain without a pre-existing
`ProxyAdmin` (like Polygon), this workaround will be absent, causing the script to fail
and preventing the router's deployment.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7f7600366
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "exactly": { | ||
| "10": "0xC0d6Bc5d052d1e74523AD79dD5A954276c9286D3", | ||
| "8453": "0x7A65824d74B0C20730B6eE4929ABcc41Cbe843Aa" | ||
| }, |
There was a problem hiding this comment.
deployRouter and setupRouter now call acct("exactly"), which resolves .accounts.exactly.<chainId> and falls back to .accounts.exactly.default (see contracts/test/Fork.t.sol). The new accounts.exactly block only defines 10 and 8453, so running the new Hyperlane flow on polygon (137) or any other chain without an env override will revert while reading deploy.json before ownership transfer/enrollment can happen. Please add a 137 and/or default entry so router deployment/configuration is not chain-fragile.
Useful? React with 👍 / 👎.
2 participants
Summary by CodeRabbit
New Features
Tests
Chores
Style/Docs