fix(swift-sdk): label provider special txs instead of Self-Transfer#4109
Conversation
ProRegTx / ProUp*Tx rows classify as direction=internal (the wallet only sees its own owner/voting/payout keys in the payload), so the transaction list and detail sheet rendered them as "Self-Transfer" with a +0.00000000 DASH amount. Extend the asset-lock displayDirection pattern to provider special kinds (2-5), give them a server.rack/orange treatment, and replace the zero amount with the tx kind (row) or hide it (detail) when no wallet value moved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughProvider-special transaction kinds now expose classification and display-name helpers. Transaction detail and list views use provider-specific labels, icons, colors, and zero-value amount handling, with tests covering provider and non-provider mappings. ChangesProvider-special transaction presentation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PersistentTransaction
participant TransactionDetailView
participant TransactionRowView
PersistentTransaction->>TransactionDetailView: provide providerSpecialName and isProviderSpecial
TransactionDetailView->>TransactionDetailView: choose description, icon, color, and amount visibility
PersistentTransaction->>TransactionRowView: provide providerSpecialName and isProviderSpecial
TransactionRowView->>TransactionRowView: choose row icon, color, and zero-value amount
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
🔍 Review in progress — actively reviewing now (commit 7633d96) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, well-scoped display-only fix that extends the existing asset-lock displayDirection override pattern to provider special transactions (kinds 2-5), replacing the misleading 'Self-Transfer' / '+0.00000000 DASH' labels with the tx kind name and a distinct orange/server.rack treatment. Logic is internally consistent across PersistentTransaction, TransactionDetailView, and TransactionRowView, correctly falls through to the real signed amount when a provider tx does move wallet value, and reuses the TransactionTypeKind enum (discriminants 2-5) that predates this PR. Only gap is missing unit test coverage for the new labeling logic.
Source: Sol reviewer gpt-5.6-sol (general), Sonnet reviewer claude-sonnet-5 (general); final verifier claude-sonnet-5. The Sol orchestrator openai/gpt-5.6-sol was orchestration-only and is not reviewer or verifier evidence.
🟡 1 suggestion(s)
🤖 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 `packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentTransaction.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentTransaction.swift:204-216: No test pins provider-special discriminants/labels
`isProviderSpecial` and `providerSpecialName` now drive every changed UI branch (icon, color, amount fallback, direction label) across three files, but nothing in `SwiftTests/SwiftDashSDKTests` exercises the mapping. A table-driven test asserting `TransactionTypeKind(rawValue: 2...5)` maps to the expected `providerSpecialName` strings (and that other kinds return `nil`) would catch a future drift from Rust's `transaction_type_to_u8` discriminants or an accidental label typo without needing UI automation. The test target already exists (e.g. `TransactionDecoderTests.swift`), so this is a low-cost addition.
Table-driven coverage for isProviderSpecial / providerSpecialName / displayDirection over all TransactionTypeKind discriminants, so a drift from Rust's transaction_type_to_u8 or a label typo fails in unit tests instead of needing UI automation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swift (1)
38-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider delegating special-kind branches to
displayDirectionThe first three branches of
typeDescriptionnow duplicatePersistentTransaction.displayDirectionexactly (asset lock, asset unlock, provider special). If a label changes in one place, the other can drift. Consider delegating the special-kind cases todisplayDirectionand keeping only thenetAmountfallback local:♻️ Optional refactor
private var typeDescription: String { - if transaction.isAssetLock { return "Asset Lock" } - if transaction.isAssetUnlock { return "Asset Unlock" } - if let name = transaction.providerSpecialName { return name } + if transaction.isAssetLock || transaction.isAssetUnlock || transaction.isProviderSpecial { + return transaction.displayDirection + } switch transaction.netAmount { case let amount where amount > 0: return "Received"🤖 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 `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swift` around lines 38 - 50, Update the typeDescription computed property to reuse PersistentTransaction.displayDirection for asset locks, asset unlocks, and provider-special transactions instead of duplicating those labels. Keep the existing netAmount-based Received, Sent, and Self-Transfer fallback behavior local.
🤖 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.
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swift`:
- Around line 38-50: Update the typeDescription computed property to reuse
PersistentTransaction.displayDirection for asset locks, asset unlocks, and
provider-special transactions instead of duplicating those labels. Keep the
existing netAmount-based Received, Sent, and Self-Transfer fallback behavior
local.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1bafea8a-38f9-4179-a486-2f2b8f354d97
📒 Files selected for processing (4)
packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentTransaction.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionListView.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/PersistentTransactionDisplayTests.swift
typeDescription's asset-lock/unlock/provider branches duplicated PersistentTransaction.displayDirection verbatim; delegate to it so a label change can't drift between the detail sheet and list rows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Issue being fixed or feature implemented
Provider special transactions (ProRegTx / ProUpRegTx / ProUpServTx / ProUpRevTx) classify as
direction=internal— the wallet only sees its own owner/voting/payout keys referenced in the payload — so SwiftExampleApp rendered them as "Self-Transfer" with a misleading "+0.00000000 DASH" amount.Verified repro: mainnet wallet, Provider Owner Keys account, ProRegTx
5076f9ed731bcb7b5ddf47e21bf1262d22d99665ca89bb6329e6e706d605da85(height 1030673).What was done?
Extends the existing asset-lock/unlock display override pattern to provider special kinds (2–5):
PersistentTransaction(SwiftDashSDK): addedisProviderSpecialandproviderSpecialNamehelpers and wired them intodisplayDirection— Storage Explorer list/detail views pick up the correct label with no view changes.TransactionDetailView: header shows the provider tx name instead of "Self-Transfer", uses aserver.rackicon in orange, and hides the amount line entirely for payload-only provider txs (netAmount == 0).TransactionRowView(TransactionListView.swift): same icon/color treatment; the amount slot shows the tx kind (e.g. "Provider Registration") instead of "+0.00000000 DASH" when no wallet value moved. A provider tx that does move value (e.g. wallet-funded collateral) still shows its real signed amount.How Has This Been Tested?
xcodebuild -sdk iphonesimulator) with no errors.transactionTypeKinddiscriminant; the mainnet repro row (kind 2) renders "Provider Registration" on relaunch without a rescan.Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes