Fix #351 — burn_for_basket early return for zero/empty recipients#424
Merged
Junman140 merged 1 commit intoJun 25, 2026
Merged
Conversation
|
@temisan0x 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! 🚀 |
📝 WalkthroughWalkthroughThe burning contract switches oracle calls to ChangesBurning Contract Oracle, Basket, and Admin Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BurningContract
participant Oracle
participant SToken
participant ReserveTracker
rect rgba(70, 130, 180, 0.5)
Note over Caller,ReserveTracker: redeem_basket flow
Caller->>BurningContract: redeem_basket(acbu_amount, recipients)
BurningContract->>BurningContract: reject if recipients empty
BurningContract->>BurningContract: compute & validate basket weights
loop per currency leg i
BurningContract->>Oracle: ORACLE_GET_ACBU_RATE_WITH_TS
Oracle-->>BurningContract: (acbu_rate, oracle_ts)
BurningContract->>Oracle: ORACLE_GET_RATE_WITH_TS(currency_i)
Oracle-->>BurningContract: (currency_rate, oracle_ts)
BurningContract->>BurningContract: compute net_acbu_i → native_i
BurningContract->>ReserveTracker: check reserve sufficiency
ReserveTracker-->>BurningContract: ok
BurningContract->>SToken: transfer_from(spender, recipient_i, native_i)
BurningContract->>BurningContract: emit BurnEvent(leg i)
end
BurningContract->>BurningContract: assert Σgross == acbu_amount && Σfee == total_fee
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
acbu_burning/src/lib.rs (1)
160-170: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject future-dated oracle timestamps too.
The freshness check only rejects old timestamps. A future
oracle_timestamp/rate_timestamppasses and extends the accepted freshness window.Proposed fix
- if current_time > oracle_timestamp.saturating_add(UPDATE_INTERVAL_SECONDS) { + if oracle_timestamp > current_time + || current_time > oracle_timestamp.saturating_add(UPDATE_INTERVAL_SECONDS) + { env.panic_with_error(ContractError::OracleError); }Apply the same pattern to each
rate_timestampcheck.Also applies to: 312-413
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@acbu_burning/src/lib.rs` around lines 160 - 170, The freshness checks in the oracle lookup flow only reject stale timestamps, so future-dated values can incorrectly pass; update the timestamp validation around the oracle fetch logic in the rate retrieval path to reject both old and future timestamps. Apply the same fix wherever the timestamp freshness pattern is used, including the checks tied to oracle_timestamp and rate_timestamp in the contract’s rate-reading functions, by comparing against current_time before accepting the value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@acbu_burning/src/lib.rs`:
- Around line 235-245: The BurnEvent timestamp is using the ledger time instead
of the oracle sample time, so update the BurnEvent construction in redeem_single
and redeem_basket to emit the rate’s source timestamp. Replace the current
timestamp assignment in the BurnEvent literal with rate_timestamp (the timestamp
returned alongside ORACLE_GET_RATE_WITH_TS) so indexers can match each
BurnEvent.rate to the exact oracle sample used.
- Around line 266-274: The empty-recipient path in this transfer flow still does
storage work before short-circuiting, and it currently panics instead of
returning the intended no-op. Update the handler around
reentrancy_guard::acquire_guard, Self::check_paused, and user.require_auth so
recipients.is_empty() is checked first and exits immediately with the no-op
result; only perform guard/pause/auth/storage work after that. Also replace the
ContractError::InvalidRecipient panic in this branch with the expected
successful no-op return.
In `@acbu_burning/tests/fuzz_test.rs`:
- Around line 47-51: Update the fuzz oracle in fuzz_test.rs so the
single-currency basket check treats num_recipients == 0 as a valid no-op, and
only asserts an error when num_recipients is non-zero and not exactly 1. Adjust
the expectation logic around the res result in the fuzz test to reflect this
rule while keeping the single-recipient success case unchanged.
In `@acbu_burning/tests/test.rs`:
- Around line 249-257: The empty-recipient redeem basket test is asserting the
wrong contract for try_redeem_basket on Burning. Update
test_redeem_basket_rejects_empty_recipients in test.rs so it expects the
zero-address Vec case to succeed as a no-op after the auth check, rather than
returning an error. Adjust the assertion to verify success and, if needed,
confirm no state changes for the empty basket path using the existing
setup_test, ctx.burning, and ctx.user helpers.
---
Outside diff comments:
In `@acbu_burning/src/lib.rs`:
- Around line 160-170: The freshness checks in the oracle lookup flow only
reject stale timestamps, so future-dated values can incorrectly pass; update the
timestamp validation around the oracle fetch logic in the rate retrieval path to
reject both old and future timestamps. Apply the same fix wherever the timestamp
freshness pattern is used, including the checks tied to oracle_timestamp and
rate_timestamp in the contract’s rate-reading functions, by comparing against
current_time before accepting the value.
🪄 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: e8694038-f695-4074-8560-7e059c051289
📒 Files selected for processing (4)
acbu_burning/src/lib.rsacbu_burning/tests/fuzz_test.rsacbu_burning/tests/redeem_basket.rsacbu_burning/tests/test.rs
Comment on lines
235
to
245
| let burn_event = BurnEvent { | ||
| transaction_id: tx_id, | ||
| user: user.clone(), | ||
| acbu_amount, // gross amount burned (unchanged — was already correct) | ||
| net_acbu, // explicit post-fee net so indexer needs no arithmetic | ||
| acbu_amount, | ||
| net_acbu, | ||
| local_amount: stoken_out, | ||
| currency: currency.clone(), | ||
| fee, // total fee for this redemption | ||
| fee, | ||
| rate, | ||
| timestamp: env.ledger().timestamp(), | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Emit the oracle timestamp that belongs to the emitted rate.
BurnEvent.rate comes from ORACLE_GET_RATE_WITH_TS, but timestamp is currently the ledger timestamp. Use rate_timestamp so indexers can reconcile the exact rate sample.
Proposed fix
- timestamp: env.ledger().timestamp(),
+ timestamp: rate_timestamp,Apply this in both redeem_single and redeem_basket.
Also applies to: 448-458
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@acbu_burning/src/lib.rs` around lines 235 - 245, The BurnEvent timestamp is
using the ledger time instead of the oracle sample time, so update the BurnEvent
construction in redeem_single and redeem_basket to emit the rate’s source
timestamp. Replace the current timestamp assignment in the BurnEvent literal
with rate_timestamp (the timestamp returned alongside ORACLE_GET_RATE_WITH_TS)
so indexers can match each BurnEvent.rate to the exact oracle sample used.
temisan0x
force-pushed
the
fix/351-burn-basket-early-return
branch
from
319dd69 to
170d8e2
Compare
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.
Fix #351 — burn_for_basket early return for zero/empty recipients
Problem
When
recipientswas empty,redeem_basketstill ran several storagereads and allocated
Vec::newbefore hitting the guard — wasting gason a guaranteed no-op.
Fix
recipients.is_empty()check to immediately afteruser.require_auth(), before any storage reads or heap allocations.Vec<i128>immediately (no-op)instead of panicking with
InvalidRecipient.Test changes
test.rs— renamedtest_redeem_basket_rejects_empty_recipientstotest_redeem_basket_allows_empty_recipients_as_noopand flippedassertion to expect success with empty return vec.
fuzz_test.rs— updated proptest to assertnum_recipients == 0succeeds as no-op; only mismatched non-zero counts error.
CI note
The
fetch_token_wasm.shCI failure is unrelated to this change.The script clones
soroban-examplesatv21.6.0and expects pathcontracts/tokens/stellar_assetwhich no longer exists at that tag.Closes #351