Skip to content

fix(deps)!: pin platform-wallet/dash-sdk to PR #3968 head, adapt to fail-fast load-skip API#874

Open
Claudius-Maginificent wants to merge 5 commits into
docs/platform-wallet-migration-designfrom
feat/pin-platform-wallet-pr3968
Open

fix(deps)!: pin platform-wallet/dash-sdk to PR #3968 head, adapt to fail-fast load-skip API#874
Claudius-Maginificent wants to merge 5 commits into
docs/platform-wallet-migration-designfrom
feat/pin-platform-wallet-pr3968

Conversation

@Claudius-Maginificent

@Claudius-Maginificent Claudius-Maginificent commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Why this PR exists

  • Problem: as part of evaluating dashpay/platform PR #3968 (feat/platform-wallet-storage-rehydration, wallet persistence-reader work), dash-evo-tool's platform dependencies (dash-sdk, rs-sdk-trusted-context-provider, platform-wallet, platform-wallet-storage) are pinned to PR #3968's exact head commit — kept in step as that PR moves.
  • Current pin: 93b967f9c7ab0164b47fe825d2bae58b3974625c (branch feat/platform-wallet-storage-rehydration, base v4.1-dev). Previously pinned at 0503f80df5529b1d4f91e58c49abe08cde7b283f.
  • What breaks without it: nothing — this is a dependency-pin change plus the source adaptation it requires, not a bug fix. Without the adaptation, the build fails outright against the pinned rev.
  • Status/scope note for reviewers: PR #3968 is itself open and unmerged on the platform side. This PR captures a deliberate, tested adaptation to that specific rev — not a claim that dash-evo-tool should stay pinned there long-term.
  • Blocking relationship: stacked atop PR fix: round-2 full-repo architecture audit (21 waves + shielded-screen consolidation) #873 (round-2 architecture audit) — base is fix/round-2-architecture-audit.

What was done

Initial pin (0503f80d)

  • Re-pinned the four platform git dependencies in Cargo.toml/Cargo.lock to PR #3968's then-head commit.
  • Adapted src/wallet_backend/ to a fail-fast wallet-load model, matching that rev's actual shape (load_from_persistor() returns a plain Result<(), PlatformWalletError>, not a tri-state LoadOutcome): removed the SkipReason/CorruptKindPersistedLoadSkip mapping, the "N wallets skipped" banner machinery, and the on_wallet_skipped_on_load event-handler override. Any load failure — including a wallet that was already registered — is treated as fatal, per product decision (a corrupted or duplicate row is a bug to surface loudly, not silently skip).
  • Commented out the pwm.shutdown() clean-shutdown report check (returns () at that rev, no report type yet) with a TODO(platform-pr3968) marker to restore once platform re-adds it.
  • Pruned 6 PlatformWalletError match arms that didn't exist at that rev (RehydrationTopologyUnsupported, RehydrationPoolMismatch, RehydrationPoolTypeMismatch, PersisterLoad, AddressNonceMismatch, ShieldedShutdownIncomplete) across two exhaustive matches.
  • Replaced 2 From<PersistenceError> conversions (which didn't exist at that rev) with explicit PlatformWalletError::Persistence(e.to_string()) construction.
  • Removed the kittest coverage (tests/kittest/skipped_wallets_banner.rs, 3 tests) that existed solely to cover the now-deleted banner.

Re-pin to new head (93b967f9)

Upstream PR #3968 moved forward; re-pinned to its new head and re-diffed the relevant crates (dash-sdk, rs-sdk-trusted-context-provider, platform-wallet, platform-wallet-storage) between the two SHAs to find the actual public-API delta, rather than assuming the prior adaptation still applied. Cargo.lock also picked up a transitive rust-dashcore bump (48a07d3fb3fb8244) that platform now requires.

Two of the six previously-pruned pieces came back, forced by the compiler on the now-non-exhaustive matches:

  • PlatformWalletError::PersisterLoad(#[from] PersistenceError) restored (upstream 728f480f) — the two contact-request / dashpay-payment persister record sites go back to e.into() (typed PersisterLoad) instead of the manual Persistence(String) stringification the old pin forced.
  • PlatformWalletError::AddressNonceMismatch restored (upstream aae79a86) — re-added to both exhaustive matches (map_shielded_op_error, identity_op_error_kind), bucketed as a generic wallet-backend/Other error, unchanged behavior.

Everything else from the initial adaptation still stands at the new head: load_from_persistor() is still fail-fast (no LoadOutcome/SkipReason, so the skip-banner machinery stays removed — there's no per-wallet skip data to surface), shutdown() is still () (report check stays commented with its TODO), and the remaining four PlatformWalletError variants (RehydrationTopologyUnsupported, RehydrationPoolMismatch, RehydrationPoolTypeMismatch, ShieldedShutdownIncomplete) are still absent upstream.

Testing

  • cargo +nightly fmt --all -- --check — clean
  • cargo clippy --all-features --all-targets -- -D warnings — clean
  • cargo test --all-features --workspace --no-fail-fast — 1372/1372 lib, 10/10 e2e, 184/184 kittest, 3/3 legacy-table-surface, doctests clean

Breaking changes

None for end users. Internally: wallet-load behavior is "any load failure is fatal" — a deliberate product decision made at the initial pin, unchanged by the re-pin.

Checklist

  • Adapted to PR #3968's current head, full workspace clippy + tests green
  • Platform-side follow-ups filed (duplicate-row protection, orphaned module file — from the initial pin)
  • Human review
  • Confirm whether/when to keep this pin vs. wait for platform to catch dash-evo-tool up to include #3968's work

Attribution

🤖 Co-authored by Claudius the Magnificent AI Agent

lklimek and others added 2 commits July 9, 2026 09:15
…ail-fast load-skip API

Points dash-sdk, rs-sdk-trusted-context-provider, platform-wallet, and
platform-wallet-storage at dashpay/platform PR #3968's head commit
(0503f80df5529b1d4f91e58c49abe08cde7b283f, feat/platform-wallet-storage-rehydration)
instead of the dash-evo-tool integration branch.

This rev predates several APIs the dash-evo-tool branch had added, so the
consuming code is adapted rather than upstream:

- `SkipReason`/`CorruptKind`/`PlatformEventHandler::on_wallet_skipped_on_load`
  don't exist at this rev — `load_from_persistor()` returns a plain
  `Result<(), PlatformWalletError>` instead of the tri-state `LoadOutcome`.
  Removed the DET-side `PersistedLoadSkip` mapping, the "N wallets skipped"
  banner machinery, and the trait override; any load failure (including a
  wallet that was already registered) is now treated as fatal, per product
  decision — DET's existing empty-manager gate should prevent
  `AlreadyRegistered` from firing on this path; if it fires anyway, that is a
  DET-side bug and should surface loudly rather than be silently skipped.
- `pwm.shutdown()` returns `()` at this rev, not a clean-shutdown report —
  commented out the report check (user-confirmed) rather than dropping it
  outright, pending platform restoring the report type.
- Six `PlatformWalletError` variants (`RehydrationTopologyUnsupported`,
  `RehydrationPoolMismatch`, `RehydrationPoolTypeMismatch`, `PersisterLoad`,
  `AddressNonceMismatch`, `ShieldedShutdownIncomplete`) don't exist at this
  rev. All six already folded into the same generic wrapper/bucket as every
  other case at both call sites, so removing their match arms changes no
  behavior for variants that still exist.
- `From<PersistenceError> for PlatformWalletError` doesn't exist at this rev —
  the two call sites now build `PlatformWalletError::Persistence(String)`
  directly instead of `.into()`.

Full workspace clippy (--all-features --all-targets -- -D warnings) and the
full test suite are clean: 1372/1372 lib, 10/10 e2e, 184/184 kittest,
3/3 legacy-table-surface, doctests clean.

See the platform TODO (fc45c3da) for the upstream ask to simplify
load_from_persistor's error model and restore the removed variants/report
type once platform catches this rev up to dash-evo-tool branch.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
skipped_wallets_banner_text and the "N wallets skipped" banner it fed were
removed in the prior commit as part of adapting to PR #3968's fail-fast
load-skip API — this kittest file existed solely to cover that banner and
has nothing left to test.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)
  • master
  • v1.0-dev

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 372274bb-c950-4388-8390-81db2148fc5d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/pin-platform-wallet-pr3968

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread src/wallet_backend/mod.rs Outdated

// Every remaining variant → generic WalletBackend wrapper.
//
// TODO(platform-pr3968): `RehydrationTopologyUnsupported`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only AddressNonceMismatch, PersisterLoad, and maybe ShieldedShutdownIncomplete will be restored, others are permanently dropped.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in bac274b. Verified against the pinned rev (93b967f): `PersisterLoad` and `AddressNonceMismatch` already exist and are routed in the match arm — nothing to restore there. Trimmed both TODOs (the shielded-op map at ~L2281 and the identity-op-kind map at ~L2512) to list only `ShieldedShutdownIncomplete` as the variant that may return; dropped the three `Rehydration*` names since they are permanently gone upstream. The exhaustive (no _ arm) match still forces a review here the moment platform re-adds any variant. Bilby out. 🐾

Comment thread src/wallet_backend/mod.rs Outdated
| P::AddressNonceMismatch { .. }
| P::ShieldedShutdownIncomplete { .. } => IdentityOpErrorKind::Other,
| P::ShieldedSpendUnconfirmed { .. } => IdentityOpErrorKind::Other,
// TODO(platform-pr3968): `PersisterLoad`, `RehydrationTopologyUnsupported`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only AddressNonceMismatch, PersisterLoad, and maybe ShieldedShutdownIncomplete will be restored, others are permanently dropped.

Base automatically changed from fix/round-2-architecture-audit to docs/platform-wallet-migration-design July 9, 2026 14:01
…ore typed persistence errors

Re-points dash-sdk, rs-sdk-trusted-context-provider, platform-wallet, and
platform-wallet-storage from 0503f80d to dashpay/platform PR #3968's new
head 93b967f9c7ab0164b47fe825d2bae58b3974625c (branch
feat/platform-wallet-storage-rehydration, base v4.1-dev). Cargo.lock moves
every platform git package plus the transitive rust-dashcore pin
(48a07d3f -> b3fb8244) that platform now requires.

Upstream API delta since 0503f80d that touches DET:

- `PlatformWalletError::PersisterLoad(#[from] PersistenceError)` restored
  (upstream 728f480f). The two contact-request / dashpay-payment record
  sites go back to `e.into()` — a typed `PersisterLoad` wrapping the
  upstream `PersistenceError` — instead of the manual `Persistence(String)`
  stringification the 0503f80d pin forced.
- `PlatformWalletError::AddressNonceMismatch` restored (upstream aae79a86).
  Both exhaustive matches (`map_shielded_op_error`,
  `identity_op_error_kind`) regain their arms; the address-nonce fault
  buckets as a generic wallet-backend / Other error, unchanged from before.

Unchanged at the new head, so the 0503f80d adaptation stands:

- `load_from_persistor()` still returns `Result<(), PlatformWalletError>` —
  no `LoadOutcome`/`SkipReason` tri-state — so the skip-banner machinery
  stays removed (no per-wallet skip data exists to surface).
- `shutdown()` still returns `()` — the clean-shutdown report check stays
  commented with its TODO(platform-pr3968) marker.
- `RehydrationTopologyUnsupported`, `RehydrationPoolMismatch`,
  `RehydrationPoolTypeMismatch`, and `ShieldedShutdownIncomplete` still
  absent — the match TODOs are trimmed to just these four.

The new upstream classified-SPV-peer API (SpvPeerInfo / connected_peers)
is additive and unconsumed by DET; no adaptation needed.

Full workspace clippy (--all-features --all-targets -- -D warnings) and the
test suite (--all-features --workspace --no-fail-fast) are clean: 1372 lib,
10 e2e, 184 kittest, 3 legacy-table-surface, doctests, backend-e2e gate.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@lklimek lklimek marked this pull request as ready for review July 10, 2026 12:44
@thepastaclaw

thepastaclaw commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

✅ Review complete (commit bac274b)

lklimek and others added 2 commits July 10, 2026 14:45
The two error-mapping TODOs claimed RehydrationTopologyUnsupported,
RehydrationPoolMismatch, RehydrationPoolTypeMismatch, and
ShieldedShutdownIncomplete would be re-added to PlatformWalletError.
Per maintainer review the three Rehydration* variants are permanently
dropped upstream; only ShieldedShutdownIncomplete may return. Trim the
lists to the single variant that may reappear — the exhaustive match
already forces a review here when any upstream variant is added.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Source: opus general + opus rust-quality (initial lanes superseded after base-scope correction; corrective lanes ok); gpt-5.6-sol general + rust-quality (initial and corrective lanes failed: OAuth refresh token invalidated); opus verifier (corrective ok; initial verifier superseded).

PR #874 re-pins dash-sdk/platform-wallet to a fixed rev and adapts the wallet-backend seam to the upstream fail-fast load API, removing the persisted-wallet skip/banner path. The authoritative diff is exactly seven files; findings the prior pass raised against src/backend_task/error.rs and src/backend_task/dashpay/incoming_payments.rs are out of scope (those files are unchanged in this PR) and are dropped. The fail-fast whole-load behavior is a deliberate product decision, so findings critiquing it (and the commented-out report block, which carries a documenting TODO) are dropped. One valid in-scope nitpick remains: a doc comment that still references the removed skip banner.

Prior Reconciliation

  • 045908b0: NOT APPLICABLE — the checkpoint was skipped before reviewer launch and has no verifier output or trusted findings.

Carried-Forward Prior Findings

  • None

New Findings In Latest Delta

  • [nitpick] src/wallet_backend/mod.rs:464-465register_persisted_wallets still claims it raises a skipped-wallet warning banner.

💬 1 nitpick(s)

Finding details

In src/wallet_backend/mod.rs:

  • [NITPICK] src/wallet_backend/mod.rs:464-465register_persisted_wallets doc still claims it raises a skip banner

    The doc comment still says this pass “loads, logs, and raises a warning banner for any skipped wallet.” This PR removes all skip/banner handling: the pinned upstream load is fail-fast and the per-wallet skip concept no longer exists. The function now loads and logs, while any failure aborts the whole pass via TaskError. Update the comment to remove the obsolete banner/skip language so it matches the new behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants