Add Jest configuration and CI workflow for SDK unit tests #234
Open
jaymhorsh wants to merge 6 commits intoAlien-Protocol:mainfrom
Open
Add Jest configuration and CI workflow for SDK unit tests #234jaymhorsh wants to merge 6 commits intoAlien-Protocol:mainfrom
jaymhorsh wants to merge 6 commits intoAlien-Protocol:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR adds an Escrow Changes
Sequence DiagramsequenceDiagram
participant Owner as Owner
participant Escrow as EscrowContract
participant Token as TokenContract
participant Storage as Storage
participant Events as EventLog
Owner->>Escrow: deposit(commitment, amount)
Escrow->>Escrow: validate amount > 0
Escrow->>Storage: load vault config & state
Escrow->>Owner: require_auth()
Owner->>Escrow: authorized
Escrow->>Escrow: check vault.is_active
Escrow->>Token: transfer(owner -> contract, amount)
Token->>Token: update balances
Escrow->>Storage: increment vault.balance
Escrow->>Events: publish DEPOSIT(commitment, owner, amount, new_balance)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
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 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: 7
🧹 Nitpick comments (5)
zk/sdk/src/__tests__/smoke.test.ts (1)
3-5: Remove ad-hoc Jest global declarations and use the official typings.
@types/jest(^29.5.14) is already installed as a devDependency and ts-jest is configured as the preset injest.config.ts. These manual declarations are redundant and risk drifting from Jest's actual signatures. Delete lines 3–5 and rely on the installed type definitions instead.🤖 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, Delete the manual Jest global declarations (the three `declare` lines for describe, it, and expect) from the test file and rely on the installed `@types/jest` and the `ts-jest` preset; also scan `smoke.test.ts` for any other ad-hoc Jest declarations and remove them so the file uses the official Jest typings for `describe`, `it`, and `expect`.zk/sdk/package.json (1)
5-6: Consider adding a build process ortsconfig.jsonfor clarity, given the absence of compiled output.With
"type": "commonjs"and"main"pointing tosrc/index.ts, this package requires TypeScript tooling to be consumed. Since this is a private workspace package, the current setup is acceptable for monorepo-internal use (tests run successfully viats-jest). However, the missingtsconfig.jsonmakes the TypeScript configuration implicit. If external consumers or different tooling contexts are expected in the future, consider either: (1) adding atsconfig.jsonto document the build configuration, or (2) implementing a build script that outputs todist/index.jsand updating"main"accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zk/sdk/package.json` around lines 5 - 6, The package.json currently declares "type": "commonjs" and "main": "src/index.ts", which implies TypeScript sources are consumed directly; add an explicit TypeScript build or configuration: create a tsconfig.json at the package root to document compiler options (or add a build script that compiles src/*.ts to dist/*.js), update package.json scripts (e.g., "build") and change "main" to point to the compiled entry (e.g., "dist/index.js") if you implement compilation; ensure test/dev workflows still point to src when using ts-jest or run the build step before publishing/consuming externally so consumers don’t depend on implicit TS configuration.gateway-contract/contracts/auction_contract/auction.md (1)
314-329: Clarify the status of unused event types.Lines 318-322 list five event types, but lines 326-328 note that only two are actually emitted. Consider adding a note similar to the error section (line 312) to clarify that
AuctionCreatedEvent,BidPlacedEvent, andBidRefundedEventare defined but not currently emitted by any public entry point.📝 Suggested clarification
## 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. + +Note: `AuctionCreatedEvent`, `BidPlacedEvent`, and `BidRefundedEvent` are defined in the events module but are not currently emitted by any of the documented entry points.🤖 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 314 - 329, Add a brief clarification to the "Event Types (Contract-Wide)" section stating that AuctionCreatedEvent, BidPlacedEvent, and BidRefundedEvent are defined in events.rs but are not currently emitted by any public entry point; reference the defined event types (AuctionCreatedEvent, BidPlacedEvent, BidRefundedEvent, AuctionClosedEvent, UsernameClaimedEvent) and the currently emitting entry points (close_auction → AuctionClosedEvent, claim_username → UsernameClaimedEvent) so readers understand which events are declared versus which are actually emitted.gateway-contract/contracts/escrow_contract/src/test.rs (2)
115-121: Strengthen event assertion to validate DEPOSIT payload.This currently checks only event count for the escrow contract. Assert the event topic/data as well so a wrong event type cannot pass.
🤖 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 test currently counts events from env.events().all() filtered by contract_id using deposit_events; enhance it to locate the specific event for contract_id (e.g., by using .find or .filter_map on env.events().all()) and assert the event's topic/name equals "DEPOSIT" and that the event data/payload contains the expected fields/values (amount, sender, etc.) rather than only checking count; reference the variables/functions deposit_events, env.events().all(), and contract_id to find and validate the event payload and fail the test if the topic or data do not match expected values.
146-156: Prefer explicit auth-failure assertion over broad#[should_panic].Using
#[should_panic]can pass on unrelated panics.try_depositwith an explicitis_err()/auth error match is more deterministic.🤖 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 146 - 156, Replace the brittle #[should_panic] test in test_deposit_non_owner_panics with an explicit error assertion: call the fallible method (e.g., client.try_deposit or the SDK equivalent) instead of client.deposit, assert the result is Err (or match the specific auth error variant), and remove #[should_panic]; keep the same setup using setup_test and create_vault and assert that the caller (from) is rejected due to authorization rather than allowing any panic to satisfy the test.
🤖 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/sdk_tests.yml:
- Around line 25-26: Replace the non-deterministic "npm install" invocation in
the "Install dependencies" step with a deterministic CI install: use "npm ci"
(or the same "npm ci || npm install --prefer-offline" fallback pattern used in
the other workflow) so installs use the lockfile and avoid lockfile drift;
update the step that currently runs "npm install" (step name "Install
dependencies") to run the deterministic command instead.
In `@gateway-contract/contracts/auction_contract/auction.md`:
- Around line 12-59: Update the Edge Cases for create_auction to explicitly call
out that min_bid is an i128 and therefore can be negative (a likely nonsensical
value), mention that the current implementation writes min_bid to
AuctionKey::MinBid(id) without validation, and either document this as an
accepted design decision or recommend adding a validation (e.g., require min_bid
> 0) and where that would fail (AuctionError::AuctionNotOpen is unrelated) so
reviewers know the gap and suggested remediation.
- Around line 60-107: The place_bid implementation and docs miss validation
against negative bid amounts: add an explicit check in place_bid (before token
transfers and comparisons with AuctionKey::HighestBid(id) and min_bid) that
rejects any amount <= 0 (or < 0 depending on desired policy) and aborts with a
clear error (e.g., AuctionError::InvalidBidAmount or reuse
AuctionError::BidTooLow) to prevent negative/invalid bids, and update the Edge
Cases section to document rejection of negative bids and the requirement that
min_bid must be non-negative when auctions are created.
- Around line 144-192: Add a short note to the contract overview stating that
the singleton username auction uses unsigned 128-bit amounts (u128) for bids and
events (e.g., close_auction's AuctionClosedEvent winning_bid: u128 and storage
for HighestBid), while the ID-indexed auction flow uses signed 128-bit amounts
(i128) for min_bid, bids and helpers (e.g., place_bid signatures and
auction_get_min_bid returns i128); mention this intentional difference so
readers know to convert/handle types when interacting between flows.
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 typed event struct
(e.g., DepositEvent with fields commitment: BytesN<32>, owner: Address, amount:
i128, new_balance: i128) and use the same pattern as
VaultCancelEvent/PayExecEvent: construct DepositEvent { ... } and call
.publish(env) to emit it from the deposit function, ensuring the struct is
annotated with #[contractevent] to match other events in the file.
In `@gateway-contract/contracts/escrow_contract/src/lib.rs`:
- Line 133: The update to state.balance is currently unchecked (state.balance +=
amount) and can overflow; replace it with a checked addition using
state.balance.checked_add(amount) and handle the None case by returning an
explicit error (e.g., a custom Err variant or ContractError) so the function
fails safely instead of wrapping on overflow; update the code path that contains
state.balance and amount (the function that performs the deposit/state update in
lib.rs) to use the checked result and assign the new balance only on success.
In `@zk/sdk/src/__tests__/smoke.test.ts`:
- Around line 7-10: The test currently only asserts that the imported sdk
exists; update the assertion to check for a concrete runtime export so the test
fails if the public API is emptied — for example, replace or extend the
assertion in the "zk sdk smoke" test to assert that sdk.MerkleProofGenerator is
defined (or use typeof sdk.MerkleProofGenerator === "function" if it should be
callable); locate the test in the file where sdk is imported and change the
expect(sdk).toBeDefined() line to expect(sdk.MerkleProofGenerator).toBeDefined()
(or the appropriate exported symbol name) so the smoke test verifies an actual
exported member.
---
Nitpick comments:
In `@gateway-contract/contracts/auction_contract/auction.md`:
- Around line 314-329: Add a brief clarification to the "Event Types
(Contract-Wide)" section stating that AuctionCreatedEvent, BidPlacedEvent, and
BidRefundedEvent are defined in events.rs but are not currently emitted by any
public entry point; reference the defined event types (AuctionCreatedEvent,
BidPlacedEvent, BidRefundedEvent, AuctionClosedEvent, UsernameClaimedEvent) and
the currently emitting entry points (close_auction → AuctionClosedEvent,
claim_username → UsernameClaimedEvent) so readers understand which events are
declared versus which are actually emitted.
In `@gateway-contract/contracts/escrow_contract/src/test.rs`:
- Around line 115-121: The test currently counts events from env.events().all()
filtered by contract_id using deposit_events; enhance it to locate the specific
event for contract_id (e.g., by using .find or .filter_map on
env.events().all()) and assert the event's topic/name equals "DEPOSIT" and that
the event data/payload contains the expected fields/values (amount, sender,
etc.) rather than only checking count; reference the variables/functions
deposit_events, env.events().all(), and contract_id to find and validate the
event payload and fail the test if the topic or data do not match expected
values.
- Around line 146-156: Replace the brittle #[should_panic] test in
test_deposit_non_owner_panics with an explicit error assertion: call the
fallible method (e.g., client.try_deposit or the SDK equivalent) instead of
client.deposit, assert the result is Err (or match the specific auth error
variant), and remove #[should_panic]; keep the same setup using setup_test and
create_vault and assert that the caller (from) is rejected due to authorization
rather than allowing any panic to satisfy the test.
In `@zk/sdk/package.json`:
- Around line 5-6: The package.json currently declares "type": "commonjs" and
"main": "src/index.ts", which implies TypeScript sources are consumed directly;
add an explicit TypeScript build or configuration: create a tsconfig.json at the
package root to document compiler options (or add a build script that compiles
src/*.ts to dist/*.js), update package.json scripts (e.g., "build") and change
"main" to point to the compiled entry (e.g., "dist/index.js") if you implement
compilation; ensure test/dev workflows still point to src when using ts-jest or
run the build step before publishing/consuming externally so consumers don’t
depend on implicit TS configuration.
In `@zk/sdk/src/__tests__/smoke.test.ts`:
- Around line 3-5: Delete the manual Jest global declarations (the three
`declare` lines for describe, it, and expect) from the test file and rely on the
installed `@types/jest` and the `ts-jest` preset; also scan `smoke.test.ts` for
any other ad-hoc Jest declarations and remove them so the file uses the official
Jest typings for `describe`, `it`, and `expect`.
🪄 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: dbb8b782-c19b-41a2-a2f2-662e1480d57d
📒 Files selected for processing (8)
.github/workflows/sdk_tests.ymlgateway-contract/contracts/auction_contract/auction.mdgateway-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
+25
to
+26
| - name: Install dependencies | ||
| run: npm install |
There was a problem hiding this comment.
Use deterministic dependency installation in CI.
npm install is non-deterministic for CI and can introduce lockfile drift behavior between runs. Switch to npm ci (optionally with the same fallback pattern used in .github/workflows/zk_circuits.yml, Line 52).
Suggested change
- name: Install dependencies
- run: npm install
+ run: npm ci --legacy-peer-deps || npm ci📝 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
| - name: Install dependencies | |
| run: npm install | |
| - name: Install dependencies | |
| run: npm ci --legacy-peer-deps || npm ci |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/sdk_tests.yml around lines 25 - 26, Replace the
non-deterministic "npm install" invocation in the "Install dependencies" step
with a deterministic CI install: use "npm ci" (or the same "npm ci || npm
install --prefer-offline" fallback pattern used in the other workflow) so
installs use the lockfile and avoid lockfile drift; update the step that
currently runs "npm install" (step name "Install dependencies") to run the
deterministic command instead.
Comment on lines
+12
to
+59
| ### 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. | ||
|
|
There was a problem hiding this comment.
Document the risk of negative min_bid values.
The specification correctly notes that "contract currently does not enforce min_bid > 0" (line 58), but this deserves more prominence. Since min_bid is i128, negative values are technically valid but likely nonsensical for an auction. Consider explicitly documenting this as a design decision or potential validation gap.
📝 Suggested addition to Edge Cases section
#### 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.
+- **Negative min_bid**: `i128` type permits negative values; no validation prevents this scenario.📝 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
| ### 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: `create_auction` | |
| Creates a new auction identified by `id`. | |
| #### Interface | |
🧰 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)
🤖 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 12 - 59,
Update the Edge Cases for create_auction to explicitly call out that min_bid is
an i128 and therefore can be negative (a likely nonsensical value), mention that
the current implementation writes min_bid to AuctionKey::MinBid(id) without
validation, and either document this as an accepted design decision or recommend
adding a validation (e.g., require min_bid > 0) and where that would fail
(AuctionError::AuctionNotOpen is unrelated) so reviewers know the gap and
suggested remediation.
Comment on lines
+60
to
+107
| ### 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`. | ||
|
|
There was a problem hiding this comment.
Flag the absence of negative bid validation.
The amount parameter is i128, allowing negative values. While the implementation checks amount < min_bid and amount <= highest_bid, a negative amount combined with a negative min_bid could theoretically pass validation. This edge case is not documented.
📝 Suggested addition to Edge Cases section
#### 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`.
+- **Negative bid amounts**: `i128` type permits negative values; validation relies on comparison with `min_bid` and `highest_bid` without explicit non-negativity check.🤖 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 60 -
107, The place_bid implementation and docs miss validation against negative bid
amounts: add an explicit check in place_bid (before token transfers and
comparisons with AuctionKey::HighestBid(id) and min_bid) that rejects any amount
<= 0 (or < 0 depending on desired policy) and aborts with a clear error (e.g.,
AuctionError::InvalidBidAmount or reuse AuctionError::BidTooLow) to prevent
negative/invalid bids, and update the Edge Cases section to document rejection
of negative bids and the requirement that min_bid must be non-negative when
auctions are created.
Comment on lines
+144
to
+192
| ### 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`. | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the type inconsistency between auction flows
echo "=== Singleton username auction (instance storage) ==="
rg -A3 'pub fn get_highest_bid' gateway-contract/contracts/auction_contract/src/storage.rs
echo ""
echo "=== ID-indexed auction (persistent storage) ==="
rg -A3 'pub fn auction_get_highest_bid' gateway-contract/contracts/auction_contract/src/storage.rs
echo ""
echo "=== Event emission in close_auction ==="
rg -B2 -A2 'emit_auction_closed' gateway-contract/contracts/auction_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 930
🏁 Script executed:
#!/bin/bash
# Check for min_bid and place_bid type usage in both flows
echo "=== Checking min_bid type in ID-indexed flow ==="
rg -B2 -A2 'fn auction_get_min_bid|min_bid.*i128|min_bid.*u128' gateway-contract/contracts/auction_contract/src/
echo ""
echo "=== Checking place_bid function signature ==="
rg -B2 -A4 'pub fn place_bid' gateway-contract/contracts/auction_contract/src/lib.rs
echo ""
echo "=== Checking auction.md overview section ==="
head -50 gateway-contract/contracts/auction_contract/auction.mdRepository: Alien-Protocol/Alien-Gateway
Length of output: 4240
Document the type inconsistency for bid amounts across auction flows.
The singleton username auction uses u128 for highest bids (line 181: winning_bid: u128), while the ID-indexed auction uses i128 throughout: min_bid, highest bids, and bid amounts in place_bid. This type difference between the two coexisting flows should be explicitly documented in the overview section to prevent confusion.
The inconsistency appears systematically across the ID-indexed flow: min_bid: i128 in function signatures and storage functions like auction_get_min_bid() returns i128, while singleton flow storage returns u128.
Add a note to the overview section clarifying this difference:
Suggested documentation addition
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`).
+
+**Note**: The two flows use different numeric types for bid amounts:
+- Singleton username auction: `u128` (unsigned) for highest_bid
+- ID-indexed auction: `i128` (signed) for min_bid, highest_bid, and bid amounts
Both flows use Soroban auth (`require_auth`) and ledger timestamp checks to enforce access and timing constraints.🤖 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 144 -
192, Add a short note to the contract overview stating that the singleton
username auction uses unsigned 128-bit amounts (u128) for bids and events (e.g.,
close_auction's AuctionClosedEvent winning_bid: u128 and storage for
HighestBid), while the ID-indexed auction flow uses signed 128-bit amounts
(i128) for min_bid, bids and helpers (e.g., place_bid signatures and
auction_get_min_bid returns i128); mention this intentional difference so
readers know to convert/handle types when interacting between flows.
Comment on lines
+169
to
+181
| /// Emits a DEPOSIT event with topics (symbol!("DEPOSIT"), commitment) | ||
| /// and data (owner, amount, new_balance). | ||
| pub fn deposit( | ||
| env: &Env, | ||
| commitment: BytesN<32>, | ||
| owner: Address, | ||
| amount: i128, | ||
| new_balance: i128, | ||
| ) { | ||
| env.events().publish( | ||
| (symbol_short!("DEPOSIT"), commitment), | ||
| (owner, amount, new_balance), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify deprecated publish usage and typed deposit event presence.
rg -n 'env\.events\(\)\.publish\(' gateway-contract/contracts/escrow_contract/src/events.rs gateway-contract/contracts/escrow_contract/src/lib.rs
rg -n 'struct\s+DepositEvent|fn\s+deposit\s*\(' gateway-contract/contracts/escrow_contract/src/events.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 186
🏁 Script executed:
# Check soroban-sdk version and examine event patterns
cat gateway-contract/Cargo.toml | grep -A 5 "soroban-sdk"Repository: Alien-Protocol/Alien-Gateway
Length of output: 158
🏁 Script executed:
# View the full events.rs file to understand event patterns
wc -l gateway-contract/contracts/escrow_contract/src/events.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 132
🏁 Script executed:
# Check other event functions to see if they use the old or new pattern
rg -n 'pub fn|env\.events\(\)\.publish|#\[contractevent\]' gateway-contract/contracts/escrow_contract/src/events.rs -B 2 -A 5Repository: Alien-Protocol/Alien-Gateway
Length of output: 3430
Replace deprecated deposit event emission API (CI blocker).
At Line 178, env.events().publish(...) is deprecated and fails clippy with -D warnings. All other events in the file use the typed #[contractevent] pattern with .publish(env). Replace with a DepositEvent struct to match the pattern used by VaultCancelEvent, PayExecEvent, and other events.
Proposed fix
@@
#[contractevent]
#[derive(Clone, Debug, Eq, PartialEq)]
+pub struct DepositEvent {
+ /// The commitment identifier of the vault.
+ #[topic]
+ pub commitment: BytesN<32>,
+ /// The owner funding the deposit.
+ pub owner: Address,
+ /// Amount deposited.
+ pub amount: i128,
+ /// Resulting vault balance.
+ pub new_balance: i128,
+}
+
+#[contractevent]
+#[derive(Clone, Debug, Eq, PartialEq)]
pub struct VaultCancelEvent {
@@
pub fn deposit(
env: &Env,
commitment: BytesN<32>,
owner: Address,
amount: i128,
new_balance: i128,
) {
- env.events().publish(
- (symbol_short!("DEPOSIT"), commitment),
- (owner, amount, new_balance),
- );
+ DepositEvent {
+ commitment,
+ owner,
+ amount,
+ new_balance,
+ }
+ .publish(env);
}🧰 Tools
🪛 GitHub Actions: Checks
[error] 178-178: Clippy (with -D warnings) failed: use of deprecated method soroban_sdk::events::Events::publish—use the #[contractevent] macro on a contract event type. (in env.events().publish(...))
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway-contract/contracts/escrow_contract/src/events.rs` around lines 169 -
181, The deposit function currently uses the deprecated
env.events().publish(...) API; replace it by defining a typed event struct
(e.g., DepositEvent with fields commitment: BytesN<32>, owner: Address, amount:
i128, new_balance: i128) and use the same pattern as
VaultCancelEvent/PayExecEvent: construct DepositEvent { ... } and call
.publish(env) to emit it from the deposit function, ensuring the struct is
annotated with #[contractevent] to match other events in the file.
| let token_client = token::Client::new(&env, &config.token); | ||
| token_client.transfer(&config.owner, &env.current_contract_address(), &amount); | ||
|
|
||
| state.balance += amount; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify arithmetic sites on vault balances that may require checked ops.
rg -n 'state\.balance\s*[\+\-]=' gateway-contract/contracts/escrow_contract/src/lib.rs
rg -n 'pub struct VaultState|pub balance:\s*i128' gateway-contract/contracts/escrow_contract/src/types.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 270
🏁 Script executed:
# Get context around line 133
sed -n '120,150p' gateway-contract/contracts/escrow_contract/src/lib.rs | cat -n
# Check lines 194 and 407 for similar issues
sed -n '185,200p' gateway-contract/contracts/escrow_contract/src/lib.rs | cat -n
sed -n '400,415p' gateway-contract/contracts/escrow_contract/src/lib.rs | cat -n
# Look for checked arithmetic patterns already used in the file
rg -n 'checked_add|checked_sub|checked_mul|overflow' gateway-contract/contracts/escrow_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 2873
🏁 Script executed:
# Look at the full function signatures
sed -n '100,160p' gateway-contract/contracts/escrow_contract/src/lib.rs | cat -n
# Check VaultState struct definition and any constraints
sed -n '35,80p' gateway-contract/contracts/escrow_contract/src/types.rs | cat -n
# Look for amount type and any constraints
rg -n 'fn.*amount|amount:\s*i128|amount:\s*u128' gateway-contract/contracts/escrow_contract/src/lib.rs | head -20Repository: Alien-Protocol/Alien-Gateway
Length of output: 4900
🏁 Script executed:
# Look for any balance limits or maximum value constraints
rg -n 'MAX|LIMIT|balance.*>' gateway-contract/contracts/escrow_contract/src/
# Check what EscrowError types are available
rg -n 'enum EscrowError|InvalidAmount' gateway-contract/contracts/escrow_contract/src/
# Look for other arithmetic operations to see if checked ops are used elsewhere
rg -n 'checked_add|checked_sub|saturating' gateway-contract/contracts/escrow_contract/src/Repository: Alien-Protocol/Alien-Gateway
Length of output: 2052
🏁 Script executed:
# Show the complete EscrowError enum
cat gateway-contract/contracts/escrow_contract/src/errors.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 1729
Protect vault balance update from overflow.
At Line 133, state.balance += amount is unchecked. On extreme values this can overflow and break balance accounting; use checked_add with explicit error handling.
Proposed fix
- state.balance += amount;
+ state.balance = state
+ .balance
+ .checked_add(amount)
+ .unwrap_or_else(|| panic_with_error!(&env, EscrowError::InvalidAmount));📝 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; | |
| state.balance = state | |
| .balance | |
| .checked_add(amount) | |
| .unwrap_or_else(|| panic_with_error!(&env, EscrowError::InvalidAmount)); |
🤖 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` at line 133, The
update to state.balance is currently unchecked (state.balance += amount) and can
overflow; replace it with a checked addition using
state.balance.checked_add(amount) and handle the None case by returning an
explicit error (e.g., a custom Err variant or ContractError) so the function
fails safely instead of wrapping on overflow; update the code path that contains
state.balance and amount (the function that performs the deposit/state update in
lib.rs) to use the checked result and assign the new balance only on success.
Comment on lines
+7
to
+10
| describe("zk sdk smoke", () => { | ||
| it("imports index without throwing", () => { | ||
| expect(sdk).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
Strengthen the smoke assertion to verify an actual runtime export.
expect(sdk).toBeDefined() is almost tautological after a successful import. Assert a concrete export (e.g., sdk.MerkleProofGenerator) so the test fails if the public runtime API is accidentally emptied.
Suggested improvement
describe("zk sdk smoke", () => {
it("imports index without throwing", () => {
- expect(sdk).toBeDefined();
+ expect(sdk.MerkleProofGenerator).toBeDefined();
});
});📝 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
| describe("zk sdk smoke", () => { | |
| it("imports index without throwing", () => { | |
| expect(sdk).toBeDefined(); | |
| }); | |
| describe("zk sdk smoke", () => { | |
| it("imports index without throwing", () => { | |
| expect(sdk.MerkleProofGenerator).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 7 - 10, The test currently
only asserts that the imported sdk exists; update the assertion to check for a
concrete runtime export so the test fails if the public API is emptied — for
example, replace or extend the assertion in the "zk sdk smoke" test to assert
that sdk.MerkleProofGenerator is defined (or use typeof sdk.MerkleProofGenerator
=== "function" if it should be callable); locate the test in the file where sdk
is imported and change the expect(sdk).toBeDefined() line to
expect(sdk.MerkleProofGenerator).toBeDefined() (or the appropriate exported
symbol name) so the smoke test verifies an actual exported member.
|
@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! 🚀 |
Contributor
|
@jaymhorsh , why u make changes in contracts |
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 #200
Summary by CodeRabbit
New Features
Documentation
Tests