feat: Cross-Chain E2E CI — Stellar ↔ EVM#109
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR enables and wires a cross-chain E2E workflow: it activates the GitHub Actions job, adds a test token, changes Soroban contracts to require explicit on-chain actor auth, introduces EVM nonce tracking, and refactors the TypeScript/Stellar scripts to a multi-actor flow for tests. Changes
Sequence Diagram(s)sequenceDiagram
participant StellarAdmin as Stellar Admin
participant AdCreator as Ad Creator (Stellar)
participant OrderCreator as Order Creator (Stellar)
participant AdManager as Ad Manager Contract
participant OrderPortal as Order Portal Contract
participant EVMAdmin as EVM Admin
participant EVMOrderCreator as EVM Order Creator
StellarAdmin->>AdManager: create_ad(..., creator)
AdManager->>AdManager: creator.require_auth()\nstore ad.maker = creator
AdCreator->>AdManager: fund_ad(...)
AdManager->>AdManager: ad.maker.require_auth()\nupdate ad balance
OrderCreator->>OrderPortal: create_order(..., bridger)
OrderPortal->>OrderPortal: bridger.require_auth()\ncreate order
OrderCreator->>AdManager: lock_for_order(...)
AdManager->>AdManager: ad.maker.require_auth()\nlock funds
EVMOrderCreator->>EVMAdmin: submit proof/tx
EVMAdmin->>OrderPortal: unlock(..., ad_recipient)
OrderPortal->>OrderPortal: ad_recipient.require_auth()\nverify proof, release tokens
OrderCreator->>AdManager: unlock(..., order_recipient)
AdManager->>AdManager: order_recipient.require_auth()\nrelease ad maker funds
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/stellar/contracts/ad-manager/src/lib.rs (1)
232-275:⚠️ Potential issue | 🟠 MajorBind
creatorinto the signedcreate_adrequest.
creator.require_auth()proves who pays, but the manager pre-auth no longer proves who is allowed to become the maker. As written, any funded account that obtains the same manager signature can swap in its owncreator, front-run the intended LP, and consume thead_idfirst. Includecreatorincreate_ad_request_hashand verify that version instead.Minimal callsite change
let message = auth::create_ad_request_hash( &env, + &creator, &ad_id, &ad_token, initial_amount, order_chain_id, &ad_recipient,You'd also need the corresponding
auth::create_ad_request_hash(...)update so the creator bytes are hashed into the signed payload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/stellar/contracts/ad-manager/src/lib.rs` around lines 232 - 275, The create_ad flow currently omits binding the caller into the signed payload, allowing any funded account to reuse a manager signature with a swapped creator; update the call to auth::create_ad_request_hash in the function that builds the message so it includes the creator (e.g., pass creator.as_bytes() or its BytesN/Address representation into create_ad_request_hash), then update the auth::create_ad_request_hash signature/implementation to include and hash the creator bytes, and ensure Self::verify_request uses the new hashed message variant; keep creator.require_auth() as-is but verify against the new creator-bound hash so the manager signature is valid only for that specific creator.scripts/cross-chain-e2e/run.ts (1)
591-623:⚠️ Potential issue | 🟠 MajorSend the EVM
unlockfrom the intended non-admin actor.Line 615 still submits
unlockthrough the admin-boundevm.orderPortalinstance withnonces.next(). That means this E2E is not actually exercising the role-separated unlock path, and it will be wrong ifOrderPortal.unlock()now depends onmsg.sender. If the order creator is the intended caller, use the creator-bound instance here; if it's another actor, plumb that wallet in instead.Suggested fix if the order creator is the intended caller
- const tx = await evm.orderPortal.getFunction("unlock")( + const tx = await orderPortalAsCreator.getFunction("unlock")( unlockSig, evmAuthToken2, EVM_TIME_TO_EXPIRE, evmOrderParams, nullifierHash, targetRoot, proofHex, - { nonce: nonces.next() }, + { nonce: orderCreatorNonces.next() }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cross-chain-e2e/run.ts` around lines 591 - 623, The test is sending the unlock via the admin-bound evm.orderPortal instance and nonces.next(), so change the call to invoke OrderPortal.unlock from the intended actor's bound instance and nonce/signer instead of evmSigner/nonces.next(); locate the evm.orderPortal.getFunction("unlock") call and replace it to use the order-creator (or other intended actor) bound contract instance and that actor's nonce/signer (e.g., the creator-bound orderPortal instance and its nonce provider) so msg.sender in OrderPortal.unlock reflects the correct actor.
🧹 Nitpick comments (4)
scripts/cross-chain-e2e/lib/proof.ts (1)
163-266: Clean up Barretenberg and the temp MMR dir in afinallyblock.If anything throws after
Barretenberg.new()or after the LevelDB directory is created, this function leaks both resources and leaves stale/tmp/e2e-mmr-*state behind. Wrapping the proof pipeline intry/finallywill make failure cases much easier to debug in CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cross-chain-e2e/lib/proof.ts` around lines 163 - 266, Wrap the pipeline that starts with creating Barretenberg (Barretenberg.new()) and initializing the LevelDB/MMR (dbPath, new LevelDB(...), await db.init(), new MMR(...)) in a try/finally so resources are always cleaned: move all work (MMR append/getMerkleProof, Noir/Honk execution, proof generation, building public inputs) into the try block and call cleanup in the finally block (fs.rmSync(dbPath, { recursive: true, force: true }) and await bb.destroy()). Ensure bb and dbPath are defined in the outer scope so the finally can check and safely cleanup only if they were created.contracts/stellar/tests/integration_test.rs (1)
514-520: Use the fixture ad creator here, notadmin_addr.This setup still creates the ad as the manager account, so these tests never exercise the new
creator.require_auth()path with a non-manager maker. That leaves the multi-actor auth change largely untested at the contract-test level. Consider funding/derivingtp.ad_creatorand passing that address here instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/stellar/tests/integration_test.rs` around lines 514 - 520, The test is creating the ad with the manager account (admin_addr) instead of the fixture ad creator, so it never exercises creator.require_auth(); replace admin_addr with the fixture creator (tp.ad_creator) in the ad_manager.create_ad call and ensure you derive/fund tp.ad_creator and use the corresponding signature and pubkey for that actor (i.e., construct ad_creator_signature and ad_creator_pubkey and pass them in place of signature/admin_pubkey) so the call runs under the non-manager creator and triggers creator.require_auth().scripts/cross-chain-e2e/run.ts (2)
55-63: Fail fast on missing environment variables.The
!assertions only move the failure to whichever later call touchesundefined. A tinyrequireEnv()helper would make CI/local misconfig much easier to diagnose.One simple way to tighten this up
+function requireEnv(name: string): string { + const value = process.env[name]; + if (!value) throw new Error(`Missing required env: ${name}`); + return value; +} + -const ROOT_DIR = process.env.ROOT_DIR!; -const EVM_RPC_URL = process.env.EVM_RPC_URL!; -const EVM_ADMIN_PRIVATE_KEY = process.env.EVM_ADMIN_PRIVATE_KEY!; -const EVM_ORDER_CREATOR_PRIVATE_KEY = - process.env.EVM_ORDER_CREATOR_PRIVATE_KEY!; -const STELLAR_AD_CREATOR_ACCOUNT = process.env.STELLAR_AD_CREATOR_ACCOUNT!; -const STELLAR_ORDER_CREATOR_ACCOUNT = - process.env.STELLAR_ORDER_CREATOR_ACCOUNT!; -const AD_CREATOR_EVM_RECIPIENT = process.env.AD_CREATOR_EVM_RECIPIENT!; +const ROOT_DIR = requireEnv("ROOT_DIR"); +const EVM_RPC_URL = requireEnv("EVM_RPC_URL"); +const EVM_ADMIN_PRIVATE_KEY = requireEnv("EVM_ADMIN_PRIVATE_KEY"); +const EVM_ORDER_CREATOR_PRIVATE_KEY = requireEnv( + "EVM_ORDER_CREATOR_PRIVATE_KEY", +); +const STELLAR_AD_CREATOR_ACCOUNT = requireEnv("STELLAR_AD_CREATOR_ACCOUNT"); +const STELLAR_ORDER_CREATOR_ACCOUNT = requireEnv( + "STELLAR_ORDER_CREATOR_ACCOUNT", +); +const AD_CREATOR_EVM_RECIPIENT = requireEnv("AD_CREATOR_EVM_RECIPIENT");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cross-chain-e2e/run.ts` around lines 55 - 63, Replace the blind non-null assertions for env vars (ROOT_DIR, EVM_RPC_URL, EVM_ADMIN_PRIVATE_KEY, EVM_ORDER_CREATOR_PRIVATE_KEY, STELLAR_AD_CREATOR_ACCOUNT, STELLAR_ORDER_CREATOR_ACCOUNT, AD_CREATOR_EVM_RECIPIENT) with a fail-fast helper: implement a small requireEnv(key) that reads process.env[key], throws a descriptive Error if missing, and use requireEnv("ROOT_DIR") etc. Update where these constants are initialized to call requireEnv for each symbol so CI/local runs error immediately with a clear message rather than later with an undefined value.
403-431: Build the EVM/proof/Stellar order payloads from one source.These blocks re-encode the same order in three shapes. A later field change can update one and silently desync the Solidity call, the proof hash, and the Soroban JSON. A small canonical builder/helper would make this much harder to break.
Also applies to: 470-485
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cross-chain-e2e/run.ts` around lines 403 - 431, Create a single canonical builder (e.g., buildOrderPayload or createCanonicalOrder) that returns a base order object with the shared fields (orderChainToken, adChainToken, amount, bridger, orderRecipient, orderChainId, orderPortal, adChainId, adManager, adId, adCreator, adRecipient, salt) and use it to derive the three shapes currently constructed separately (evmOrderParams, orderParams, and the payload built at the later block around 470-485); replace the duplicated literal objects with calls to that builder and lightweight mappers to produce the EVM-specific, proof/hash-specific, and Soroban/Stellar-specific shapes so all fields originate from one source of truth (update usages of evmOrderParams and orderParams to use the new builder/mappers).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/stellar/contracts/test-token/src/lib.rs`:
- Around line 17-24: The mint function currently allows anyone to mint; modify
the contract to persist an admin/minter in the constructor and enforce auth in
mint: in __constructor(e, owner, initial_supply) store owner (or a specified
admin/minter) in contract storage (e.g., a named data key) after calling
Base::mint, and in pub fn mint(e: &Env, to: Address, amount: i128) check the
caller/sender against that stored admin/minter and return or abort if not
authorized before calling Base::mint; update any access utilities or helpers
used to read/write the admin/minter value accordingly.
In `@scripts/cross-chain-e2e/lib/stellar.ts`:
- Around line 69-80: The generateAndFundKey helper currently swallows all errors
in both try/catch blocks, masking real failures; update generateAndFundKey so
each catch captures the thrown error (e.g., catch (err)) and only suppresses it
when the error message or exit code clearly indicates the idempotent cases
("already exists" for stellar(`keys generate`) and "already funded" / equivalent
for stellar(`keys fund`) — otherwise rethrow the error; keep using stellar(...)
for command execution and return getAddress(name) as before.
In `@scripts/cross-chain-e2e/run.ts`:
- Around line 136-140: The synchronous call to getPublicKey from
import("@noble/ed25519") depends on a sha512Sync backend being configured
elsewhere (ed.etc.sha512Sync) and will break if import ordering changes; fix by
either explicitly configuring the noble ed25519 hash backend in this file before
calling getPublicKey (set ed.etc.sha512Sync to the same implementation used in
scripts/cross-chain-e2e/lib/signing.ts) or replace the synchronous call with the
asynchronous API getPublicKeyAsync and await it (use
import("@noble/ed25519").getPublicKeyAsync) so no external configuration is
required; update references to stellarAdminPubKey/stellerAdminSecretKey
accordingly.
In `@scripts/run_cross_chain_e2e.sh`:
- Around line 114-116: The script currently force-kills whatever process is
listening on $ANVIL_PORT; instead, detect the PID(s) from lsof -ti
:"$ANVIL_PORT", inspect each PID's command (e.g. via /proc/<pid>/comm or ps -o
comm= -p <pid>) and only kill when the command matches the anvil binary (or
known anvil spawners); otherwise return a non-zero error and exit (fail fast) so
the user can resolve the port conflict. Update the section around the current
lsof/kill call in run_cross_chain_e2e.sh to implement this safe-check logic
using ANVIL_PORT and the discovered PIDs before sending any kill signals.
---
Outside diff comments:
In `@contracts/stellar/contracts/ad-manager/src/lib.rs`:
- Around line 232-275: The create_ad flow currently omits binding the caller
into the signed payload, allowing any funded account to reuse a manager
signature with a swapped creator; update the call to
auth::create_ad_request_hash in the function that builds the message so it
includes the creator (e.g., pass creator.as_bytes() or its BytesN/Address
representation into create_ad_request_hash), then update the
auth::create_ad_request_hash signature/implementation to include and hash the
creator bytes, and ensure Self::verify_request uses the new hashed message
variant; keep creator.require_auth() as-is but verify against the new
creator-bound hash so the manager signature is valid only for that specific
creator.
In `@scripts/cross-chain-e2e/run.ts`:
- Around line 591-623: The test is sending the unlock via the admin-bound
evm.orderPortal instance and nonces.next(), so change the call to invoke
OrderPortal.unlock from the intended actor's bound instance and nonce/signer
instead of evmSigner/nonces.next(); locate the
evm.orderPortal.getFunction("unlock") call and replace it to use the
order-creator (or other intended actor) bound contract instance and that actor's
nonce/signer (e.g., the creator-bound orderPortal instance and its nonce
provider) so msg.sender in OrderPortal.unlock reflects the correct actor.
---
Nitpick comments:
In `@contracts/stellar/tests/integration_test.rs`:
- Around line 514-520: The test is creating the ad with the manager account
(admin_addr) instead of the fixture ad creator, so it never exercises
creator.require_auth(); replace admin_addr with the fixture creator
(tp.ad_creator) in the ad_manager.create_ad call and ensure you derive/fund
tp.ad_creator and use the corresponding signature and pubkey for that actor
(i.e., construct ad_creator_signature and ad_creator_pubkey and pass them in
place of signature/admin_pubkey) so the call runs under the non-manager creator
and triggers creator.require_auth().
In `@scripts/cross-chain-e2e/lib/proof.ts`:
- Around line 163-266: Wrap the pipeline that starts with creating Barretenberg
(Barretenberg.new()) and initializing the LevelDB/MMR (dbPath, new LevelDB(...),
await db.init(), new MMR(...)) in a try/finally so resources are always cleaned:
move all work (MMR append/getMerkleProof, Noir/Honk execution, proof generation,
building public inputs) into the try block and call cleanup in the finally block
(fs.rmSync(dbPath, { recursive: true, force: true }) and await bb.destroy()).
Ensure bb and dbPath are defined in the outer scope so the finally can check and
safely cleanup only if they were created.
In `@scripts/cross-chain-e2e/run.ts`:
- Around line 55-63: Replace the blind non-null assertions for env vars
(ROOT_DIR, EVM_RPC_URL, EVM_ADMIN_PRIVATE_KEY, EVM_ORDER_CREATOR_PRIVATE_KEY,
STELLAR_AD_CREATOR_ACCOUNT, STELLAR_ORDER_CREATOR_ACCOUNT,
AD_CREATOR_EVM_RECIPIENT) with a fail-fast helper: implement a small
requireEnv(key) that reads process.env[key], throws a descriptive Error if
missing, and use requireEnv("ROOT_DIR") etc. Update where these constants are
initialized to call requireEnv for each symbol so CI/local runs error
immediately with a clear message rather than later with an undefined value.
- Around line 403-431: Create a single canonical builder (e.g.,
buildOrderPayload or createCanonicalOrder) that returns a base order object with
the shared fields (orderChainToken, adChainToken, amount, bridger,
orderRecipient, orderChainId, orderPortal, adChainId, adManager, adId,
adCreator, adRecipient, salt) and use it to derive the three shapes currently
constructed separately (evmOrderParams, orderParams, and the payload built at
the later block around 470-485); replace the duplicated literal objects with
calls to that builder and lightweight mappers to produce the EVM-specific,
proof/hash-specific, and Soroban/Stellar-specific shapes so all fields originate
from one source of truth (update usages of evmOrderParams and orderParams to use
the new builder/mappers).
🪄 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: 7a7bb29f-3b90-4f08-bca5-76a580e65133
⛔ Files ignored due to path filters (1)
contracts/stellar/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.github/workflows/cross-chain-e2e.ymlcontracts/stellar/contracts/ad-manager/src/lib.rscontracts/stellar/contracts/order-portal/src/lib.rscontracts/stellar/contracts/test-token/Cargo.tomlcontracts/stellar/contracts/test-token/src/lib.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.rsscripts/cross-chain-e2e/lib/evm.tsscripts/cross-chain-e2e/lib/proof.tsscripts/cross-chain-e2e/lib/signing.tsscripts/cross-chain-e2e/lib/stellar.tsscripts/cross-chain-e2e/run.tsscripts/run_cross_chain_e2e.sh
💤 Files with no reviewable changes (1)
- scripts/cross-chain-e2e/lib/signing.ts
Cross-Chain E2E CI — Stellar ↔ EVM
Summary
Wires up an end-to-end integration test that deploys ProofBridge on a local
Stellar network (ad chain) and an Anvil EVM node (order chain), then drives the
full bridge flow with four distinct role-separated actors. Runs on every PR
that touches
contracts/,proof_circuits/,scripts/, or the workflowitself.
Actors
Four actors, each provisioned by the shell orchestrator and passed to the TS
runner via env vars (no inline key generation):
admin(ed25519)AdManager; signs manager pre-auth forcreate_ad/lock_for_order/unlock.0OrderPortal; signs manager pre-auth forcreateOrder/unlock.alice(Stellar key) + Anvil2address (recv-only)1key +bridger(Stellar key)All role identities are configurable via env vars (
STELLAR_ADMIN_ACCOUNT,EVM_ADMIN_PRIVATE_KEY, etc.), with Anvil prefunded keys as defaults.Flow
Verifier(with VK),MerkleManager, native XLMSAC,
AdManager. WireMerkleManagerto allowAdManageras manager.Verifier,MerkleManager,wNativeToken, testERC20,
OrderPortal. GrantMANAGER_ROLEtoOrderPortal.set_chain/set_token_routeon both sides).create_adon Stellar — ad creator is tx source; admin signs managerpre-auth. Ad creator locks AMOUNT XLM into the ad.
createOrderon EVM — order creator ismsg.sender(own wallet + noncetracker); admin signs pre-auth. Order creator pays AMOUNT test tokens.
lock_for_orderon Stellar — ad creator is tx source soad.maker.require_auth()is auto-signed.unlock— order creator is tx source soorder_recipient.require_auth()is auto-signed. Releases AMOUNT XLM tothe order creator's Stellar address.
unlock— admin submits; releases AMOUNT test tokens to the adcreator's EVM recipient.
Assertions
Open (1)→Filled (2).9*AMOUNT(minted 10×, spent 1×),ad-creator EVM recipient ends at exactly
AMOUNT.ad creator net
-AMOUNT - fees, order creator net+AMOUNT - fees.Fee budget is 1 XLM (
FEE_SLOP = 10_000_000stroops), generous ceilingfor localnet.
Running locally
Requires:
docker,stellar-cli,foundry(anvil,forge),pnpm,node 18+, Rust +wasm32v1-nonetarget.Summary by CodeRabbit
New Features
Bug Fixes
Chores