Conversation
🦋 Changeset detectedLatest commit: a778f0d 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}
There was a problem hiding this comment.
Code Review
This pull request introduces Hyperlane integration for cross-chain EXA token transfers, adding new deployment scripts and tests. The changes are significant and lay the groundwork for bridging the EXA token.
My review identified a critical issue in the router deployment script where the mailbox address is incorrectly initialized to the zero address, which would break all cross-chain functionality. I also found a high-severity issue in the router setup logic that assumes router addresses are the same across all chains, which may not hold true in production and could lead to configuration errors. Additionally, I've pointed out a minor inefficiency in the deployEXA script function. Addressing these points will improve the robustness and correctness of the Hyperlane integration.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
contracts/.gas-snapshotcontracts/deploy.jsoncontracts/package.jsoncontracts/remappings.txtcontracts/script/Redeployer.s.solcontracts/test/HypEXA.t.solcontracts/test/Redeployer.t.solcspell.jsonpackage.json
|
✅ 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
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
contracts/script/Redeployer.s.sol (1)
140-146:⚠️ Potential issue | 🟠 MajorGuard
setupRouteragainst undeployed router code.
getDeployedreturns the deterministic address even before deployment. Without a code-length check, role grant can succeed whileenrollRemoteRoutertargets an address with no code, leaving router config inconsistent.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 ProxyAdminNotDeployed(); error TargetNonceTooLow(); +error RouterNotDeployed();Run this read-only check to confirm the missing guard in the current implementation:
#!/bin/bash set -euo pipefail file="$(fd -p 'Redeployer.s.sol$' | head -n1)" nl -ba "$file" | sed -n '140,147p'Expected result:
CREATE3_FACTORY.getDeployed(...)is followed byvm.startBroadcast(admin)with norouter.code.lengthvalidation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.changeset/beige-sails-worry.mdcontracts/.gas-snapshotcontracts/deploy.jsoncontracts/package.jsoncontracts/remappings.txtcontracts/script/Redeployer.s.solcontracts/test/HypEXA.t.solcontracts/test/Redeployer.t.solcspell.json
contracts/.gas-snapshot
Outdated
| RedeployerTest:test_deployEXA_deploysAtSameAddress_onBase() (gas: 59986446) | ||
| RedeployerTest:test_deployExaFactory_deploysAtSameAddress_onEthereum() (gas: 276604537) | ||
| RedeployerTest:test_deployExaFactory_deploysAtSameAddress_onPolygon() (gas: 371278480) | ||
| RedeployerTest:test_deployExaFactory_deploysViaCreate3AtSameAddress_onPolygon() (gas: 48248553) | ||
| RedeployerTest:test_prepare_reverts_whenAdminIsDeployer() (gas: 32034206) | ||
| RedeployerTest:test_recoversNativeETHOnPolygon() (gas: 48419750) | ||
| RedeployerTest:test_run_reverts_whenAttackerUpgradesProxy() (gas: 42868965) | ||
| RedeployerTest:test_run_reverts_whenTargetNonceTooLow() (gas: 32499108) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add gas-budget guardrails for Redeployer critical paths.
Given the very high redeployer test gas figures, consider adding CI budget thresholds (or trend alerts) for these specific tests to catch future unintended growth early.
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3588c324-e849-4194-bc1e-e94abe72e9b5
📒 Files selected for processing (5)
.changeset/beige-sails-worry.mdcontracts/.gas-snapshotcontracts/deploy.jsoncontracts/script/Redeployer.s.solcontracts/test/HypEXA.t.sol
| "mailbox": { | ||
| "10": "0xd4C1905BB1D26BC93DAC913e13CaCC278CdCC80D", | ||
| "137": "0x5d934f4e2f797775e53561bB72aca21ba36B96BB", | ||
| "8453": "0xeA87ae93Fa0019a82A727bfd3eBd1cFCa8f64f1D" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a mailbox default (or explicit unsupported-chain guard).
The acct("mailbox") path falls back to .accounts.mailbox.default when a chain key is absent; without it, unsupported chains fail with a generic JSON key error instead of a clearer deployment-time failure.
| function deployRouter(address token) external returns (HypXERC20 router) { | ||
| address admin = acct("admin"); | ||
| vm.startBroadcast(admin); | ||
| router = HypXERC20( | ||
| CREATE3_FACTORY.deploy( | ||
| keccak256(abi.encode("HypEXA")), | ||
| abi.encodePacked( | ||
| type(TransparentUpgradeableProxy).creationCode, | ||
| abi.encode( | ||
| address(new HypXERC20(token, 1, acct("mailbox"))), | ||
| address(proxyAdmin), | ||
| abi.encodeCall(HypERC20Collateral.initialize, (address(0), address(0), admin)) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Where proxyAdmin deployment is currently validated:"
rg -nP 'address\(proxyAdmin\)\.code\.length' contracts/script/Redeployer.s.sol
echo
echo "run() guard context:"
sed -n '71,75p' contracts/script/Redeployer.s.sol
echo
echo "deployRouter() context:"
sed -n '121,138p' contracts/script/Redeployer.s.solRepository: exactly/exa
Length of output: 1088
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Full deployRouter() function signature:"
sed -n '121,121p' contracts/script/Redeployer.s.sol
echo
echo "Check for other callers of deployRouter in the file:"
rg -n 'deployRouter' contracts/script/Redeployer.s.sol
echo
echo "Check if proxyAdmin is set before deployRouter calls:"
sed -n '70,85p' contracts/script/Redeployer.s.solRepository: exactly/exa
Length of output: 1056
Add proxyAdmin code-length guard to deployRouter() to prevent proxy deployment with undeployed admin.
The run() function validates address(proxyAdmin).code.length at line 73, but deployRouter() is a separate external entry point that uses address(proxyAdmin) at line 131 without the same check. Since deployRouter() can be called independently, the proxy admin validation must be present here as well.
🛠️ Proposed fix
function deployRouter(address token) external returns (HypXERC20 router) {
address admin = acct("admin");
+ if (address(proxyAdmin).code.length == 0) revert ProxyAdminNotDeployed();
vm.startBroadcast(admin);
router = HypXERC20(
CREATE3_FACTORY.deploy(
keccak256(abi.encode("HypEXA")),1 participant
Summary by CodeRabbit
New Features
Tests
Chores
Style/Docs