Add test for SMT root replay (#211)#236
Open
jaymhorsh wants to merge 5 commits intoAlien-Protocol:mainfrom
Open
Add test for SMT root replay (#211)#236jaymhorsh wants to merge 5 commits intoAlien-Protocol:mainfrom
jaymhorsh wants to merge 5 commits intoAlien-Protocol:mainfrom
Conversation
…ll with same old_root fails with StaleRoot
📝 WalkthroughWalkthroughThis PR adds SMT root replay protection testing in the core contract, implements the escrow contract's deposit functionality with event emission, and establishes SDK testing infrastructure with Jest configuration and GitHub Actions CI workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test / Caller
participant Contract as Escrow Contract
participant Vault as Vault State
participant Token as Token System
participant Events as Event Emitter
Test->>Contract: deposit(commitment, amount)
Contract->>Contract: Validate amount > 0
Contract->>Vault: Load config & state
Contract->>Contract: Require owner auth
Contract->>Vault: Check is_active == true
Contract->>Token: Transfer amount from owner to contract
Token-->>Contract: Balance updated
Contract->>Vault: state.balance += amount
Vault-->>Contract: State persisted
Contract->>Events: Emit DEPOSIT event(commitment, owner, amount, new_balance)
Events-->>Test: Event recorded
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
|
@jaymhorsh Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
gateway-contract/contracts/auction_contract/auction.md (1)
40-44: Consider reducing repetitive bullet phrasing for readability.Several adjacent bullets start with the same opener (“Writes” / “Err(...)”). Rewording improves scanability in a spec doc.
Also applies to: 220-223, 271-273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/auction_contract/auction.md` around lines 40 - 44, The bullets under the auction creation steps are repetitive (many start with “Writes” or “Err(...)”); consolidate and rephrase them for readability by grouping related writes into a single bullet such as “Writes seller (AuctionKey::Seller(id)), asset (AuctionKey::Asset(id)), min_bid (AuctionKey::MinBid(id)), and end_time (AuctionKey::EndTime(id)); sets status to AuctionStatus::Open (AuctionKey::Status(id)),” and apply the same refactor to the other similar blocks that reference AuctionKey::Seller/Asset/MinBid/EndTime/Status and AuctionStatus::Open (the repeated groups around the other occurrences).zk/sdk/src/__tests__/smoke.test.ts (1)
3-5: Remove redundant manual type declarations.Since
@types/jestis already declared indevDependencies(line 11 ofzk/sdk/package.json), these manual type declarations are unnecessary and incomplete. The TypeScript compiler should automatically discover Jest types from@types/jest. Remove these lines and rely on the installed type definitions.♻️ Proposed fix
import * as sdk from "../index"; -declare const describe: (name: string, fn: () => void) => void; -declare const it: (name: string, fn: () => void) => void; -declare const expect: (value: unknown) => { toBeDefined: () => void }; - describe("zk sdk smoke", () => { it("imports index without throwing", () => { expect(sdk).toBeDefined();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zk/sdk/src/__tests__/smoke.test.ts` around lines 3 - 5, Remove the manual ambient declarations for describe, it, and expect from the top of the test file; these redundant lines ("declare const describe...", "declare const it...", "declare const expect...") should be deleted so TypeScript uses the installed `@types/jest` definitions instead (no other code changes required).gateway-contract/contracts/escrow_contract/src/test.rs (1)
115-121: Strengthen the event assertion to validate DEPOSIT semantics, not just count.Current assertion only checks there is one event from
contract_id; it can still pass if the wrong event/topic is emitted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/escrow_contract/src/test.rs` around lines 115 - 121, The current test only counts events from contract_id; instead filter env.events().all() by both the event contract and the expected DEPOSIT topic/pattern (e.g., check the event's topic string or discriminant) and assert that exactly one matching DEPOSIT event exists; additionally decode/inspect the matched event payload (using the same event parsing used elsewhere in tests) to assert key semantics like depositor address and amount. Locate the existing deposit_events variable and replace its filter closure to include the topic check (and then decode the event) before asserting the semantics rather than only counting events from contract_id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gateway-contract/contracts/auction_contract/auction.md`:
- Around line 1-329: The PR added an auction contract spec unrelated to the
required SMT stale-root replay work; to fix, replace or split this payload so
the PR contains the test and docs requested by issue `#211`: add the test function
named test_register_resolver_stale_root_after_first_registration to the existing
core contract test suite (locate the test module where other resolver/stale-root
tests live and implement the sequencing assertions described by the issue), and
update gateway-contract/contracts/core_contract/core.md to document the
sequencing behavior and expected stale-root replay behavior (include exact
steps/assertions used by the new test). If the auction documentation must
remain, move it to a separate PR or branch so this PR only contains the new test
and the core.md sequencing documentation.
In `@gateway-contract/contracts/escrow_contract/src/events.rs`:
- Around line 169-181: The deposit function currently uses the deprecated
env.events().publish API; replace it by defining a new contract event type
(e.g., struct DepositEvent) annotated with #[contractevent] that contains the
same fields (commitment: BytesN<32> as a topic if needed, and owner: Address,
amount: i128, new_balance: i128 as data), then update the deposit function to
construct that DepositEvent and call .publish(env) instead of
env.events().publish(...); ensure the event struct name and field names match
existing conventions used by SchedulePayEvent/PayExecEvent/etc. so the rest of
the code can find DepositEvent.publish(env).
In `@gateway-contract/contracts/escrow_contract/src/lib.rs`:
- Around line 133-134: The unchecked addition state.balance += amount can
overflow; replace it with a checked add and handle the overflow path: compute
new_balance = state.balance.checked_add(amount) and if Some(v) set state.balance
= v and call write_vault_state(&env, &commitment, &state), otherwise
return/abort with an explicit error (e.g., Err/contract trap or a specific
VaultBalanceOverflow error) so the deposit is rejected; update the handler that
currently mutates state.balance and ensure the overflow branch does not call
write_vault_state.
- Around line 130-131: The token transfer call currently passes a borrowed
address; update the call in Escrow contract so token::Client::transfer receives
env.current_contract_address() by value (not &env.current_contract_address()),
i.e., remove the needless borrow when invoking token_client.transfer with
&config.owner, env.current_contract_address(), &amount so the second parameter
matches the transfer signature that accepts impl Into<MuxedAddress>.
In `@gateway-contract/contracts/escrow_contract/src/test.rs`:
- Around line 88-183: This diff adds escrow deposit tests but misses the
required SMT stale-root replay acceptance items: add the test function named
test_register_resolver_stale_root_after_first_registration that reproduces the
stale-root replay scenario against the core contract (exercise register_resolver
/ resolve flows and assert the contract rejects a stale-root replay after the
first successful registration), and also update the core.md sequencing docs to
describe the expected registration ordering and stale-root replay protections;
implement the test using the same test harness conventions (Env, mock_all_auths,
contract client helpers) as other tests and ensure assertions compare the
returned Error::from_contract_error variant for the stale-root case.
In `@zk/sdk/package.json`:
- Around line 1-17: The PR contains unrelated ZK SDK package changes (see
package.json with name "@alien-gateway/zk-sdk") while the issue and title state
it should add a test for SMT root replay protection in
gateway-contract/contracts/core_contract/src/test.rs and documentation in
core.md; either revert or remove the ZK SDK additions from this branch and
ensure the intended changes are present: add the SMT root replay protection test
in function/file gateway-contract/contracts/core_contract/src/test.rs and update
sequencing requirements in core.md, or split the ZK SDK test-infrastructure work
into a separate PR so this one only contains the test and docs referenced in
issue `#211`.
- Around line 10-16: The devDependencies in package.json are outdated; update
the versions for "typescript", "jest", "@types/jest", and "ts-jest" under the
devDependencies object to the suggested newer releases (typescript to 6.0.2,
jest to 30.2.0, `@types/jest` to 30.0.0, and ts-jest to 29.4.6) and then run a
test install (npm/yarn) and the test suite to confirm compatibility; ensure
package.json's devDependencies keys remain unchanged and update any related
tsconfig/jest config if type/runner changes surface during testing.
---
Nitpick comments:
In `@gateway-contract/contracts/auction_contract/auction.md`:
- Around line 40-44: The bullets under the auction creation steps are repetitive
(many start with “Writes” or “Err(...)”); consolidate and rephrase them for
readability by grouping related writes into a single bullet such as “Writes
seller (AuctionKey::Seller(id)), asset (AuctionKey::Asset(id)), min_bid
(AuctionKey::MinBid(id)), and end_time (AuctionKey::EndTime(id)); sets status to
AuctionStatus::Open (AuctionKey::Status(id)),” and apply the same refactor to
the other similar blocks that reference
AuctionKey::Seller/Asset/MinBid/EndTime/Status and AuctionStatus::Open (the
repeated groups around the other occurrences).
In `@gateway-contract/contracts/escrow_contract/src/test.rs`:
- Around line 115-121: The current test only counts events from contract_id;
instead filter env.events().all() by both the event contract and the expected
DEPOSIT topic/pattern (e.g., check the event's topic string or discriminant) and
assert that exactly one matching DEPOSIT event exists; additionally
decode/inspect the matched event payload (using the same event parsing used
elsewhere in tests) to assert key semantics like depositor address and amount.
Locate the existing deposit_events variable and replace its filter closure to
include the topic check (and then decode the event) before asserting the
semantics rather than only counting events from contract_id.
In `@zk/sdk/src/__tests__/smoke.test.ts`:
- Around line 3-5: Remove the manual ambient declarations for describe, it, and
expect from the top of the test file; these redundant lines ("declare const
describe...", "declare const it...", "declare const expect...") should be
deleted so TypeScript uses the installed `@types/jest` definitions instead (no
other code changes required).
🪄 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: e0616246-b587-4ded-b39c-1e23de0a2822
📒 Files selected for processing (10)
.github/workflows/sdk_tests.ymlgateway-contract/contracts/auction_contract/auction.mdgateway-contract/contracts/core_contract/core.mdgateway-contract/contracts/core_contract/src/test.rsgateway-contract/contracts/escrow_contract/src/events.rsgateway-contract/contracts/escrow_contract/src/lib.rsgateway-contract/contracts/escrow_contract/src/test.rszk/sdk/jest.config.tszk/sdk/package.jsonzk/sdk/src/__tests__/smoke.test.ts
Comment on lines
+1
to
+329
| # Auction Contract Specification | ||
|
|
||
| The Auction contract implements two auction flows that coexist in the same contract: | ||
|
|
||
| - A singleton username auction flow keyed by instance storage (`close_auction`, `claim_username`). | ||
| - An ID-indexed auction flow keyed by persistent storage (`create_auction`, `place_bid`, `close_auction_by_id`, `claim`). | ||
|
|
||
| Both flows use Soroban auth (`require_auth`) and ledger timestamp checks to enforce access and timing constraints. | ||
|
|
||
| ## Public Entry Points | ||
|
|
||
| ### Function: `create_auction` | ||
|
|
||
| Creates a new auction identified by `id`. | ||
|
|
||
| #### Interface | ||
|
|
||
| ```rust | ||
| pub fn create_auction( | ||
| env: Env, | ||
| id: u32, | ||
| seller: Address, | ||
| asset: Address, | ||
| min_bid: i128, | ||
| end_time: u64, | ||
| ) | ||
| ``` | ||
|
|
||
| #### Authorization | ||
|
|
||
| - `seller.require_auth()` must succeed. | ||
|
|
||
| #### Requirements & Validation | ||
|
|
||
| - Auction ID must not already exist (`storage::auction_exists(&env, id) == false`). | ||
| - If ID already exists, function aborts with `AuctionError::AuctionNotOpen`. | ||
|
|
||
| #### State Transitions | ||
|
|
||
| - Writes `seller` to `AuctionKey::Seller(id)`. | ||
| - Writes bidding token `asset` to `AuctionKey::Asset(id)`. | ||
| - Writes `min_bid` to `AuctionKey::MinBid(id)`. | ||
| - Writes `end_time` to `AuctionKey::EndTime(id)`. | ||
| - Sets status to `AuctionStatus::Open` in `AuctionKey::Status(id)`. | ||
|
|
||
| #### Events Emitted | ||
|
|
||
| - None in current implementation. | ||
|
|
||
| #### Errors | ||
|
|
||
| - `AuctionError::AuctionNotOpen` when `id` already exists. | ||
| - Host auth failure if `seller` does not authorize. | ||
|
|
||
| #### Edge Cases | ||
|
|
||
| - **Duplicate auction ID**: explicitly rejected (panic with `AuctionNotOpen`). | ||
| - **No min/end validation**: contract currently does not enforce `min_bid > 0` or `end_time > now` at creation. | ||
|
|
||
| ### Function: `place_bid` | ||
|
|
||
| Places a bid on an existing auction and refunds the previously highest bidder. | ||
|
|
||
| #### Interface | ||
|
|
||
| ```rust | ||
| pub fn place_bid(env: Env, id: u32, bidder: Address, amount: i128) | ||
| ``` | ||
|
|
||
| #### Authorization | ||
|
|
||
| - `bidder.require_auth()` must succeed. | ||
|
|
||
| #### Requirements & Validation | ||
|
|
||
| - Auction must still be open by time: `env.ledger().timestamp() < auction_end_time`. | ||
| - Bid must satisfy both: | ||
| - `amount >= min_bid` | ||
| - `amount > highest_bid` | ||
|
|
||
| If timing check fails, function aborts with `AuctionError::AuctionNotOpen`. | ||
| If bid floor/outbid check fails, function aborts with `AuctionError::BidTooLow`. | ||
|
|
||
| #### State Transitions | ||
|
|
||
| 1. Transfers `amount` of auction asset token from `bidder` to contract. | ||
| 2. If previous highest bidder exists, transfers prior `highest_bid` from contract back to that bidder. | ||
| 3. Updates `AuctionKey::HighestBidder(id)` to current `bidder`. | ||
| 4. Updates `AuctionKey::HighestBid(id)` to `amount`. | ||
|
|
||
| #### Events Emitted | ||
|
|
||
| - None in current implementation. | ||
|
|
||
| #### Errors | ||
|
|
||
| - `AuctionError::AuctionNotOpen` when auction time window is closed. | ||
| - `AuctionError::BidTooLow` when bid is below min or not strictly above current highest. | ||
| - Host auth failure if `bidder` does not authorize. | ||
| - Token transfer failure if token contract transfer preconditions are not met. | ||
|
|
||
| #### Edge Cases | ||
|
|
||
| - **Zero-bid history**: first bid is accepted if it meets `min_bid` and auction is still open. | ||
| - **Equal-to-highest bid**: rejected (`amount <= highest_bid` path). | ||
| - **Late bid at exact end timestamp**: rejected because condition is `timestamp >= end_time`. | ||
|
|
||
| ### Function: `close_auction_by_id` | ||
|
|
||
| Closes an ID-indexed auction once its end time has passed. | ||
|
|
||
| #### Interface | ||
|
|
||
| ```rust | ||
| pub fn close_auction_by_id(env: Env, id: u32) | ||
| ``` | ||
|
|
||
| #### Authorization | ||
|
|
||
| - No explicit caller auth in current implementation. | ||
|
|
||
| #### Requirements & Validation | ||
|
|
||
| - Current ledger timestamp must be at least the auction end time. | ||
| - If `timestamp < end_time`, function aborts with `AuctionError::AuctionNotClosed`. | ||
|
|
||
| #### State Transitions | ||
|
|
||
| - Sets `AuctionKey::Status(id)` to `AuctionStatus::Closed`. | ||
|
|
||
| #### Events Emitted | ||
|
|
||
| - None in current implementation. | ||
|
|
||
| #### Errors | ||
|
|
||
| - `AuctionError::AuctionNotClosed` when called before end time. | ||
|
|
||
| #### Edge Cases | ||
|
|
||
| - **Early close attempt**: rejected with `AuctionNotClosed`. | ||
| - **No bids placed**: still closes successfully; later claim semantics determine payout/ownership behavior. | ||
|
|
||
| ### Function: `close_auction` | ||
|
|
||
| Closes the singleton username auction flow and emits closure metadata. | ||
|
|
||
| #### Interface | ||
|
|
||
| ```rust | ||
| pub fn close_auction( | ||
| env: Env, | ||
| username_hash: BytesN<32>, | ||
| ) -> Result<(), AuctionError> | ||
| ``` | ||
|
|
||
| #### Authorization | ||
|
|
||
| - No explicit caller auth in current implementation. | ||
|
|
||
| #### Requirements & Validation | ||
|
|
||
| - Current instance `status` must be `AuctionStatus::Open`. | ||
| - Current ledger timestamp must be at least instance `end_time`. | ||
|
|
||
| Returns: | ||
|
|
||
| - `Err(AuctionError::AuctionNotOpen)` if status is not `Open`. | ||
| - `Err(AuctionError::AuctionNotClosed)` if called before end time. | ||
|
|
||
| #### State Transitions | ||
|
|
||
| - Sets instance `DataKey::Status` to `AuctionStatus::Closed`. | ||
| - Reads instance `DataKey::HighestBidder` and `DataKey::HighestBid` for event payload. | ||
|
|
||
| #### Events Emitted | ||
|
|
||
| - Emits `AuctionClosedEvent` via `emit_auction_closed` with: | ||
| - `username_hash` | ||
| - `winner: Option<Address>` | ||
| - `winning_bid: u128` | ||
|
|
||
| #### Errors | ||
|
|
||
| - `AuctionError::AuctionNotOpen` | ||
| - `AuctionError::AuctionNotClosed` | ||
|
|
||
| #### Edge Cases | ||
|
|
||
| - **Zero bids**: event emits `winner = None`, `winning_bid = 0`. | ||
| - **Repeated close**: second close call fails with `AuctionNotOpen` because status is no longer `Open`. | ||
|
|
||
| ### Function: `claim_username` | ||
|
|
||
| Allows winner of singleton username auction to deploy/claim the username via factory contract. | ||
|
|
||
| #### Interface | ||
|
|
||
| ```rust | ||
| pub fn claim_username( | ||
| env: Env, | ||
| username_hash: BytesN<32>, | ||
| claimer: Address, | ||
| ) -> Result<(), AuctionError> | ||
| ``` | ||
|
|
||
| #### Authorization | ||
|
|
||
| - `claimer.require_auth()` must succeed. | ||
|
|
||
| #### Requirements & Validation | ||
|
|
||
| - Instance status must not already be `Claimed`. | ||
| - Instance status must be `Closed`. | ||
| - `claimer` must equal stored highest bidder. | ||
| - Factory contract address must exist in `DataKey::FactoryContract`. | ||
|
|
||
| Returns: | ||
|
|
||
| - `Err(AuctionError::AlreadyClaimed)` if already claimed. | ||
| - `Err(AuctionError::NotClosed)` if not closed. | ||
| - `Err(AuctionError::NotWinner)` if caller is not winner. | ||
| - `Err(AuctionError::NoFactoryContract)` if factory address is missing. | ||
|
|
||
| #### State Transitions | ||
|
|
||
| 1. Sets instance `DataKey::Status` to `AuctionStatus::Claimed`. | ||
| 2. Invokes factory contract method `deploy_username(username_hash, claimer)`. | ||
|
|
||
| #### Events Emitted | ||
|
|
||
| - Emits `UsernameClaimedEvent` via `emit_username_claimed` with: | ||
| - `username_hash` | ||
| - `claimer` | ||
|
|
||
| #### Errors | ||
|
|
||
| - `AuctionError::AlreadyClaimed` | ||
| - `AuctionError::NotClosed` | ||
| - `AuctionError::NotWinner` | ||
| - `AuctionError::NoFactoryContract` | ||
| - Host auth failure if `claimer` does not authorize. | ||
|
|
||
| #### Edge Cases | ||
|
|
||
| - **No bids**: no highest bidder exists, so claim fails with `NotWinner`. | ||
| - **Claim race**: first valid claim sets status to `Claimed`; subsequent claims fail with `AlreadyClaimed`. | ||
|
|
||
| ### Function: `claim` | ||
|
|
||
| Finalizes an ID-indexed auction by allowing the winner to release funds to seller. | ||
|
|
||
| #### Interface | ||
|
|
||
| ```rust | ||
| pub fn claim(env: Env, id: u32, claimant: Address) | ||
| ``` | ||
|
|
||
| #### Authorization | ||
|
|
||
| - `claimant.require_auth()` must succeed. | ||
|
|
||
| #### Requirements & Validation | ||
|
|
||
| - Auction status for `id` must be `AuctionStatus::Closed`. | ||
| - Auction must not already be claimed (`auction_is_claimed == false`). | ||
| - `claimant` must equal current highest bidder. | ||
|
|
||
| Function aborts with: | ||
|
|
||
| - `AuctionError::NotClosed` when status is not closed. | ||
| - `AuctionError::AlreadyClaimed` when already claimed. | ||
| - `AuctionError::NotWinner` when claimant is not highest bidder. | ||
|
|
||
| #### State Transitions | ||
|
|
||
| 1. Reads token `asset`, `winning_bid`, and `seller` for auction `id`. | ||
| 2. Transfers `winning_bid` from contract to `seller`. | ||
| 3. Marks `AuctionKey::Claimed(id)` as `true`. | ||
|
|
||
| #### Events Emitted | ||
|
|
||
| - None in current implementation. | ||
|
|
||
| #### Errors | ||
|
|
||
| - `AuctionError::NotClosed` | ||
| - `AuctionError::AlreadyClaimed` | ||
| - `AuctionError::NotWinner` | ||
| - Host auth failure if `claimant` does not authorize. | ||
| - Token transfer failure if transfer cannot be completed. | ||
|
|
||
| #### Edge Cases | ||
|
|
||
| - **No bids**: highest bidder is `None`; all claim attempts fail with `NotWinner`. | ||
| - **Double claim**: second successful claimant attempt is blocked by `AlreadyClaimed`. | ||
|
|
||
| ## Error Variants (Contract-Wide) | ||
|
|
||
| Defined in `errors.rs`: | ||
|
|
||
| - `NotWinner` | ||
| - `AlreadyClaimed` | ||
| - `NotClosed` | ||
| - `NoFactoryContract` | ||
| - `Unauthorized` | ||
| - `InvalidState` | ||
| - `BidTooLow` | ||
| - `AuctionNotOpen` | ||
| - `AuctionNotClosed` | ||
|
|
||
| Note: `Unauthorized` and `InvalidState` are defined but not currently emitted by the public entry points above; authorization failures are enforced primarily through host-level `require_auth`. | ||
|
|
||
| ## Event Types (Contract-Wide) | ||
|
|
||
| Defined in `events.rs`: | ||
|
|
||
| - `AuctionCreatedEvent` | ||
| - `BidPlacedEvent` | ||
| - `AuctionClosedEvent` | ||
| - `UsernameClaimedEvent` | ||
| - `BidRefundedEvent` | ||
|
|
||
| Current emission in public entry points: | ||
|
|
||
| - `close_auction` emits `AuctionClosedEvent`. | ||
| - `claim_username` emits `UsernameClaimedEvent`. | ||
| - Other listed entry points currently emit no events. | ||
|
|
There was a problem hiding this comment.
PR scope appears misaligned with the stated objective (SMT stale-root replay test).
This change set documents the auction contract, but the objective for issue #211 requires adding test_register_resolver_stale_root_after_first_registration and documenting sequencing in gateway-contract/contracts/core_contract/core.md. As-is, the acceptance criteria look unmet in this PR payload.
🧰 Tools
🪛 LanguageTool
[style] ~42-~42: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...n asset to AuctionKey::Asset(id). - Writes min_bid to AuctionKey::MinBid(id). ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~43-~43: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...min_bidtoAuctionKey::MinBid(id). - Writes end_timetoAuctionKey::EndTime(id)`...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~222-~222: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...losed. - Err(AuctionError::NotWinner) if caller is not winner. - `Err(AuctionErr...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~223-~223: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... Err(AuctionError::NoFactoryContract) if factory address is missing. #### State...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~273-~273: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...dy claimed. - AuctionError::NotWinner when claimant is not highest bidder. #### S...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway-contract/contracts/auction_contract/auction.md` around lines 1 - 329,
The PR added an auction contract spec unrelated to the required SMT stale-root
replay work; to fix, replace or split this payload so the PR contains the test
and docs requested by issue `#211`: add the test function named
test_register_resolver_stale_root_after_first_registration to the existing core
contract test suite (locate the test module where other resolver/stale-root
tests live and implement the sequencing assertions described by the issue), and
update gateway-contract/contracts/core_contract/core.md to document the
sequencing behavior and expected stale-root replay behavior (include exact
steps/assertions used by the new test). If the auction documentation must
remain, move it to a separate PR or branch so this PR only contains the new test
and the core.md sequencing documentation.
Comment on lines
+130
to
+131
| let token_client = token::Client::new(&env, &config.token); | ||
| token_client.transfer(&config.owner, &env.current_contract_address(), &amount); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all needless-borrow candidates for current_contract_address.
rg -n --type rust '&env\.current_contract_address\(\)'Repository: Alien-Protocol/Alien-Gateway
Length of output: 869
🏁 Script executed:
# First, let's look at the escrow_contract lib.rs around line 131 to understand the context
sed -n '125,135p' gateway-contract/contracts/escrow_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 507
🏁 Script executed:
# Let's also check the full scope of the transfer calls and token client setup
sed -n '125,155p' gateway-contract/contracts/escrow_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 1340
🏁 Script executed:
# Check what token module is being used - look for imports
head -50 gateway-contract/contracts/escrow_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 1862
🏁 Script executed:
# Check if there's any custom token module definition in the codebase
fd -e rs | xargs rg -l 'impl.*transfer' | head -20Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Look for soroban-sdk version in Cargo.toml to understand what version is being used
find . -name "Cargo.toml" -exec grep -H "soroban-sdk" {} \;Repository: Alien-Protocol/Alien-Gateway
Length of output: 1049
🏁 Script executed:
# Check the token client type to understand what transfer expects
rg -A5 'token::Client::new' gateway-contract/contracts/escrow_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 1028
🏁 Script executed:
# Search for soroban-sdk documentation or check what Address type is
# Let's look for any type definitions or imports that might give us clues
rg 'Address' gateway-contract/contracts/escrow_contract/src/lib.rs -B2 -A2 | head -30Repository: Alien-Protocol/Alien-Gateway
Length of output: 1330
🌐 Web query:
soroban-sdk token::Client transfer method signature Address parameter
💡 Result:
The transfer method signature of soroban_sdk::token::TokenClient is: rust pub fn transfer( &self, from: &Address, to: impl Into, amount: &i128, ) The first parameter is from: &Address, which specifies the source address holding the tokens to be transferred. The second parameter to accepts values convertible into MuxedAddress (including Address), representing the destination. This is confirmed in the official Soroban SDK documentation for TokenClient.
Citations:
- 1: https://docs.rs/soroban-sdk/latest/soroban_sdk/token/struct.TokenClient.html
- 2: https://docs.rs/soroban-sdk/latest/soroban_sdk/token/struct.StellarAssetClient.html
- 3: https://docs.rs/soroban-token-sdk/latest/soroban_token_sdk/_migrating/v23_token_transfer/index.html
🏁 Script executed:
# Let's check if there are compilation errors by looking at CI logs or checking other transfer patterns
# First, check all token_client.transfer calls in the codebase to see patterns
rg -B2 -A2 'token_client\.transfer\(' gateway-contract/contracts/Repository: Alien-Protocol/Alien-Gateway
Length of output: 2066
🏁 Script executed:
# Let's check if this matches a Soroban SDK pattern issue
# Look at how Address is typically used as a parameter
rg 'to: &env' gateway-contract/ -A1 -B1Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
Remove needless borrow in token transfer call.
Line 131 should pass env.current_contract_address() directly instead of borrowing it. The transfer method's second parameter accepts impl Into<MuxedAddress>, which expects an owned Address value, not a reference.
Proposed fix
- token_client.transfer(&config.owner, &env.current_contract_address(), &amount);
+ token_client.transfer(&config.owner, env.current_contract_address(), &amount);🧰 Tools
🪛 GitHub Actions: Checks
[error] 131-131: Clippy failed: needless_borrows_for_generic_args — the borrowed expression implements the required traits. Help suggests replacing &env.current_contract_address() usage with env.current_contract_address().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway-contract/contracts/escrow_contract/src/lib.rs` around lines 130 -
131, The token transfer call currently passes a borrowed address; update the
call in Escrow contract so token::Client::transfer receives
env.current_contract_address() by value (not &env.current_contract_address()),
i.e., remove the needless borrow when invoking token_client.transfer with
&config.owner, env.current_contract_address(), &amount so the second parameter
matches the transfer signature that accepts impl Into<MuxedAddress>.
Comment on lines
+133
to
+134
| state.balance += amount; | ||
| write_vault_state(&env, &commitment, &state); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether balance updates in escrow use checked arithmetic.
rg -n --type rust 'balance\s*[\+\-]=|checked_add\(|checked_sub\(' gateway-contract/contracts/escrow_contract/srcRepository: Alien-Protocol/Alien-Gateway
Length of output: 524
🏁 Script executed:
cat -n gateway-contract/contracts/escrow_contract/src/lib.rs | sed -n '125,145p'Repository: Alien-Protocol/Alien-Gateway
Length of output: 1043
🏁 Script executed:
cat -n gateway-contract/contracts/escrow_contract/src/lib.rs | sed -n '185,205p'Repository: Alien-Protocol/Alien-Gateway
Length of output: 790
🏁 Script executed:
cat -n gateway-contract/contracts/escrow_contract/src/lib.rs | sed -n '400,415p'Repository: Alien-Protocol/Alien-Gateway
Length of output: 647
🏁 Script executed:
rg -n "struct.*State|balance.*:" gateway-contract/contracts/escrow_contract/src --type rust -A 3 | head -30Repository: Alien-Protocol/Alien-Gateway
Length of output: 2120
🏁 Script executed:
cat -n gateway-contract/contracts/escrow_contract/src/lib.rs | sed -n '370,410p'Repository: Alien-Protocol/Alien-Gateway
Length of output: 1864
🏁 Script executed:
cat -n gateway-contract/contracts/escrow_contract/src/lib.rs | sed -n '100,138p'Repository: Alien-Protocol/Alien-Gateway
Length of output: 1852
Fix unchecked addition overflow in vault balance deposit.
Line 133 uses unchecked addition (state.balance += amount). Even though amount > 0 is validated, the addition can overflow when state.balance approaches i128::MAX. For example: if state.balance = i128::MAX - 1 and amount = 10, the operation overflows and silently wraps, corrupting the balance.
Use checked arithmetic to detect and handle overflow:
Proposed fix
- state.balance += amount;
+ state.balance = state
+ .balance
+ .checked_add(amount)
+ .unwrap_or_else(|| panic_with_error!(&env, EscrowError::InvalidAmount));
write_vault_state(&env, &commitment, &state);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| state.balance += amount; | |
| write_vault_state(&env, &commitment, &state); | |
| state.balance = state | |
| .balance | |
| .checked_add(amount) | |
| .unwrap_or_else(|| panic_with_error!(&env, EscrowError::InvalidAmount)); | |
| write_vault_state(&env, &commitment, &state); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway-contract/contracts/escrow_contract/src/lib.rs` around lines 133 -
134, The unchecked addition state.balance += amount can overflow; replace it
with a checked add and handle the overflow path: compute new_balance =
state.balance.checked_add(amount) and if Some(v) set state.balance = v and call
write_vault_state(&env, &commitment, &state), otherwise return/abort with an
explicit error (e.g., Err/contract trap or a specific VaultBalanceOverflow
error) so the deposit is rejected; update the handler that currently mutates
state.balance and ensure the overflow branch does not call write_vault_state.
Comment on lines
+88
to
+183
| #[test] | ||
| fn test_deposit_success() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
| let (contract_id, client, token, _token_admin, from, _to) = setup_test(&env); | ||
|
|
||
| let owner = Address::generate(&env); | ||
| create_vault(&env, &contract_id, &from, &owner, &token, 0); | ||
|
|
||
| let token_admin_client = StellarAssetClient::new(&env, &token); | ||
| token_admin_client.mint(&owner, &500); | ||
|
|
||
| client.deposit(&from, &200); | ||
|
|
||
| env.as_contract(&contract_id, || { | ||
| let state: VaultState = env | ||
| .storage() | ||
| .persistent() | ||
| .get(&DataKey::VaultState(from.clone())) | ||
| .unwrap(); | ||
| assert_eq!(state.balance, 200); | ||
| }); | ||
|
|
||
| let token_client = TokenClient::new(&env, &token); | ||
| assert_eq!(token_client.balance(&owner), 300); | ||
| assert_eq!(token_client.balance(&contract_id), 200); | ||
|
|
||
| let deposit_events = env | ||
| .events() | ||
| .all() | ||
| .iter() | ||
| .filter(|(event_contract, _, _)| event_contract == &contract_id) | ||
| .count(); | ||
| assert_eq!(deposit_events, 1); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_deposit_zero_amount_rejected() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
| let (contract_id, client, token, _token_admin, from, _to) = setup_test(&env); | ||
|
|
||
| create_vault( | ||
| &env, | ||
| &contract_id, | ||
| &from, | ||
| &Address::generate(&env), | ||
| &token, | ||
| 0, | ||
| ); | ||
|
|
||
| let result = client.try_deposit(&from, &0); | ||
| assert!(matches!( | ||
| result, | ||
| Err(Ok(err)) if err == Error::from_contract_error(EscrowError::InvalidAmount as u32) | ||
| )); | ||
| } | ||
|
|
||
| #[test] | ||
| #[should_panic] | ||
| fn test_deposit_non_owner_panics() { | ||
| let env = Env::default(); | ||
| let (contract_id, client, token, _token_admin, from, _to) = setup_test(&env); | ||
|
|
||
| let owner = Address::generate(&env); | ||
| create_vault(&env, &contract_id, &from, &owner, &token, 0); | ||
|
|
||
| client.deposit(&from, &10); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_deposit_inactive_vault_rejected() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
| let (contract_id, client, token, _token_admin, from, _to) = setup_test(&env); | ||
|
|
||
| let owner = Address::generate(&env); | ||
| create_vault(&env, &contract_id, &from, &owner, &token, 0); | ||
|
|
||
| env.as_contract(&contract_id, || { | ||
| let state = VaultState { | ||
| balance: 0, | ||
| is_active: false, | ||
| }; | ||
| env.storage() | ||
| .persistent() | ||
| .set(&DataKey::VaultState(from.clone()), &state); | ||
| }); | ||
|
|
||
| let result = client.try_deposit(&from, &10); | ||
| assert!(matches!( | ||
| result, | ||
| Err(Ok(err)) if err == Error::from_contract_error(EscrowError::VaultInactive as u32) | ||
| )); | ||
| } | ||
|
|
There was a problem hiding this comment.
PR objective mismatch: required SMT stale-root replay test/docs are not covered in this diff.
Issue #211 acceptance requires test_register_resolver_stale_root_after_first_registration plus core.md sequencing docs (core contract), but this changed segment adds escrow deposit tests instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway-contract/contracts/escrow_contract/src/test.rs` around lines 88 -
183, This diff adds escrow deposit tests but misses the required SMT stale-root
replay acceptance items: add the test function named
test_register_resolver_stale_root_after_first_registration that reproduces the
stale-root replay scenario against the core contract (exercise register_resolver
/ resolve flows and assert the contract rejects a stale-root replay after the
first successful registration), and also update the core.md sequencing docs to
describe the expected registration ordering and stale-root replay protections;
implement the test using the same test harness conventions (Env, mock_all_auths,
contract client helpers) as other tests and ensure assertions compare the
returned Error::from_contract_error variant for the stale-root case.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
gateway-contract/contracts/escrow_contract/src/test.rs (1)
88-183:⚠️ Potential issue | 🟠 MajorPR acceptance criteria for Issue
#211are still not addressed in this diff.This segment adds escrow
deposittests, but the required items are still the core stale-root replay test (test_register_resolver_stale_root_after_first_registration) and sequencing docs ingateway-contract/contracts/core_contract/core.md.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/escrow_contract/src/test.rs` around lines 88 - 183, The PR missed adding the required stale-root replay test and sequencing docs: add a new unit test named test_register_resolver_stale_root_after_first_registration (mirroring style of existing tests like test_deposit_success) that reproduces the stale-root replay scenario for resolver registration and asserts the contract rejects a second registration using an old root, and update the sequencing documentation in core.md to describe the expected registration ordering and stale-root handling; locate and implement the test in the same test module that contains Vault and deposit tests (use helper functions like setup_test/create_vault as needed) and update core.md with a short section explaining the sequence guarantees, how stale roots are detected, and the expected failure behavior when a replay occurs.
🧹 Nitpick comments (1)
gateway-contract/contracts/escrow_contract/src/test.rs (1)
115-122: Strengthen the DEPOSIT event assertion to validate payload, not only count.At Line 115–Line 121, counting contract events can pass even with incorrect topic/data. Assert the exact
(DEPOSIT, commitment)topic and(owner, amount, new_balance)payload.Proposed tightening
+use soroban_sdk::{symbol_short, IntoVal}; ... - let deposit_events = env - .events() - .all() - .iter() - .filter(|(event_contract, _, _)| event_contract == &contract_id) - .count(); - assert_eq!(deposit_events, 1); + let event = env + .events() + .all() + .iter() + .find(|(event_contract, topics, _)| { + event_contract == &contract_id + && *topics == (symbol_short!("DEPOSIT"), from.clone()).into_val(&env) + }) + .expect("missing DEPOSIT event"); + assert_eq!( + event.2, + (owner.clone(), 200_i128, 200_i128).into_val(&env) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/escrow_contract/src/test.rs` around lines 115 - 122, Replace the loose count assertion with a strict check that the emitted event for contract_id has the expected topic and payload: iterate env.events().all(), filter by event_contract == &contract_id, find the event whose topic equals (DEPOSIT, commitment) and then assert the event payload equals (owner, amount, new_balance); use the existing variables contract_id, DEPOSIT, commitment, owner, amount, and new_balance to match both topic and data and fail the test if no exact match is found instead of just counting events.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@gateway-contract/contracts/escrow_contract/src/test.rs`:
- Around line 88-183: The PR missed adding the required stale-root replay test
and sequencing docs: add a new unit test named
test_register_resolver_stale_root_after_first_registration (mirroring style of
existing tests like test_deposit_success) that reproduces the stale-root replay
scenario for resolver registration and asserts the contract rejects a second
registration using an old root, and update the sequencing documentation in
core.md to describe the expected registration ordering and stale-root handling;
locate and implement the test in the same test module that contains Vault and
deposit tests (use helper functions like setup_test/create_vault as needed) and
update core.md with a short section explaining the sequence guarantees, how
stale roots are detected, and the expected failure behavior when a replay
occurs.
---
Nitpick comments:
In `@gateway-contract/contracts/escrow_contract/src/test.rs`:
- Around line 115-122: Replace the loose count assertion with a strict check
that the emitted event for contract_id has the expected topic and payload:
iterate env.events().all(), filter by event_contract == &contract_id, find the
event whose topic equals (DEPOSIT, commitment) and then assert the event payload
equals (owner, amount, new_balance); use the existing variables contract_id,
DEPOSIT, commitment, owner, amount, and new_balance to match both topic and data
and fail the test if no exact match is found instead of just counting events.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ccbd118-d69e-4666-acaa-3cc7eaf7daf3
📒 Files selected for processing (3)
gateway-contract/contracts/core_contract/src/test.rsgateway-contract/contracts/escrow_contract/src/test.rszk/sdk/package.json
✅ Files skipped from review due to trivial changes (1)
- zk/sdk/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- gateway-contract/contracts/core_contract/src/test.rs
ryzen-xp
added
Blocked
CI Failed
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
2 participants
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.
Close #211
Summary by CodeRabbit
New Features
Tests
Documentation
Chores