fix(wallet): correct Platform-to-Shielded Max calculation - #971
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe transfer flow obtains Platform Shield capacity from SDK preflight, uses it for validation and Max handling, revalidates it before submission, and recovers from capacity changes during confirmation. ChangesPlatform Shield capacity
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant InternalTransferViewModel
participant PlatformAddressSyncCoordinator
participant PlatformWalletManager
participant ShieldedTransferCoordinator
InternalTransferViewModel->>PlatformAddressSyncCoordinator: request preflightShield(paymentAccount)
PlatformAddressSyncCoordinator->>PlatformWalletManager: shieldedShieldPreflight(paymentAccount)
PlatformWalletManager-->>InternalTransferViewModel: return SDK shield capacity
InternalTransferViewModel->>ShieldedTransferCoordinator: submit frozen shield amount
ShieldedTransferCoordinator->>PlatformAddressSyncCoordinator: revalidate shield capacity
ShieldedTransferCoordinator-->>InternalTransferViewModel: report capacity change or continue authorization
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (2)
DashWallet/Sources/UI/Payments/InternalTransfer/ShieldedTransferCoordinator.swift (1)
652-653: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the empty associated-value pattern and match the file's existing match style.
SwiftLint reports
empty_enum_argumentson Line 653. The otherPlatformWalletErrorchecks in this file match the error directly, for example Line 1228 (case PlatformWalletError.shieldedSpendUnconfirmed = error). Use the same shape here.As per coding guidelines "Use SwiftFormat and SwiftLint conventions".
♻️ Proposed fix
- if let walletError = error as? PlatformWalletError, - case .shieldedInsufficientBalance(_) = walletError { + if case PlatformWalletError.shieldedInsufficientBalance = error { handleFailure(CoordinatorError.platformShieldCapacityChanged( maxShieldableCredits: nil))🤖 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 `@DashWallet/Sources/UI/Payments/InternalTransfer/ShieldedTransferCoordinator.swift` around lines 652 - 653, Update the PlatformWalletError matching in the shielded transfer error handling to use the direct enum-case pattern for shieldedInsufficientBalance, removing the empty associated-value placeholder and matching the existing style used elsewhere in the file.Sources: Coding guidelines, Linters/SAST tools
DashWalletTests/SwiftDashSDKCoreLifecycleTests.swift (1)
444-448: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the remaining fail-closed branches.
The new tests cover the resolved-capacity paths and the
nilcapacity path. Three branches of the fail-closed contract stay untested:
canSubmit(requestedCredits: 0, capacity:)must returnfalse.canContinuerelies on this to keep Continue disabled for a zero amount.canShield == falsemust makemaximumDuffsreturn0andcanSubmitreturnfalse.applyPlatformShieldMaxbranches on that zero to pick between the headroom notice and the empty-balance notice.heldBackCreditsmust return0whensubmittedDuffs * 1000overflows, and when the submitted amount is at or above the displayed aggregate.These are the branches that guarantee the SDK preflight stays the sole authority, so pinning them is worthwhile.
💚 Proposed additional tests
func testPlatformShieldRejectsZeroAmountAndUnshieldableAccount() { let unshieldable = PlatformShieldCapacity( canShield: false, accountBalanceCredits: 3_921_114_000, usableBalanceCredits: 0, feeReserveCredits: 1_000_000_000, maxShieldableCredits: 0, reason: "insufficient headroom") XCTAssertEqual( PlatformShieldAmountPolicy.maximumDuffs(capacity: unshieldable), 0) XCTAssertFalse(PlatformShieldAmountPolicy.canSubmit( requestedCredits: 1_000, capacity: unshieldable)) let shieldable = PlatformShieldCapacity( canShield: true, accountBalanceCredits: 3_921_114_000, usableBalanceCredits: 3_623_849_220, feeReserveCredits: 1_000_000_000, maxShieldableCredits: 2_623_849_220) XCTAssertFalse(PlatformShieldAmountPolicy.canSubmit( requestedCredits: 0, capacity: shieldable)) } func testPlatformShieldHeldBackReportsZeroOnOverflowAndFullySubmittedBalance() { // Overflow of submittedDuffs * 1000 must never invent a remainder. XCTAssertEqual( PlatformShieldAmountPolicy.heldBackCredits( displayedPlatformCredits: 4_500_000_000, accountBalanceCredits: 3_921_114_000, submittedDuffs: UInt64.max), 0) // Nothing is held back when the whole aggregate is submitted. XCTAssertEqual( PlatformShieldAmountPolicy.heldBackCredits( displayedPlatformCredits: 2_623_849_000, accountBalanceCredits: 2_623_849_000, submittedDuffs: 2_623_849), 0) }🤖 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 `@DashWalletTests/SwiftDashSDKCoreLifecycleTests.swift` around lines 444 - 448, Add tests in SwiftDashSDKCoreLifecycleTests for the remaining PlatformShieldAmountPolicy fail-closed branches: verify canSubmit rejects zero requested credits, an unshieldable capacity yields maximumDuffs of 0 and rejects submission, and heldBackCredits returns 0 for submittedDuffs multiplication overflow and fully submitted balances. Use the proposed PlatformShieldCapacity scenarios and preserve existing resolved and nil-capacity coverage.
🤖 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
`@DashWallet/Sources/UI/Payments/InternalTransfer/InternalTransferViewModel.swift`:
- Around line 1194-1209: Update the nil-capacity branch handling the platform
shield resync barrier to set isPlatformShieldPreflightLoading to false after
cancelling the task. Add a bounded manual-rearm state such as
allowsManualPlatformShieldRearm, and update fillPlatformShieldMax to consume
that allowance by clearing awaitingPlatformShieldResync and starting one fresh
preflight when the user explicitly taps Max; preserve the existing fail-closed
behavior until that action occurs.
---
Nitpick comments:
In
`@DashWallet/Sources/UI/Payments/InternalTransfer/ShieldedTransferCoordinator.swift`:
- Around line 652-653: Update the PlatformWalletError matching in the shielded
transfer error handling to use the direct enum-case pattern for
shieldedInsufficientBalance, removing the empty associated-value placeholder and
matching the existing style used elsewhere in the file.
In `@DashWalletTests/SwiftDashSDKCoreLifecycleTests.swift`:
- Around line 444-448: Add tests in SwiftDashSDKCoreLifecycleTests for the
remaining PlatformShieldAmountPolicy fail-closed branches: verify canSubmit
rejects zero requested credits, an unshieldable capacity yields maximumDuffs of
0 and rejects submission, and heldBackCredits returns 0 for submittedDuffs
multiplication overflow and fully submitted balances. Use the proposed
PlatformShieldCapacity scenarios and preserve existing resolved and nil-capacity
coverage.
🪄 Autofix
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 Plus
Run ID: 1a4eb5f9-ef67-4aad-9103-94a54e6f132e
📒 Files selected for processing (6)
DashWallet/Sources/Infrastructure/SwiftDashSDK/PlatformAddressSyncCoordinator.swiftDashWallet/Sources/UI/Payments/InternalTransfer/InternalTransferConfirmSheet.swiftDashWallet/Sources/UI/Payments/InternalTransfer/InternalTransferScreen.swiftDashWallet/Sources/UI/Payments/InternalTransfer/InternalTransferViewModel.swiftDashWallet/Sources/UI/Payments/InternalTransfer/ShieldedTransferCoordinator.swiftDashWalletTests/SwiftDashSDKCoreLifecycleTests.swift
llbartekll
changed the title
|
One thing I'd like to see addressed before this merges: the When the live insufficient-capacity race fires, platformShieldPreflightGeneration &+= 1
platformShieldPreflightTask?.cancel()
platformShieldPreflightTask = nil
platformShieldCapacity = nil
isPlatformShieldPreflightLoading = true
awaitingPlatformShieldResync = trueSo the form sits in a loading state with no task in flight, and the only thing that can clear it is a
Could we add either a user-visible retry (a tappable retry on the notice, or letting a Max tap force one preflight attempt while awaiting), or a bounded timeout that clears The rest of the change reads well to me — the frozen |
|
Addressed the remaining automated-review feedback in 89b0334:
Validation: swiftc parse and git diff check passed. The Conventional Commit title check is also green. |
3 participants
Summary
Why
The UI previously used the aggregate displayed Platform balance when calculating Max. That amount can be larger than the value the Platform Payment account can actually shield after protocol reserve and input-selection constraints.
Companion Platform SDK PR: dashpay/platform#4360
Testing
dashpaygeneric iOS Simulator build succeeded with code signing disableddashpayhas no test action, whiledashwalletrequires the unavailable watchOS 26.5 runtimeSummary by CodeRabbit
New Features
Bug Fixes