fix(drive): return error instead of panicking on empty SetPrices direct purchase#3856
Conversation
…ct purchase
A token's pricing authority can set an empty `SetPrices` schedule
(`Some(SetPrices({}))`): structure validation explicitly skips the price,
state validation is auth-only, and apply stores it verbatim. A later
`TokenDirectPurchase` then hits the `None` arm of
`set_prices.range(..=token_count).next_back()`, which called
`set_prices.keys().next().expect("Map is not empty")` — panicking on the empty
map. This transform runs on every validator during block execution, so the
panic (via the global panic hook) crashes all validators and halts the chain.
Handle the empty schedule without panicking: when the schedule has no tiers,
return a `TokenNotForDirectSale` consensus error; otherwise keep the existing
`TokenAmountUnderMinimumSaleAmount` behaviour. Adds an end-to-end regression
test that plants an empty `SetPrices` and asserts the purchase is rejected
gracefully (no panic, buyer receives no tokens).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
✅ Review complete (commit 7009115) |
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAvoid a validator panic by handling empty ChangesToken Direct Purchase Panic Prevention
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3856 +/- ##
=========================================
Coverage 87.15% 87.16%
=========================================
Files 2641 2641
Lines 327793 327798 +5
=========================================
+ Hits 285701 285730 +29
+ Misses 42092 42068 -24
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All six reviewers converged: no in-scope findings. The PR is a tight, well-scoped fix replacing a chain-halt .expect() panic with a safe match returning TokenNotForDirectSale for empty SetPrices schedules, with a focused regression test. Out-of-scope follow-ups (rejecting empty schedules at the write boundary) are noted for maintainer triage but do not affect the review action.
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "59a6da4fb5d049353852792570c2112b9494cd8e6691ef22810cba5451a1d52d"
)Xcode manual integration:
|
Adds a direct-purchase test for a non-empty tiered SetPrices schedule whose smallest tier exceeds the requested amount, exercising the TokenAmountUnderMinimumSaleAmount path alongside the existing empty-schedule (TokenNotForDirectSale) regression test — covering both arms of the no-matching-tier branch added by this PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review of PR #3856 at head 7009115 confirms the fix correctly replaces a chain-halting .expect("Map is not empty") panic with deterministic consensus errors (TokenNotForDirectSale for empty SetPrices, TokenAmountUnderMinimumSaleAmount for non-empty below-minimum), preserving the existing BumpIdentityDataContractNonceAction fee/nonce path. The latest delta (685892..700911) is test-only, adding the below-minimum regression test to complement the existing empty-schedule test. All six agents independently found no issues; no in-scope blocking, suggestion, or nitpick findings.
Issue being fixed or feature implemented
Hardens the token direct-purchase transition against an edge case in tiered (
SetPrices) pricing schedules. In an unusual schedule configuration the transformer could fail ungracefully instead of returning a clean validation error.What was done?
Reworked the no-matching-tier branch of the direct-purchase transformer so it no longer makes an assumption about the schedule's contents. When no applicable price tier is found, the transition is rejected with an appropriate consensus error in all cases. Added a regression test covering the edge case.
How Has This Been Tested?
cargo test -p drive-abci --lib direct_selling— all tests pass, including the new regression test and the existing version-pinned baselines (existing behaviour unchanged).cargo check -p drive,cargo clippy -p drive,cargo fmt --all— clean.Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests