Conversation
🦋 Changeset detectedLatest commit: e02a973 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Summary of ChangesHello @itofarina, 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 upgrades the 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
|
WalkthroughAdds BRIDGE_ROLE and IERC7802 cross-chain mint/burn to EXA with initialize2 reinitializer for proxy admin, updates tests to use proxy-based EXA and adds EXATest, bumps OpenZeppelin and CI/Foundry tooling, adjusts deploy scripts to handle timelock/ProxyAdmin, and updates many test gas snapshots. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 |
![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.
✨ exa: add crosschain mint and burn support
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
![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.
itofarina
force-pushed
the
exa-crosschain
branch
2 times, most recently
from
5edfce9 to
df5529c
Compare
itofarina
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
itofarina
force-pushed
the
exa-crosschain
branch
from
df5529c to
be4bd71
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
contracts/periphery/EXA.sol (1)
26-29:⚠️ Potential issue | 🟠 MajorValidate
admin_ininitialize2to prevent irreversible admin lockout.Line 28 grants
DEFAULT_ADMIN_ROLEwithout validatingadmin_. Passingaddress(0)would leave admin control effectively unusable.🛠️ Proposed fix
contract EXA is ERC20VotesUpgradeable, AccessControlUpgradeable, IERC7802 { bytes32 public constant BRIDGE_ROLE = keccak256("BRIDGE_ROLE"); + error InvalidAdmin(); @@ function initialize2(address admin_) external reinitializer(2) { + if (admin_ == address(0)) revert InvalidAdmin(); __AccessControl_init(); _grantRole(DEFAULT_ADMIN_ROLE, admin_); }#!/bin/bash set -euo pipefail echo "== Verify EXA.initialize2 has/hasn't zero-address guard ==" rg -n -C3 'function initialize2|admin_ == address\(0\)|_grantRole\(DEFAULT_ADMIN_ROLE, admin_\)' contracts/periphery/EXA.sol echo echo "== Inspect OZ AccessControlUpgradeable _grantRole behavior ==" fd -HI '^AccessControlUpgradeable\.sol$' | while read -r f; do echo "-- $f" rg -n -C3 'function _grantRole|address\(0\)' "$f" doneExpected result: no
admin_ == address(0)guard ininitialize2, and_grantRoleitself does not enforce non-zero accounts.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
.changeset/silent-chefs-marry.md.gas-snapshotcontracts/periphery/EXA.soltest/EXA.t.soltest/EscrowedEXA.t.sol
itofarina
force-pushed
the
exa-crosschain
branch
2 times, most recently
from
0c63b60 to
ebde601
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
itofarina
force-pushed
the
exa-crosschain
branch
3 times, most recently
from
87bb3d8 to
62e1258
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![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.
itofarina
force-pushed
the
exa-crosschain
branch
from
62e1258 to
07bdc21
Compare
![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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #797 +/- ##
=======================================
Coverage ? 94.82%
=======================================
Files ? 31
Lines ? 2724
Branches ? 457
=======================================
Hits ? 2583
Misses ? 140
Partials ? 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
itofarina
force-pushed
the
exa-crosschain
branch
from
ab7fce0 to
f802f1d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
itofarina
force-pushed
the
exa-crosschain
branch
from
f802f1d to
4f9f2ff
Compare
itofarina
changed the title
itofarina
force-pushed
the
exa-crosschain
branch
from
4f9f2ff to
8a3c5f9
Compare
itofarina
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a3c5f93a3
ℹ️ 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".
|
|
||
| const exa = await getContract<EXA>("EXA", await getSigner(deployer)); | ||
|
|
||
| if (!(await exa.hasRole(await exa.DEFAULT_ADMIN_ROLE(), timelock))) { |
There was a problem hiding this comment.
This hasRole check assumes the proxy is already running the new EXA implementation, but in the timelock/multisig flow validateUpgrade often only schedules/proposes the upgrade and returns before execution; in that state the proxy still points to the old EXA, which does not implement hasRole/DEFAULT_ADMIN_ROLE, so this call reverts and aborts the deploy script. This can break the first rollout of this commit on live networks unless the upgrade is executed manually before rerunning.
Useful? React with 👍 / 👎.
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: 6a24e1be-a859-4711-87b5-f3db75d9febb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
.changeset/silent-chefs-marry.md.changeset/slick-feet-jump.md.gas-snapshot.github/workflows/release.yaml.github/workflows/test.yml.gitignorecontracts/periphery/EXA.soldeploy/EXA.tsdeploy/Markets.tsfoundry.tomlpackage.jsontest/EXA.t.soltest/EscrowedEXA.t.soltest/hardhat/21_exa.ts
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 20 | ||
| node-version: 22 | ||
| cache: pnpm | ||
| - uses: foundry-rs/foundry-toolchain@v1 | ||
| with: | ||
| version: v1.3.6 | ||
| version: v1.5.1 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
What is the latest version of Foundry toolchain?
💡 Result:
The latest Foundry toolchain release is Foundry v1.6.0 (released January 22, 2026). [1]
A pre-release is also listed: v1.6.0-rc1. [1]
Source(s)
[1] (getfoundry.sh)
Citations:
LGTM - Version upgrades look appropriate, though a newer version exists.
Node.js 22 is the current LTS version. The Foundry toolchain upgrade to v1.5.1 is a solid improvement. However, Foundry v1.6.0 (released January 2026) is now the latest stable version and could be considered for future updates if you want to stay on the cutting edge.
itofarina
force-pushed
the
exa-crosschain
branch
from
8a3c5f9 to
e02a973
Compare
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: efd6c12d-5a40-4c5a-8a6e-ee9fb71ee4a5
📒 Files selected for processing (8)
.changeset/silent-chefs-marry.md.changeset/slick-feet-jump.md.gas-snapshotcontracts/periphery/EXA.soldeploy/EXA.tstest/EXA.t.soltest/EscrowedEXA.t.soltest/hardhat/21_exa.ts
| if (!(await exa.hasRole(await exa.DEFAULT_ADMIN_ROLE(), timelock))) { | ||
| const proxyAdmin = await getContract<ProxyAdmin>("ProxyAdmin", await getSigner(deployer)); | ||
| await timelockPropose(proxyAdmin, "upgradeAndCall", [ | ||
| exa.target, | ||
| (await get("EXA_Implementation")).address, | ||
| exa.interface.encodeFunctionData("initialize2", [timelock]), | ||
| ]); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any BRIDGE_ROLE grants in deployment or scripts
rg -n 'BRIDGE_ROLE|grantRole.*bridge' --type ts --type js -C2Repository: exactly/protocol
Length of output: 42
🏁 Script executed:
# First, find the EXA contract file
find . -type f -name "*EXA*" | grep -E "\.(sol|ts|js)$" | head -20Repository: exactly/protocol
Length of output: 257
🏁 Script executed:
# Search for mint and burn function definitions
rg -n "function mint|function burn" --type ts --type js --type sol -A3Repository: exactly/protocol
Length of output: 87
🏁 Script executed:
# Search for role definitions (including different naming patterns)
rg -n "ROLE|keccak256.*role|bytes32.*=" --type ts --type js --type sol | grep -i "admin\|bridge\|role" | head -30Repository: exactly/protocol
Length of output: 87
🏁 Script executed:
# Read the EXA.sol file to understand role structure
cat ./contracts/periphery/EXA.sol | head -100Repository: exactly/protocol
Length of output: 2845
🏁 Script executed:
# Search for BRIDGE_ROLE, mint, burn in EXA.sol
grep -n "BRIDGE_ROLE\|function mint\|function burn\|bytes32.*ROLE\|initialize2" ./contracts/periphery/EXA.solRepository: exactly/protocol
Length of output: 494
🏁 Script executed:
# Check the deploy/EXA.ts file structure
cat ./deploy/EXA.tsRepository: exactly/protocol
Length of output: 4163
Add BRIDGE_ROLE grant to timelock during EXA deployment or confirm it's granted via a separate governance proposal.
The mint() and burn() functions require BRIDGE_ROLE to be callable (they delegate to crosschainMint and crosschainBurn which are guarded by onlyRole(BRIDGE_ROLE)), but the deployment script only grants DEFAULT_ADMIN_ROLE to timelock via initialize2. Without BRIDGE_ROLE being granted—either in this deployment or via a subsequent governance proposal—the mint/burn functions remain non-functional. Ensure the role grant path is explicitly documented and implemented.
| function test_mint_reverts_whenCallerLacksBridgeRole() external { | ||
| vm.expectRevert(); | ||
| exa.mint(bob, 100e18); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider asserting specific revert selectors in negative-path tests.
Using bare vm.expectRevert() can mask unrelated failures. For more robust regression tests, assert the expected AccessControlUnauthorizedAccount selector.
Example for one test
function test_mint_reverts_whenCallerLacksBridgeRole() external {
- vm.expectRevert();
+ vm.expectRevert(
+ abi.encodeWithSelector(
+ IAccessControl.AccessControlUnauthorizedAccount.selector,
+ address(this),
+ exa.BRIDGE_ROLE()
+ )
+ );
exa.mint(bob, 100e18);
}Also applies to: 174-177, 189-192, 205-208
2 participants
Summary by CodeRabbit
New Features
Tests
Chores