Skip to content

feat(platform-wallet): serde support #3637

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Claudius-Maginificent wants to merge 7 commits into
base: v3.1-dev
Choose a base branch
from
Open

feat(platform-wallet): serde support #3637

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1eb2d2d
feat(platform-wallet): add optional serde derives behind serde feature
lklimek May 11, 2026
9485e54
refactor(rs-platform-wallet): drop redundant address_funds serde adapter
lklimek May 13, 2026
6d82a99
docs(rs-platform-wallet): tighten comments per coding-best-practices
lklimek May 13, 2026
561ccb4
feat(rs-platform-wallet): consume rust-dashcore#761 via branch pin, d…
lklimek May 13, 2026
6c11ece
fix(rs-platform-wallet-ffi): drop stale ptr arg from integration_tests
lklimek May 13, 2026
f23183c
chore(rs-platform-wallet-ffi): use Result::is_err in group_info tests
lklimek May 13, 2026
5fe906e
Merge remote-tracking branch 'origin/v3.1-dev' into feat/rs-platform-…
lklimek May 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 24 additions & 22 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 14 additions & 9 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,20 @@ members = [
]

[workspace.dependencies]
dashcore = { git = "https://github.com/dashpay/rust-dashcore", rev = "53130869e5b9343ae59016323e5e5269e717a8fd" }
dash-network-seeds = { git = "https://github.com/dashpay/rust-dashcore", rev = "53130869e5b9343ae59016323e5e5269e717a8fd" }
dash-spv = { git = "https://github.com/dashpay/rust-dashcore", rev = "53130869e5b9343ae59016323e5e5269e717a8fd" }
dash-spv-ffi = { git = "https://github.com/dashpay/rust-dashcore", rev = "53130869e5b9343ae59016323e5e5269e717a8fd" }
key-wallet = { git = "https://github.com/dashpay/rust-dashcore", rev = "53130869e5b9343ae59016323e5e5269e717a8fd" }
key-wallet-ffi = { git = "https://github.com/dashpay/rust-dashcore", rev = "53130869e5b9343ae59016323e5e5269e717a8fd" }
key-wallet-manager = { git = "https://github.com/dashpay/rust-dashcore", rev = "53130869e5b9343ae59016323e5e5269e717a8fd" }
dash-network = { git = "https://github.com/dashpay/rust-dashcore", rev = "53130869e5b9343ae59016323e5e5269e717a8fd" }
dashcore-rpc = { git = "https://github.com/dashpay/rust-dashcore", rev = "53130869e5b9343ae59016323e5e5269e717a8fd" }
# Temporary: branch-pinned to dashpay/rust-dashcore#761 (`feat/key-wallet-serde-derives`)
# for serde derives on AssetLockFundingType + DerivedAddress. The branch has been
# rebased onto 53130869 so it carries ONLY the serde additions (no v0.42-dev drift).
# Drop the branch= override and restore rev= once #761 merges to v0.42-dev and a
# subsequent dashcore rev bump flows the new rev in.
dashcore = { git = "https://github.com/dashpay/rust-dashcore", branch = "feat/key-wallet-serde-derives" }
dash-network-seeds = { git = "https://github.com/dashpay/rust-dashcore", branch = "feat/key-wallet-serde-derives" }
dash-spv = { git = "https://github.com/dashpay/rust-dashcore", branch = "feat/key-wallet-serde-derives" }
dash-spv-ffi = { git = "https://github.com/dashpay/rust-dashcore", branch = "feat/key-wallet-serde-derives" }
key-wallet = { git = "https://github.com/dashpay/rust-dashcore", branch = "feat/key-wallet-serde-derives" }
key-wallet-ffi = { git = "https://github.com/dashpay/rust-dashcore", branch = "feat/key-wallet-serde-derives" }
key-wallet-manager = { git = "https://github.com/dashpay/rust-dashcore", branch = "feat/key-wallet-serde-derives" }
dash-network = { git = "https://github.com/dashpay/rust-dashcore", branch = "feat/key-wallet-serde-derives" }
dashcore-rpc = { git = "https://github.com/dashpay/rust-dashcore", branch = "feat/key-wallet-serde-derives" }
Comment on lines +52 to +65
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw May 13, 2026

Choose a reason for hiding this comment

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

🟡 Suggestion: 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.

💡 Suggested change
Suggested change
# Temporary: branch-pinned to dashpay/rust-dashcore#761 (`feat/key-wallet-serde-derives`)
# for serde derives on AssetLockFundingType + DerivedAddress. The branch has been
# rebased onto 53130869 so it carries ONLY the serde additions (no v0.42-dev drift).
# Drop the branch= override and restore rev= once #761 merges to v0.42-dev and a
# subsequent dashcore rev bump flows the new rev in.
dashcore = { git = "https://github.com/dashpay/rust-dashcore", branch = "feat/key-wallet-serde-derives" }
dash-network-seeds = { git = "https://github.com/dashpay/rust-dashcore", branch = "feat/key-wallet-serde-derives" }
dash-spv = { git = "https://github.com/dashpay/rust-dashcore", branch = "feat/key-wallet-serde-derives" }
dash-spv-ffi = { git = "https://github.com/dashpay/rust-dashcore", branch = "feat/key-wallet-serde-derives" }
key-wallet = { git = "https://github.com/dashpay/rust-dashcore", branch = "feat/key-wallet-serde-derives" }
key-wallet-ffi = { git = "https://github.com/dashpay/rust-dashcore", branch = "feat/key-wallet-serde-derives" }
key-wallet-manager = { git = "https://github.com/dashpay/rust-dashcore", branch = "feat/key-wallet-serde-derives" }
dash-network = { git = "https://github.com/dashpay/rust-dashcore", branch = "feat/key-wallet-serde-derives" }
dashcore-rpc = { git = "https://github.com/dashpay/rust-dashcore", branch = "feat/key-wallet-serde-derives" }
dashcore = { git = "https://github.com/dashpay/rust-dashcore", rev = "56a84402c8cb006d65b12e400785a62d67441930" }
dash-network-seeds = { git = "https://github.com/dashpay/rust-dashcore", rev = "56a84402c8cb006d65b12e400785a62d67441930" }
dash-spv = { git = "https://github.com/dashpay/rust-dashcore", rev = "56a84402c8cb006d65b12e400785a62d67441930" }
dash-spv-ffi = { git = "https://github.com/dashpay/rust-dashcore", rev = "56a84402c8cb006d65b12e400785a62d67441930" }
key-wallet = { git = "https://github.com/dashpay/rust-dashcore", rev = "56a84402c8cb006d65b12e400785a62d67441930" }
key-wallet-ffi = { git = "https://github.com/dashpay/rust-dashcore", rev = "56a84402c8cb006d65b12e400785a62d67441930" }
key-wallet-manager = { git = "https://github.com/dashpay/rust-dashcore", rev = "56a84402c8cb006d65b12e400785a62d67441930" }
dash-network = { git = "https://github.com/dashpay/rust-dashcore", rev = "56a84402c8cb006d65b12e400785a62d67441930" }
dashcore-rpc = { git = "https://github.com/dashpay/rust-dashcore", rev = "56a84402c8cb006d65b12e400785a62d67441930" }

source: ['claude', 'codex']

🤖 Fix this 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.

Comment on lines +57 to +65
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw May 14, 2026

Choose a reason for hiding this comment

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

🟡 Suggestion: 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.

source: ['codex']

🤖 Fix this 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.


# Optimize heavy crypto crates even in dev/test builds so that
# Halo 2 proof generation and verification run at near-release speed.
Expand Down
4 changes: 2 additions & 2 deletions packages/rs-platform-wallet-ffi/src/tokens/group_info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ mod tests {
fn test_decode_other_signer_null_action_id() {
unsafe {
let result = decode_group_info(2, 0, std::ptr::null(), false);
assert!(matches!(result, Err(_)), "expected Err(NullPointer)");
assert!(result.is_err(), "expected Err(NullPointer)");
}
}

Expand All @@ -120,7 +120,7 @@ mod tests {
fn test_decode_invalid_kind() {
unsafe {
let result = decode_group_info(99, 0, std::ptr::null(), false);
assert!(matches!(result, Err(_)), "expected Err(InvalidParameter)");
assert!(result.is_err(), "expected Err(InvalidParameter)");
}
}
}
2 changes: 0 additions & 2 deletions packages/rs-platform-wallet-ffi/tests/integration_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ fn test_wallet_from_mnemonic() {
let result = platform_wallet_info_create_from_mnemonic(
Network::Testnet.into(),
mnemonic.as_ptr(),
std::ptr::null(),
&mut handle,
);

Expand Down Expand Up @@ -266,7 +265,6 @@ fn test_full_workflow() {
let result = platform_wallet_info_create_from_mnemonic(
Network::Testnet.into(),
mnemonic.as_ptr(),
std::ptr::null(),
&mut wallet_handle,
);
assert_eq!(result.code, PlatformWalletFFIResultCode::Success);
Expand Down
10 changes: 10 additions & 0 deletions packages/rs-platform-wallet/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ tracing = "0.1"
# Encoding
hex = "0.4"
bs58 = "0.5"
serde = { version = "1", default-features = false, features = ["derive"], optional = true }
serde_json = "1.0"

# Image processing (DIP-15 avatar hash + fingerprint)
Expand Down Expand Up @@ -65,6 +66,15 @@ default = ["bls", "eddsa"]
bls = ["key-wallet/bls", "key-wallet-manager/bls"]
eddsa = ["key-wallet/eddsa", "key-wallet-manager/eddsa"]
shielded = ["dep:grovedb-commitment-tree", "dep:zip32", "dash-sdk/shielded", "dpp/shielded-client"]
# Opt-in serde derives on the changeset types. Activates `key-wallet/serde`,
# `key-wallet-manager/serde` (both via dashpay/rust-dashcore#761, branch-pinned in
# workspace Cargo.toml), and `dash-sdk/serde`. `dpp` derives serde unconditionally.
serde = [
"dep:serde",
"key-wallet/serde",
"key-wallet-manager/serde",
"dash-sdk/serde",
Comment on lines +69 to +76
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw May 13, 2026

Choose a reason for hiding this comment

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

🟡 Suggestion: 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.

source: ['claude']

🤖 Fix this 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/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.

]
# Forward to the upstream `key-wallet` / `key-wallet-manager`
# `keep-finalized-transactions` feature. With it OFF (the default),
# chainlocked transactions are evicted from the in-memory
Expand Down
Loading
Loading