fix(wallet): hold back enough on Identity Max for the network to accept it - #1172
romchornyi wants to merge 4 commits into
Conversation
…pt it Max from Identity to Transparent held back 0.002 DASH, a reserve borrowed from identity funding (the opposite direction, where it must stay small). Platform checks an IdentityCreditWithdrawal against amount + 0.004 DASH, so every Max was refused at Confirm with the raw protocol error. The reserve is now owned by the withdrawal and depends on the target: - Transparent: the 0.004 DASH consensus minimum + 0.001 margin (0.005, the same the evonode withdrawal holds back). - Platform: 0.002 DASH, unchanged — its minimum is 0.000065 DASH. Max, the Continue gate and the inline validation all use it. A balance refusal that still reaches Confirm (the balance moved in between) is shown as a sentence instead of the protocol dump, once the SDK reports it typed (dashpay/platform#5206). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 36 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (47)
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 |
|
✅ Final review complete — no blockers (commit 57f7631) · triage: low |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The PR correctly separates identity-withdrawal fee reserves by target and threads the target-specific reserve through Max, validation, and Continue gating. The consensus-floor arithmetic and error handling are otherwise sound, but two non-blocking issues remain: the new user-facing string is absent from the source localization catalog, and a doc comment describes a typed SDK error path that the current withdrawal/transfer implementation does not produce.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: security-auditor); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
lowbygpt-6-astra(effort low) — The diff is a small, contained adjustment to target-specific withdrawal fee reserves and their validation call sites, with straightforward error messaging and regression tests, rather than a large or intricate change to funds movement. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer,glm-5.3-flash— security-auditor (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 99% left, weekly 82% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `DashWallet/Sources/UI/Payments/InternalTransfer/IdentityWithdrawViewModel.swift`:
- [SUGGESTION] DashWallet/Sources/UI/Payments/InternalTransfer/IdentityWithdrawViewModel.swift:184-185: Add the new withdrawal error to the source localization catalog
This new user-visible string is passed through NSLocalizedString but is absent from DashWallet/en.lproj/Localizable.strings. English currently falls back to the key, but the source catalog is what localization tooling and Transifex use to discover strings, so translators cannot receive this message. Add the key/value pair to the English source catalog and let the localization tooling propagate it to other locales.
- [NITPICK] DashWallet/Sources/UI/Payments/InternalTransfer/IdentityWithdrawViewModel.swift:176-180: Document that the typed balance-refusal branch is not yet reachable for withdrawals
The comment says this branch is reachable when the balance changes between Continue and Confirm. In the current SDK route used here, ManagedPlatformWallet.withdrawCredits and transferCreditsToAddresses call FFI operations whose Rust wrappers stringify operation failures into InvalidIdentityData; the typed InsufficientIdentityCredits promotion is used by the DPNS document-trade path instead. Consequently, a stale-balance withdrawal currently keeps the SDK's raw description and does not enter this branch. Rewrite the comment as an explicit TODO describing the SDK prerequisite, while retaining the branch for the typed error once the withdrawal/transfer mapping is shipped.
| "Your Identity balance can't cover this amount plus the network fee. Enter a smaller amount and try again.", | ||
| comment: "Identity withdrawal — Platform refused the amount as more than the balance can cover") |
There was a problem hiding this comment.
🟡 Suggestion: Add the new withdrawal error to the source localization catalog
This new user-visible string is passed through NSLocalizedString but is absent from DashWallet/en.lproj/Localizable.strings. English currently falls back to the key, but the source catalog is what localization tooling and Transifex use to discover strings, so translators cannot receive this message. Add the key/value pair to the English source catalog and let the localization tooling propagate it to other locales.
source: glm-5.3-flash (phase1-reviewer: general, security-auditor)
There was a problem hiding this comment.
Fixed in 9d858d0 — the key is now in all 43 Localizable.strings catalogs (English everywhere until Transifex supplies translations), placed where the catalog tooling sorts it; bartycrouch lint reports only the two pre-existing duplicates.
There was a problem hiding this comment.
Resolved (re-reviewed at 9d858d0b): Your new error key is present in the English source catalog and all 42 other app catalogs, with English values pending translation. I verified that each catalog contains exactly the intended three-line addition, remains UTF-8 without a BOM, and passes plist syntax validation.
| /// Platform's balance refusal (`IdentityInsufficientBalanceError`) reads | ||
| /// as a protocol dump with the identity id and two raw credit figures; | ||
| /// say what it means instead. Reachable when the balance moved between | ||
| /// Continue and Confirm — the reserve above keeps Max itself clear of it. | ||
| /// Everything else keeps the SDK's own description. |
There was a problem hiding this comment.
💬 Nitpick: Document that the typed balance-refusal branch is not yet reachable for withdrawals
The comment says this branch is reachable when the balance changes between Continue and Confirm. In the current SDK route used here, ManagedPlatformWallet.withdrawCredits and transferCreditsToAddresses call FFI operations whose Rust wrappers stringify operation failures into InvalidIdentityData; the typed InsufficientIdentityCredits promotion is used by the DPNS document-trade path instead. Consequently, a stale-balance withdrawal currently keeps the SDK's raw description and does not enter this branch. Rewrite the comment as an explicit TODO describing the SDK prerequisite, while retaining the branch for the typed error once the withdrawal/transfer mapping is shipped.
source: glm-5.3-flash (phase1-reviewer: general, security-auditor)
There was a problem hiding this comment.
Fixed in 9d858d0 — the doc comment now carries a TODO(SwiftDashSDK) saying the .insufficientIdentityCredits branch is not reached until the SDK ships dashpay/platform#5206; the branch itself stays.
There was a problem hiding this comment.
Resolved (re-reviewed at 9d858d0b): Your new TODO explicitly identifies dashpay/platform#5206 as the prerequisite for reaching the typed balance-refusal branch, while preserving the future handling. I traced both current SDK operations and confirmed that their balance refusals still become untyped InvalidIdentityData, matching the documentation.
… dependency - Add the new refusal string to every Localizable.strings (English in all locales until Transifex supplies translations), at the position the catalog tooling sorts it to. - Say in the doc comment that the typed branch is not reached until the SDK ships dashpay/platform#5206, as a TODO rather than a present-tense claim. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At exact head 9d858d0, the complete PR diff consistently applies target-specific reserves to Identity Max, inline validation, and Continue; both prior findings are fixed, and no new in-scope defects were identified. Independent validation passed the standalone identity suite (9/9), extracted production reserve/error-mapping checks (3/3), and content, UTF-8 encoding, and plist validation for all 43 localization catalogs. Full app tests and runtime smoke remain unverified because the pinned DashUIKit requires Swift tools 6.3 while the installed toolchain provides 6.1; SwiftFormat lint also reports formatting violations.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The diff changes IdentityWithdrawViewModel and InternalTransferViewModel logic governing spendable balances and fee reserves for identity-to-transparent/platform withdrawals, directly affecting funds movement and transaction amount selection. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
llbartekll
left a comment
llbartekll
left a comment
There was a problem hiding this comment.
LGTM — approving.
Checked:
- Root cause is right. The old
feeHeadroomCredits(0.002 DASH) was below the 0.004 DASHcredit_withdrawalminimum that consensus adds to the amount. So every Identity → Transparent Max was refused. A target-specific reserve is the right fix. LeavingPlatformPaymentIdentityFundingPolicyalone is also right, since its reserve covers the funding direction. - Call sites. Max, inline validation, the Continue gate and the VM-level
feeReserveCreditsall takeresolvedWithdrawalTarget. No caller of the oldfeeHeadroomCreditsis left. The confirm sheet still showsminimumFeeCredits, a lower bound, which is fine. - Target flip after Max.
routeDidChange()→clearMaxSelection()drops the Max flag and keeps the typed amount. Platform-Max followed by a switch to Transparent then shows "Insufficient balance" with Continue disabled, rather than sending something consensus would refuse. That matches how the other routes behave. - Arithmetic in the new test. For both targets,
balance ≥ max + minimumFee. A 400M-credit balance on Transparent gives 0./1000flooring to duffs only lowers the amount. - Error mapping.
InternalTransferRunnergoes throughwithdrawExecutor.withdraw, souserFacingMessage(for:)is on the path that showed the raw protocol text. The TODO about the typed error waiting on dashpay/platform#5206 is accurate. - Localization. All 43 catalogs get the key once,
plutil -lintpasses on all of them, and the encoding (UTF-8) matches the base branch.
One non-blocking nit inline.
| /// exposed for IdentityCreditWithdrawal / identity credit transfer. | ||
| static func feeReserveCredits(target: IdentityWithdrawalTarget) -> UInt64 { | ||
| switch target { | ||
| case .transparent: return minimumFeeCredits(target: .transparent) + 100_000_000 |
There was a problem hiding this comment.
nit (non-blocking): this is now the second literal for the same 0.005 DASH IdentityCreditWithdrawal reserve. EvonodeWithdrawalViewModel.feeReserveCredits is 500_000_000, and the doc comment above says the two are "the same". If either one is retuned later, that claim stops being true without anything flagging it. Consider having one derive from the other, e.g. EvonodeWithdrawalViewModel.feeReserveCredits = IdentityWithdrawViewModel.feeReserveCredits(target: .transparent), or a tiny assertion test. Fine as a follow-up.
There was a problem hiding this comment.
Done in 57f7631: EvonodeWithdrawalViewModel.feeReserveCredits and estimatedFeeCredits now read IdentityWithdrawViewModel.feeReserveCredits(target: .transparent) / minimumFeeCredits(target: .transparent), so there is one source for both. Values are unchanged.
…dentity one Both run IdentityCreditWithdrawal, so the masternode reserve and fee figure now read IdentityWithdrawViewModel's values instead of repeating the literals. Values are unchanged: 500,000,000 and 400,000,000 credits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Reviewed the complete base-to-head diff at 57f7631 and the relevant validation, Max, confirmation, and withdrawal callers; no in-scope actionable issues were found. Target-specific reserves are applied consistently, masternode withdrawals retain their existing values through the shared source, and both prior findings are fixed. Verification was static only: the supplied exact-head CI snapshot contains successful accessibility, PR-title, and CodeRabbit checks, but no build or test results; runtime smoke results remain author-reported evidence.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The diff makes a small, contained change to target-specific withdrawal reserves and their validation call sites, with error messaging and regression coverage, rather than large or intricate changes to funds movement or consensus rules. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 49% left, 5h 73% left - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
This branch has not been deployed
4 participants
Issue being fixed or feature implemented
On Internal transfer, Max from Identity to Transparent is always refused by the network. Max held back 0.002 DASH, but Platform checks an IdentityCreditWithdrawal against
amount + 0.004 DASH(STATE_TRANSITION_MIN_FEES_VERSION1.credit_withdrawal). The user confirms, then gets a banner with the raw protocol error:Insufficient identity <id> balance <n> required <n>. On testnet the refusal'srequiredwas the submitted amount + exactly 400,000,000 credits.The 0.002 reserve was
PlatformPaymentIdentityFundingPolicy.feeHeadroomCredits, which belongs to the opposite direction (funding an identity from addresses). There it is kept small on purpose, so raising it was not an option.Stacked on #1140 — it uses that PR's
IdentityWithdrawViewModel.minimumFeeCredits(target:)and its error-slot rules. Base isfix/max-notice-info-in-error-slot; this PR will be retargeted todeveloponce #1140 merges.What was done?
IdentityWithdrawViewModel.feeReserveCredits(target:)replacesfeeHeadroomCreditsand is owned here:.transparent:minimumFeeCredits(.transparent)+ 0.001 margin = 0.005 DASH.EvonodeWithdrawalViewModelalready holds back the same amount for the same transition..platform: 0.002 DASH, unchanged (its minimum is 0.000065).spendableCredits(balanceCredits:target:).InternalTransferViewModelpassesresolvedWithdrawalTargetto Max, the Continue gate, the inline validation andfeeReserveCredits.IdentityWithdrawViewModel.userFacingMessage(for:):PlatformWalletError.insufficientIdentityCreditsbecomes "Your Identity balance can't cover this amount plus the network fee. Enter a smaller amount and try again." After the reserve fix this can only happen if the balance changes between Continue and Confirm. Today the SDK reports this refusal as an untypedInvalidIdentityData; fix(platform-wallet): report an identity balance refusal on withdrawal as insufficient credits platform#5206 makes it typed. Until the SDK carries that change, the old text is shown in that rare case.How Has This Been Tested?
IdentityBalanceRefreshTests.testIdentityMaxLeavesTheConsensusMinimumFeeForEachTarget: for both targets, Max leaves at least the consensus minimum fee. The suite passes 13/13 on an iOS 26.5 simulator, andscripts/test_identity_balance.pypasses 9/9.dd461eb8bd+ fix(platform-wallet): report an identity balance refusal on withdrawal as insufficient credits platform#5206:Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code