Open
Conversation
📝 WalkthroughWalkthroughThis PR implements the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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: 3
🧹 Nitpick comments (3)
gateway-contract/contracts/auction_contract/auction.md (2)
58-58: Consider adding validation at auction creation.The specification notes that
create_auctioncurrently does not enforcemin_bid > 0orend_time > nowat creation time. This could allow:
- Auctions with zero or negative minimum bids
- Auctions that are already expired at creation
While not immediately breaking, these omissions could lead to confusing states or usability issues.
Consider adding validation in the
create_auctionimplementation:
min_bid > 0to ensure meaningful minimum bidsend_time > env.ledger().timestamp()to prevent expired auctions at creation🤖 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` at line 58, Add validation in the create_auction implementation to reject auctions with non-positive minimum bids and already-expired end times: check that min_bid > 0 and end_time > env.ledger().timestamp() (or equivalent current timestamp call) before persisting, and return a clear error (e.g., ErrInvalidMinBid / ErrInvalidEndTime or a suitable ContractError variant) when either check fails; update any Auction struct/creation path to enforce these guards so auctions cannot be created with zero/negative min_bid or end_time <= now.
46-48: Remove or implement dead event emission functions for consistencyThe specification correctly documents "None in current implementation" for
create_auction. However,events.rsdefinesemit_auction_created(),emit_bid_placed(), andemit_bid_refunded()which are never called anywhere in the codebase. Onlyemit_auction_closed()andemit_username_claimed()are actually invoked (inlib.rs).This creates two maintenance issues:
- Dead code if events will never be emitted for these operations
- Incomplete implementation if events should be emitted but aren't
Either implement event emission consistently across all entry points or remove the unused emission functions to reduce dead code.
🤖 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 46 - 48, The repository defines event helper functions emit_auction_created, emit_bid_placed, and emit_bid_refunded in events.rs but never calls them (only emit_auction_closed and emit_username_claimed are used in lib.rs), so either remove these unused functions or wire them into the relevant entry points (e.g., call emit_auction_created from create_auction, emit_bid_placed from the bid placement handler, and emit_bid_refunded where refunds occur) to make event emission consistent; update events.rs to delete the three unused functions if you choose removal, or update lib.rs (and the corresponding handlers for create_auction and bid/refund flows) to invoke the appropriate emit_* functions and ensure tests/exports reflect the change.gateway-contract/contracts/escrow_contract/src/test.rs (1)
115-121: Consider strengthening the event assertion.The current assertion counts all events from the contract but doesn't verify that the emitted event is specifically a
DEPOSITevent with the correct data (commitment, owner, amount=200, new_balance=200). This could mask issues where the wrong event type is emitted.💡 Example of a more specific assertion
// After fixing events.rs to use DepositEvent struct: use crate::events::DepositEvent; use soroban_sdk::IntoVal; let events = env.events().all(); let deposit_event = events .iter() .find(|(contract, topics, _)| { contract == &contract_id && topics.len() > 0 && topics.get(0).unwrap() == "DEPOSIT".into_val(&env) }); assert!(deposit_event.is_some(), "DEPOSIT event not found");🤖 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 only counts events (deposit_events) and can miss wrong event types; instead locate the DEPOSIT event from env.events().all() filtering by contract_id and topic "DEPOSIT", then deserialize or match it against the DepositEvent struct (use crate::events::DepositEvent and soroban_sdk::IntoVal) and assert its fields equal the expected commitment, owner, amount == 200 and new_balance == 200; update the assertions in the test that references env.events().all() and contract_id to perform these specific checks.
🤖 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 is missing the escrow deposit feature: add a public
entry fn deposit(env: Env, commitment: BytesN<32>, amount: i128) in lib.rs that
requires the vault owner to authorize (use VaultState/VaultKey to load the vault
and check owner.require_auth()), validate amount > 0, perform the token transfer
from caller to contract (use the token client transfer_from or transfer
semantics already used in the contract), update and persist VaultState.balance,
and emit a DepositEvent via an emit_deposit helper; also add a DepositEvent
definition (e.g., struct DepositEvent { commitment: BytesN<32>, depositor:
Address, amount: i128 }) and emit function in events.rs, and add tests in
test.rs that cover successful deposit (balance updated + event), auth rejection,
zero/negative amount rejection, and token transfer failure/allowance issues
referencing deposit, VaultState, VaultKey, emit_deposit, and DepositEvent so the
new behavior is exercised.
In `@gateway-contract/contracts/escrow_contract/src/events.rs`:
- Around line 168-182: Replace the deprecated env.events().publish() call inside
the deposit function by defining a #[contractevent] struct DepositEvent (with
#[topic] pub commitment: BytesN<32>, and fields pub owner: Address, pub amount:
i128, pub new_balance: i128) near the other event definitions, then update the
deposit(env, commitment, owner, amount, new_balance) helper to construct
DepositEvent { commitment, owner, amount, new_balance } and call .publish(env)
on it instead of env.events().publish(...).
In `@gateway-contract/contracts/escrow_contract/src/lib.rs`:
- Around line 130-131: Clippy flags a needless borrow on
env.current_contract_address() in the token_client.transfer call; update the
call in the escrow_contract (where token::Client::new is used and
token_client.transfer(&config.owner, &env.current_contract_address(), &amount)
is invoked) to pass the contract address by value (or the expected
ownership/reference type) instead of using an extra & before
env.current_contract_address(), e.g. pass env.current_contract_address()
(matching transfer's parameter type) so the needless borrow is removed.
---
Nitpick comments:
In `@gateway-contract/contracts/auction_contract/auction.md`:
- Line 58: Add validation in the create_auction implementation to reject
auctions with non-positive minimum bids and already-expired end times: check
that min_bid > 0 and end_time > env.ledger().timestamp() (or equivalent current
timestamp call) before persisting, and return a clear error (e.g.,
ErrInvalidMinBid / ErrInvalidEndTime or a suitable ContractError variant) when
either check fails; update any Auction struct/creation path to enforce these
guards so auctions cannot be created with zero/negative min_bid or end_time <=
now.
- Around line 46-48: The repository defines event helper functions
emit_auction_created, emit_bid_placed, and emit_bid_refunded in events.rs but
never calls them (only emit_auction_closed and emit_username_claimed are used in
lib.rs), so either remove these unused functions or wire them into the relevant
entry points (e.g., call emit_auction_created from create_auction,
emit_bid_placed from the bid placement handler, and emit_bid_refunded where
refunds occur) to make event emission consistent; update events.rs to delete the
three unused functions if you choose removal, or update lib.rs (and the
corresponding handlers for create_auction and bid/refund flows) to invoke the
appropriate emit_* functions and ensure tests/exports reflect the change.
In `@gateway-contract/contracts/escrow_contract/src/test.rs`:
- Around line 115-121: The test currently only counts events (deposit_events)
and can miss wrong event types; instead locate the DEPOSIT event from
env.events().all() filtering by contract_id and topic "DEPOSIT", then
deserialize or match it against the DepositEvent struct (use
crate::events::DepositEvent and soroban_sdk::IntoVal) and assert its fields
equal the expected commitment, owner, amount == 200 and new_balance == 200;
update the assertions in the test that references env.events().all() and
contract_id to perform these specific checks.
🪄 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: 702391b9-55da-4be4-ad6e-98641c8f68be
📒 Files selected for processing (4)
gateway-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.rs
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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if escrow_contract changes exist in the repository
echo "=== Searching for escrow_contract files ==="
fd -t f . gateway-contract/contracts/escrow_contract 2>/dev/null || echo "No escrow_contract directory found"
echo -e "\n=== Searching for deposit function in escrow contexts ==="
rg -n "fn deposit" --type rust -g "*escrow*" -C 3
echo -e "\n=== Searching for DEPOSIT event definitions ==="
rg -n "DEPOSIT|DepositEvent" --type rust -g "*escrow*" -C 2
echo -e "\n=== Checking files changed in this PR branch ==="
git diff --name-only origin/main...HEAD 2>/dev/null || echo "Could not determine git diff"Repository: Alien-Protocol/Alien-Gateway
Length of output: 746
Critical: PR does not implement stated objectives.
This PR claims to implement deposit(env: Env, commitment: BytesN<32>, amount: i128) for EscrowContract (issue #189), including vault owner authentication, token transfers, VaultState.balance updates, DEPOSIT event emission, and validation. However, the deposit function does not exist in escrow_contract/src/lib.rs, and no DEPOSIT or DepositEvent definitions exist in escrow_contract/src/events.rs.
Additionally, the file under review (auction_contract/auction.md) is a specification document for AuctionContract with functions like create_auction, place_bid, and claim_username—none of which relate to the escrow deposit feature.
The escrow_contract directory structure is in place, but the actual implementation of the deposit feature is missing. Commit the implementation to:
- escrow_contract/src/lib.rs (deposit function)
- escrow_contract/src/events.rs (DEPOSIT event definition)
- escrow_contract/src/test.rs (deposit tests)
🧰 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 is missing the escrow deposit feature: add a public entry fn deposit(env:
Env, commitment: BytesN<32>, amount: i128) in lib.rs that requires the vault
owner to authorize (use VaultState/VaultKey to load the vault and check
owner.require_auth()), validate amount > 0, perform the token transfer from
caller to contract (use the token client transfer_from or transfer semantics
already used in the contract), update and persist VaultState.balance, and emit a
DepositEvent via an emit_deposit helper; also add a DepositEvent definition
(e.g., struct DepositEvent { commitment: BytesN<32>, depositor: Address, amount:
i128 }) and emit function in events.rs, and add tests in test.rs that cover
successful deposit (balance updated + event), auth rejection, zero/negative
amount rejection, and token transfer failure/allowance issues referencing
deposit, VaultState, VaultKey, emit_deposit, and DepositEvent so the new
behavior is exercised.
Comment on lines
+168
to
+182
|
|
||
| /// 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.
Fix Clippy error: use #[contractevent] struct instead of deprecated env.events().publish().
The pipeline is failing because env.events().publish() is deprecated in soroban-sdk 23.0.0. The recommended pattern (already used elsewhere in this file) is to define a #[contractevent] struct and call .publish(env) on it.
🐛 Proposed fix using the contractevent pattern
Add the event struct near the other event definitions (around line 79):
/// Event emitted when tokens are deposited into a vault.
#[contractevent]
#[derive(Clone, Debug, Eq, PartialEq)]
pub struct DepositEvent {
/// The commitment identifier of the vault receiving the deposit.
#[topic]
pub commitment: BytesN<32>,
/// The address of the vault owner who made the deposit.
pub owner: Address,
/// The amount of tokens deposited.
pub amount: i128,
/// The new balance after the deposit.
pub new_balance: i128,
}Then update the helper:
- /// Emits a DEPOSIT event with topics (symbol!("DEPOSIT"), commitment)
- /// and data (owner, amount, new_balance).
+ /// Emits a `DepositEvent` to the host.
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).
[error] Command cargo clippy -- -D warnings failed with exit code 101 because escrow_contract could not compile due to 2 previous errors.
🤖 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 168 -
182, Replace the deprecated env.events().publish() call inside the deposit
function by defining a #[contractevent] struct DepositEvent (with #[topic] pub
commitment: BytesN<32>, and fields pub owner: Address, pub amount: i128, pub
new_balance: i128) near the other event definitions, then update the
deposit(env, commitment, owner, amount, new_balance) helper to construct
DepositEvent { commitment, owner, amount, new_balance } and call .publish(env)
on it instead of env.events().publish(...).
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.
Fix Clippy error: remove needless borrow on env.current_contract_address().
The pipeline is failing due to clippy::needless-borrows-for-generic-args. The transfer method accepts the address without requiring an explicit borrow here.
🐛 Proposed fix
let token_client = token::Client::new(&env, &config.token);
- token_client.transfer(&config.owner, &env.current_contract_address(), &amount);
+ token_client.transfer(&config.owner, env.current_contract_address(), &amount);📝 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
| let token_client = token::Client::new(&env, &config.token); | |
| token_client.transfer(&config.owner, &env.current_contract_address(), &amount); | |
| let token_client = token::Client::new(&env, &config.token); | |
| token_client.transfer(&config.owner, env.current_contract_address(), &amount); |
🧰 Tools
🪛 GitHub Actions: Checks
[error] 131-131: Clippy (with -D warnings) failed: needless borrows for generic args (rule: clippy::needless-borrows-for-generic-args). Borrowed expression does not implement required traits; help suggests using env.current_contract_address().
[error] Command cargo clippy -- -D warnings failed with exit code 101 because escrow_contract could not compile due to 2 previous errors.
🤖 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, Clippy flags a needless borrow on env.current_contract_address() in the
token_client.transfer call; update the call in the escrow_contract (where
token::Client::new is used and token_client.transfer(&config.owner,
&env.current_contract_address(), &amount) is invoked) to pass the contract
address by value (or the expected ownership/reference type) instead of using an
extra & before env.current_contract_address(), e.g. pass
env.current_contract_address() (matching transfer's parameter type) so the
needless borrow is removed.
jaymhorsh
changed the title
ryzen-xp
added
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.
Add deposit(env: Env, commitment: BytesN<32>, amount: i128) to EscrowContract
Authenticate the caller as the vault owner
Transfer amount tokens from the caller to the contract address using token::Client
Increment VaultState.balance by amount
Emit a DEPOSIT event
Validate amount > 0 and vault is active
Close #189