Conversation
|
@beebozy 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! 🚀 |
📝 WalkthroughWalkthroughReplaces id-scoped auction implementation with a username-hash–keyed auction contract: adds AuctionState/Bid types, new DataKey storage layout and helpers, new public entrypoints (create_auction, place_bid, close_auction, refund_losers), refund events, and updated tests. Changes
Sequence Diagram(s)sequenceDiagram
actor Creator
actor Bidder1
actor Bidder2
participant Contract
participant Storage
participant Ledger as "XLM Ledger"
Creator->>Contract: create_auction(username_hash, start, end, reserve)
Contract->>Storage: set_auction(AuctionState)
Storage-->>Contract: ack
Bidder1->>Contract: place_bid(username_hash, amount=100)
Contract->>Storage: get_auction(username_hash)
Storage-->>Contract: AuctionState
Contract->>Storage: set_bid(username_hash, Bid{100})
Contract->>Storage: add_bidder(username_hash, Bidder1)
Storage-->>Contract: ack
Bidder2->>Contract: place_bid(username_hash, amount=150)
Contract->>Storage: get_auction(username_hash)
Storage-->>Contract: AuctionState
Contract->>Storage: set_bid(username_hash, Bid{150})
Contract->>Storage: add_bidder(username_hash, Bidder2)
Storage-->>Contract: ack
Creator->>Contract: close_auction(username_hash)
Contract->>Storage: get_auction(username_hash)
Storage-->>Contract: AuctionState
Contract->>Storage: set_auction(is_settled=true)
Storage-->>Contract: ack
Caller->>Contract: refund_losers(username_hash)
Contract->>Storage: get_all_bidders(username_hash)
Storage-->>Contract: [Bidder1,Bidder2]
Contract->>Storage: get_bid(username_hash, Bidder1)
Storage-->>Contract: Bid{100}
Contract->>Ledger: transfer 100 -> Bidder1
Contract->>Storage: remove_bid(username_hash, Bidder1)
Contract->>Contract: emit BID_RFND(Bidder1,100)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 6
🧹 Nitpick comments (2)
gateway-contract/contracts/auction_contract/src/types.rs (1)
1-1: Remove unused imports.
EnvandVecare imported but not used in this file. Onlycontracttype,Address, andBytesNare needed.Suggested fix
-use soroban_sdk::{contracttype, Address, BytesN, Env, Vec}; +use soroban_sdk::{contracttype, Address, BytesN};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/auction_contract/src/types.rs` at line 1, The imports list in types.rs includes unused symbols Env and Vec; remove those from the use statement so only contracttype, Address, and BytesN remain (update the use line that currently references soroban_sdk::{contracttype, Address, BytesN, Env, Vec} to drop Env and Vec) to eliminate unused-import warnings.gateway-contract/contracts/auction_contract/src/lib.rs (1)
65-89: Consider adding input validation for auction parameters.The function doesn't validate that:
start_time < end_time(could create an auction that's already ended)reserve_price >= 0(negative reserve would be nonsensical)Also, fix formatting per pipeline errors (run
cargo fmt).Suggested validation
pub fn create_auction( env: Env, creator: Address, username_hash: BytesN<32>, start_time: u64, end_time: u64, reserve_price: i128, ) { creator.require_auth(); + assert!(start_time < end_time, "start_time must be before end_time"); + assert!(reserve_price >= 0, "reserve_price cannot be negative"); assert!( !has_auction(&env, &username_hash), "auction already exists" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-contract/contracts/auction_contract/src/lib.rs` around lines 65 - 89, create_auction currently allows invalid auctions; add explicit input validation in create_auction to assert start_time < end_time and reserve_price >= 0 before constructing AuctionState (return early or panic with clear messages like "start_time must be before end_time" and "reserve_price must be non-negative"); keep existing creator.require_auth() and has_auction check order, then call set_auction as before, and finally run cargo fmt to fix formatting pipeline errors.
🤖 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/src/lib.rs`:
- Around line 1-578: The file fails rustfmt checks; run rustfmt (cargo fmt) to
reformat the source so the style errors are resolved across the contract and
tests (affecting items like the module headers, function signatures such as
create_auction, close_auction, refund_losers, place_bid, and the test module).
Re-run cargo fmt at the repo root and commit the resulting changes so cargo fmt
--check passes in CI; no behavioral code changes are required—only apply the
automatic formatting edits.
- Around line 542-567: The test currently creates a second Env (env2) which has
no contract state so it doesn't actually verify auth behavior; instead remove
env2 and use the same Env/contract/client (created via Env::default,
env.register_contract and AuctionContractClient::new) for the whole test,
perform setup (create_auction and close_auction) with env.mock_all_auths() only
for the setup phase, then clear the mock (or avoid mock_all_auths from the start
and only mock around the authenticated setup calls) and call
refund_losers(&hash) on the same client to assert it succeeds without
require_auth; reference: refund_losers, create_auction, close_auction,
env.mock_all_auths, AuctionContractClient::new.
- Around line 209-214: The auction currently refunds losers from contract
balance but never receives bidder funds; in the place_bid function add a call to
native_client.transfer to move the bid amount from the bidder into the contract
(use env.current_contract_address() as recipient and the bidder as sender with
the bid amount variable used in place_bid), so that refund_losers can later
transfer XLM from the contract to losing bidders; ensure the transfer uses the
same amount variable used for bids and handles any transfer result/error
consistently with other native_client.transfer calls.
- Around line 177-180: The native token client is being constructed with
env.current_contract_address(), which is the auction contract's address; update
the code to load the SAC address stored at contract initialization (or stored in
contract storage) and pass that SAC address into StellarAssetClient::new instead
of env.current_contract_address(); locate the initialization logic (or add it)
to persist the native SAC address (e.g., in contract storage) and update any
calls that create the token client (StellarAssetClient::new) to read that stored
SAC address so transfer calls use the correct native asset contract.
In `@gateway-contract/contracts/auction_contract/src/storage.rs`:
- Around line 242-262: The doc comment claiming a nonexistent MAX_BATCH_SIZE is
enforced is misleading; either remove or correct that comment in the place that
documents place_bid/lib.rs, or implement the promised enforcement: define a
constant MAX_BATCH_SIZE and add a check in place_bid (returning an error or
rejecting bids when batch size > MAX_BATCH_SIZE) before proceeding. Locate
references to place_bid and the documentation in lib.rs (and any doc comments
near add_bidder if present) and either update the text to accurately describe
current behavior or add the MAX_BATCH_SIZE constant and batch-size validation in
place_bid so the comment becomes truthful.
---
Nitpick comments:
In `@gateway-contract/contracts/auction_contract/src/lib.rs`:
- Around line 65-89: create_auction currently allows invalid auctions; add
explicit input validation in create_auction to assert start_time < end_time and
reserve_price >= 0 before constructing AuctionState (return early or panic with
clear messages like "start_time must be before end_time" and "reserve_price must
be non-negative"); keep existing creator.require_auth() and has_auction check
order, then call set_auction as before, and finally run cargo fmt to fix
formatting pipeline errors.
In `@gateway-contract/contracts/auction_contract/src/types.rs`:
- Line 1: The imports list in types.rs includes unused symbols Env and Vec;
remove those from the use statement so only contracttype, Address, and BytesN
remain (update the use line that currently references
soroban_sdk::{contracttype, Address, BytesN, Env, Vec} to drop Env and Vec) to
eliminate unused-import warnings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2bb4f7c-4741-4926-9f11-3f08da069b2a
📒 Files selected for processing (3)
gateway-contract/contracts/auction_contract/src/lib.rsgateway-contract/contracts/auction_contract/src/storage.rsgateway-contract/contracts/auction_contract/src/types.rs
| #![no_std] | ||
| use soroban_sdk::{contract, contractimpl}; | ||
|
|
||
| mod storage; | ||
| mod types; | ||
|
|
||
| pub use storage::{ | ||
| add_bidder, get_all_bidders, get_auction, get_bid, has_auction, remove_bid, set_auction, | ||
| set_bid, DataKey, | ||
| }; | ||
| pub use types::{AuctionState, Bid}; | ||
|
|
||
| use soroban_sdk::{ | ||
| contract, contractimpl, contracttype, symbol_short, token, Address, BytesN, Env, Symbol, Vec, | ||
| }; | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Event symbols | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| /// Topic symbol for the per-loser refund event. | ||
| /// | ||
| /// Topics: `[BID_REFUNDED, username_hash]` | ||
| /// Data : [`BidRefundedEvent`] | ||
| const BID_REFUNDED: Symbol = symbol_short!("BID_RFND"); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Event payload types | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| /// Data payload emitted with every `BID_RFND` event. | ||
| /// | ||
| /// Published once per outbid bidder when `refund_losers` is called so that | ||
| /// off-chain indexers can reconcile balances without replaying every ledger. | ||
| #[contracttype] | ||
| #[derive(Clone, Debug)] | ||
| pub struct BidRefundedEvent { | ||
| /// Address that received the refund. | ||
| pub bidder: Address, | ||
| /// Exact XLM amount (stroops) returned. | ||
| pub amount: i128, | ||
| /// Auction the bid belonged to. | ||
| pub auction_hash: BytesN<32>, | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Contract | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| #[contract] | ||
| pub struct AuctionContract; | ||
|
|
||
| #[contractimpl] | ||
| impl AuctionContract {} | ||
| impl AuctionContract { | ||
| // ----------------------------------------------------------------------- | ||
| // Auction lifecycle | ||
| // ----------------------------------------------------------------------- | ||
|
|
||
| /// Create a new auction identified by `username_hash`. | ||
| /// | ||
| /// The hash must not already be in use. The `creator` must authorise | ||
| /// the call. | ||
| /// | ||
| /// # Panics | ||
| /// * `"auction already exists"` — `username_hash` is already registered. | ||
| pub fn create_auction( | ||
| env: Env, | ||
| creator: Address, | ||
| username_hash: BytesN<32>, | ||
| start_time: u64, | ||
| end_time: u64, | ||
| reserve_price: i128, | ||
| ) { | ||
| creator.require_auth(); | ||
| assert!( | ||
| !has_auction(&env, &username_hash), | ||
| "auction already exists" | ||
| ); | ||
|
|
||
| let state = AuctionState { | ||
| creator, | ||
| start_time, | ||
| end_time, | ||
| reserve_price, | ||
| highest_bid: 0, | ||
| highest_bidder: None, | ||
| is_settled: false, | ||
| }; | ||
| set_auction(&env, &username_hash, &state); | ||
| } | ||
|
|
||
| /// Mark an auction as settled so that `refund_losers` becomes callable. | ||
| /// | ||
| /// Only the auction creator may close it, and only after `end_time` has | ||
| /// been reached. Once closed the auction is permanently settled — | ||
| /// `is_settled` cannot be reset to `false`. | ||
| /// | ||
| /// # Panics | ||
| /// * `"auction not found"` — unknown `username_hash`. | ||
| /// * `"not authorised"` — caller is not the auction `creator`. | ||
| /// * `"auction not yet ended"` — ledger timestamp < `end_time`. | ||
| /// * `"auction already settled"` — already closed. | ||
| pub fn close_auction(env: Env, caller: Address, username_hash: BytesN<32>) { | ||
| caller.require_auth(); | ||
|
|
||
| let mut state = get_auction(&env, &username_hash).expect("auction not found"); | ||
|
|
||
| assert!(caller == state.creator, "not authorised"); | ||
| assert!( | ||
| env.ledger().timestamp() >= state.end_time, | ||
| "auction not yet ended" | ||
| ); | ||
| assert!(!state.is_settled, "auction already settled"); | ||
|
|
||
| state.is_settled = true; | ||
| set_auction(&env, &username_hash, &state); | ||
| } | ||
|
|
||
| // ----------------------------------------------------------------------- | ||
| // Refunds | ||
| // ----------------------------------------------------------------------- | ||
|
|
||
| /// Refund every non-winning bidder their full XLM after the auction closes. | ||
| /// | ||
| /// **Trustless** — no authentication required; anyone may trigger refunds. | ||
| /// This allows a keeper, a cron job, or any third party to ensure losers | ||
| /// are made whole without relying on the auction creator. | ||
| /// | ||
| /// ## Eligibility | ||
| /// An auction is eligible for `refund_losers` when `is_settled == true` | ||
| /// (set by `close_auction`). Calling before the auction is closed panics | ||
| /// with `"auction not closed"`. | ||
| /// | ||
| /// ## Per-bidder logic | ||
| /// For every address in `AllBidders`: | ||
| /// 1. Skip if the address is `highest_bidder` (the winner). | ||
| /// 2. Skip if no `Bid` record exists — they were already refunded in a | ||
| /// prior call (idempotency guard). | ||
| /// 3. **Effect**: delete the `Bid` record from storage. | ||
| /// 4. **Interaction**: transfer `bid.amount` XLM from the contract to the | ||
| /// bidder via the native token client. | ||
| /// 5. **Event**: emit `BID_RFND` with a [`BidRefundedEvent`] payload. | ||
| /// | ||
| /// Steps 3–5 follow the Checks–Effects–Interactions pattern to prevent | ||
| /// reentrancy from causing a double-refund. | ||
| /// | ||
| /// ## Repeated calls | ||
| /// Safe to call multiple times. Bidders whose records were already | ||
| /// removed are silently skipped, so the second and subsequent calls | ||
| /// transfer nothing and emit no events. | ||
| /// | ||
| /// # Arguments | ||
| /// * `env` — the contract environment. | ||
| /// * `username_hash` — 32-byte commitment hash identifying the auction. | ||
| /// | ||
| /// # Panics | ||
| /// * `"auction not found"` — no auction exists for `username_hash`. | ||
| /// * `"auction not closed"` — auction is still active (`is_settled == false`). | ||
| pub fn refund_losers(env: Env, username_hash: BytesN<32>) { | ||
| // ── CHECKS ────────────────────────────────────────────────────────── | ||
|
|
||
| let state = get_auction(&env, &username_hash).expect("auction not found"); | ||
|
|
||
| // Reject if auction is still open — losers cannot be determined yet. | ||
| assert!(state.is_settled, "auction not closed"); | ||
|
|
||
| // Capture winner before iterating; None when no bids were placed. | ||
| let winner: Option<Address> = state.highest_bidder.clone(); | ||
|
|
||
| // Snapshot the bidder list. Iterating over this Vec while we remove | ||
| // individual Bid keys underneath is safe — AllBidders is not mutated. | ||
| let bidders: Vec<Address> = get_all_bidders(&env, &username_hash); | ||
|
|
||
| // Native XLM client — the Stellar native asset contract is addressed | ||
| // by the network's built-in token address. In the gateway contract | ||
| // this address is stored at init time; here we derive it the standard | ||
| // Soroban way. | ||
| let native_client = token::StellarAssetClient::new( | ||
| &env, | ||
| &env.current_contract_address(), | ||
| ); | ||
|
|
||
| // ── EFFECTS + INTERACTIONS (CEI applied per-bidder) ───────────────── | ||
|
|
||
| for bidder in bidders.iter() { | ||
| // Skip the winner — their bid stays locked until the claim step. | ||
| if let Some(ref w) = winner { | ||
| if bidder == *w { | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| // Read the bid. A None here means this bidder was already | ||
| // refunded in a previous call to refund_losers — skip to avoid | ||
| // a double-refund. | ||
| let bid = match get_bid(&env, &username_hash, &bidder) { | ||
| Some(b) => b, | ||
| None => continue, | ||
| }; | ||
|
|
||
| let refund_amount = bid.amount; | ||
|
|
||
| // EFFECT: remove the bid record before the transfer. If the | ||
| // native_client.transfer panicked and the transaction was | ||
| // reverted, the remove would also be reverted, leaving the bid | ||
| // intact for a retry. On success, the record is gone and a | ||
| // second call finds None above. | ||
| remove_bid(&env, &username_hash, &bidder); | ||
|
|
||
| // INTERACTION: return XLM to the bidder. | ||
| native_client.transfer( | ||
| &env.current_contract_address(), | ||
| &bidder, | ||
| &refund_amount, | ||
| ); | ||
|
|
||
| // EVENT: one emission per refunded loser. | ||
| env.events().publish( | ||
| (BID_REFUNDED, username_hash.clone()), | ||
| BidRefundedEvent { | ||
| bidder: bidder.clone(), | ||
| amount: refund_amount, | ||
| auction_hash: username_hash.clone(), | ||
| }, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // ----------------------------------------------------------------------- | ||
| // Bidding | ||
| // ----------------------------------------------------------------------- | ||
|
|
||
| /// Place or update a bid on auction `username_hash`. | ||
| /// | ||
| /// * The auction must exist and must not be settled. | ||
| /// * `amount` must strictly exceed the current `highest_bid`. | ||
| /// * `bidder` must authorise the call. | ||
| /// | ||
| /// # Panics | ||
| /// * `"auction not found"` — unknown hash. | ||
| /// * `"auction already settled"` — bidding is closed. | ||
| /// * `"bid must exceed highest bid"` — `amount` is not strictly greater. | ||
| pub fn place_bid(env: Env, username_hash: BytesN<32>, bidder: Address, amount: i128) { | ||
| bidder.require_auth(); | ||
|
|
||
| let mut state = get_auction(&env, &username_hash).expect("auction not found"); | ||
|
|
||
| assert!(!state.is_settled, "auction already settled"); | ||
| assert!(amount > state.highest_bid, "bid must exceed highest bid"); | ||
|
|
||
| let bid = Bid { | ||
| bidder: bidder.clone(), | ||
| amount, | ||
| timestamp: env.ledger().timestamp(), | ||
| }; | ||
|
|
||
| // Maintain bidder list before writing the bid record so the | ||
| // AllBidders key is always at least as fresh as any Bid key. | ||
| add_bidder(&env, &username_hash, bidder.clone()); | ||
| set_bid(&env, &username_hash, &bidder, &bid); | ||
|
|
||
| state.highest_bid = amount; | ||
| state.highest_bidder = Some(bidder); | ||
| set_auction(&env, &username_hash, &state); | ||
| } | ||
|
|
||
| // ----------------------------------------------------------------------- | ||
| // View helpers | ||
| // ----------------------------------------------------------------------- | ||
|
|
||
| /// Returns the full [`AuctionState`] for `username_hash`. | ||
| /// | ||
| /// # Panics | ||
| /// `"auction not found"` when the hash is unknown. | ||
| pub fn get_auction(env: Env, username_hash: BytesN<32>) -> AuctionState { | ||
| get_auction(&env, &username_hash).expect("auction not found") | ||
| } | ||
|
|
||
| /// Returns `true` if an auction record exists for `username_hash`. | ||
| pub fn has_auction(env: Env, username_hash: BytesN<32>) -> bool { | ||
| has_auction(&env, &username_hash) | ||
| } | ||
|
|
||
| /// Returns the [`Bid`] placed by `bidder` on `username_hash`, or `None`. | ||
| pub fn get_bid(env: Env, username_hash: BytesN<32>, bidder: Address) -> Option<Bid> { | ||
| get_bid(&env, &username_hash, &bidder) | ||
| } | ||
|
|
||
| /// Returns every address that has bid on `username_hash`. | ||
| pub fn get_all_bidders(env: Env, username_hash: BytesN<32>) -> Vec<Address> { | ||
| get_all_bidders(&env, &username_hash) | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Tests | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| #[cfg(test)] | ||
| mod test { | ||
| use super::*; | ||
| use soroban_sdk::{ | ||
| testutils::{Address as _, Events, Ledger, LedgerInfo}, | ||
| Address, BytesN, Env, IntoVal, | ||
| }; | ||
|
|
||
| // ── helpers ────────────────────────────────────────────────────────────── | ||
|
|
||
| fn test_hash(env: &Env) -> BytesN<32> { | ||
| BytesN::from_array(env, &[1u8; 32]) | ||
| } | ||
|
|
||
| /// Standard fixture: env + client + four addresses + one open auction. | ||
| struct Setup { | ||
| env: Env, | ||
| client: AuctionContractClient<'static>, | ||
| creator: Address, | ||
| alice: Address, | ||
| bob: Address, | ||
| charlie: Address, | ||
| hash: BytesN<32>, | ||
| } | ||
|
|
||
| impl Setup { | ||
| fn new() -> Self { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
|
|
||
| let creator = Address::generate(&env); | ||
| let alice = Address::generate(&env); | ||
| let bob = Address::generate(&env); | ||
| let charlie = Address::generate(&env); | ||
|
|
||
| let contract_id = env.register_contract(None, AuctionContract); | ||
| let client = AuctionContractClient::new(&env, &contract_id); | ||
| let hash = test_hash(&env); | ||
|
|
||
| client.create_auction( | ||
| &creator, | ||
| &hash, | ||
| &0u64, // start_time | ||
| &1000u64, // end_time | ||
| &100i128, // reserve_price | ||
| ); | ||
|
|
||
| Setup { env, client, creator, alice, bob, charlie, hash } | ||
| } | ||
|
|
||
| /// Advance ledger past end_time and mark the auction settled. | ||
| fn close(&self) { | ||
| self.env.ledger().set(LedgerInfo { | ||
| timestamp: 1001, | ||
| ..self.env.ledger().get() | ||
| }); | ||
| self.client.close_auction(&self.creator, &self.hash); | ||
| } | ||
| } | ||
|
|
||
| // ── close_auction ───────────────────────────────────────────────────── | ||
|
|
||
| #[test] | ||
| fn close_sets_is_settled_true() { | ||
| let s = Setup::new(); | ||
| s.close(); | ||
| assert!(s.client.get_auction(&s.hash).is_settled); | ||
| } | ||
|
|
||
| #[test] | ||
| #[should_panic(expected = "auction not yet ended")] | ||
| fn close_before_end_time_panics() { | ||
| let s = Setup::new(); | ||
| // ledger timestamp is 0 — below end_time 1000. | ||
| s.client.close_auction(&s.creator, &s.hash); | ||
| } | ||
|
|
||
| #[test] | ||
| #[should_panic(expected = "auction already settled")] | ||
| fn close_twice_panics() { | ||
| let s = Setup::new(); | ||
| s.close(); | ||
| s.client.close_auction(&s.creator, &s.hash); // second call | ||
| } | ||
|
|
||
| // ── refund_losers — guard checks ────────────────────────────────────── | ||
|
|
||
| #[test] | ||
| #[should_panic(expected = "auction not found")] | ||
| fn refund_losers_unknown_hash_panics() { | ||
| let s = Setup::new(); | ||
| let bad = BytesN::from_array(&s.env, &[9u8; 32]); | ||
| s.client.refund_losers(&bad); | ||
| } | ||
|
|
||
| #[test] | ||
| #[should_panic(expected = "auction not closed")] | ||
| fn refund_losers_before_close_panics() { | ||
| let s = Setup::new(); | ||
| // is_settled is false — must be rejected. | ||
| s.client.refund_losers(&s.hash); | ||
| } | ||
|
|
||
| // ── refund_losers — no-bid edge case ───────────────────────────────── | ||
|
|
||
| #[test] | ||
| fn refund_losers_with_no_bids_is_noop() { | ||
| let s = Setup::new(); | ||
| s.close(); | ||
| s.client.refund_losers(&s.hash); // must not panic | ||
|
|
||
| // No BID_RFND events expected. | ||
| let rfnd_count = s | ||
| .env | ||
| .events() | ||
| .all() | ||
| .iter() | ||
| .filter(|e| { | ||
| e.1.get(0) | ||
| .map(|t| { | ||
| let sym: Symbol = t.into_val(&s.env); | ||
| sym == BID_REFUNDED | ||
| }) | ||
| .unwrap_or(false) | ||
| }) | ||
| .count(); | ||
| assert_eq!(rfnd_count, 0); | ||
| } | ||
|
|
||
| // ── refund_losers — winner is not refunded ──────────────────────────── | ||
|
|
||
| #[test] | ||
| fn winner_bid_record_is_not_removed() { | ||
| let s = Setup::new(); | ||
| s.client.place_bid(&s.hash, &s.alice, &500i128); | ||
| s.close(); | ||
| s.client.refund_losers(&s.hash); | ||
|
|
||
| // Alice's bid record must still exist. | ||
| assert!( | ||
| s.client.get_bid(&s.hash, &s.alice).is_some(), | ||
| "winner bid record was incorrectly removed" | ||
| ); | ||
| } | ||
|
|
||
| // ── refund_losers — multi-bidder: all losers refunded ───────────────── | ||
|
|
||
| #[test] | ||
| fn all_losers_refunded_winner_kept() { | ||
| let s = Setup::new(); | ||
|
|
||
| s.client.place_bid(&s.hash, &s.alice, &200i128); // loser | ||
| s.client.place_bid(&s.hash, &s.bob, &300i128); // loser | ||
| s.client.place_bid(&s.hash, &s.charlie, &400i128); // winner | ||
|
|
||
| s.close(); | ||
| s.client.refund_losers(&s.hash); | ||
|
|
||
| // Losers' bid records must be deleted. | ||
| assert!( | ||
| s.client.get_bid(&s.hash, &s.alice).is_none(), | ||
| "alice bid should be gone after refund" | ||
| ); | ||
| assert!( | ||
| s.client.get_bid(&s.hash, &s.bob).is_none(), | ||
| "bob bid should be gone after refund" | ||
| ); | ||
|
|
||
| // Winner's record must remain. | ||
| assert!( | ||
| s.client.get_bid(&s.hash, &s.charlie).is_some(), | ||
| "winner bid must not be removed" | ||
| ); | ||
|
|
||
| // Auction state must still name Charlie as highest_bidder. | ||
| let state = s.client.get_auction(&s.hash); | ||
| assert_eq!(state.highest_bidder, Some(s.charlie.clone())); | ||
| } | ||
|
|
||
| // ── refund_losers — correct event count ─────────────────────────────── | ||
|
|
||
| #[test] | ||
| fn emits_one_event_per_loser() { | ||
| let s = Setup::new(); | ||
|
|
||
| s.client.place_bid(&s.hash, &s.alice, &200i128); // loser → event | ||
| s.client.place_bid(&s.hash, &s.bob, &300i128); // loser → event | ||
| s.client.place_bid(&s.hash, &s.charlie, &400i128); // winner → no event | ||
|
|
||
| s.close(); | ||
| s.client.refund_losers(&s.hash); | ||
|
|
||
| let rfnd_count = s | ||
| .env | ||
| .events() | ||
| .all() | ||
| .iter() | ||
| .filter(|e| { | ||
| e.1.get(0) | ||
| .map(|t| { | ||
| let sym: Symbol = t.into_val(&s.env); | ||
| sym == BID_REFUNDED | ||
| }) | ||
| .unwrap_or(false) | ||
| }) | ||
| .count(); | ||
|
|
||
| assert_eq!(rfnd_count, 2, "expected exactly 2 BID_RFND events"); | ||
| } | ||
|
|
||
| // ── refund_losers — idempotency ─────────────────────────────────────── | ||
|
|
||
| #[test] | ||
| fn second_call_is_noop_no_extra_events() { | ||
| let s = Setup::new(); | ||
|
|
||
| s.client.place_bid(&s.hash, &s.alice, &200i128); // loser | ||
| s.client.place_bid(&s.hash, &s.charlie, &400i128); // winner | ||
|
|
||
| s.close(); | ||
| s.client.refund_losers(&s.hash); // first call — refunds Alice | ||
| s.client.refund_losers(&s.hash); // second call — must be a no-op | ||
|
|
||
| // Still only one BID_RFND event total. | ||
| let rfnd_count = s | ||
| .env | ||
| .events() | ||
| .all() | ||
| .iter() | ||
| .filter(|e| { | ||
| e.1.get(0) | ||
| .map(|t| { | ||
| let sym: Symbol = t.into_val(&s.env); | ||
| sym == BID_REFUNDED | ||
| }) | ||
| .unwrap_or(false) | ||
| }) | ||
| .count(); | ||
|
|
||
| assert_eq!(rfnd_count, 1, "second call must not produce extra events"); | ||
| } | ||
|
|
||
| // ── refund_losers — trustless: no auth required ─────────────────────── | ||
|
|
||
| #[test] | ||
| fn refund_losers_requires_no_auth() { | ||
| // Build a closed auction with mocked auth. | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
|
|
||
| let creator = Address::generate(&env); | ||
| let contract_id = env.register_contract(None, AuctionContract); | ||
| let client = AuctionContractClient::new(&env, &contract_id); | ||
| let hash = test_hash(&env); | ||
|
|
||
| client.create_auction(&creator, &hash, &0u64, &1000u64, &100i128); | ||
| env.ledger().set(LedgerInfo { | ||
| timestamp: 1001, | ||
| ..env.ledger().get() | ||
| }); | ||
| client.close_auction(&creator, &hash); | ||
|
|
||
| // Clear the auth mock so only explicit require_auth calls would fail. | ||
| // refund_losers has no require_auth — it must still succeed. | ||
| let env2 = Env::default(); | ||
| let client2 = AuctionContractClient::new(&env2, &contract_id); | ||
| // Calling on the same contract address without mock_all_auths. | ||
| // Any require_auth inside refund_losers would panic here. | ||
| client2.refund_losers(&hash); | ||
| } | ||
|
|
||
| // ── place_bid — cannot bid on settled auction ───────────────────────── | ||
|
|
||
| #[test] | ||
| #[should_panic(expected = "auction already settled")] | ||
| fn place_bid_after_close_panics() { | ||
| let s = Setup::new(); | ||
| s.close(); | ||
| s.client.place_bid(&s.hash, &s.alice, &200i128); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Run cargo fmt to fix CI failures.
The pipeline reports multiple cargo fmt --check failures throughout this file (lines 71, 174, 207, 335, 344, 446, 480, 511, 576). Run cargo fmt to auto-fix all formatting issues before merging.
🧰 Tools
🪛 GitHub Actions: Checks
[error] 71-71: cargo fmt --check failed due to Rust formatting differences. Example change: assert! macro formatting was expected to be split across multiple lines.
[error] 174-174: cargo fmt --check failed due to Rust formatting differences. Example change: token::StellarAssetClient::new call should be formatted with line breaks.
[error] 207-207: cargo fmt --check failed due to Rust formatting differences. Example change: native_client.transfer call should be formatted with line breaks.
[error] 335-335: cargo fmt --check failed due to Rust formatting differences. Example change: create_auction argument formatting (line breaks/spacing) did not match rustfmt output.
[error] 344-344: cargo fmt --check failed due to Rust formatting differences. Example change: Setup struct literal formatting was expected in multi-line form.
[error] 446-446: cargo fmt --check failed due to Rust formatting differences. Example change: comment spacing/line formatting in place_bid calls.
[error] 480-480: cargo fmt --check failed due to Rust formatting differences. Example change: comment spacing/line formatting in place_bid calls.
[error] 511-511: cargo fmt --check failed due to Rust formatting differences. Example change: comment spacing/line formatting in place_bid call.
[error] 576-576: cargo fmt --check failed due to Rust formatting differences in test code around place_bid call.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway-contract/contracts/auction_contract/src/lib.rs` around lines 1 - 578,
The file fails rustfmt checks; run rustfmt (cargo fmt) to reformat the source so
the style errors are resolved across the contract and tests (affecting items
like the module headers, function signatures such as create_auction,
close_auction, refund_losers, place_bid, and the test module). Re-run cargo fmt
at the repo root and commit the resulting changes so cargo fmt --check passes in
CI; no behavioral code changes are required—only apply the automatic formatting
edits.
| let native_client = token::StellarAssetClient::new( | ||
| &env, | ||
| &env.current_contract_address(), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
How to get native XLM token address in Soroban SDK for transfers?
💡 Result:
To transfer native XLM via the Soroban token interface, you don’t use a special “XLM address” baked into soroban-sdk. Instead you use the Stellar Asset Contract (SAC) contract ID for the native asset (it’s deterministic per network) and pass that Address into token::TokenClient. The SAC is the standard way contracts interact with XLM and issued assets. [1]
Get the native XLM SAC contract address (off-chain)
Use the Stellar CLI to derive it:
stellar contract id asset --network <testnet|mainnet|...> --asset nativeThis returns the contract address (C...) you should use as the “XLM token address”. [2]
(Older Soroban CLI flows also expose this via soroban lab token id --asset native --network ....) [3]
Use it for transfers (on-chain contract code)
Once you have that address, treat it like any other SEP-41 token contract:
use soroban_sdk::{Env, Address};
use soroban_sdk::token::TokenClient;
pub fn pay_xlm(e: &Env, xlm_sac: &Address, from: &Address, to: &Address, amount: i128) {
let xlm = TokenClient::new(e, xlm_sac);
xlm.transfer(from, to, &amount);
}TokenClient works with any token contract address, including an SAC address. [4]
Sources
[1] Stellar docs: Stellar Asset Contract (SAC) overview / “only way for contracts to interact with…native XLM” https://developers.stellar.org/docs/tokens/stellar-asset-contract
[2] Stellar docs (CLI cookbook): stellar contract id asset --asset native https://developers.stellar.org/docs/tools/cli/cookbook/deploy-stellar-asset-contract
[3] Token Playground (CLI): soroban lab token id --asset native https://dev.to/esteblock/token-playground-chapter-9-read-the-native-token-xlm-using-soroban-cli-cb3
[4] Stellar docs: instantiate token::TokenClient with a token identifier/address and call transfer https://developers.stellar.org/docs/build/guides/tokens/stellar-asset-contract
🏁 Script executed:
# First, let's read the file around lines 177-180 to see the exact code
sed -n '170,190p' gateway-contract/contracts/auction_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 955
🏁 Script executed:
# Check the imports to see what types are being used
head -50 gateway-contract/contracts/auction_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 1649
🏁 Script executed:
# Search for other usages of native token or token client in this file
rg -n "token::" gateway-contract/contracts/auction_contract/src/lib.rs | head -20Repository: Alien-Protocol/Alien-Gateway
Length of output: 135
🏁 Script executed:
# Check if there's initialization code that stores native token address
rg -n "init\|new\|native" gateway-contract/contracts/auction_contract/src/lib.rs | head -30Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Look for the storage or data structures that might contain token addresses
rg -n "struct\|const\|static" gateway-contract/contracts/auction_contract/src/lib.rs | head -20Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Search for init function and how it might store native token address
rg -n "fn init\|fn new\|#\[contractimpl\]" gateway-contract/contracts/auction_contract/src/lib.rs -A 10Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Search for storage patterns - check the storage module
cat gateway-contract/contracts/auction_contract/src/storage.rs | head -100Repository: Alien-Protocol/Alien-Gateway
Length of output: 4073
🏁 Script executed:
# Search for how native token address might be accessed
rg -n "native\|token_address\|xlm\|XLM" gateway-contract/contracts/auction_contract/src/Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Look at the entire lib.rs to understand the contract structure better
wc -l gateway-contract/contracts/auction_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 130
🏁 Script executed:
# Check if there's any documentation or examples of how this contract uses native token
rg -n "refund\|transfer" gateway-contract/contracts/auction_contract/src/lib.rs | head -20Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Find the refund_losers function completely
rg -n "fn refund_losers" gateway-contract/contracts/auction_contract/src/lib.rs -A 70Repository: Alien-Protocol/Alien-Gateway
Length of output: 7858
🏁 Script executed:
# Search for any public contract functions (entry points)
rg -n "#\[contractimpl\]|pub fn " gateway-contract/contracts/auction_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 704
🏁 Script executed:
# Let's check how native_client is actually used after creation
sed -n '177,226p' gateway-contract/contracts/auction_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 1897
🏁 Script executed:
# Search for any comments or documentation about StellarAssetClient usage
rg -n "StellarAssetClient\|native_client" gateway-contract/contracts/auction_contract/src/lib.rs -B 3 -A 3Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Check if there's any setup or initialization happening elsewhere
find gateway-contract/contracts/auction_contract -name "*.rs" -type fRepository: Alien-Protocol/Alien-Gateway
Length of output: 358
🏁 Script executed:
# Check the Soroban SDK usage - look for any documentation or other token client patterns
rg -n "TokenClient\|token::" gateway-contract/contracts/auction_contract/src/ -B 1 -A 1Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Check if StellarAssetClient is imported from soroban_sdk
grep -n "use soroban_sdk" gateway-contract/contracts/auction_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 120
🏁 Script executed:
# Look at the create_auction function to see if there's any initialization pattern
sed -n '52,102p' gateway-contract/contracts/auction_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 1827
🏁 Script executed:
# Check other Soroban contracts in the repo to see how they handle native XLM
find gateway-contract -name "*.rs" -type f -exec grep -l "StellarAssetClient\|native.*token\|XLM" {} \;Repository: Alien-Protocol/Alien-Gateway
Length of output: 126
🏁 Script executed:
# Search for any Soroban SDK documentation or examples in comments
rg -n "developers.stellar\|soroban.*native\|SAC\|asset.*contract" gateway-contract/Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Let's check the Soroban SDK version or look for documentation in the repo
find gateway-contract -name "Cargo.toml" -type f | head -3Repository: Alien-Protocol/Alien-Gateway
Length of output: 206
🏁 Script executed:
# Check the Cargo.toml to see soroban-sdk version
cat gateway-contract/contracts/auction_contract/Cargo.tomlRepository: Alien-Protocol/Alien-Gateway
Length of output: 344
🏁 Script executed:
# Look for any README or documentation
find gateway-contract -name "README*" -o -name "*.md" | head -10Repository: Alien-Protocol/Alien-Gateway
Length of output: 310
🏁 Script executed:
# Let me check the test setup to see how the native token is supposed to work
sed -n '300,380p' gateway-contract/contracts/auction_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 2532
🏁 Script executed:
# Check the workspace Cargo.toml for soroban-sdk version
cat gateway-contract/Cargo.tomlRepository: Alien-Protocol/Alien-Gateway
Length of output: 632
🏁 Script executed:
# Let's look at the auction.md documentation
cat gateway-contract/contracts/auction_contract/auction.mdRepository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Search for how other contracts might handle token transfers
find gateway-contract -name "*.rs" -type f -exec grep -l "transfer\|token" {} \;Repository: Alien-Protocol/Alien-Gateway
Length of output: 405
🏁 Script executed:
# Check the escrow contract's token handling
grep -n "token\|transfer\|StellarAsset" gateway-contract/contracts/escrow_contract/src/lib.rs | head -30Repository: Alien-Protocol/Alien-Gateway
Length of output: 184
🏁 Script executed:
# Let's look at the escrow contract transfer implementation
rg -n "transfer|token::" gateway-contract/contracts/escrow_contract/src/lib.rs -B 2 -A 5Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Search for how the escrow contract gets the native token address
rg -n "native\|current_contract\|token.*address" gateway-contract/contracts/escrow_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Let's check the soroban-sdk v23 documentation by looking at usage patterns
web_search
Soroban SDK v23 StellarAssetClient vs TokenClient native XLMRepository: Alien-Protocol/Alien-Gateway
Length of output: 168
🌐 Web query:
Soroban SDK v23 StellarAssetClient native XLM token address
💡 Result:
In Soroban SDK v23 there isn’t a single “native XLM token address” you can hardcode. XLM is represented in contracts via the Stellar Asset Contract (SAC), whose contract ID (address) is deterministic per network and must be derived (or looked up) for the asset. (developers.stellar.org)
To get the XLM SAC contract address (ID), use the Stellar CLI with the native-asset identifier "native": (developers.stellar.org)
stellar contract id asset --asset native --network testnet
# or --network mainnetUse the resulting C… contract ID as the address you pass to TokenClient / StellarAssetClient. (developers.stellar.org)
Citations:
- 1: https://developers.stellar.org/docs/tokens/stellar-asset-contract
- 2: https://developers.stellar.org/docs/tools/cli/stellar-cli
- 3: https://developers.stellar.org/docs/tokens/stellar-asset-contract
🌐 Web query:
Soroban SDK TokenClient StellarAssetClient difference
💡 Result:
-
token::TokenClient: a generic Rust client for calling any Soroban token contract that implements the SEP-41 “TokenInterface” (e.g.,transfer,balance,approve/allowance, etc.). Use this when you want interoperability with any SEP-41 token (including Stellar Asset Contracts). [1][2] -
token::StellarAssetClient: a Rust client for calling the Stellar Asset Contract–specific interface (“StellarAssetInterface” / CAP-46-6 standardized asset), which includes extra SAC-only functions such asmint,clawback,set_admin,set_authorized, and related admin/authorization queries. Use this when you specifically need SAC behavior beyond SEP-41. [1][3]
Rule of thumb: default to TokenClient; switch to StellarAssetClient only when you need SAC-only admin/issuer controls (mint/clawback/authorization/admin changes). [1]
Sources:
[1] https://developers.stellar.org/docs/build/guides/tokens/stellar-asset-contract
[2] https://docs.rs/soroban-sdk/latest/soroban_sdk/token/struct.TokenClient.html
[3] https://docs.rs/soroban-sdk/latest/soroban_sdk/token/struct.StellarAssetClient.html
🏁 Script executed:
# Let's look at the entire refund_losers function and the native_client.transfer call more carefully
sed -n '173,215p' gateway-contract/contracts/auction_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 1782
Critical: Native token client initialized with wrong address.
StellarAssetClient::new is being passed env.current_contract_address(), which returns the auction contract's address, not the Stellar Asset Contract (SAC) address for native XLM. This will cause transfer calls to fail.
The SAC address for native XLM is deterministic per network and must be obtained via the Stellar CLI (stellar contract id asset --asset native --network <testnet|mainnet>), then either stored during contract initialization or accessed from storage. The current code does neither—the misleading comment claims the address is "stored at init time," but there is no such initialization logic.
To fix: store the native token SAC address and pass it to the token client, rather than using 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/auction_contract/src/lib.rs` around lines 177 -
180, The native token client is being constructed with
env.current_contract_address(), which is the auction contract's address; update
the code to load the SAC address stored at contract initialization (or stored in
contract storage) and pass that SAC address into StellarAssetClient::new instead
of env.current_contract_address(); locate the initialization logic (or add it)
to persist the native SAC address (e.g., in contract storage) and update any
calls that create the token client (StellarAssetClient::new) to read that stored
SAC address so transfer calls use the correct native asset contract.
| // INTERACTION: return XLM to the bidder. | ||
| native_client.transfer( | ||
| &env.current_contract_address(), | ||
| &bidder, | ||
| &refund_amount, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a deposit mechanism elsewhere
rg -n "transfer.*bidder" gateway-contract/contracts/auction_contract/
rg -n "native.*transfer" gateway-contract/contracts/auction_contract/Repository: Alien-Protocol/Alien-Gateway
Length of output: 296
🏁 Script executed:
cat -n gateway-contract/contracts/auction_contract/src/lib.rs | sed -n '242,280p'Repository: Alien-Protocol/Alien-Gateway
Length of output: 1889
🏁 Script executed:
rg -A 30 "fn place_bid" gateway-contract/contracts/auction_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 1378
🏁 Script executed:
rg -B 5 -A 15 "transfer.*amount\|transfer.*bid" gateway-contract/contracts/auction_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
rg -B 5 -A 40 "fn settle_auction" gateway-contract/contracts/auction_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
rg -n "native_client\|transfer\|deposit" gateway-contract/contracts/auction_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
cat -n gateway-contract/contracts/auction_contract/src/lib.rs | sed -n '190,230p'Repository: Alien-Protocol/Alien-Gateway
Length of output: 1901
🏁 Script executed:
rg -n "pub fn|fn " gateway-contract/contracts/auction_contract/src/lib.rs | head -20Repository: Alien-Protocol/Alien-Gateway
Length of output: 1198
🏁 Script executed:
cat -n gateway-contract/contracts/auction_contract/src/lib.rs | head -70Repository: Alien-Protocol/Alien-Gateway
Length of output: 2781
🏁 Script executed:
rg -B 5 -A 5 "native_client" gateway-contract/contracts/auction_contract/src/lib.rs | head -40Repository: Alien-Protocol/Alien-Gateway
Length of output: 1216
🏁 Script executed:
cat -n gateway-contract/contracts/auction_contract/src/lib.rs | sed -n '320,450p'Repository: Alien-Protocol/Alien-Gateway
Length of output: 4931
🏁 Script executed:
rg -B 10 -A 20 "fn new\(\)" gateway-contract/contracts/auction_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 995
place_bid must transfer XLM from bidder to contract for refund_losers to work.
The refund_losers function attempts to transfer XLM from the contract to losing bidders (line 210), but place_bid never transfers XLM from the bidder into the contract. Add the corresponding transfer in place_bid to deposit the bid amount, otherwise the contract will have no funds to refund and refund_losers will fail at runtime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway-contract/contracts/auction_contract/src/lib.rs` around lines 209 -
214, The auction currently refunds losers from contract balance but never
receives bidder funds; in the place_bid function add a call to
native_client.transfer to move the bid amount from the bidder into the contract
(use env.current_contract_address() as recipient and the bidder as sender with
the bid amount variable used in place_bid), so that refund_losers can later
transfer XLM from the contract to losing bidders; ensure the transfer uses the
same amount variable used for bids and handles any transfer result/error
consistently with other native_client.transfer calls.
| pub fn place_bid(env: Env, username_hash: BytesN<32>, bidder: Address, amount: i128) { | ||
| bidder.require_auth(); | ||
|
|
||
| let mut state = get_auction(&env, &username_hash).expect("auction not found"); | ||
|
|
||
| assert!(!state.is_settled, "auction already settled"); | ||
| assert!(amount > state.highest_bid, "bid must exceed highest bid"); | ||
|
|
||
| let bid = Bid { | ||
| bidder: bidder.clone(), | ||
| amount, | ||
| timestamp: env.ledger().timestamp(), | ||
| }; | ||
|
|
||
| // Maintain bidder list before writing the bid record so the | ||
| // AllBidders key is always at least as fresh as any Bid key. | ||
| add_bidder(&env, &username_hash, bidder.clone()); | ||
| set_bid(&env, &username_hash, &bidder, &bid); | ||
|
|
||
| state.highest_bid = amount; | ||
| state.highest_bidder = Some(bidder); | ||
| set_auction(&env, &username_hash, &state); | ||
| } |
There was a problem hiding this comment.
Missing reserve price validation and previous bid handling.
-
Reserve price not enforced: Bids are only checked against
highest_bid, notreserve_price. A first bid of 1 stroop would be accepted even ifreserve_priceis 100. -
Previous bid handling unclear: If a bidder places multiple bids (each overwriting the previous), the doc in
types.rssays "placing a new bid overwrites the previous record", but the previous bid amount isn't refunded or accounted for.
Suggested fix for reserve price
assert!(!state.is_settled, "auction already settled");
+assert!(amount >= state.reserve_price, "bid below reserve price");
assert!(amount > state.highest_bid, "bid must exceed highest bid");| #[test] | ||
| fn refund_losers_requires_no_auth() { | ||
| // Build a closed auction with mocked auth. | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
|
|
||
| let creator = Address::generate(&env); | ||
| let contract_id = env.register_contract(None, AuctionContract); | ||
| let client = AuctionContractClient::new(&env, &contract_id); | ||
| let hash = test_hash(&env); | ||
|
|
||
| client.create_auction(&creator, &hash, &0u64, &1000u64, &100i128); | ||
| env.ledger().set(LedgerInfo { | ||
| timestamp: 1001, | ||
| ..env.ledger().get() | ||
| }); | ||
| client.close_auction(&creator, &hash); | ||
|
|
||
| // Clear the auth mock so only explicit require_auth calls would fail. | ||
| // refund_losers has no require_auth — it must still succeed. | ||
| let env2 = Env::default(); | ||
| let client2 = AuctionContractClient::new(&env2, &contract_id); | ||
| // Calling on the same contract address without mock_all_auths. | ||
| // Any require_auth inside refund_losers would panic here. | ||
| client2.refund_losers(&hash); | ||
| } |
There was a problem hiding this comment.
Test logic is flawed — env2 doesn't have the contract state.
This test attempts to verify trustless operation by creating a fresh Env without mock_all_auths(), but:
env2is a completely separate environment that doesn't share state withenv- The contract registered on
envisn't accessible fromenv2 - This test likely passes only because it fails silently or the assertion logic is never reached
A correct approach would be to not call mock_all_auths() in a dedicated setup, then verify refund_losers succeeds while an authenticated method like close_auction would fail without auth.
Suggested test approach
#[test]
fn refund_losers_requires_no_auth() {
// Don't mock auth - real auth checks will apply
let env = Env::default();
let creator = Address::generate(&env);
let contract_id = env.register_contract(None, AuctionContract);
let client = AuctionContractClient::new(&env, &contract_id);
let hash = test_hash(&env);
// These need auth - mock just for setup
env.mock_all_auths();
client.create_auction(&creator, &hash, &0u64, &1000u64, &100i128);
env.ledger().set(LedgerInfo {
timestamp: 1001,
..env.ledger().get()
});
client.close_auction(&creator, &hash);
// Clear mock - from here, require_auth() calls would panic
// But refund_losers has no require_auth, so it should succeed
// Note: Soroban test env may not support clearing mocks mid-test;
// alternative is to verify via auth inspection
client.refund_losers(&hash); // should succeed without auth
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway-contract/contracts/auction_contract/src/lib.rs` around lines 542 -
567, The test currently creates a second Env (env2) which has no contract state
so it doesn't actually verify auth behavior; instead remove env2 and use the
same Env/contract/client (created via Env::default, env.register_contract and
AuctionContractClient::new) for the whole test, perform setup (create_auction
and close_auction) with env.mock_all_auths() only for the setup phase, then
clear the mock (or avoid mock_all_auths from the start and only mock around the
authenticated setup calls) and call refund_losers(&hash) on the same client to
assert it succeeds without require_auth; reference: refund_losers,
create_auction, close_auction, env.mock_all_auths, AuctionContractClient::new.
| pub fn add_bidder(env: &Env, hash: &BytesN<32>, bidder: Address) { | ||
| let key = DataKey::AllBidders(hash.clone()); | ||
| let mut bidders: Vec<Address> = env | ||
| .storage() | ||
| .persistent() | ||
| .get(&key) | ||
| .unwrap_or_else(|| Vec::new(env)); | ||
|
|
||
| // Deduplication: only append if the address is not already in the list. | ||
| for existing in bidders.iter() { | ||
| if existing == bidder { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| bidders.push_back(bidder); | ||
| env.storage().persistent().set(&key, &bidders); | ||
| env.storage() | ||
| .persistent() | ||
| .extend_ttl(&key, LEDGER_THRESHOLD, LEDGER_BUMP); | ||
| } No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify MAX_BATCH_SIZE constant exists and is enforced
rg -n "MAX_BATCH_SIZE" gateway-contract/Repository: Alien-Protocol/Alien-Gateway
Length of output: 194
🏁 Script executed:
# Check if MAX_BATCH_SIZE is defined in lib.rs
rg -n "MAX_BATCH_SIZE\s*=" gateway-contract/contracts/auction_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Get the actual code at line 260 and surrounding context to see the formatting issue
sed -n '255,265p' gateway-contract/contracts/auction_contract/src/storage.rs | cat -nRepository: Alien-Protocol/Alien-Gateway
Length of output: 317
🏁 Script executed:
# Check if lib.rs exists and search for any batch size or bidder limits
ls -la gateway-contract/contracts/auction_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 176
🏁 Script executed:
# Search across entire gateway-contract for MAX_BATCH_SIZE definition (not just references)
rg "MAX_BATCH_SIZE\s*[:=]" gateway-contract/Repository: Alien-Protocol/Alien-Gateway
Length of output: 54
🏁 Script executed:
# Look for any batch size related constants or validation
rg -i "batch.*size|max.*bidder" gateway-contract/contracts/auction_contract/src/Repository: Alien-Protocol/Alien-Gateway
Length of output: 190
🏁 Script executed:
# Search lib.rs for calls to add_bidder and any batch-related logic
rg -n "add_bidder|bidder" gateway-contract/contracts/auction_contract/src/lib.rs | head -30Repository: Alien-Protocol/Alien-Gateway
Length of output: 1924
🏁 Script executed:
# Get a broader view of lib.rs to understand the context
wc -l gateway-contract/contracts/auction_contract/src/lib.rs && head -100 gateway-contract/contracts/auction_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway
Length of output: 3328
Remove misleading doc comment about MAX_BATCH_SIZE enforcement.
The doc comment claims MAX_BATCH_SIZE is enforced at the call site in lib.rs, but this constant does not exist anywhere in the codebase, nor is any batch size validation performed in place_bid. Either remove the misleading claim or implement the enforced limit it promises.
🧰 Tools
🪛 GitHub Actions: Checks
[error] 260-260: cargo fmt --check failed due to Rust formatting differences (blank line/formatting near persistent().extend_ttl(...)).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway-contract/contracts/auction_contract/src/storage.rs` around lines 242
- 262, The doc comment claiming a nonexistent MAX_BATCH_SIZE is enforced is
misleading; either remove or correct that comment in the place that documents
place_bid/lib.rs, or implement the promised enforcement: define a constant
MAX_BATCH_SIZE and add a check in place_bid (returning an error or rejecting
bids when batch size > MAX_BATCH_SIZE) before proceeding. Locate references to
place_bid and the documentation in lib.rs (and any doc comments near add_bidder
if present) and either update the text to accurately describe current behavior
or add the MAX_BATCH_SIZE constant and batch-size validation in place_bid so the
comment becomes truthful.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/src/lib.rs`:
- Around line 69-77: Add a validation that end_time is strictly greater than
start_time in the auction-creation path: after the creator.require_auth() call
and before checking/creating the auction (around where has_auction(&env,
&username_hash) is used), insert an assertion or conditional that enforces
end_time > start_time (e.g., assert!(end_time > start_time, "end_time must be
greater than start_time")). This ensures the auction window is valid before
proceeding to create the auction.
- Around line 245-248: The bid path allows bids before the auction opens; update
the bid validation in the function using get_auction (where you check
state.is_settled and amount > state.highest_bid) to also verify the current
chain time by calling env.ledger().timestamp() and comparing it to
state.start_time (e.g. assert env.ledger().timestamp() >= state.start_time or
return an appropriate error/message if the auction hasn't started) before
accepting the bid.
🪄 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: e25e445e-f879-4b93-be0e-7298c13c98f0
📒 Files selected for processing (2)
gateway-contract/contracts/auction_contract/src/lib.rsgateway-contract/contracts/auction_contract/src/storage.rs
| start_time: u64, | ||
| end_time: u64, | ||
| reserve_price: i128, | ||
| ) { | ||
| creator.require_auth(); | ||
| assert!( | ||
| !has_auction(&env, &username_hash), | ||
| "auction already exists" | ||
| ); |
There was a problem hiding this comment.
No validation that end_time > start_time in auction creation.
The function accepts any start_time and end_time values without verifying the auction window is valid. An auction with end_time <= start_time would be immediately closed or have undefined behavior.
Suggested fix
creator.require_auth();
assert!(
!has_auction(&env, &username_hash),
"auction already exists"
);
+assert!(end_time > start_time, "end_time must be after start_time");📝 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.
| start_time: u64, | |
| end_time: u64, | |
| reserve_price: i128, | |
| ) { | |
| creator.require_auth(); | |
| assert!( | |
| !has_auction(&env, &username_hash), | |
| "auction already exists" | |
| ); | |
| start_time: u64, | |
| end_time: u64, | |
| reserve_price: i128, | |
| ) { | |
| creator.require_auth(); | |
| assert!( | |
| !has_auction(&env, &username_hash), | |
| "auction already exists" | |
| ); | |
| assert!(end_time > start_time, "end_time must be after start_time"); |
🧰 Tools
🪛 GitHub Actions: Checks
[error] 71-71: cargo fmt --check failed. rustfmt reported formatting changes needed (e.g., assert! formatting collapsed to single line).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway-contract/contracts/auction_contract/src/lib.rs` around lines 69 - 77,
Add a validation that end_time is strictly greater than start_time in the
auction-creation path: after the creator.require_auth() call and before
checking/creating the auction (around where has_auction(&env, &username_hash) is
used), insert an assertion or conditional that enforces end_time > start_time
(e.g., assert!(end_time > start_time, "end_time must be greater than
start_time")). This ensures the auction window is valid before proceeding to
create the auction.
| let mut state = get_auction(&env, &username_hash).expect("auction not found"); | ||
|
|
||
| // Get winner and winning bid | ||
| let winner = storage::get_highest_bidder(&env); | ||
| let winning_bid = storage::get_highest_bid(&env); | ||
| assert!(!state.is_settled, "auction already settled"); | ||
| assert!(amount > state.highest_bid, "bid must exceed highest bid"); |
There was a problem hiding this comment.
Bids can be placed before the auction starts.
The function validates the auction exists and isn't settled, but doesn't check if env.ledger().timestamp() >= state.start_time. Users could place bids before the auction officially opens.
Suggested fix
let mut state = get_auction(&env, &username_hash).expect("auction not found");
assert!(!state.is_settled, "auction already settled");
+assert!(
+ env.ledger().timestamp() >= state.start_time,
+ "auction not yet started"
+);
+assert!(amount >= state.reserve_price, "bid below reserve price");
assert!(amount > state.highest_bid, "bid must exceed highest bid");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway-contract/contracts/auction_contract/src/lib.rs` around lines 245 -
248, The bid path allows bids before the auction opens; update the bid
validation in the function using get_auction (where you check state.is_settled
and amount > state.highest_bid) to also verify the current chain time by calling
env.ledger().timestamp() and comparing it to state.start_time (e.g. assert
env.ledger().timestamp() >= state.start_time or return an appropriate
error/message if the auction hasn't started) before accepting the bid.
2 participants
Hi, kindly review the PR
closes #100
Summary by CodeRabbit
New Features
Tests