refactor: unify AuctionContract Result APIs and structured events#414
Conversation
…d claim functionality
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 12 minutes and 30 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR refactors the AuctionContract to unify error handling by replacing panic-based failures with explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…d-structured-events
|
@Amarjeet325 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: 2
🧹 Nitpick comments (4)
onchain/contracts/auction_contract/src/events.rs (1)
4-8: Oldsymbol_short!constants are now dead code.With
#[contractevent]structs publishing events via.publish(env)(which uses snake_case of the struct name as the event symbol, e.g.bid_placed_event), these manualSymbolconstants are no longer referenced by any emission path. They're only kept alive by_touch_event_symbolsinlib.rs:20-28. Consider removing both the constants here and the_touch_event_symbolshelper + imports inlib.rsas part of the cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/auction_contract/src/events.rs` around lines 4 - 8, Remove the unused manual Symbol constants (AUCTION_CREATED, BID_PLACED, AUCTION_CLOSED, USERNAME_CLAIMED, BID_REFUNDED) from events.rs and delete the related dead-code touch helper and its imports in lib.rs (the `_touch_event_symbols` function and any imports solely used to keep those constants alive); ensure no other code references these constants before removing them and run tests to verify contract event emission now relies on the #[contractevent] structs' snake_case symbols.onchain/contracts/auction_contract/src/lib.rs (2)
102-104: Semantic misuse:AlreadyClaimedreturned for a double-refund case.
AlreadyClaimed(1002) refers to username claim semantics; using it here conflates two distinct domain concepts and makes off-chain error handling ambiguous (a client can't tell whether a refund or a claim was duplicated). Consider adding a dedicatedAlreadyRefundedvariant inerrors.rsand returning it here; updatetest_refund_bid_double_refund_panicsaccordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/auction_contract/src/lib.rs` around lines 102 - 104, The code currently returns errors::AuctionError::AlreadyClaimed from the refund guard when storage::auction_is_bid_refunded(&env, id, &bidder) is true, which is the wrong semantic; add a new enum variant AuctionError::AlreadyRefunded in errors.rs, update any matching arms/exports, change the return here to Err(errors::AuctionError::AlreadyRefunded), and adjust the test test_refund_bid_double_refund_panics to expect AlreadyRefunded instead of AlreadyClaimed.
14-28:_touch_event_symbolsand the old symbol imports can be removed.Since events now use
#[contractevent]-derived structs which auto-generate their own event symbols on.publish(env), the manualsymbol_short!constants inevents.rsand the_touch_event_symbolsshim keeping them alive against WASM linker stripping are no longer needed. Dropping them simplifies the module and removes dead symbols from the compiled artifact. See the related comment onevents.rs:4-8.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/auction_contract/src/lib.rs` around lines 14 - 28, Remove the dead shim and old imports: delete the `_touch_event_symbols` function and its tuple reference to AUCTION_CREATED, BID_PLACED, AUCTION_CLOSED, USERNAME_CLAIMED, BID_REFUNDED from lib.rs, and also remove any now-unused `use crate::events::{...}` lines that only served that shim; ensure no other code references `_touch_event_symbols` and run a build to confirm the redundant symbol constants and imports can be dropped safely.onchain/contracts/auction_contract/src/indexed.rs (1)
16-24: Error variants are overloaded across semantically distinct conditions.Several distinct failure modes collapse to the same variant, which makes off-chain error handling ambiguous:
create_auctionreturnsAuctionNotOpenfor "auction already exists" (Line 17) andAuctionNotClosedfor "end_time in the past" (Line 20).place_bidreturnsAuctionNotOpenfor "auction missing" (Line 41), "status not Open" (Line 44), and "past end_time" (Line 48) — three different causes under one code.Consider introducing more precise variants (e.g.,
AuctionAlreadyExists,InvalidEndTime,AuctionExpired) so clients can distinguish the actual failure. This was an acceptance criterion of the refactor ("consistent structured errors"), and the tests intest.rscurrently only pin error codes, not cause.Also applies to: 40-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@onchain/contracts/auction_contract/src/indexed.rs` around lines 16 - 24, The current errors::AuctionError enum overloads variants causing ambiguity; add distinct variants like AuctionAlreadyExists, InvalidEndTime (or AuctionEndInPast), AuctionMissing, AuctionNotOpenStatus, and AuctionExpired, then update create_auction to return AuctionAlreadyExists when storage::auction_exists(env, id) is true and InvalidEndTime when end_time <= env.ledger().timestamp(), and update place_bid to return AuctionMissing when storage::auction_exists(env, id) is false, AuctionNotOpenStatus when the stored status != Open, and AuctionExpired when end_time <= env.ledger().timestamp(); also update any match arms, tests (test.rs) and places that construct/compare AuctionError to use the new variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@onchain/contracts/auction_contract/src/indexed.rs`:
- Around line 79-95: The close_auction_by_id function currently lacks a guard on
the auction status allowing duplicate closes; update close_auction_by_id to
fetch the current status via storage::auction_get_status(env, id) and verify it
equals types::AuctionStatus::Open before proceeding, returning
errors::AuctionError::AuctionNotOpen (or another appropriate error) if not Open,
then only call storage::auction_set_status(env, id,
types::AuctionStatus::Closed) and emit events after that check; keep existing
timestamp and existence checks but insert the status guard immediately after
existence validation.
In `@onchain/contracts/auction_contract/src/lib.rs`:
- Line 122: The refund event is using a hardcoded zero username_hash which
diverges from other emitters; replace the literal &[0u8; 32] passed to
events::emit_bid_refunded in lib.rs with the auction's stored username hash by
calling storage::auction_get_username_hash(env, id) (the same lookup used in
indexed.rs) and pass that value into emit_bid_refunded so the refund event uses
the actual username_hash for that auction ID.
---
Nitpick comments:
In `@onchain/contracts/auction_contract/src/events.rs`:
- Around line 4-8: Remove the unused manual Symbol constants (AUCTION_CREATED,
BID_PLACED, AUCTION_CLOSED, USERNAME_CLAIMED, BID_REFUNDED) from events.rs and
delete the related dead-code touch helper and its imports in lib.rs (the
`_touch_event_symbols` function and any imports solely used to keep those
constants alive); ensure no other code references these constants before
removing them and run tests to verify contract event emission now relies on the
#[contractevent] structs' snake_case symbols.
In `@onchain/contracts/auction_contract/src/indexed.rs`:
- Around line 16-24: The current errors::AuctionError enum overloads variants
causing ambiguity; add distinct variants like AuctionAlreadyExists,
InvalidEndTime (or AuctionEndInPast), AuctionMissing, AuctionNotOpenStatus, and
AuctionExpired, then update create_auction to return AuctionAlreadyExists when
storage::auction_exists(env, id) is true and InvalidEndTime when end_time <=
env.ledger().timestamp(), and update place_bid to return AuctionMissing when
storage::auction_exists(env, id) is false, AuctionNotOpenStatus when the stored
status != Open, and AuctionExpired when end_time <= env.ledger().timestamp();
also update any match arms, tests (test.rs) and places that construct/compare
AuctionError to use the new variants.
In `@onchain/contracts/auction_contract/src/lib.rs`:
- Around line 102-104: The code currently returns
errors::AuctionError::AlreadyClaimed from the refund guard when
storage::auction_is_bid_refunded(&env, id, &bidder) is true, which is the wrong
semantic; add a new enum variant AuctionError::AlreadyRefunded in errors.rs,
update any matching arms/exports, change the return here to
Err(errors::AuctionError::AlreadyRefunded), and adjust the test
test_refund_bid_double_refund_panics to expect AlreadyRefunded instead of
AlreadyClaimed.
- Around line 14-28: Remove the dead shim and old imports: delete the
`_touch_event_symbols` function and its tuple reference to AUCTION_CREATED,
BID_PLACED, AUCTION_CLOSED, USERNAME_CLAIMED, BID_REFUNDED from lib.rs, and also
remove any now-unused `use crate::events::{...}` lines that only served that
shim; ensure no other code references `_touch_event_symbols` and run a build to
confirm the redundant symbol constants and imports can be dropped safely.
🪄 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: 2063cb05-30e3-483e-937d-6d717ebdd876
📒 Files selected for processing (6)
onchain/contracts/auction_contract/src/events.rsonchain/contracts/auction_contract/src/indexed.rsonchain/contracts/auction_contract/src/lib.rsonchain/contracts/auction_contract/src/singleton.rsonchain/contracts/auction_contract/src/storage.rsonchain/contracts/auction_contract/src/test.rs
| pub fn close_auction_by_id(env: &Env, id: u32) -> Result<(), errors::AuctionError> { | ||
| if !storage::auction_exists(env, id) { | ||
| soroban_sdk::panic_with_error!(env, errors::AuctionError::AuctionNotOpen); | ||
| return Err(errors::AuctionError::AuctionNotOpen); | ||
| } | ||
| let end_time = storage::auction_get_end_time(env, id); | ||
| if env.ledger().timestamp() < end_time { | ||
| soroban_sdk::panic_with_error!(env, errors::AuctionError::AuctionNotClosed); | ||
| return Err(errors::AuctionError::AuctionNotClosed); | ||
| } | ||
| storage::auction_set_status(env, id, types::AuctionStatus::Closed); | ||
|
|
||
| let username_hash = storage::auction_get_username_hash(env, id); | ||
| let winner = storage::auction_get_highest_bidder(env, id); | ||
| let winning_bid = storage::auction_get_highest_bid(env, id) as u128; | ||
| events::emit_auction_closed(env, &username_hash, winner, winning_bid); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Missing status guard in close_auction_by_id allows duplicate close events.
There's no check that the auction is currently Open. After the auction has been closed (or even claimed — auction_set_claimed is a separate flag and does not mutate Status), any caller can re-invoke close_auction_by_id since:
auction_existsstill returnstrueenv.ledger().timestamp() >= end_timestill holds- No auth is required on this entrypoint (see
lib.rs:127)
This re-emits auction_closed_event with the stored (possibly stale) highest bidder/bid, which will confuse off-chain indexers and event consumers. Add an explicit AuctionStatus::Open guard up front.
🛡️ Proposed fix
pub fn close_auction_by_id(env: &Env, id: u32) -> Result<(), errors::AuctionError> {
if !storage::auction_exists(env, id) {
return Err(errors::AuctionError::AuctionNotOpen);
}
+ if storage::auction_get_status(env, id) != types::AuctionStatus::Open {
+ return Err(errors::AuctionError::AuctionNotOpen);
+ }
let end_time = storage::auction_get_end_time(env, id);
if env.ledger().timestamp() < end_time {
return Err(errors::AuctionError::AuctionNotClosed);
}
storage::auction_set_status(env, id, types::AuctionStatus::Closed);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@onchain/contracts/auction_contract/src/indexed.rs` around lines 79 - 95, The
close_auction_by_id function currently lacks a guard on the auction status
allowing duplicate closes; update close_auction_by_id to fetch the current
status via storage::auction_get_status(env, id) and verify it equals
types::AuctionStatus::Open before proceeding, returning
errors::AuctionError::AuctionNotOpen (or another appropriate error) if not Open,
then only call storage::auction_set_status(env, id,
types::AuctionStatus::Closed) and emit events after that check; keep existing
timestamp and existence checks but insert the status guard immediately after
existence validation.
3 participants
Closes #386
Summary
Refactored the AuctionContract to unify error handling and standardize structured event emission across all auction flows.
Changes Made
Result<_, AuctionError>Internal Improvements
Tests
Result-based APIsValidation
cargo testpassescargo clippypasses with no warningsSummary by CodeRabbit
Refactor
Tests