chore(kotlin-sdk): tx-decode follow-up — blob hardening, net_from_ord hoist, prevVout docs (#4187 review)#4192
Conversation
Follow-up to dashpay#4187 review: tx_decode.rs added the third private copy of the FFINetwork ordinal mapping (after transactions.rs and identity.rs). Hoist a single shared copy into support.rs and update all callers — including a fourth copy in persistence.rs with identical semantics. rs_sdk_ffi::FFINetwork and platform_wallet_ffi::FFINetwork are both re-exports of dash_network::ffi::FFINetwork, so the one helper serves every module. The exhaustive ordinal test moves to support.rs with it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follow-up to dashpay#4187 review: the output encoder always wrote script_pubkey_len but only appended the bytes when the pointer was non-null, so a defensively-null script pointer paired with a nonzero length field would shift every subsequent field in the blob. Write the length and bytes together — a null (or empty) script encodes as length 0 — and pin the hazard with a hand-built null-script fixture whose length field deliberately lies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…gned view Follow-up to dashpay#4187 review: Kotlin exposes prevVout as a signed Int, so the coinbase sentinel 0xFFFFFFFF reads as -1 while the Swift wrapper's UInt32 shows 4294967295. Keep the field type and the pinned -1 test unchanged (raw u32 bits preserved bit-for-bit) and say so in KDoc, and add a prevVoutUnsigned: Long convenience view (0..4294967295) for callers who want the numeric u32 / Swift-parity rendering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review round 2: the array-level writes had the same half-defensive shape the script fix closed — counts written unconditionally, records only if the pointer was non-null, so a lying (null array, nonzero count) would desync every field parseBlob reads after. Counts and records now write together, pinned by a lying-fixture test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🕓 Ready for review — 30 ahead in queue (commit d6d0313) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe JNI SDK centralizes network ordinal conversion and hardens packed transaction encoding for null pointers. The Kotlin SDK documents signed ChangesJNI transaction updates
Kotlin unsigned transaction index
Estimated code review effort: 2 (Simple) | ~15 minutes 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 |
|
Reviewed (two independent passes) — all three #4187 follow-up items are done completely and correctly: the null-pointer/length-lie hazards are guarded at both the script and array levels with tests that hand-build lying FFI structs, the |
The tx-decode follow-up (#4192) merged rustfmt-drifted code: the crate is absent from the rs package filter, so its own push run never executed the workspace formatting check, while every unrelated PR that does trigger the Rust workspace job now fails cargo fmt --check --all on its merge ref. Formatting-only, no semantic change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Small follow-up promised on #4187, addressing @shumkov's three post-approval review notes on the Kotlin tx-decode JNI binding:
rs-unified-sdk-jni/src/tx_decode.rs) — the output encoder wrotescript_pubkey_lenunconditionally but the script bytes only when the pointer was non-null, so a defensively-null pointer with a nonzero length field could shift every subsequent field in the blob. Length and bytes are now written together: a null (or empty) script encodes as length 0, never a nonzero length without its bytes. Pinned by a new unit test with a hand-built null-script fixture whose length field deliberately lies. The cross-pinned Kotlin/Rust fixture blob is unchanged.net_from_orddeduplication — tx_decode.rs had added the third private copy (after transactions.rs and identity.rs). There is now a single sharedsupport::net_from_ord;rs_sdk_ffi::FFINetworkandplatform_wallet_ffi::FFINetworkare both re-exports ofdash_network::ffi::FFINetwork, so the one helper serves every module. A fourth identical copy in persistence.rs was consolidated too. The exhaustive ordinal test moved to support.rs.prevVoutsignedness — documented rather than retyped:DecodedTransaction.Input.prevVoutstays a signedIntcarrying the raw u32 bits (the coinbase sentinel0xFFFFFFFFreads as-1; Swift'sUInt32shows4294967295), with KDoc spelling that out and a newprevVoutUnsigned: Longconvenience view for the numeric u32 / Swift-parity rendering. The existing pinned-1test is untouched; assertions for the unsigned view were added alongside it.Verified:
cargo test -p rs-unified-sdk-jni(37 pass),cargo clippy -p rs-unified-sdk-jni --all-targetsclean, and kotlin-sdk:sdk:compileDebugKotlin+:sdk:testDebugUnitTestgreen (TransactionDecoderTest4/4). Each commit builds and tests green standalone.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation