Repository navigation
feat(kotlin-sdk): topUpExternalIdentityFromAddresses - #5906
HashEngineering wants to merge 2 commits into
Conversation
Bridge platform_wallet_top_up_external_identity_from_addresses_with_signer to Kotlin: a JNI export in rs-unified-sdk-jni, CreditsNative, and IdentityCredits.topUpExternalIdentityFromAddresses. It tops up an identity the wallet does not manage from the wallet's Platform-address inputs; Rust fetches the identity from Platform and never adds it to the wallet. The JNI layer does not refuse empty inputs: the library owns that rule. An unknown identity maps to DashSdkError.NotFound. Parity: identity.external_top_up_from_addresses is now supported for the Kotlin SDK, verified by ExternalIdentityTopUpTest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The JVM tests inject the native call, so a wrong symbol name or descriptor would only fail on device. FfiSmokeTest now calls the real CreditsNative.topUpExternalIdentityFromAddresses with a 31-byte id and expects the JNI argument check's code 1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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. |
|
✅ Final review complete — no blockers (commit 9111d54) · triage: normal · Phase 2 only (queue backlog) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The Kotlin/JNI external identity top-up path is correctly layered and preserves Rust ownership of validation, signing, lifecycle handling, and balance reconciliation. One input-marshalling bug remains: invalid Kotlin address types can be silently truncated into valid types before reaching Rust validation.
🟡 1 suggestion(s)
Approved: nothing here blocks the merge. The non-blocking findings are optional: fix them forward, in this PR or a follow-up, without waiting for another review round.
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Reject invalid address types before encoding funding inputs
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/credits/IdentityCredits.kt:69
FundingInput.addressType is an unconstrained Int, but DataOutputStream.writeByte retains only the low eight bits. An input with addressType = 256 is therefore encoded as 0 and decoded by JNI as P2PKH; addressType = 257 becomes 1 and is decoded as P2SH. The Rust funding_rows validator cannot recover the original value, so the external top-up can sign and spend against a different address type than the Kotlin caller supplied. Validate that addressType is exactly 0 or 1 before encoding, preferably in FundingInput.init so every use of this wire model is protected.
init {
require(addressType == 0 || addressType == 1) {
"FundingInput.addressType must be 0 (P2PKH) or 1 (P2SH), got $addressType"
}
require(hash.size == 20) { "FundingInput.hash must be 20 bytes, got ${hash.size}" }
require(credits > 0) { "FundingInput.credits must be positive, got $credits" }
}
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, security-auditor)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a contained Kotlin-to-JNI feature addition that crosses an FFI boundary and initiates an external identity top-up, but it does not change the Rust funds-handling logic itself or meet the critical-tier bar for a large intricate change to that critical surface. - Phase 1 reviewers: not run (skipped for throughput: 19 PRs queued, above the 10 limit)
- Second Phase-2 pass: on this first full review every Phase-2 reviewer also ran a second, independent time, and the verifier weighed both sets
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review (PastaClaw). How to work through them:
- Verify each finding against the code; fix only what is real, as narrowly as the finding.
- If a finding is wrong, decline it on its thread in one or two sentences; the bot answers once and stops.
- To defer one, link a tracking issue on its thread (or reply asking the bot to file one).
- Suggestions don't block the merge, but most are worth fixing in this PR; judge each on its merits.
- Keep the PR's scope; don't grow it to satisfy hardening suggestions.
- Carried items were raised in an earlier round: read the thread first, and don't re-fix what you already fixed, declined or deferred there. Disputed and deferred items are already handled and are not listed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/credits/IdentityCredits.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/credits/IdentityCredits.kt:69: Reject invalid address types before encoding funding inputs
`FundingInput.addressType` is an unconstrained `Int`, but `DataOutputStream.writeByte` retains only the low eight bits. An input with `addressType = 256` is therefore encoded as `0` and decoded by JNI as P2PKH; `addressType = 257` becomes `1` and is decoded as P2SH. The Rust `funding_rows` validator cannot recover the original value, so the external top-up can sign and spend against a different address type than the Kotlin caller supplied. Validate that `addressType` is exactly `0` or `1` before encoding, preferably in `FundingInput.init` so every use of this wire model is protected.
Issue being fixed or feature implemented
Kotlin binding for #5860's external identity top-up: top up an identity the wallet does not manage (someone else's) from the wallet's Platform-address credits. #5860 added the Rust composite, the FFI export and the Swift wrapper, and recorded Kotlin as unsupported in the parity manifest.
Android needs it for dash-wallet's Advanced-mode External Send (dashpay/dash-wallet#1680): its Platform → identity send currently calls the managed
topUpFromAddresses, which refuses an identity the wallet does not manage withIdentityNotFound.Stacked on #5860. The base is #5860's branch; this PR will be retargeted to
v5.1-devonce #5860 merges.What was done?
rs-unified-sdk-jni/src/credits.rs):CreditsNative.topUpExternalIdentityFromAddresses, a thin marshaler overplatform_wallet_top_up_external_identity_from_addresses_with_signer. It reads the same inputs blob astopUpFromAddresses. Unlike that export, it does not refuse empty inputs itself: the library owns that rule, and an emptyVec's pointer is non-null, so it passes the FFI's pointer check and reaches the library.CreditsNativedeclaration andIdentityCredits.topUpExternalIdentityFromAddresses(walletHandle, identityId, inputs, signerHandle), gated and routed throughmapNativeErrorsliketopUpFromAddresses. An identity that does not exist on Platform surfaces asDashSdkError.NotFound(platform-wallet code 7) before anything is signed. The native call is injectable (AddressTopUpNativeCall) for JVM tests.identity.external_top_up_from_addressesis nowsupportedfor the Kotlin SDK (example app stillunsupported: it has no screen that pays someone else's identity, as on Swift).PARITY_SUMMARY.mdregenerated.No Rust library or FFI changes, no new error codes.
How Has This Been Tested?
ExternalIdentityTopUpTest(JVM, 3 tests): the handles, id and encoded inputs reach the export and its balance is returned; platform-wallet code 7 maps toNotFound; empty inputs are passed to Rust, not refused in Kotlin.FfiSmokeTest.externalIdentityTopUpSymbolBindsAndRejectsInvalidInput(instrumented): calls the real export with a 31-byte id and expects the JNI argument check's code 1, proving the symbol and descriptor bind.FfiSmokeTestpasses 8/8 on an x86_64 Pixel 9 Pro emulator (API 36) against a dev-profilebuild_android.sh --abi x86_64library../gradlew :sdk:testDebugUnitTest --tests 'org.dashfoundation.dashsdk.credits.*': 5/5 pass.cargo clippy -p rs-unified-sdk-jni --all-targets -- -D warningsandcargo fmt --check: clean.python3 scripts/check_sdk_parity_manifest.py --write-summary: OK.Breaking Changes
None. Adds a method; existing APIs are unchanged.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code