chore: tidy follow-ups from the shielded withdrawal fee review#3802
Conversation
Fast-follows from the final delta review (all low/nit, no consensus impact): - L1: add a compile-time guard `const _: () = assert!(MIN_WITHDRAWAL_AMOUNT == 190_000)` on both dpp withdrawal consts, so a future dashcore ASSET_UNLOCK_TX_SIZE change breaks the build (a prompt to re-sync SYSTEM_LIMITS_V1.min_withdrawal_amount) instead of silently drifting. - N1: refresh stale 190_000 references in the address-withdrawal validator tests to the v12 floor (1_000_000) and lift the valid-helper input above the floor (2_000_000) so the happy path is genuinely above-floor, not sitting on the boundary. - N2: correct the stale "above MIN_WITHDRAWAL_AMOUNT" comments in the shielded integration tests (the 1_000_000 recipient is now AT the v12 floor, not above it). - N3: lock the builders' new returned fee against the wallet's independently-reserved exact_fee via debug_assert_eq! at all three shielded operation sites. Verified: clippy --all-features --all-targets -D warnings clean; dpp address_credit_withdrawal 25, platform-wallet shielded 23 (the live debug_assert confirms fee_used == exact_fee), drive-abci shielded_withdrawal 20 pass, 0 failures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 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 (5)
📝 WalkthroughWalkthroughThis PR adds compile-time assertions enforcing that ChangesWithdrawal Amount Assertions and Fee Synchronization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 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 |
|
✅ Review complete (commit 228c144) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both agents reviewed the same diff and reported no findings. The PR is a small, well-scoped follow-up cleanup: compile-time assertions pinning MIN_WITHDRAWAL_AMOUNT, test comment refreshes to v12 floor, and debug_assert_eq! on shielded fee invariants. No consensus impact, no public API changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3802 +/- ##
=============================================
+ Coverage 69.91% 87.16% +17.25%
=============================================
Files 19 2627 +2608
Lines 2712 321557 +318845
=============================================
+ Hits 1896 280299 +278403
- Misses 816 41258 +40442
🚀 New features to boost your workflow:
|
|
✅ 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: "bfa5983c63a27f54153af3aa21043fffe5db62f4b2fd1bbe42ea2c6a33d851c1"
)Xcode manual integration:
|
Issue being fixed or feature implemented
Post-merge cleanup of the low/nit items surfaced by the final review of #3800 (already merged into
v3.1-dev). All four are low/nit hygiene with no consensus impact — none was merge-blocking, so they are split into this small follow-up rather than reopening #3800.What was done?
MIN_WITHDRAWAL_AMOUNT— fix(drive): charge fees for unshield and shielded withdrawal #3800 moved the consensus withdrawal floor into the versionedsystem_limits.min_withdrawal_amount, leaving the dppMIN_WITHDRAWAL_AMOUNTconst as the v11 baseline that must stay in sync withSYSTEM_LIMITS_V1. Addedconst _: () = assert!(MIN_WITHDRAWAL_AMOUNT == 190_000)on both definitions so a future dashcoreASSET_UNLOCK_TX_SIZEchange breaks the build (a prompt to re-sync) instead of silently drifting the doc comment.190_000test references in the address-withdrawal validator tests to the v12 floor (1_000_000), and lifted the valid-path helper input above the floor (2_000_000) so the happy path is genuinely above-floor rather than sitting on the boundary.1_000_000recipient is now at the v12 floor).exact_feeviadebug_assert_eq!at all three shielded operation sites (fix(drive): charge fees for unshield and shielded withdrawal #3800 made the builders return the applied fee; nothing asserted it).How Has This Been Tested?
cargo clippy --all-features --all-targets -- -D warningsclean across dpp / platform-wallet / drive-abci. Suites pass with 0 failures: dppaddress_credit_withdrawal25, platform-walletshielded23 (the livedebug_assertconfirmsfee_used == exact_fee), drive-abcishielded_withdrawal20.Breaking Changes
None. No consensus or public-API behavior change (the
const _guard is compile-time only; thedebug_assertis debug-build only).Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Tests