Closed
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! 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces a stub auction contract with a full auction implementation: new types ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Contract as Auction Contract
participant Storage as On-Chain Storage
Client->>Contract: create_auction(creator, hash, start_time, end_time, reserve_price)
activate Contract
Contract->>Contract: creator.require_auth()
Contract->>Storage: has_auction(hash)
Storage-->>Contract: false
Contract->>Storage: set_auction(hash, AuctionState)
Storage-->>Contract: OK
Contract-->>Client: OK
deactivate Contract
sequenceDiagram
participant Client
participant Contract as Auction Contract
participant Storage as On-Chain Storage
Client->>Contract: place_bid(hash, bidder, amount)
activate Contract
Contract->>Contract: bidder.require_auth()
Contract->>Storage: get_auction(hash)
Storage-->>Contract: AuctionState
Contract->>Contract: assert(amount > highest_bid)
Contract->>Storage: get_all_bidders(hash)
Storage-->>Contract: Vec<Address>
Contract->>Storage: add_bidder(hash, bidder)
Storage-->>Contract: OK
Contract->>Storage: set_bid(hash, bidder, Bid)
Storage-->>Contract: OK
Contract->>Storage: set_auction(hash, updated_state)
Storage-->>Contract: OK
Contract-->>Client: OK
deactivate Contract
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 30-51: The create_auction function lacks validation of time
bounds; update create_auction to assert that end_time > start_time and also
check start_time is not in the past by comparing start_time against the current
ledger timestamp (obtainable from Env via env.ledger().timestamp() or
equivalent), returning/aborting with a clear error message if either check
fails; keep the existing creator.require_auth() and has_auction check and
perform these time validations before constructing and calling set_auction.
- Around line 78-98: place_bid currently allows bids after auction end or after
settlement and never enforces the reserve price; update place_bid to (1) fetch
env.ledger().timestamp() into a variable and assert timestamp <= state.end_time
with a clear message, (2) assert state.is_settled == false, and (3) enforce
reserve price for the first meaningful bid by requiring that if
state.highest_bid is 0 (or None depending on representation) the incoming amount
>= state.reserve_price, otherwise require amount > state.highest_bid; reference
the existing symbols place_bid, get_auction, state.end_time, state.is_settled,
state.reserve_price, state.highest_bid, and env.ledger().timestamp() when adding
these checks before creating and recording the Bid.
In `@gateway-contract/contracts/auction_contract/src/storage.rs`:
- Around line 223-246: The doc comment in add_bidder refers to a non-existent
MAX_BATCH_SIZE; either remove or correct that reference: update the comment
above pub fn add_bidder to state that deduplication is O(n) and relies on
caller-enforced bounds (or the protocol-level expected small bidder count), or
alternatively define a concrete constant (e.g. MAX_BIDDERS) and enforce it where
bidders are added (check length in add_bidder after loading
DataKey::AllBidders(hash) and return/error if >= MAX_BIDDERS) so the
documentation matches the actual enforcement; ensure you modify the
DataKey::AllBidders/add_bidder comment and any caller sites that should enforce
the bound to keep behavior consistent.
In `@gateway-contract/contracts/auction_contract/src/types.rs`:
- Line 1: Remove the unused imports BytesN, Env, and Vec from the top-level use
statement in types.rs so only contracttype and Address are imported; locate the
use line that currently reads use soroban_sdk::{contracttype, Address, BytesN,
Env, Vec}; and edit it to import only contracttype and Address to resolve the
build failure caused by unused imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6622892d-29a7-4cd9-bde9-dbc0091e3b32
📒 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
Comment on lines
+30
to
+51
| pub fn create_auction( | ||
| env: Env, | ||
| creator: Address, | ||
| hash: BytesN<32>, | ||
| start_time: u64, | ||
| end_time: u64, | ||
| reserve_price: i128, | ||
| ) { | ||
| creator.require_auth(); | ||
| assert!(!has_auction(&env, &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, &hash, &state); | ||
| } |
There was a problem hiding this comment.
Missing validation for auction time constraints.
The function doesn't validate that end_time > start_time. An auction with invalid time bounds would be permanently unusable since bids could never be placed within a valid window. Consider also validating that start_time is not in the past.
🛡️ Proposed fix
pub fn create_auction(
env: Env,
creator: Address,
hash: BytesN<32>,
start_time: u64,
end_time: u64,
reserve_price: i128,
) {
creator.require_auth();
assert!(!has_auction(&env, &hash), "auction already exists");
+ assert!(end_time > start_time, "end_time must be after start_time");
+ assert!(reserve_price >= 0, "reserve_price cannot be negative");
let state = AuctionState {📝 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
| pub fn create_auction( | |
| env: Env, | |
| creator: Address, | |
| hash: BytesN<32>, | |
| start_time: u64, | |
| end_time: u64, | |
| reserve_price: i128, | |
| ) { | |
| creator.require_auth(); | |
| assert!(!has_auction(&env, &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, &hash, &state); | |
| } | |
| pub fn create_auction( | |
| env: Env, | |
| creator: Address, | |
| hash: BytesN<32>, | |
| start_time: u64, | |
| end_time: u64, | |
| reserve_price: i128, | |
| ) { | |
| creator.require_auth(); | |
| assert!(!has_auction(&env, &hash), "auction already exists"); | |
| assert!(end_time > start_time, "end_time must be after start_time"); | |
| assert!(reserve_price >= 0, "reserve_price cannot be negative"); | |
| let state = AuctionState { | |
| creator, | |
| start_time, | |
| end_time, | |
| reserve_price, | |
| highest_bid: 0, | |
| highest_bidder: None, | |
| is_settled: false, | |
| }; | |
| set_auction(&env, &hash, &state); | |
| } |
🤖 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 30 - 51,
The create_auction function lacks validation of time bounds; update
create_auction to assert that end_time > start_time and also check start_time is
not in the past by comparing start_time against the current ledger timestamp
(obtainable from Env via env.ledger().timestamp() or equivalent),
returning/aborting with a clear error message if either check fails; keep the
existing creator.require_auth() and has_auction check and perform these time
validations before constructing and calling set_auction.
Comment on lines
+78
to
+98
| pub fn place_bid(env: Env, hash: BytesN<32>, bidder: Address, amount: i128) { | ||
| bidder.require_auth(); | ||
|
|
||
| let mut state = get_auction(&env, &hash).expect("auction not found"); | ||
| assert!(amount > state.highest_bid, "bid must exceed highest bid"); | ||
|
|
||
| let bid = Bid { | ||
| bidder: bidder.clone(), | ||
| amount, | ||
| timestamp: env.ledger().timestamp(), | ||
| }; | ||
|
|
||
| // Update the bidder list before writing the bid record so that the | ||
| // AllBidders key is always at least as fresh as any Bid key. | ||
| add_bidder(&env, &hash, bidder.clone()); | ||
| set_bid(&env, &hash, &bidder, &bid); | ||
|
|
||
| state.highest_bid = amount; | ||
| state.highest_bidder = Some(bidder); | ||
| set_auction(&env, &hash, &state); | ||
| } |
There was a problem hiding this comment.
Missing critical bid validation: auction must be active and not settled.
place_bid allows bids on auctions that have ended (env.ledger().timestamp() > end_time) or been settled (is_settled == true). Additionally, the first bid should meet or exceed reserve_price, but this is never checked.
🔧 Proposed fix
pub fn place_bid(env: Env, hash: BytesN<32>, bidder: Address, amount: i128) {
bidder.require_auth();
let mut state = get_auction(&env, &hash).expect("auction not found");
+ let now = env.ledger().timestamp();
+ assert!(!state.is_settled, "auction is already settled");
+ assert!(now >= state.start_time, "auction has not started");
+ assert!(now <= state.end_time, "auction has ended");
assert!(amount > state.highest_bid, "bid must exceed highest bid");
+ assert!(amount >= state.reserve_price, "bid must meet reserve price");
let bid = Bid {
bidder: bidder.clone(),
amount,
- timestamp: env.ledger().timestamp(),
+ timestamp: now,
};📝 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
| pub fn place_bid(env: Env, hash: BytesN<32>, bidder: Address, amount: i128) { | |
| bidder.require_auth(); | |
| let mut state = get_auction(&env, &hash).expect("auction not found"); | |
| assert!(amount > state.highest_bid, "bid must exceed highest bid"); | |
| let bid = Bid { | |
| bidder: bidder.clone(), | |
| amount, | |
| timestamp: env.ledger().timestamp(), | |
| }; | |
| // Update the bidder list before writing the bid record so that the | |
| // AllBidders key is always at least as fresh as any Bid key. | |
| add_bidder(&env, &hash, bidder.clone()); | |
| set_bid(&env, &hash, &bidder, &bid); | |
| state.highest_bid = amount; | |
| state.highest_bidder = Some(bidder); | |
| set_auction(&env, &hash, &state); | |
| } | |
| pub fn place_bid(env: Env, hash: BytesN<32>, bidder: Address, amount: i128) { | |
| bidder.require_auth(); | |
| let mut state = get_auction(&env, &hash).expect("auction not found"); | |
| let now = env.ledger().timestamp(); | |
| assert!(!state.is_settled, "auction is already settled"); | |
| assert!(now >= state.start_time, "auction has not started"); | |
| assert!(now <= state.end_time, "auction has ended"); | |
| assert!(amount > state.highest_bid, "bid must exceed highest bid"); | |
| assert!(amount >= state.reserve_price, "bid must meet reserve price"); | |
| let bid = Bid { | |
| bidder: bidder.clone(), | |
| amount, | |
| timestamp: now, | |
| }; | |
| // Update the bidder list before writing the bid record so that the | |
| // AllBidders key is always at least as fresh as any Bid key. | |
| add_bidder(&env, &hash, bidder.clone()); | |
| set_bid(&env, &hash, &bidder, &bid); | |
| state.highest_bid = amount; | |
| state.highest_bidder = Some(bidder); | |
| set_auction(&env, &hash, &state); | |
| } |
🤖 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 78 - 98,
place_bid currently allows bids after auction end or after settlement and never
enforces the reserve price; update place_bid to (1) fetch
env.ledger().timestamp() into a variable and assert timestamp <= state.end_time
with a clear message, (2) assert state.is_settled == false, and (3) enforce
reserve price for the first meaningful bid by requiring that if
state.highest_bid is 0 (or None depending on representation) the incoming amount
>= state.reserve_price, otherwise require amount > state.highest_bid; reference
the existing symbols place_bid, get_auction, state.end_time, state.is_settled,
state.reserve_price, state.highest_bid, and env.ledger().timestamp() when adding
these checks before creating and recording the Bid.
Comment on lines
+223
to
+246
| /// # Implementation note | ||
| /// The deduplication check is O(n) in the number of existing bidders. This | ||
| /// is acceptable because the number of bidders per auction is bounded by | ||
| /// `MAX_BATCH_SIZE` enforced at the call site in `lib.rs`. | ||
| 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); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for MAX_BATCH_SIZE definition in the codebase
rg -n 'MAX_BATCH_SIZE' --type rustRepository: Alien-Protocol/Alien-Gateway
Length of output: 194
Remove or clarify the reference to MAX_BATCH_SIZE in the documentation comment.
The comment references MAX_BATCH_SIZE as a bound enforced at the call site, but this constant does not exist in the codebase. Either define this constant and enforce it, or update the documentation to accurately describe the actual bounds/safeguards in place.
🤖 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 223
- 246, The doc comment in add_bidder refers to a non-existent MAX_BATCH_SIZE;
either remove or correct that reference: update the comment above pub fn
add_bidder to state that deduplication is O(n) and relies on caller-enforced
bounds (or the protocol-level expected small bidder count), or alternatively
define a concrete constant (e.g. MAX_BIDDERS) and enforce it where bidders are
added (check length in add_bidder after loading DataKey::AllBidders(hash) and
return/error if >= MAX_BIDDERS) so the documentation matches the actual
enforcement; ensure you modify the DataKey::AllBidders/add_bidder comment and
any caller sites that should enforce the bound to keep behavior consistent.
| @@ -0,0 +1,48 @@ | |||
| use soroban_sdk::{contracttype, Address, BytesN, Env, Vec}; | |||
There was a problem hiding this comment.
Remove unused imports to fix the build failure.
The pipeline is failing because BytesN, Env, and Vec are imported but never used in this file. Only contracttype and Address are needed.
🔧 Proposed fix
-use soroban_sdk::{contracttype, Address, BytesN, Env, Vec};
+use soroban_sdk::{contracttype, Address};📝 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
| use soroban_sdk::{contracttype, Address, BytesN, Env, Vec}; | |
| use soroban_sdk::{contracttype, Address}; |
🧰 Tools
🪛 GitHub Actions: Checks
[error] 1-1: cargo clippy failed: unused imports BytesN, Env, and Vec. Error: -D unused-imports implied by -D warnings.
[error] 1-1: Build failed for crate auction_contract due to previous error (clippy reported unused imports).
🤖 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, Remove
the unused imports BytesN, Env, and Vec from the top-level use statement in
types.rs so only contracttype and Address are imported; locate the use line that
currently reads use soroban_sdk::{contracttype, Address, BytesN, Env, Vec}; and
edit it to import only contracttype and Address to resolve the build failure
caused by unused imports.
Contributor
|
@beebozy , fix this code today |
Author
|
Okay @ryzen-xp.. If you mean I should update the fork and re implement.. I won't be able to do it today.. tomorrow I should because, I have been busy outside of drips.. I hope you understand |
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.
hi, kindly go through the implementation.. Closes #95
Summary by CodeRabbit