Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds root-level CI workflows for EVM and Stellar contracts, removes a legacy EVM workflow under Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions (Workflow)
participant Runner as Runner (ubuntu-latest)
participant Checkout as Checkout (repo + submodules)
participant Tooling as Tooling (Foundry / Rust / Node / pnpm / Stellar CLI / nargo / bb)
participant BuildTest as Build & Test (forge / cargo / stellar contract / scripts/build_circuits.sh)
GH->>Runner: trigger (push/pr to relevant paths or workflow_dispatch)
Runner->>Checkout: checkout repository + recursive submodules
Runner->>Tooling: install toolchains & configure pnpm/Node
Runner->>Tooling: run scripts/build_circuits.sh (when invoked)
Tooling->>BuildTest: run format checks (forge fmt / cargo fmt)
Tooling->>BuildTest: build artifacts (forge build / stellar contract build / nargo/bb)
Tooling->>BuildTest: run tests (forge test / cargo test / integration_test)
BuildTest-->>GH: report results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/evm/src/Verifier.sol (1)
233-240:⚠️ Potential issue | 🔴 Critical
expcomputes the wrong power for most exponentsLine 236-239 currently squares
basein a doubling loop and returns it, which only matchesbase^exponentfor special exponents (e.g., powers of two). This breaks field arithmetic correctness.Proposed fix
function exp(Fr base, Fr exponent) pure returns (Fr) { - if (Fr.unwrap(exponent) == 0) return Fr.wrap(1); - - for (uint256 i = 1; i < Fr.unwrap(exponent); i += i) { - base = base * base; - } - return base; + uint256 e = Fr.unwrap(exponent); + Fr result = Fr.wrap(1); + Fr b = base; + while (e > 0) { + if ((e & 1) == 1) { + result = result * b; + } + b = b * b; + e >>= 1; + } + return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/evm/src/Verifier.sol` around lines 233 - 240, The exp function currently replaces base with base*base in a doubling loop and returns base, which computes only powers of two; fix it to perform proper exponentiation by squaring: unwrap the exponent into a local uint256 (e.g., uint256 e = Fr.unwrap(exponent)), initialize an accumulator result = Fr.wrap(1), then while e > 0 if e & 1 == 1 multiply result by base, then square base (base = base * base) and shift e >>= 1; finally return result; update references to Fr.unwrap/Fr.wrap and the function exp signature accordingly.
🧹 Nitpick comments (1)
.github/workflows/evm-contracts.yml (1)
26-31: Pin external GitHub Actions to immutable commit SHAs.Actions use mutable tags (
@v1,@v4) instead of commit SHAs, which weakens workflow supply-chain integrity. Replace with SHA-pinneduses:entries for all four actions on lines 26, 31, 37, and 40.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/evm-contracts.yml around lines 26 - 31, The workflow uses mutable action tags (e.g., actions/checkout@v4 and foundry-rs/foundry-toolchain@v1) which should be replaced with immutable commit SHAs; update each `uses:` entry for the four actions mentioned (the ones currently using `@v4/`@v1 tags) to reference their corresponding full commit SHA instead of the tag, ensuring you fetch the exact commit SHAs from the upstream repos and replace the `uses:` values consistently so the workflow is pinned to immutable versions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/evm-contracts.yml:
- Around line 5-11: The workflow path filters only include "contracts/evm/**"
and the workflow file itself; update both the "paths" and "pull_request.paths"
arrays in evm-contracts.yml to also match repo-root dependency inputs used when
installing deps (e.g. package.json, package-lock.json, yarn.lock,
pnpm-lock.yaml, pnpm-workspace.yaml, and any monorepo/tooling manifests like
turbo.json or workspace config) so changes to those files will trigger the job;
modify the same filters referenced in the diff and the dependency-install
section (the block around the install step currently at lines 45-47) so both
push and PR triggers include those root files.
In @.github/workflows/stellar-contracts.yml:
- Around line 55-60: The workflow is downloading and extracting the Stellar CLI
without any integrity checks; update the run step that fetches
/tmp/stellar-cli.tar.gz to compute and record a SHA256 hash immediately after
download (e.g., compute sha256sum of /tmp/stellar-cli.tar.gz and write it to the
job log and an artifact) and make the job optionally verify against a pinned
value supplied via an input or env var (e.g., STELLAR_CLI_SHA256) so CI fails if
a known-good hash is provided and doesn't match; if no pinned checksum is
available, at minimum persist the computed hash for auditing and add a TODO note
to switch to official checksums/GPG signatures once Stellar publishes them.
In `@contracts/evm/src/Verifier.sol`:
- Around line 776-778: The staticcall to the precompile (address(0x08)) decodes
result unconditionally which can revert if the call failed or returned empty
data; change the logic in the function around the (bool success, bytes memory
result) = address(0x08).staticcall(input) so that you first check success (and
optionally result.length > 0) and only call abi.decode(result, (bool)) when
success is true, returning false (or the appropriate fallback) otherwise; update
any return to use the safely decoded value (e.g., decodedResult) only after the
success check.
---
Outside diff comments:
In `@contracts/evm/src/Verifier.sol`:
- Around line 233-240: The exp function currently replaces base with base*base
in a doubling loop and returns base, which computes only powers of two; fix it
to perform proper exponentiation by squaring: unwrap the exponent into a local
uint256 (e.g., uint256 e = Fr.unwrap(exponent)), initialize an accumulator
result = Fr.wrap(1), then while e > 0 if e & 1 == 1 multiply result by base,
then square base (base = base * base) and shift e >>= 1; finally return result;
update references to Fr.unwrap/Fr.wrap and the function exp signature
accordingly.
---
Nitpick comments:
In @.github/workflows/evm-contracts.yml:
- Around line 26-31: The workflow uses mutable action tags (e.g.,
actions/checkout@v4 and foundry-rs/foundry-toolchain@v1) which should be
replaced with immutable commit SHAs; update each `uses:` entry for the four
actions mentioned (the ones currently using `@v4/`@v1 tags) to reference their
corresponding full commit SHA instead of the tag, ensuring you fetch the exact
commit SHAs from the upstream repos and replace the `uses:` values consistently
so the workflow is pinned to immutable versions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b88939cb-e76f-4823-8137-32fe3cbee75b
📒 Files selected for processing (26)
.github/workflows/evm-contracts.yml.github/workflows/stellar-contracts.ymlcontracts/evm/.github/workflows/test.ymlcontracts/evm/src/Verifier.solcontracts/stellar/contracts/ad-manager/src/errors.rscontracts/stellar/contracts/ad-manager/src/lib.rscontracts/stellar/contracts/ad-manager/src/storage.rscontracts/stellar/contracts/ad-manager/src/test.rscontracts/stellar/contracts/ad-manager/src/token.rscontracts/stellar/contracts/ad-manager/src/validation.rscontracts/stellar/contracts/merkle-manager/src/mmr.rscontracts/stellar/contracts/order-portal/src/errors.rscontracts/stellar/contracts/order-portal/src/lib.rscontracts/stellar/contracts/order-portal/src/storage.rscontracts/stellar/contracts/order-portal/src/test.rscontracts/stellar/contracts/order-portal/src/token.rscontracts/stellar/contracts/order-portal/src/validation.rscontracts/stellar/lib/proofbridge-core/src/eip712.rscontracts/stellar/lib/proofbridge-core/src/test.rscontracts/stellar/lib/proofbridge-core/src/token.rscontracts/stellar/test_snapshots/test_ad_manager_lock_for_order.1.jsoncontracts/stellar/test_snapshots/test_ad_manager_unlock_with_bridger_proof.1.jsoncontracts/stellar/test_snapshots/test_full_cross_chain_flow.1.jsoncontracts/stellar/test_snapshots/test_nullifier_prevents_double_unlock.1.jsoncontracts/stellar/test_snapshots/test_order_portal_create_and_unlock.1.jsoncontracts/stellar/tests/integration_test.rs
💤 Files with no reviewable changes (1)
- contracts/evm/.github/workflows/test.yml
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/stellar-contracts.yml (1)
6-12:⚠️ Potential issue | 🟠 MajorWorkflow trigger paths miss required build inputs.
Line 7 and Line 11 only watch
contracts/stellar/**, but this workflow also depends onscripts/build_circuits.shandproof_circuits/deposits. Changes there won’t trigger CI, so breakages can slip through.💡 Suggested patch
push: branches: [main] paths: - "contracts/stellar/**" + - "proof_circuits/**" + - "scripts/build_circuits.sh" - ".github/workflows/stellar-contracts.yml" pull_request: paths: - "contracts/stellar/**" + - "proof_circuits/**" + - "scripts/build_circuits.sh" - ".github/workflows/stellar-contracts.yml"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/stellar-contracts.yml around lines 6 - 12, The workflow trigger lists under the push and pull_request "paths" blocks only include "contracts/stellar/**" and miss other inputs; update both paths arrays (the push.paths and pull_request.paths entries) to also include "scripts/build_circuits.sh" and "proof_circuits/deposits" so changes to the build script and proof circuit sources will trigger the CI run alongside changes to "contracts/stellar/**".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/stellar-contracts.yml:
- Around line 58-61: The shell commands mkdir -p, tar -xzf, chmod, and echo in
the install block use unquoted variables which can trigger
word-splitting/globbing (SC2086); update those commands to quote
environment-variable expansions such as "$HOME/.local/bin", the tar archive path
"/tmp/stellar-cli.tar.gz" (and the -C target "$HOME/.local/bin"), and the
GITHUB_PATH destination "$GITHUB_PATH" so each command (mkdir -p
"$HOME/.local/bin", tar -xzf "/tmp/stellar-cli.tar.gz" -C "$HOME/.local/bin"
stellar, chmod +x "$HOME/.local/bin"/stellar, echo "$HOME/.local/bin" >>
"$GITHUB_PATH") safely handles paths with spaces or special characters.
In `@scripts/build_circuits.sh`:
- Around line 123-126: Add an explicit check that the provided path exists and
is a directory before attempting to cd; e.g., before setting TARGET_PATH use a
test like if [ ! -e "$1" ] || [ ! -d "$1" ] then print a clear error (to stderr)
and call usage (or exit), then proceed with TARGET_PATH="$(cd "$1" && pwd)";
reference the existing usage function and the TARGET_PATH/PROVE variables when
making the change.
---
Duplicate comments:
In @.github/workflows/stellar-contracts.yml:
- Around line 6-12: The workflow trigger lists under the push and pull_request
"paths" blocks only include "contracts/stellar/**" and miss other inputs; update
both paths arrays (the push.paths and pull_request.paths entries) to also
include "scripts/build_circuits.sh" and "proof_circuits/deposits" so changes to
the build script and proof circuit sources will trigger the CI run alongside
changes to "contracts/stellar/**".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ea4f209-f832-4d84-8b75-a761c2afd006
📒 Files selected for processing (4)
.github/workflows/evm-contracts.yml.github/workflows/stellar-contracts.ymlcontracts/stellar/contracts/verifier/tests/build_circuits.shscripts/build_circuits.sh
💤 Files with no reviewable changes (1)
- contracts/stellar/contracts/verifier/tests/build_circuits.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/evm-contracts.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit