Conversation
🦋 Changeset detectedLatest commit: ac57e92 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 |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 56 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds Hyperlane/mailbox addresses and remappings, pins/updates dev dependencies, refreshes gas snapshots, extends the Redeployer script with EXA/router deploy & upgrade + role scheduling, adds a forked multi-chain HypEXA test suite, tweaks one Redeployer test expectation, and introduces minor config/wordlist files; no public signatures were 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, payload)
OPRouter->>Hyperlane: sendMessage(payload)
Hyperlane->>BaseRouter: deliverMessage(payload)
BaseRouter->>BASE: handle(message) -> mint/credit/burn
BaseRouter->>Hyperlane: (optional) 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 | ✅ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 476de3b6c0
ℹ️ 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".
| function setupRouter(uint32 remoteDomain) external { | ||
| address router = CREATE3_FACTORY.getDeployed(acct("admin"), keccak256(abi.encode("HypEXA"))); | ||
| if (router.code.length == 0) revert RouterNotDeployed(); | ||
| vm.broadcast(acct("exactly")); |
There was a problem hiding this comment.
exactly
setupRouter sends the enrollment transaction with vm.broadcast(acct("exactly")), but for the supported OP/Base configs this account is the protocol owner Safe, not a key-controlled EOA. In real forge script --broadcast runs, this sender cannot be signed, so enrollRemoteRouter never lands on-chain even though fork tests pass (tests can impersonate any address). Use a signer-controlled updater path (or governance proposal flow) for enrollment instead of broadcasting directly from the owner Safe.
Useful? React with 👍 / 👎.
| function upgradeEXA(address proxy) external { | ||
| address admin = acct("admin"); | ||
| ProxyAdmin p = ProxyAdmin(address(uint160(uint256(vm.load(proxy, ERC1967Utils.ADMIN_SLOT))))); | ||
| vm.broadcast(p.owner()); |
There was a problem hiding this comment.
ProxyAdmin.owner()
upgradeEXA uses vm.broadcast(p.owner()) as the sender. On deployments where the proxy admin owner is a contract wallet (e.g., timelock/safe), this makes the upgrade path non-executable in production because there is no private key to sign from that address; only fork-style impersonation succeeds. This function should route upgrades through a proposer/executor governance flow or require an actual signer account.
Useful? React with 👍 / 👎.
| "exactly": { | ||
| "10": "0xC0d6Bc5d052d1e74523AD79dD5A954276c9286D3", | ||
| "8453": "0x7A65824d74B0C20730B6eE4929ABcc41Cbe843Aa" | ||
| }, |
There was a problem hiding this comment.
🚩 No deploy.json entry for exactly account on Polygon (chain 137)
The new exactly account in contracts/deploy.json:15-18 only has entries for chains 10 (OP) and 8453 (Base), but not for 137 (Polygon). The mailbox entry at contracts/deploy.json:47-49 does include Polygon. If deployRouter or setupRouter were called on Polygon in production, acct("exactly") would fall through to the default key lookup, which also doesn't exist for exactly, causing a revert in the JSON parsing. The test handles this correctly with set("exactly", makeAddr("exactly")) at contracts/test/HypEXA.t.sol:43. This is likely intentional if the exactly multisig hasn't been established on Polygon yet, but worth confirming.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Summary by CodeRabbit
New Features
Tests
Chores
Style/Docs