feat(platform-wallet): serde support#3637
Conversation
Add a new `serde` Cargo feature on `platform-wallet`. When enabled, every type carried in a `PlatformWalletChangeSet` gains `serde::Serialize` / `serde::Deserialize` derives via `#[cfg_attr(feature = "serde", derive(...))]`: - `CoreChangeSet`, `IdentityChangeSet`, `IdentityEntry`, `IdentityKeysChangeSet`, `IdentityKeyEntry`, `IdentityKeyDerivationIndices`, `ContactChangeSet`, `ContactRequestEntry`, `SentContactRequestKey`, `ReceivedContactRequestKey`, `PlatformAddressChangeSet`, `PlatformAddressBalanceEntry`, `AssetLockChangeSet`, `AssetLockEntry`, `TokenBalanceChangeSet`, `WalletMetadataEntry`, `AccountRegistrationEntry`, `AccountAddressPoolEntry`, and the top-level `PlatformWalletChangeSet`. - Per-identity / DashPay leaf types referenced inside those changesets: `BlockTime`, `IdentityStatus`, `DpnsNameInfo`, `DashPayProfile`, `ContactRequest`, `EstablishedContact`, `PaymentEntry`, `PaymentDirection`, `PaymentStatus`, `AssetLockStatus`. The feature activates `key-wallet/serde` (which transitively flips `dashcore/serde` and `dash-network/serde`) so every upstream leaf type already wired with `#[cfg_attr(feature = "serde", ...)]` (TransactionRecord, Utxo, InstantLock, AccountType, AddressInfo, AddressPoolType, ExtendedPubKey, Network) round-trips cleanly. Two upstream types lack their own serde feature and use `#[serde(with = ...)]` adapters in the new `src/changeset/serde_adapters.rs` module: - `AssetLockFundingType` (key-wallet, no `serde` derive) — encoded as a stable u8 tag matching the prior hand-rolled blob layout. - `AddressFunds` (dash-sdk re-export, no serde derive) — encoded as a `(nonce, balance)` shadow struct. One field is marked `#[serde(skip)]`: - `CoreChangeSet::addresses_derived` carries `key_wallet_manager::DerivedAddress`, which has no serde derive AND no `key-wallet-manager/serde` feature to activate. The breadcrumb is written to a typed table by persisters, not via a changeset blob, so skipping costs nothing. `cargo build -p platform-wallet` (no features) and `cargo build -p platform-wallet --features serde` both build clean. `cargo test -p platform-wallet` passes (8 lib tests, 121 integration tests) with and without the new feature. The change is opt-in; the default-feature build is byte-identical to its prior shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces optional ChangesSerde Serialization Feature for Wallet Types
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
lklimek
changed the title
Per audit: `AddressFunds` is first-party (lives in `rs-sdk/src/platform/address_sync/types.rs`) and its inner fields (`AddressNonce = u32`, `Credits = u64`) come from `dpp`, which derives serde unconditionally. The `#[serde(with = "serde_adapters::address_funds")]` adapter was pure boilerplate re-implementing what `derive(Serialize, Deserialize)` would produce. Replaced with a direct `#[cfg_attr(feature = "serde", derive(...))]` on `AddressFunds`. The `asset_lock_funding_type` adapter is retained — the upstream `AssetLockFundingType` in `rust-dashcore` key-wallet has no serde derives, and the adapter also encodes a stable `u8` wire-tag for on-disk blob compatibility that's worth keeping documented. Net: -~30 LOC. `platform-wallet`'s `serde` feature now propagates `dash-sdk/serde` so the gated derive on `AddressFunds` is activated when the parent feature is on. (Stray `cargo fmt -p platform-wallet` hit one pre-existing formatting nit in `wallet/platform_addresses/wallet.rs` — included since the quality gate runs fmt.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Audit-driven cleanup landed (
|
Scope: PR #3637's own diff against v3.1-dev only (the serde-feature PR). Pre-existing v3.1-dev comments left untouched. Drivers: - Verbosity: the Cargo.toml `serde` feature note was 12 lines enumerating every leaf type that gains a derive — collapsed to 6 lines covering the meaningful "what activates" + "which field is skipped" bits. - Verbosity: the `addresses_derived` `#[serde(skip)]` paragraph trimmed from 7 lines to 4, keeping the external-constraint citation (upstream type has no serde derive, no feature to activate) and the pointer to where persisters actually store the breadcrumb. - Present-state, not history: the `asset_lock_funding_type` adapter docstring referenced "the hand-rolled BlobWriter used before the serde swap" — that's PR history, dropped. Kept the meaningful "stable u8 tag for on-disk blob compat" rationale. - Verbosity / redundant-with-cfg: the `serde_adapters.rs` module header said "compiled only when the serde feature is on" with a paragraph pointing back to the `#[cfg(feature = "serde")]` line that imports it. The cfg gate is self-documenting; trimmed from 6 lines to 2 and rephrased "upstream types that don't (yet) derive" to "upstream types lacking their own derives" now that only one adapter remains. No tombstones added. No code semantics touched. cargo fmt + check (both feature flag states) + clippy --all-features + lib tests (121 passing) all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rop serde workarounds Workspace `Cargo.toml` swapped `rev = "53130869..."` for `branch = "feat/key-wallet-serde-derives"` on all 9 rust-dashcore crates to consume PR #761's serde derives ahead of upstream merge. Cargo's `[patch]` mechanism doesn't support same-source-different-branch redirects (rust-lang/cargo#5478, #10756); direct branch pinning is the working mechanism. Cargo.lock now resolves the 12 dashcore-sourced crates at `56a84402c8cb006d65b12e400785a62d67441930`, which the branch has been rebased onto so it carries ONLY the serde additions (no v0.42-dev drift). Dropped `serde_adapters::asset_lock_funding_type` adapter — type now derives upstream via #761. `serde_adapters.rs` deleted and removed from `changeset/mod.rs`. Dropped `#[serde(skip)]` on `CoreChangeSet::addresses_derived` — `DerivedAddress` now derives upstream (with `serde_pubkey33` adapter packaged inside #761 for the 33-byte pubkey field). Added `"key-wallet-manager/serde"` to platform-wallet's serde feature. Wire-format note: the upstream derive on `AssetLockFundingType` produces externally-tagged enum representation; the dropped custom adapter produced a stable `u8` wire-tag. PR #3637 is unreleased and the feature is opt-in — no consumer was persisting blobs with the prior format. Follow-up after #761 merges to v0.42-dev: swap `branch = "..."` back to `rev = "<new-rev>"` and remove the temp comment in workspace Cargo.toml. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Consuming dashpay/rust-dashcore#761 (
|
|
✅ Review complete (commit 5fe906e) |
`platform_wallet_info_create_from_mnemonic` now takes 3 args (network, mnemonic, out_handle); the integration tests still passed a 4th `std::ptr::null()` between `mnemonic` and `out_handle`, breaking the mac nextest job. Sync both call sites with the current FFI signature. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-existing `matches!(result, Err(_))` patterns trip `clippy::redundant_pattern_matching` under the workspace's `-D warnings` gate. Swap to `result.is_err()` so the clippy step stays green for the crates this PR touches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "90462ea6f3e56ebfb20f26c3ea326d07dffcbe63696c35dee81a822d563f79fa"
)Xcode manual integration:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3637 +/- ##
============================================
- Coverage 87.16% 87.15% -0.01%
============================================
Files 2607 2606 -1
Lines 319420 319221 -199
============================================
- Hits 278413 278216 -197
+ Misses 41007 41005 -2
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Purely additive PR adding opt-in serde derives behind a new serde feature on rs-platform-wallet changeset/wallet types plus AddressFunds in rs-sdk. No blocking issues. Two material suggestions: (1) the temporary workspace-wide switch from rev = to branch = on nine rust-dashcore crates weakens reproducibility, and (2) several newly-Serialize-able changesets use composite/struct BTreeMap keys ((Identifier, KeyID), OutPoint, SentContactRequestKey, (Identifier, Identifier)) which compile cleanly but fail at runtime under serde_json. Minor coverage and style nits round out the review.
Reviewed commit: f23183c
🟡 3 suggestion(s) | 💬 1 nitpick(s)
1 additional finding
🟡 suggestion: Newly-serializable changesets use non-string map keys — incompatible with `serde_json`
packages/rs-platform-wallet/src/changeset/changeset.rs (lines 358-363)
Several types behind the new serde feature use BTreeMap/BTreeSet keys that serde-derives can encode for binary formats (bincode/CBOR/MessagePack) but that serde_json rejects at runtime with key must be a string:
IdentityKeysChangeSet::upserts: BTreeMap<(Identifier, KeyID), _>(line 360)IdentityKeysChangeSet::removed: BTreeSet<(Identifier, KeyID)>(line 362)ContactChangeSet::sent_requests/incoming_requests/establishedkeyed bySentContactRequestKey/ReceivedContactRequestKey(lines 553/557/565)AssetLockChangeSet::asset_locks: BTreeMap<OutPoint, AssetLockEntry>(line 687)TokenBalanceChangeSet::balances: BTreeMap<(Identifier, Identifier), u64>(line 744)PlatformWalletChangeSet::dashpay_profiles/dashpay_payments_overlaykeyed byIdentifier(lines 896/900)
If the only intended consumer is a bincode-backed persister this is a latent footgun rather than an immediate bug, but the feature is public on a published crate and the PR description frames serde as the persistence path. A downstream consumer enabling platform-wallet/serde will compile cleanly and only discover the JSON limitation at runtime. Options: (a) reshape these as Vec<(K, V)>, (b) apply #[serde(with = ...)]/serde_with to flatten tuple/struct keys into strings, or (c) explicitly document in the feature flag that JSON is unsupported.
🤖 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 `Cargo.toml`:
- [SUGGESTION] lines 52-65: Pin rust-dashcore by `rev` instead of mutable `branch`
All nine `rust-dashcore`-derived workspace dependencies were switched from a fixed `rev = "53130869…"` to `branch = "feat/key-wallet-serde-derives"`. Cargo.lock currently locks the resolved commit (`56a84402c8cb006d65b12e400785a62d67441930`), but any `cargo update`, fresh lockfile generation, or upstream force-push/rebase/branch-deletion will silently re-resolve every dashcore-derived crate to whatever HEAD is at that moment. The blast radius covers the whole workspace, not just the two crates that actually need the new serde derives. Pinning to the current branch tip by `rev = "56a84402c8cb006d65b12e400785a62d67441930"` carries identical bits today, is immune to rebases of dashpay/rust-dashcore#761, and leaves a clean `rev` → `rev` swap when the upstream merges. This matches the hygiene pattern used at every other previous dashcore consumption point in this workspace.
In `packages/rs-platform-wallet/src/changeset/changeset.rs`:
- [SUGGESTION] lines 358-363: Newly-serializable changesets use non-string map keys — incompatible with `serde_json`
Several types behind the new `serde` feature use `BTreeMap`/`BTreeSet` keys that serde-derives can encode for binary formats (bincode/CBOR/MessagePack) but that `serde_json` rejects at runtime with `key must be a string`:
- `IdentityKeysChangeSet::upserts: BTreeMap<(Identifier, KeyID), _>` (line 360)
- `IdentityKeysChangeSet::removed: BTreeSet<(Identifier, KeyID)>` (line 362)
- `ContactChangeSet::sent_requests`/`incoming_requests`/`established` keyed by `SentContactRequestKey`/`ReceivedContactRequestKey` (lines 553/557/565)
- `AssetLockChangeSet::asset_locks: BTreeMap<OutPoint, AssetLockEntry>` (line 687)
- `TokenBalanceChangeSet::balances: BTreeMap<(Identifier, Identifier), u64>` (line 744)
- `PlatformWalletChangeSet::dashpay_profiles`/`dashpay_payments_overlay` keyed by `Identifier` (lines 896/900)
If the only intended consumer is a bincode-backed persister this is a latent footgun rather than an immediate bug, but the feature is public on a published crate and the PR description frames serde as the persistence path. A downstream consumer enabling `platform-wallet/serde` will compile cleanly and only discover the JSON limitation at runtime. Options: (a) reshape these as `Vec<(K, V)>`, (b) apply `#[serde(with = ...)]`/`serde_with` to flatten tuple/struct keys into strings, or (c) explicitly document in the feature flag that JSON is unsupported.
In `packages/rs-platform-wallet/Cargo.toml`:
- [SUGGESTION] lines 69-76: No round-trip serde tests cover the new serde surface
The PR adds `Serialize`/`Deserialize` derives on ~16 public types but no encode→decode test exercises them. The existing 121/121 test count cited in the description is pre-existing — `cargo check --features serde` only proves the feature compiles. The derives use default representation (field names as map keys, field order for non-self-describing formats), so any future field rename/reorder or upstream serde-impl change in transitive deps (`TransactionRecord`, `Utxo`, `InstantLock`, `AssetLockFundingType`, `AddressInfo`, `ExtendedPubKey`, `PlatformP2PKHAddress`, `AddressFunds`, `IdentityPublicKey`, `AssetLockProof`) can silently break the wire format with no signal in this crate's test suite. A small set of bincode round-trip tests on `PlatformWalletChangeSet`, `IdentityEntry`, and `AssetLockEntry` would lock down the format and catch regressions before they reach a persister.
![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
🤖 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 `@packages/rs-platform-wallet/src/changeset/changeset.rs`:
- Around line 867-868: CoreChangeSet and PlatformWalletChangeSet currently add
serde derives via #[cfg_attr(feature = "serde", ...)] but they contain upstream
types (TransactionRecord, Utxo, InstantLock, etc.) that do not implement serde
yet, causing build failures when the serde feature is enabled; fix this by
removing or disabling the conditional serde derives until the upstream
feat/key-wallet-serde-derives branch lands — either delete the
#[cfg_attr(feature = "serde", derive(...))] lines on the CoreChangeSet and
PlatformWalletChangeSet structs, or replace them with a new feature gate (e.g.,
#[cfg_attr(feature = "key-wallet-serde", derive(...))]) that you only enable
once the upstream types implement serde, and keep a comment referencing
TransactionRecord, Utxo, and InstantLock so future maintainers know why the
serde derives are withheld.
🪄 Autofix (Beta)
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
Run ID: 17f27c68-f361-4ba4-97a2-cf0618592baa
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlpackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/changeset/changeset.rspackages/rs-platform-wallet/src/wallet/asset_lock/tracked.rspackages/rs-platform-wallet/src/wallet/identity/types/block_time.rspackages/rs-platform-wallet/src/wallet/identity/types/dashpay/contact_request.rspackages/rs-platform-wallet/src/wallet/identity/types/dashpay/established_contact.rspackages/rs-platform-wallet/src/wallet/identity/types/dashpay/payment.rspackages/rs-platform-wallet/src/wallet/identity/types/dashpay/profile.rspackages/rs-platform-wallet/src/wallet/identity/types/key_storage.rspackages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rspackages/rs-sdk/src/platform/address_sync/types.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The serde derives added in this PR are mechanically consistent with the types they touch, and I did not confirm any consensus, runtime, or persistence-path regression in the wallet code itself. Three of the reported issues depend on assumptions the source does not make: the new feature is documented as serde support for changeset types, not for the separately documented startup snapshot path, and it does not promise JSON-specific encoding or backward-compatible on-disk schema evolution. The remaining review-worthy issue is the workspace-wide move from fixed git revisions to a floating branch, which introduces avoidable build drift on the next lockfile refresh.
Reviewed commit: 5fe906e
🟡 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 `Cargo.toml`:
- [SUGGESTION] lines 57-65: Floating branch pins weaken workspace reproducibility
This PR replaces nine `rust-dashcore` workspace dependencies that were pinned to commit `53130869e5b9343ae59016323e5e5269e717a8fd` with `branch = "feat/key-wallet-serde-derives"`. The comment above these lines explains the intent, but the manifest still stops identifying the exact upstream source being reviewed. `Cargo.lock` keeps this checkout stable today, yet any future `cargo update` or lockfile regeneration can silently pull a different branch head into consensus-sensitive crates. Keeping the temporary serde workaround on a fixed `rev` preserves the same functionality without introducing that drift.
…wallet-serde # Conflicts: # Cargo.lock # Cargo.toml
lklimek
added
the
ready for final review
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the worktree is exactly at 083c6cc8f2e2efab049e395fffcae03d11a9f60f. The one carried-forward prior finding is resolved: Cargo.toml no longer floats on branch = "feat/key-wallet-serde-derives" and now pins the rust-dashcore family to rev = "f569e7b7b99dfe589c41f9ba7d36fbbe6805acdc". I also checked the current rs-platform-wallet serde-related surface and did not confirm any new correctness, consensus, or Rust-quality issues in the reviewed changes.
github-project-automation
Bot
moved this from In review / testing
to Done
in Platform team
Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 participants
Issue being fixed or feature implemented
Splits the
rs-platform-walletserde-support additions out of PR #3625 (feat(platform-wallet): add platform-wallet-storage crate (sqlite persister)) so they can land independently onv3.1-dev. Original commit (e26945cfdfby @lklimek) stays in PR #3625; this PR cherry-picks just the serde-additive changes for separate review/merge cadence — same hygiene pattern used for PR #3636 (birth_height_overridesplit from PR #3549).What was done?
Adds optional
serdederives behind aserdefeature flag onrs-platform-wallet's public types. 12 files, +154 lines. Touches:packages/rs-platform-wallet/Cargo.toml— new[features] serde = ["dep:serde", "key-wallet/serde", "key-wallet-manager/serde", "dash-sdk/serde"]entry + optionalserdedependency. Propagation reachesrs-sdk(AddressFunds),key-wallet(AssetLockFundingType— upstream derive after feat(key-wallet,key-wallet-manager): add serde derives for AssetLockFundingType and DerivedAddress rust-dashcore#761), andkey-wallet-manager(DerivedAddress— upstream derive after fix commenting #761).packages/rs-platform-wallet/src/changeset/—changeset.rsandmod.rsupdated with#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]attributes on the changeset types. Noserde_adaptersmodule — upstream now derives bothAssetLockFundingType(via#761) andDerivedAddress(via#761with aserde_pubkey33adapter for the 33-byte pubkey field), so the changeset types serialize via plain derives.packages/rs-sdk/src/platform/address_sync/types.rs—AddressFundsgets a directcfg_attrderive.packages/rs-platform-wallet/src/wallet/—cfg_attrderives on 8 leaf types inasset_lock/tracked.rs,identity/types/block_time.rs,identity/types/key_storage.rs, andidentity/types/dashpay/{contact_request,established_contact,payment,profile}.rs.Cargo.toml— temporary: 9 rust-dashcore deps swapped fromrev = "53130869..."tobranch = "feat/key-wallet-serde-derives"to consume feat(key-wallet,key-wallet-manager): add serde derives for AssetLockFundingType and DerivedAddress rust-dashcore#761 ahead of its upstream merge. Cargo's[patch]mechanism doesn't support same-source-different-branch redirects (rust-lang/cargo#5478, #10756); direct branch pinning is the working alternative. fix commenting #761's branch has been rebased onto53130869so it carries ONLY the serde additions (no v0.42-dev drift). Follow-up: drop the branch pin and restorerev = "<new>"after fix commenting #761 merges tov0.42-devand a subsequent dashcore rev bump flows into this workspace.No production-logic changes — purely additive serde derive lines + adapter helpers, behind a default-off feature gate.
Breaking Changes
None. The additions are behind a
serdefeature that's off by default.How Has This Been Tested?
cargo check -p platform-wallet --no-default-features— cleancargo check -p platform-wallet --features serde— cleancargo check -p platform-wallet --tests --all-features— cleancargo clippy -p platform-wallet --tests --all-features --no-deps -- -D warnings— zero warningscargo test -p platform-wallet --features serde --lib— 121/121 passcargo fmt— cleanCompanion to
PR #3625 retains the same commit (
e26945cfdf); this is a hygiene split for independent review cadence. When this PR merges tov3.1-dev, the cherry-pick in PR #3625 becomes a duplicate-by-content and git drops it on rebase.Checklist
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
serdefeature is enabled, allowing wallet state persistence and data interchange capabilities across the platform.Chores