feat(kotlin-sdk): add L1 invitation create/claim JNI bridge + DIP-13 invitation persistence - #4240
feat(kotlin-sdk): add L1 invitation create/claim JNI bridge + DIP-13 invitation persistence#4240bfoss765 wants to merge 3 commits into
Conversation
…tation persistence Adds the Android JNI + kotlin-sdk bridge for the existing (iOS-shipped) DIP-13 invitation FFI (create_invitation / claim_invitation), plus durable invitation persistence. - rs-unified-sdk-jni: JNI createInvitation/claimInvitation marshalers over platform_wallet_create_invitation / platform_wallet_claim_invitation; wire the on_persist_invitations_fn trampoline (tramp_persist_invitations). - kotlin-sdk: IdentityNative externs, IdentityRegistration create/claim wrappers, NativePersistenceBridge invitation dispatch, and PlatformWalletPersistenceHandler overrides that declare the INVITATIONS capability (0x02) gating funded invite creation. - Room: new invitations table (InvitationEntity/InvitationDao), DashDatabase v7 -> v8 with MIGRATION_7_8 and exported 8.json. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🕓 Ready for review — next in queue (commit e061707) |
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds DIP-13 DashPay invitation creation and claiming APIs, JNI implementations, invitation persistence callbacks, Room entities and DAO operations, and a Room version 7-to-8 migration. ChangesDashPay invitations
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/InvitationDao.kt (1)
15-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
@Upsert;@Insert(REPLACE)is delete-then-insert, not an in-place overwrite.
INSERT OR REPLACEdeletes the conflicting row before inserting. That is harmless today (no table referencesinvitations, and every column is supplied on each call), but it makes the KDoc's "overwrites the row in place" inaccurate and would silently cascade if a child table is ever added.♻️ Proposed change
-import androidx.room.OnConflictStrategy -import androidx.room.Insert +import androidx.room.Upsert @@ - `@Insert`(onConflict = OnConflictStrategy.REPLACE) + `@Upsert` suspend fun upsert(invitation: InvitationEntity)🤖 Prompt for 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. In `@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/InvitationDao.kt` around lines 15 - 23, Update InvitationDao.upsert to use Room’s `@Upsert` annotation instead of `@Insert`(onConflict = OnConflictStrategy.REPLACE), and remove the now-unused conflict-strategy import. Keep the existing suspend method and InvitationEntity parameter unchanged.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.kt (2)
314-344: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSilently dropped
inviterUsernamewheninviterIdentityIdis omitted.Only the "id present, username missing" direction is validated. If a caller passes
inviterUsernamewithoutinviterIdentityId, neither this method,IdentityNative.createInvitation, nor the Rustplatform_wallet_create_invitationcore (which gatesInviterInfopurely oninviter_identity_id.is_null()) ever reads the username — it's silently discarded and the caller gets a plain funding voucher with no error. Also, the KDoc here omits the "(only used when inviterIdentityId is non-null)" clarification thatIdentityNative.ktalready documents for the same parameter.♻️ Proposed fix
inviterIdentityId?.let { require(it.size == 32) { "inviterIdentityId must be 32 bytes, got ${it.size}" } require(inviterUsername != null) { "inviterUsername is required when inviterIdentityId is provided" } } + require(inviterIdentityId != null || inviterUsername == null) { + "inviterIdentityId is required when inviterUsername is provided" + }🤖 Prompt for 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. In `@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.kt` around lines 314 - 344, Update createInvitation to reject any non-null inviterUsername when inviterIdentityId is null, while preserving the existing requirement that an identity ID requires a username. Add the corresponding KDoc clarification that inviterUsername is only used when inviterIdentityId is non-null, and ensure the validation occurs before the invitation is created.
325-404: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo unit tests included for the new
createInvitation/claimInvitationvalidation logic.Boundary checks (amount/index/nowUnix ranges, inviter id/username coupling, uri/keys emptiness) are cheap to unit test and easy to regress silently later.
As per coding guidelines: "Keep pull requests focused, link related issues, include tests, and complete the pull request template."
🤖 Prompt for 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. In `@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.kt` around lines 325 - 404, Add unit tests covering validation in createInvitation and claimInvitation, including invalid amountDuffs, fundingAccountIndex, nowUnix, inviterIdentityId size/username coupling, blank uri, empty keys, and negative identityIndex. Verify each invalid input fails before invoking the corresponding IdentityNative method, while preserving successful validation behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.kt`:
- Around line 314-344: Update createInvitation to reject any non-null
inviterUsername when inviterIdentityId is null, while preserving the existing
requirement that an identity ID requires a username. Add the corresponding KDoc
clarification that inviterUsername is only used when inviterIdentityId is
non-null, and ensure the validation occurs before the invitation is created.
- Around line 325-404: Add unit tests covering validation in createInvitation
and claimInvitation, including invalid amountDuffs, fundingAccountIndex,
nowUnix, inviterIdentityId size/username coupling, blank uri, empty keys, and
negative identityIndex. Verify each invalid input fails before invoking the
corresponding IdentityNative method, while preserving successful validation
behavior.
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/InvitationDao.kt`:
- Around line 15-23: Update InvitationDao.upsert to use Room’s `@Upsert`
annotation instead of `@Insert`(onConflict = OnConflictStrategy.REPLACE), and
remove the now-unused conflict-strategy import. Keep the existing suspend method
and InvitationEntity parameter unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f8611829-42f6-4767-b020-a618f6b23ba3
📒 Files selected for processing (10)
packages/kotlin-sdk/sdk/schemas/org.dashfoundation.dashsdk.persistence.DashDatabase/8.jsonpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/IdentityNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativePersistenceBridge.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/DashDatabase.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/InvitationDao.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/entities/InvitationEntity.ktpackages/rs-unified-sdk-jni/src/identity.rspackages/rs-unified-sdk-jni/src/persistence.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The JNI marshalling and Room invitation schema are generally consistent, but three in-scope blockers remain. Invitation funding-index persistence can silently succeed without writing the security-critical address pool, clear-all omits the new invitation table, and coroutine cancellation can discard the only bearer URI after the native operation has already funded the voucher.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking
2 additional finding(s) omitted (not in diff).
🤖 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:507-511: Fail when the invitation funding account is missing
This handler now advertises the complete invitation-creation persistence capability, but the address-pool callback still reports success when its account lookup fails. For account type 5 (`IdentityInvitation`), this pool is the only state restored after restart that marks the exported voucher funding index as used; the invitation metadata row stores the index but is not used to rebuild the address pool. Consequently, `return@stage` lets the pre-broadcast `store()` and `flush()` gate succeed while dropping the funding-index update, allowing the same bearer voucher key to be selected again after restart. The Swift backend deliberately reports failure for this exact type-5 lookup miss. Throwing during staged transaction replay makes `onChangesetEnd` return nonzero and aborts creation before broadcast.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/services/DataManager.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/services/DataManager.kt:48-53: Clear-all leaves persisted invitations behind
The new `invitations` table has no foreign key to `wallets`, so deleting wallet and account rows does not remove its contents. No other `DataManager` category clears this table, which means both the wallet-category clear and the UI's "Clear All Data" operation leave sent-invitation metadata on disk despite promising to delete all local records. Those rows can become visible again if the same wallet ID is imported later.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.kt:333: Coroutine cancellation can lose the bearer URI after funding the voucher
`gate.op` runs the blocking JNI call through `withContext(Dispatchers.IO)`. As `TeardownGate` itself documents, cancellation while native code is running can be observed only when `withContext` dispatches the completed result back, causing that result to be discarded. The native invitation operation may already have broadcast the asset lock, persisted its record, waited for proof, and generated the bearer URI by then; JNI cannot observe the Kotlin cancellation. Because Room intentionally stores no URI or voucher key, and the Android bridge exposes neither URI regeneration nor invitation reclaim, the caller can lose the only shareable credential after funds have moved. Invitation creation needs a non-cancellable completion/handoff contract or a recovery API that can regenerate or retrieve the URI from durable state.
| inviterUsername: String? = null, | ||
| nowUnix: Long, | ||
| coreSignerHandle: Long, | ||
| ): CreatedInvitation = gate.op { |
There was a problem hiding this comment.
🔴 Blocking: Coroutine cancellation can lose the bearer URI after funding the voucher
gate.op runs the blocking JNI call through withContext(Dispatchers.IO). As TeardownGate itself documents, cancellation while native code is running can be observed only when withContext dispatches the completed result back, causing that result to be discarded. The native invitation operation may already have broadcast the asset lock, persisted its record, waited for proof, and generated the bearer URI by then; JNI cannot observe the Kotlin cancellation. Because Room intentionally stores no URI or voucher key, and the Android bridge exposes neither URI regeneration nor invitation reclaim, the caller can lose the only shareable credential after funds have moved. Invitation creation needs a non-cancellable completion/handoff contract or a recovery API that can regenerate or retrieve the URI from durable state.
source: ['codex']
…e claim (dashpay#4240) A voucher islock is signed at creation, so by claim time (minutes-to-hours later, possibly across a testnet platform-4.1 quorum rotation) Drive may reject it with InvalidInstantAssetLockProofSignatureError ("try chain asset lock proof instead"). claim_invitation now recovers exactly as the register/top-up paths do: - reconstruct_asset_lock_proof / assemble_asset_lock_proof return a ReconstructedProof { primary, chain_fallback }. When the link carried an islock AND the funding tx is already chain-locked, a ChainLock proof over the SAME credit output is built as chain_fallback. - The primary proof is submitted through submit_with_cl_height_retry. If it is an IS proof rejected with is_instant_lock_proof_invalid(), the ChainLock fallback is resubmitted over the same outpoint. If no fallback exists (tx not yet chain-locked), AssetLockNotChainLocked surfaces a clear retry signal. Adds unit tests for the three assemble outcomes (chainlock-only, islock+chainlocked→fallback, islock+not-chainlocked→no-fallback). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The L1-invite change added the INVITATIONS capability (0x02) via onPersistInvitationUpsert/Removal, so persistenceCapabilitiesBits() is now 0xbf, not 0xbd. Update the stale assertions (the test still expected the pre-invitation bitmask and asserted INVITATIONS absent). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765
changed the title
2 participants
Summary
Standalone Android counterpart to the DIP-13 invitation flow. This adds the
Kotlin-SDK / JNI bridge for the existing, iOS-shipped
create_invitation/claim_invitationplatform-wallet FFI — no newRust wallet logic; the whole fund / broadcast / persist / proof / export
pipeline already lives in
rs-platform-walletandrs-platform-wallet-ffion
v4.2-devand ships on iOS. This PR just wires Android to it and makesthe required persistence durable.
It is the standalone Android change, not stacked on any other branch.
What's included
JNI bridge (
rs-unified-sdk-jni)Java_..._createInvitation— thin marshaler overplatform_wallet_create_invitation: funds a one-time asset-lock voucherand returns
outpoint[36] || utf8 dashpay://invite URI. The URI embedsthe bearer voucher key and is never logged.
Java_..._claimInvitation— thin marshaler overplatform_wallet_claim_invitation: registers a new identity funded by theimported voucher. Decodes the invitee's key rows via
decode_registration_pubkeys_blob— the same codec pathregisterIdentityWithFundinguses on this base.on_persist_invitations_fn(tramp_persist_invitations), replacingthe previous
None.DIP-13 invitation persistence + capability gate (
kotlin-sdk)IdentityNativeexterns;IdentityRegistration.createInvitation/claimInvitationsuspend wrappers (withCreatedInvitationwhosetoStringredacts the bearer URI).NativePersistenceBridge.onPersistInvitationUpsert/...Removaldispatch;
PlatformWalletPersistenceHandleroverrides that persist to Room.CAPABILITY_INVITATIONS = 0x02added topersistenceCapabilitiesBits().This is load-bearing: the Rust
create_invitationdurability gate refusesto move any funds unless the backend reports the
INVITATIONScapability,which is only true when the upsert callback is genuinely durable. A no-op
store would defeat the gate and risk re-exporting a one-time voucher key
after a restart, so the flow fails closed on a backend that can't record it.
Room storage
invitationstable (InvitationEntity/InvitationDao), one row per36-byte funding outpoint, cascaded on wallet deletion via
deleteWalletData.DashDatabasev7 → v8 withMIGRATION_7_8and exported8.json.Migration-number reconciliation
The change was originally authored against an integration branch whose
DashDatabasewas already at version 8 (an unrelated feature held itsMIGRATION_7_8slot), so it landed there as v8 → v9 /9.json. Onv4.2-devthe current schema is version 7 (MIGRATION_6_7is the lastmigration). The invitation migration has therefore been renumbered to
MIGRATION_7_8(version 8) and the exported schema regenerated as8.json(a fresh Room export from this base — v7 tables +invitations),so there is no collision on
v4.2-dev.Verification
cargo check -p rs-unified-sdk-jni— clean../gradlew :sdk:compileDebugKotlin(JDK17) —BUILD SUCCESSFUL(Room KSP re-exported
8.json).L1 (Standard, asset-lock + islock) and shielded invites created
successfully end-to-end.
Summary by CodeRabbit
New Features
Bug Fixes