feat(sdk): add DashPay persistence and read bridges to Kotlin SDK (migration K1)#4025
feat(sdk): add DashPay persistence and read bridges to Kotlin SDK (migration K1)#4025shumkov wants to merge 3 commits into
Conversation
Reviewed spec (5 lenses: feasibility, scope, security, adversarial, domain-fit) for porting the PR #3841 DashPay completion to the Kotlin SDK, in three stacked milestones (K1 persistence+reads, K2 sync service+seedless unlock+writes, K3 DashPay tab UI). PARITY.md: the FriendsView row claimed parity against a Swift view that PR #3841 deleted; mark the DashPay surface as in-migration instead of counting it ported. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
First milestone of the DashPay Swift→Kotlin migration (docs/dashpay/KOTLIN_MIGRATION_SPEC.md). Persistence — the two missing DashPay stores, mirroring the SwiftData models added by #3841: - DashpayContactProfileEntity (cached contact profiles; persister- projected with is_present=false tombstone DELETEs so a removed on-chain profile can't leave a stale name/avatar) and DashpayPaymentEntity (payment history; pull-persisted via the new refreshDashPayPayments — the sweep reconciles in-memory only). - Room v2→v3 additive migration + exported schema 3.json. - Both bridge directions: identity-entry persist now delivers contact-profile deltas (new onPersistContactProfileDelta bridge slot), and the wallet-list restore path marshals the payments / contact_profiles arrays that were previously null-stubbed in rs-unified-sdk-jni/src/persistence.rs (staging/seal/free extended). Also corrects the stale tombstone doc comments in identity_persistence.rs that contradicted the projection code. Read bridges — new rs-unified-sdk-jni/src/dashpay.rs + DashpayNative.kt (6 exports, JSON-string convention per getDashPayProfile): managed_identity_get_dashpay_payments, platform_wallet_get_contact_profile, managed_identity_get_dashpay_profile, managed_identity_get_dashpay_sync_state, platform_wallet_search_dpns_names (wallet-scoped — the AddContactView call path), platform_wallet_manager_get_account_balances. Kotlin surface: Dashpay.{payments,syncState,getContactProfile,searchDpnsNames}, PlatformWalletManager.{accountBalances,refreshDashPayPayments}. Tests: red-tier coverage per the reviewed spec — 3 new JVM handler tests (profile delta upsert, tombstone delete, payments+profiles restore round-trip; 27/27 green), a new instrumented migration test validating MIGRATION_2_3 against the exported schema, and an instrumented DashPay persist→wipe→restore→re-read round-trip through the real JNI staging/seal/free pipeline (runs on the CI emulator; JVM tests cannot reach that code). build_android.sh --verify not run locally (no NDK on this machine) — covered by kotlin-sdk-build.yml. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three-lens review of 007af13 (JNI memory safety, persistence correctness, test rigor) — no must-fixes; folded the should-fixes: - refreshDashPayPayments now preserves createdAt and skips unchanged rows (a Room @upsert rewrites every column, so the unconditional upsert clobbered createdAt with 'now' and re-fired the payments Flow on every refresh — Swift's persistDashpayPayments deliberately change-detects; now mirrored). - Empty-txid payment rows are filtered before restore staging (the Rust restore fold inserts an "" map key rather than skipping; the build_payment_restore comment claimed otherwise — comment corrected). - Rollback test for onPersistContactProfileDelta (a delta that wrote eagerly instead of staging would have passed every existing test). - Instrumented round-trip grew a tombstone-restore leg (deleted profile row restores as an absent Rust cache entry; exercises the empty-array marshaling path) and an accountBalances smoke assertion (that FFI array marshal/free path previously had no coverage at any tier); searchDpnsNames deferral to the testnet tier is now documented in the test docstring. - Clarified the owner-missing skip comment on the delta handler (the identity upsert is staged into the same buffer first, so the lookup succeeds at replay — the skip is defensive, not a retry mechanism). JVM suite: 28/28 green. Instrumented tests compile; execution remains CI-emulator-bound (no NDK locally), as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
shumkov
requested review from
QuantumExplorer,
ZocoLini,
lklimek and
llbartekll
as code owners
|
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 |
|
✅ Review complete (commit 9c9b061) |
shumkov
changed the title
There was a problem hiding this comment.
Code Review
Source: reviewers = claude ffi-engineer opus; claude general opus; codex ffi-engineer gpt-5.5; codex general gpt-5.5; verifier = claude opus; specialists = ffi-engineer.
K1 DashPay persistence + read bridges is well-structured: JNI entries wrap catch_unwind, alloc/free are paired, Room migration has a schema-diff test, and the persistence-handler tombstone/upsert paths are covered. Real findings are minor: three agents converge on searchDpnsNames silently returning null on interior-NUL prefixes, and refreshDashPayPayments quietly drops persistence when the owner identity row hasn't landed. One adjacent nitpick rounds out the review.
🟡 3 suggestion(s) | 💬 1 nitpick(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 `packages/rs-unified-sdk-jni/src/dashpay.rs`:
- [SUGGESTION] packages/rs-unified-sdk-jni/src/dashpay.rs:308-315: searchDpnsNames returns null silently on JString/CString failure, indistinguishable from an empty result
Both early-return paths in the JNI entry — `env.get_string(&prefix)` on error and `CString::new(prefix_str)` on interior NUL — return `ptr::null_mut()` without throwing. A successful call returns a JSON array string (empty `"[]"` is legal), so the Kotlin caller sees `String?` = null and cannot tell a boundary failure from `no matches`. The AddContactView flow would silently show 'no results' for a user-typed prefix containing `\^@`. The rest of this file (e.g. `read_id32` at lines 34-38) uses `DashSDKException` via the throw helpers — do the same here for consistency and to surface the failure to Kotlin.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:554-558: refreshDashPayPayments returns success JSON without persisting when the owner identity row is missing
When `identityDao().getByIdentityId(identityId)` returns null the function returns the raw JSON as if the refresh succeeded, but no rows are persisted. The KDoc emphasizes this is the *only* durable path for payment rows (particularly Sent+memo entries), so a persistence-order race between identity upsert and this refresh silently drops writes that only this refresh can materialize — a subsequent relaunch will be missing exactly those entries. The inline comment acknowledges the skip is intentional ("the rows just aren't persisted this round"), but there is no signal in logs or in the return value. At minimum emit a `Log.w` here so the failure mode is observable in the field; ideally have the caller either await the identity row or receive a distinguishable signal.
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:559-594: The skip-unchanged / createdAt-preservation branch of refreshDashPayPayments is untested
The reason this method is more than a straight `@Upsert` is the invariant described in the inline comment: a blind upsert would clobber `createdAt` and re-fire the payments Flow on every refresh. Neither `WalletManagerRoundTripTest` nor `PlatformWalletPersistenceHandlerTest` exercises `refreshDashPayPayments` — the round-trip test writes rows via the DAO directly. Add a Robolectric-level test that (1) seeds an existing `DashpayPaymentEntity`, (2) invokes `refreshDashPayPayments` with matching JSON, and asserts `createdAt` is unchanged and no upsert occurs. This guards the invariant this branch exists to defend.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [NITPICK] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:926-934: onPersistContactProfileDelta silently drops the delta when the owner identity is missing
The code comment explicitly documents that the identity upsert is always staged before its contact-profile deltas, so this `return@stage` should be unreachable — meaning it's exactly the case that must not fail silently if the ordering assumption is ever violated. A single `Log.w` (or whatever logging facility the project uses) makes an ordering regression observable instead of manifesting downstream as a phantom missing profile. Consistent with the surrounding defensive style, just noisier.
| guard(&mut env, ptr::null_mut(), |env| { | ||
| let prefix_str: String = match env.get_string(&prefix) { | ||
| Ok(s) => s.into(), | ||
| Err(_) => return ptr::null_mut(), | ||
| }; | ||
| let Ok(prefix_c) = std::ffi::CString::new(prefix_str) else { | ||
| return ptr::null_mut(); | ||
| }; |
There was a problem hiding this comment.
🟡 Suggestion: searchDpnsNames returns null silently on JString/CString failure, indistinguishable from an empty result
Both early-return paths in the JNI entry — env.get_string(&prefix) on error and CString::new(prefix_str) on interior NUL — return ptr::null_mut() without throwing. A successful call returns a JSON array string (empty "[]" is legal), so the Kotlin caller sees String? = null and cannot tell a boundary failure from no matches. The AddContactView flow would silently show 'no results' for a user-typed prefix containing \u0000. The rest of this file (e.g. read_id32 at lines 34-38) uses DashSDKException via the throw helpers — do the same here for consistency and to surface the failure to Kotlin.
| guard(&mut env, ptr::null_mut(), |env| { | |
| let prefix_str: String = match env.get_string(&prefix) { | |
| Ok(s) => s.into(), | |
| Err(_) => return ptr::null_mut(), | |
| }; | |
| let Ok(prefix_c) = std::ffi::CString::new(prefix_str) else { | |
| return ptr::null_mut(); | |
| }; | |
| let prefix_str: String = match env.get_string(&prefix) { | |
| Ok(s) => s.into(), | |
| Err(_) => { | |
| crate::support::throw_sdk_exception(env, 1, "searchDpnsNames: failed to read prefix string"); | |
| return ptr::null_mut(); | |
| } | |
| }; | |
| let Ok(prefix_c) = std::ffi::CString::new(prefix_str) else { | |
| crate::support::throw_sdk_exception(env, 1, "searchDpnsNames: prefix contained an interior NUL"); | |
| return ptr::null_mut(); | |
| }; |
source: ['claude', 'codex']
| // networkRaw rides on the owner identity row (the persist-path | ||
| // convention); when the identity row hasn't landed yet the read | ||
| // still succeeds — the rows just aren't persisted this round. | ||
| val networkRaw = database.identityDao().getByIdentityId(identityId)?.networkRaw | ||
| ?: return@withContext json |
There was a problem hiding this comment.
🟡 Suggestion: refreshDashPayPayments returns success JSON without persisting when the owner identity row is missing
When identityDao().getByIdentityId(identityId) returns null the function returns the raw JSON as if the refresh succeeded, but no rows are persisted. The KDoc emphasizes this is the only durable path for payment rows (particularly Sent+memo entries), so a persistence-order race between identity upsert and this refresh silently drops writes that only this refresh can materialize — a subsequent relaunch will be missing exactly those entries. The inline comment acknowledges the skip is intentional ("the rows just aren't persisted this round"), but there is no signal in logs or in the return value. At minimum emit a Log.w here so the failure mode is observable in the field; ideally have the caller either await the identity row or receive a distinguishable signal.
source: ['claude']
| // Skip-unchanged + preserve createdAt, mirroring Swift's | ||
| // persistDashpayPayments: a Room @Upsert rewrites every column, | ||
| // so an unconditional upsert would clobber createdAt with "now" | ||
| // on every refresh and re-fire the payments Flow (re-rendering | ||
| // an open payment list) even when nothing moved. | ||
| val existingByTxid = database.dashpayDao() | ||
| .getPaymentsByOwner(identityId) | ||
| .associateBy { it.txid } | ||
| val rows = JSONArray(json) | ||
| val entities = ArrayList<DashpayPaymentEntity>(rows.length()) | ||
| for (i in 0 until rows.length()) { | ||
| val row = rows.getJSONObject(i) | ||
| val txid = row.optString("txid", "") | ||
| val counterparty = row.optString("counterpartyId", "").hexToBytesOrNull() | ||
| if (txid.isEmpty() || counterparty == null || counterparty.size != 32) continue | ||
| val existing = existingByTxid[txid] | ||
| val entity = DashpayPaymentEntity( | ||
| networkRaw = networkRaw, | ||
| ownerIdentityId = identityId, | ||
| counterpartyIdentityId = counterparty, | ||
| amountDuffs = row.optLong("amountDuffs"), | ||
| directionRaw = row.optInt("direction"), | ||
| statusRaw = row.optInt("status"), | ||
| txid = txid, | ||
| memo = if (row.has("memo")) row.getString("memo") else null, | ||
| createdAt = existing?.createdAt ?: java.util.Date(), | ||
| ) | ||
| val unchanged = existing != null && | ||
| existing.counterpartyIdentityId.contentEquals(entity.counterpartyIdentityId) && | ||
| existing.amountDuffs == entity.amountDuffs && | ||
| existing.directionRaw == entity.directionRaw && | ||
| existing.statusRaw == entity.statusRaw && | ||
| existing.memo == entity.memo | ||
| if (!unchanged) entities.add(entity) | ||
| } | ||
| if (entities.isNotEmpty()) database.dashpayDao().upsertPayments(entities) |
There was a problem hiding this comment.
🟡 Suggestion: The skip-unchanged / createdAt-preservation branch of refreshDashPayPayments is untested
The reason this method is more than a straight @Upsert is the invariant described in the inline comment: a blind upsert would clobber createdAt and re-fire the payments Flow on every refresh. Neither WalletManagerRoundTripTest nor PlatformWalletPersistenceHandlerTest exercises refreshDashPayPayments — the round-trip test writes rows via the DAO directly. Add a Robolectric-level test that (1) seeds an existing DashpayPaymentEntity, (2) invokes refreshDashPayPayments with matching JSON, and asserts createdAt is unchanged and no upsert occurs. This guards the invariant this branch exists to defend.
source: ['claude']
| stage(walletId) { db -> | ||
| // Owner identity must exist (networkRaw is read off it). In the | ||
| // real flow this lookup always succeeds: the identity upsert is | ||
| // staged into the same buffer BEFORE its contact-profile deltas | ||
| // (persist_identity_upsert loops the deltas after the identity | ||
| // call), so at replay the owner row is visible in the same | ||
| // transaction. The skip is defensive, matching the sibling | ||
| // contact paths. | ||
| val owner = db.identityDao().getByIdentityId(ownerId) ?: return@stage |
There was a problem hiding this comment.
💬 Nitpick: onPersistContactProfileDelta silently drops the delta when the owner identity is missing
The code comment explicitly documents that the identity upsert is always staged before its contact-profile deltas, so this return@stage should be unreachable — meaning it's exactly the case that must not fail silently if the ordering assumption is ever violated. A single Log.w (or whatever logging facility the project uses) makes an ordering regression observable instead of manifesting downstream as a phantom missing profile. Consistent with the surrounding defensive style, just noisier.
source: ['claude']
2 participants
Issue being fixed or feature implemented
The Kotlin SDK (#3999) is a port of SwiftExampleApp snapshotted before PR #3841 completed DashPay on iOS. This is milestone K1 of the DashPay Swift→Kotlin migration (spec:
docs/dashpay/KOTLIN_MIGRATION_SPEC.md, added here after a five-lens design review): the persistence completion + read-bridge layer that K2 (sync service, seedless unlock, writes) and K3 (the DashPay tab UI) build on. Stacked onfeat/kotlin-sdk-and-example-app.What was done?
Persistence — the two missing DashPay stores (mirroring the SwiftData models from #3841):
DashpayContactProfileEntity(cached contact profiles). Persister-projected with tombstone semantics: anis_present == falsedelta DELETEs the row, so a contact who removed their on-chain profile can't leave a stale name/avatar behind.DashpayPaymentEntity(payment history). Pull-persisted via the newPlatformWalletManager.refreshDashPayPayments— the recurring sweep reconciles payments in-memory without persisting, so this refresh is the only durable path (matching iOS); it change-detects and preservescreatedAt.3.json(validated by a new instrumented migration test).onPersistContactProfileDeltabridge slot), and the wallet-list restore path marshals the previously null-stubbedpayments/contact_profilesarrays (staging/seal/free extended inrs-unified-sdk-jni/src/persistence.rs). Also corrects stale doc comments inidentity_persistence.rsthat contradicted the tombstone-emitting projection code.Read bridges — new
rs-unified-sdk-jni/src/dashpay.rs+DashpayNative.kt(JSON-string convention per the existinggetDashPayProfile):managed_identity_get_dashpay_payments,platform_wallet_get_contact_profile,managed_identity_get_dashpay_profile,managed_identity_get_dashpay_sync_state,platform_wallet_search_dpns_names(wallet-scoped — theAddContactViewcall path, distinct from the SDK-scopeddpnsSearch),platform_wallet_manager_get_account_balances. Kotlin surface:Dashpay.{payments,syncState,getContactProfile,searchDpnsNames},PlatformWalletManager.{accountBalances,refreshDashPayPayments}.Bookkeeping: PARITY.md interim correction — the
FriendsView.swiftrow claimed parity against a view #3841 deleted; the DashPay tab is now marked in-migration instead of counted ported.Code-reviewed by three independent lenses (JNI memory safety, persistence correctness vs the Swift reference, test rigor) — zero must-fixes; all should-fixes folded.
How Has This Been Tested?
_present-flag gating, tombstone delete, changeset-rollback for the new delta slot, and the persist→load round-trip of payments (memo/direction/status) + contact profiles (incl.checkedAtMsfidelity) + ignored senders.DashDatabaseMigrationTest(v2→v3 and full v1→v3 chain validated against exported schemas over seeded rows) and a DashPay restore round-trip inWalletManagerRoundTripTestthrough the real JNI staging/seal/free pipeline — inject Room fixtures, reload on a fresh manager, read back through the FFI getters and assert field equality; includes a tombstone-restore leg (absent-profile + empty-array marshaling) and anaccountBalancesarray marshal/free smoke. ⚠ These compile locally but execute only inkotlin-sdk-build.yml(no NDK/emulator on the dev machine) — this PR's CI run is their first execution.searchDpnsNamesis network-bound and deferred to the-Ptestnet=truetier (documented in the test docstring).cargo check/clippy/fmtclean onrs-unified-sdk-jni.Breaking Changes
None. The Room migration is additive; new JNI exports only. (
DashDatabasebumps to schema v3 — downgrade to an older SDK build is not supported, as before.)Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code