refactor(sdk): dedup shared wallet code + remove dead FFI/JNI chains#4106
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ 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:
✨ 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 |
|
🕓 Ready for review — 11 ahead in queue (commit 011c866) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified this is a byte-for-byte behavior-preserving extraction: the shared candidate_address_set helper in wallet.rs is identical to the two previously duplicated inline blocks in transfer.rs and withdrawal.rs, including error variants, address union/conversion, and lock release timing before the network fetch. cargo check passes cleanly and no in-scope issues survive review.
Source: reviewers gpt-5.6-sol (Sol reviewer), claude-sonnet-5 (Sonnet reviewer), and claude-opus-4-8 (Opus reviewer), general lane; verifier claude-sonnet-5. Orchestrator openai/gpt-5.6-sol (orchestration-only).
transfer.rs::auto_select_inputs and withdrawal.rs::plan_withdrawal both built the platform-address candidate SET with an identical block — take the wallet-manager read lock, resolve the payment account, and union the transient derived pool (addresses.addresses) with the hydrated address_balances map. The block was copy-pasted (8bd3a85 retrofitted transfer onto the shape withdrawal already had), so a future fix to how spend candidates are enumerated would have to be made twice or the two live spend paths would silently diverge. Extract it into PlatformAddressWallet::candidate_address_set, called from both. Behavior is unchanged: each caller keeps its own downstream handling (transfer tolerates an empty set; withdrawal errors early), and balances are still read fresh from the chain via AddressInfo::fetch_many at each call site. Drops two now-unused imports (BTreeSet, PlatformP2PKHAddress) from both callers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
platform_wallet_derive_identity_private_key_at_slot picked its key source by wallet capability and took a wallet-manager READ lock in its capability check. But its only intended caller — the identity-key persist callback — fires while platform-wallet holds the wallet-manager WRITE lock, so the read-lock reacquisition would deadlock. Every real flow therefore uses the resolver-keyed sibling (dash_sdk_derive_identity_key_at_slot_with_resolver, shared with iOS) instead, leaving this export and its whole chain uncalled: - Rust export + IdentityPrivateKeyFFI (identity_private_key_at_slot.rs) - its lib.rs module + re-export - the JNI trampoline + import in rs-unified-sdk-jni (kept net_from_ord) - the Kotlin IdentityNative.deriveIdentityPrivateKey binding Verified no caller in-repo or org-wide. Doc comments that explained 'why the resolver variant, not the handle one' now stand alone. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Four native bindings declared but never called from Kotlin (verified no in-repo or org-wide caller); removing both the Kotlin external fun and its Java_... JNI trampoline for each: - createCallbacks / destroyCallbacks — an alternate persistence-vtable entry point superseded by the live wallet-manager path (WalletManagerNative.nativeCreate -> build_vtable). The whole PersistenceNative object is dead, so the file is removed; the live build_vtable / KotlinPersistenceCtx are kept. - managedIdentityDashPayProfile - resolverSupportsKeyType - walletGetId Also drops the now-unused guard/JClass/jlong imports and refreshes the persistence module docs (incl. a stale 'lands in a later milestone' note). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Drop two no-op #[allow(clippy::too_many_arguments)] on the top-up exports (6 args each, below clippy's default threshold of 7). - Replace Identifier::from_bytes(&[u8;32]).unwrap_or_default() with the infallible Identifier::from([u8;32]): the input is a fixed 32-byte array so the length check never fails and the zero-default was unreachable — the idiom just read as if a malformed id silently became all-zeros. - Drop a stale ADDR-02 test-case reference from a transfer.rs comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
762e88b to
f5afcb6
Compare
…edup Resolve modify/delete conflict on identity_private_key_at_slot.rs: keep the deletion — the incoming change was only a module-doc tweak (the ExtendedPrivKey Drop note), no new callers of the dead FFI chain. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
516b265
into
feat/kotlin-sdk-and-example-app
…nd-example-app Brings in DIP-13/15 DashPay invitations (#4041). Conflict resolution: - persistence.rs: kept the round_lock/RoundGuardState round serializer (a superset of v4.1-dev's store_round) + grafted #4041's persists_durably() invitation-durability attestation. Dropped v4.1-dev's duplicate store_round round test (RoundProbe collided with #4071's). - asset_lock/build.rs: kept both ensure_identity_topup_account and #4041's persist_asset_lock_account_pools; tightened broadcast_funded_asset_lock + invitation create_invitation to <ExtendedPubKeySigner> to match #4106's asset-lock signer tightening (extended the UnreachableSigner test double). - rs-unified-sdk-jni: left on_persist_invitations_fn None so Android's persists_durably() stays fail-closed (the invitation flow refuses to run without durable funding-index persistence). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Issue being fixed or feature implemented
Cleanup from the duplication/slop review of the Kotlin-SDK PR (#3999), stacked on
feat/kotlin-sdk-and-example-app. Three kinds of change, all behavior-preserving. Net −651 lines.What was done?
Dedup
Extracted the byte-identical candidate-address-SET union block (wallet-manager read lock → resolve payment account → union the derived pool with the hydrated
address_balancesmap) fromtransfer.rs::auto_select_inputsandwithdrawal.rs::plan_withdrawalintoPlatformAddressWallet::candidate_address_set. Both are live spend paths and the block was copy-pasted (8bd3a858dbretrofitted transfer onto withdrawal's shape) — a divergence risk. Behavior unchanged: each caller keeps its own empty-handling (transfer tolerates an empty set; withdrawal errors early), and balances are still read fresh from chain viaAddressInfo::fetch_manyat each site.Dead-code removal
Bindings declared but never called — verified zero callers in-repo and org-wide (
gh search code --owner dashpay); the Kotlin bindings are allinternal(not public API). Each removes both the Kotlinexternal funand itsJava_...JNI trampoline.platform_wallet_derive_identity_private_key_at_slot(~465 LOC). It deadlocks in its only intended context — the identity-key persist callback fires while platform-wallet holds the wallet-manager write lock, and this export re-takes a read lock in its capability check — so every real flow uses the resolver-keyed siblingdash_sdk_derive_identity_key_at_slot_with_resolver(the same one iOS uses). Removed the Rust export +IdentityPrivateKeyFFI, itslib.rsmodule/re-export, the JNI trampoline + import (keptnet_from_ord), and the KotlinderiveIdentityPrivateKeybinding.createCallbacks/destroyCallbacks— an alternate persistence-vtable entry point superseded by the liveWalletManagerNative.nativeCreate → build_vtablepath; the wholePersistenceNativeobject was dead, so the file is removed (keptbuild_vtable/KotlinPersistenceCtx, which the live path uses).managedIdentityDashPayProfile,resolverSupportsKeyType,walletGetId.Also drops the now-unused
guard/JClass/jlongimports and refreshes the persistence module docs (incl. a stale "wiring lands in a later milestone" note).Minor cleanups
#[allow(clippy::too_many_arguments)]on the top-up exports (6 args each, below clippy's threshold).Identifier::from_bytes(&[u8;32]).unwrap_or_default()with the infallibleIdentifier::from([u8;32])— the input is a fixed 32-byte array, so the length check never fails and the zero-default was unreachable (it read as if a malformed id silently became all-zeros).ADDR-02test-case reference from atransfer.rscomment.How Has This Been Tested?
cargo check -p platform-wallet -p platform-wallet-ffi -p rs-unified-sdk-jni— clean (warnings-as-errors).cargo fmt --check— clean.candidate_address_setregression tests pass through the new helper:plan_withdrawal_sizes_inputs_from_chain_not_doubled_cache,plan_withdrawal_finds_candidates_from_balance_map_when_pool_empty.Breaking Changes
None. All removed symbols were uncalled (internal bindings, no public API, no org-wide reference); the dedup is behavior-preserving.
Checklist:
Deferred (from the same #3999 review, with reasons)
release_row/free_rows) — on inspection these are named, documented, unit-tested single-zeroization-site helpers, not vestigial; left as-is.