Repository navigation
feat(wallet): remove CrowdNode - #1268
romchornyi wants to merge 10 commits into
Conversation
Remove the CrowdNode integration: its screens, Home shortcut and balance reminder, the Explore staking entry, the leftover-balance warnings on Send, Buy credits and Coinbase transfer, its notifications, the API client and the CrowdNode transaction matching. Past CrowdNode transactions show as ordinary rows. Shared pieces that lived in the CrowdNode folders move out: TransactionObserver and ObservedTransaction (transaction notifications, Receive) to Models/Transactions/Observation, GroupedTransactionsScreen (CoinJoin groups) to UI/Tx/Details. The selected-input send path, used only for CrowdNode signal transactions, goes; its fee model stays as estimatedFee for the Max reserve. Upgrades are handled for data older builds left behind: - shortcut raw values are pinned; a saved retired slot becomes the first action not already on the bar - delivered notifications on the retired thread or id are removed on activation, and a migration marks the retired topic's events seen - CrowdNode's UserDefaults and crowdnode.io website data are removed once, after protected data is available The Coinbase deposit error reuses the translated "Couldn't complete the transfer". 132 keys used only by the removed code leave the catalogs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- fill a retired shortcut slot after the availability fallbacks, so it never adds a button the bar already shows - record the crowdnode.io website data as removed only once WebKit reports it done, and match the domain on a label boundary - run the cleanup after launch rather than inside it - drop the crowdnode PR-title scope and a stale comment Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- ShortcutActionType.restoredBar applies the availability fallbacks and fills a retired slot last, against the bar as shown; HomeViewModel calls it instead of mapping inline - crowdnode.io website data is removed on the first activation, never during a background launch, and matched in any letter case - the crowdnode PR-title scope stays: cleanup code still carries it Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Call RetiredCrowdNodeDefaults from AppDelegate's handleDidBecomeActive instead of launch plus a self-removing observer, so the defaults pass and the website-data pass both retry on every activation until done. Comments now say where the retired shortcut slot is mapped and that the availability fallbacks don't check for duplicates. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 201 files, which is 101 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry.
|
|
✅ Final review complete — no blockers (commit 2047727) · triage: critical |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The CrowdNode removal is internally consistent and the supplied exact-head CI checks are successful. One blocking lifecycle gap remains: activation removes delivered legacy notifications but leaves older pending CrowdNode requests scheduled by the previous one-second notification path, allowing a retired message to appear after activation.
🔴 1 blocking
1 finding(s) not shown inline (GitHub refused the PR diff as too large)
🔴 Blocking: Activation does not cancel pending legacy CrowdNode notifications
DashWallet/Sources/Infrastructure/Notifications/NotificationLifecycle.swift:142-151
clearTray only queries delivered notifications and calls removeDeliveredNotifications. Older builds scheduled the legacy CrowdNode request with a one-second trigger, so reopening or upgrading the app before that trigger fires leaves the request pending; it can then deliver the retired CrowdNode message after activation. The existing UserNotificationCenterClient already exposes removePendingNotificationRequests, and the activation reconciliation should cancel the known legacy identifiers (CrowNode/Now) in addition to removing delivered requests. Add regression coverage for a pending legacy request.
source: gpt-6.1-sol (phase2-reviewer: general)
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: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate cross-cutting change that modifies wallet send and transaction-building paths in SwiftDashSDKTransactionSender and WalletSendService, including coin-selection-related behavior and fee handling, which affects funds movement. - 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 quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- 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— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); 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/Infrastructure/Notifications/NotificationLifecycle.swift`:
- [BLOCKING] DashWallet/Sources/Infrastructure/Notifications/NotificationLifecycle.swift:142-151: Activation does not cancel pending legacy CrowdNode notifications
`clearTray` only queries delivered notifications and calls `removeDeliveredNotifications`. Older builds scheduled the legacy `CrowdNode` request with a one-second trigger, so reopening or upgrading the app before that trigger fires leaves the request pending; it can then deliver the retired CrowdNode message after activation. The existing `UserNotificationCenterClient` already exposes `removePendingNotificationRequests`, and the activation reconciliation should cancel the known legacy identifiers (`CrowNode`/`Now`) in addition to removing delivered requests. Add regression coverage for a pending legacy request.
Older builds scheduled the CrowdNode deposit and transaction notifications under fixed identifiers with a one-second trigger. One still pending at activation would land after the tray was cleared, so activation now cancels pending requests for those identifiers too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Re the blocking finding (pending legacy CrowdNode notifications): confirmed and fixed in 731df5c. Before eae2f05, 🤖 Generated with Claude Code |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The CrowdNode removal is internally consistent, and the exact-head CI snapshot reports successful simulator tests, accessibility, target-membership, and recovery checks. One security-relevant cleanup gap remains: wallet deletion can report success before retired CrowdNode UserDefaults and asynchronous WebKit data cleanup are guaranteed to be complete.
🟡 1 suggestion(s)
1 finding(s) not shown inline (GitHub refused the PR diff as too large)
🟡 Suggestion: Successful wallet wipe leaves retired CrowdNode data behind
DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletWiper.swift:318-329
The wipe commit clears the SDK and current app-owned stores, but it no longer clears the retired CrowdNode defaults at the deletion boundary. RetiredCrowdNodeDefaults.removeAllOnce() is only invoked from AppDelegate.handleDidBecomeActive, and its WebKit cookie/website-data removal is asynchronous and not awaited by either full wipe or per-wallet deletion. If a wallet is removed before activation cleanup runs or while the WebKit purge is still pending, the wiper can report success while CrowdNode account addresses, balances, state, and web-session data remain in app storage. Cleanup of retired sensitive data should be part of the fail-closed wipe/delete boundary, or the deletion flow must explicitly await and verify it before reporting success.
source: gpt-6.1-sol (phase2-reviewer: security-auditor)
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: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large cross-cutting change that modifies wallet transaction construction and signing paths in SwiftDashSDKTransactionSender and WalletSendService, directly affecting funds movement and coin 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 quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- 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— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); 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/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletWiper.swift`:
- [SUGGESTION] DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKWalletWiper.swift:318-329: Successful wallet wipe leaves retired CrowdNode data behind
The wipe commit clears the SDK and current app-owned stores, but it no longer clears the retired CrowdNode defaults at the deletion boundary. `RetiredCrowdNodeDefaults.removeAllOnce()` is only invoked from `AppDelegate.handleDidBecomeActive`, and its WebKit cookie/website-data removal is asynchronous and not awaited by either full wipe or per-wallet deletion. If a wallet is removed before activation cleanup runs or while the WebKit purge is still pending, the wiper can report success while CrowdNode account addresses, balances, state, and web-session data remain in app storage. Cleanup of retired sensitive data should be part of the fail-closed wipe/delete boundary, or the deletion flow must explicitly await and verify it before reporting success.
The CrowdNode keys were removed only by the activation cleanup. The full wipe now removes all of them at its commit point, and per-wallet deletion removes that wallet's keys, as the old CrowdNode reset did. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Re the suggestion (wallet wipe leaves retired CrowdNode data behind): fixed in 0751b6e. The UserDefaults part now sits at the deletion boundary again, as the old
The crowdnode.io cookies and website data are not added to the wipe: the old wipe never cleared them, and removing them is asynchronous WebKit work that the wipe's background queue shouldn't wait on. The activation cleanup removes them and keeps retrying until WebKit reports completion. 🤖 Generated with Claude Code |
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 0751b6e and found no actionable in-scope defects. Both prior findings are fixed: wallet deletion clears retired CrowdNode defaults independently of activation, and activation cancels pending legacy notifications. Validation was static-only; current GitHub checks report passing simulator tests, test-target membership, recovery, and accessibility checks.
🔴 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: security-auditor); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a large, cross-cutting feature-removal change spanning UI, notifications, persisted settings cleanup, transaction handling, and project configuration, but the diff does not itself change a critical surface such as funds movement, cryptography, key handling, or storage migrations. - 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 quota lookup failed),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 high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); 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.
The retired CrowdNode clean-up now removes only CrowdNode's UserDefaults. The cookies and website data its web screens left stay, as in Android's CrowdNode removal (dash-wallet #1664); nothing in the app opens the site. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> SDD-Published-From: 5efeecdfc70c22c6ffec09ebf457cbf46aabd3f8
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At af201e8, the CrowdNode removal is consistent across UI entry points, sending, transaction observation, notifications, persistence cleanup, and resources; both prior findings are fixed. A static merge check confirms eight conflicting paths against develop, which has advanced 13 commits; resolving those conflicts is the sole remaining suggestion. This review was static-only: current checks show a passing title check and a skipped CodeRabbit review, but no build or XCTest results independently corroborating the reported local validation.
🟡 1 suggestion(s)
1 finding(s) not shown inline (GitHub refused the PR diff as too large)
🟡 Suggestion: Resolve conflicts with current develop before landing
DashWallet.xcodeproj/project.pbxproj:1-5
A static merge of this exact head with develop at 3201192 produces eight conflicting paths, including the project, SDK host and sender, HomeView, and four modify/delete conflicts involving retired CrowdNode code and tests. Integrate the updated base while preserving the CrowdNode deletions and the newer wallet-key and sender changes. Also preserve #1286's grouped-transaction navigation update at the relocated GroupedTransactionsScreen path. Revalidate the resulting integration with the dashpay build and unit-test target; validation reported for this head does not cover the conflict resolution.
source: muse-spark-1.3-contributor (phase1-reviewer: general)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-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) — This large, cross-cutting removal directly deletes coin-selection, signing, and broadcast logic in SwiftDashSDKTransactionSender.buildAndSignFromAddress and adds persistent-data migration behavior in NotifiedEventStore and RetiredCrowdNodeDefaults. - 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 quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- 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 each finding against the current code and only fix it if needed.
In `DashWallet.xcodeproj/project.pbxproj`:
- [SUGGESTION] DashWallet.xcodeproj/project.pbxproj:1-5: Resolve conflicts with current develop before landing
A static merge of this exact head with develop at 3201192bafb4851f1692c10e6476d68d5027ce3a produces eight conflicting paths, including the project, SDK host and sender, HomeView, and four modify/delete conflicts involving retired CrowdNode code and tests. Integrate the updated base while preserving the CrowdNode deletions and the newer wallet-key and sender changes. Also preserve #1286's grouped-transaction navigation update at the relocated GroupedTransactionsScreen path. Revalidate the resulting integration with the dashpay build and unit-test target; validation reported for this head does not cover the conflict resolution.
…oinbase error and wipe tests SDD-Published-From: df35aed12eb5c1e43de381ce44c14a5a472da345
SDD-Published-From: 5e969b9c1834a2db127bb7b7eb54eb787ef6ec37
SDD-Published-From: 6e04ea97046e8e7ce4acce2b2bdc1ff4502cd6c8
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of the complete diff at 2047727 found no actionable in-scope defects, and all three prior findings are fixed. CrowdNode retirement preserves surviving transaction observers, fee calculations, shortcut values, and wallet-deletion ordering. Current CI passes target membership, recovery, and accessibility checks; simulator tests remain pending, and no local builds or tests were run.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-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) — This large, cross-cutting removal directly deletes the selected-input funds-movement path in SwiftDashSDKTransactionSender.buildAndSignFromAddress and introduces the MarkRetiredNotificationTopicsSeen storage migration in NotifiedEventStore.swift. - 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 quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- 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.
Issue being fixed or feature implemented
Jira MO-1115: remove the CrowdNode feature from the app — no CrowdNode screens, validations or messaging, and no search for CrowdNode transactions in the wallet's history.
This goes further than Android today, which keeps a withdraw-only mode (dash-wallet #1490, #1497). Both apps delete CrowdNode's stored settings after the upgrade.
What was done?
Removed
Models/CrowdNodeandUI/CrowdNode(storyboard included), the CrowdNode notification producer, and their assets.WalletSendService/SwiftDashSDKTransactionSender, used only for CrowdNode signal transactions. Its fee model stays asestimatedFee, because the Max fee reserve uses it.crowdnodetopic and thestakingroute. A tap on an old notification just opens the app.Moved, because other features use them
TransactionObserver/ObservedTransaction(transaction notifications, Receive) →Models/Transactions/Observation, trimmed to what those callers read.GroupedTransactionsScreen(CoinJoin groups) →UI/Tx/Details.Upgrade handling for what older builds left behind
DWGlobalOptions.shortcuts, so every case after the removed one is pinned (coinbase = 19…nodes = 25) and 18 is retired. Restoring a saved bar moved into one pure function,ShortcutActionType.restoredBar: it applies the existing availability fallbacks, then fills a saved 18 with the first action the bar doesn't show yet (Spend, the faucet on testnet, then Explore, Receive…), so the bar keeps its size and the retired slot adds no duplicate. Duplicates the existing fallbacks can produce on their own are a separate ticket.crowdnodethread or with the legacyCrowdNodeid are removed on every activation. A migration (MarkRetiredNotificationTopicsSeen) marks that topic's unseen events seen, so a background notification posted before the first activation doesn't count them in the badge; the rows stay for dedup.RetiredCrowdNodeDefaultsremoves CrowdNode's 17 UserDefaults keys — global, bare legacy and<key>_<walletIdHex>for every wallet; one key starts with a Cyrillic "с", so the list is copied byte for byte. A per-wallet name is removed only when a lowercase hex wallet id follows the underscore, as the old CrowdNode code wrote it, so another setting that merely starts the same way stays. It is called fromhandleDidBecomeActiveand retries on each activation until done; it skips an activation without protected data (before the first unlock the defaults read back empty). Removing a wallet deletes that wallet's keys, and the full wipe deletes them all; the wipe's UserDefaults resets moved fromperformWipeintoresetUserDefaultsStoresForWipe()so a test can call them. crowdnode.io cookies and website data stay, as in Android's CrowdNode removal (dash-wallet #1664): nothing in the app opens the site any more.Decisions — recorded in the decision log as ADR-0006 (remove CrowdNode entirely, as Android does), ADR-0007 (pin shortcut raw values and refill the retired slot), ADR-0008 (remove CrowdNode defaults by exact key list on activation) and ADR-0009 (retire the CrowdNode topic by marking its events seen), all Proposed. Numbers 0003–0005 are taken by #1311.
How Has This Been Tested?
dashpayscheme builds for the simulator against platform1721ca7a8b(.platform-ref).ShortcutActionTypeTests(pinned raw values;restoredBarfrom raw saved values, including slots that fall back to Spend/Receive),RetiredCrowdNodeDefaultsTests(every key form, the Cyrillic key, foreign keys untouched, a non-hex suffix kept, once-only, no protected data → nothing recorded),WiperRemovesCrowdNodeKeysTests(wallet removal and the full wipe),MaxSendFeeReserveTests(the Max fee reserve is unchanged),CoinbaseDepositFailedErrorTests(the translated transfer error), migration and tray tests inNotifiedEventStoreTests/NotificationLifecycleTests, and the transaction-notification tests for the former deposit amount.scripts/test_core_drain.py(wiper harness): 27 passed.scripts/a11y_audit.py --check: no new findings; the baseline only loses entries of deleted files.scripts/ci/check_test_target_membership.py: ok.Breaking Changes
None for the app's API. Users who still hold funds at CrowdNode can no longer withdraw from the app; they withdraw on crowdnode.io.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code