feat(platform-wallet): pure transaction decoder + thin FFI/Swift wrappers#3981
feat(platform-wallet): pure transaction decoder + thin FFI/Swift wrappers#3981llbartekll wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds a new transaction decoding feature spanning three layers: a pure Rust decoder in rs-platform-wallet producing structured DecodedTransaction/DecodedInput/DecodedOutput with address derivation, a C-ABI FFI wrapper in rs-platform-wallet-ffi with marshalling and memory-free functions, and a Swift SDK TransactionDecoder consuming the FFI, each with accompanying unit tests. ChangesTransaction Decode Feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant SwiftCaller
participant TransactionDecoder
participant platform_wallet_decode_transaction
participant decode_transaction
SwiftCaller->>TransactionDecoder: decode(txData, network)
TransactionDecoder->>platform_wallet_decode_transaction: tx bytes, network
platform_wallet_decode_transaction->>decode_transaction: tx_bytes, network
decode_transaction-->>platform_wallet_decode_transaction: DecodedTransaction (inputs/outputs, addresses)
platform_wallet_decode_transaction->>platform_wallet_decode_transaction: marshal into DecodedTransactionFFI
platform_wallet_decode_transaction-->>TransactionDecoder: FFI pointer
TransactionDecoder->>TransactionDecoder: convert to Data/String/arrays
TransactionDecoder->>platform_wallet_decode_transaction: free decoded pointer (defer)
TransactionDecoder-->>SwiftCaller: DecodedTransaction
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
llbartekll
force-pushed
the
feat/core-tx-decode-ffi
branch
from
574363a to
5620fb0
Compare
llbartekll
changed the title
…wrappers Host apps that consume the platform-wallet FFI need to match arbitrary persisted transactions against external addresses and exact amounts (e.g. the CrowdNode on-chain API: "did a tx pay amount X to address Y?"). The wallet layer persists every transaction's raw bytes but only materializes TXO rows for the wallet's own outputs, so matching an external payment's per-output (address, amount) was unevaluable from either the FFI or the Swift SDK. Layered so the logic lives in the pure crate and the FFI stays a marshalling shell (mirroring how mnemonic_words.rs forwards to key_wallet::Mnemonic): - rs-platform-wallet/src/transaction_decode.rs (pure, no FFI, no unsafe): decode_transaction(bytes, network) -> DecodedTransaction — consensus-decode (rejecting trailing bytes), per-output (Address::from_script, value, script), per-input (OutPoint + best-effort P2PKH sender address from the scriptSig). 6 pure-Rust unit tests. - rs-platform-wallet-ffi/src/tx_decode.rs: platform_wallet_decode_transaction / ..._free — thin C-ABI marshalling over the pure fn + #[repr(C)] structs. 3 boundary tests (C round-trip, null/empty params, null-free safety). - swift-sdk TransactionDecoder.swift: typed DecodedTransaction wrapper with PlatformWalletError mapping, defer-freed FFI memory, txidDisplayHex helper. 7 unit tests. Purely additive — no breaking changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
llbartekll
force-pushed
the
feat/core-tx-decode-ffi
branch
from
5620fb0 to
7cb62fe
Compare
llbartekll
requested review from
QuantumExplorer,
ZocoLini,
lklimek and
shumkov
as code owners
|
✅ Review complete (commit 7cb62fe) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/swift-sdk/SwiftTests/SwiftDashSDKTests/TransactionDecoderTests.swift (2)
58-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove tautological assertion.
Line 61's
XCTAssertEqual(decoded.txid, Data(decoded.txid.reversed().reversed()))is always true by construction (double-reversal restores the original) — it doesn't verify anything aboutdecoded.txid's correctness. The line above it (comparingtxidDisplayHex) already does the real work.🧹 Proposed fix
func testTxidDisplayHexMatchesExplorerOrder() throws { let decoded = try TransactionDecoder.decode(fixtureData, network: .testnet) XCTAssertEqual(decoded.txidDisplayHex, Self.fixtureTxidDisplay) - XCTAssertEqual(decoded.txid, Data(decoded.txid.reversed().reversed())) }🤖 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/swift-sdk/SwiftTests/SwiftDashSDKTests/TransactionDecoderTests.swift` around lines 58 - 62, The test in TransactionDecoderTests.testTxidDisplayHexMatchesExplorerOrder contains a tautological assertion that always passes, so remove the redundant XCTAsertEqual on decoded.txid and keep the meaningful check against Self.fixtureTxidDisplay; if you want additional coverage, replace it with an assertion that compares decoded.txid to an independently derived expected value from the fixture rather than reversing the same data twice.
70-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the specific thrown error, not just "any error".
testMalformedBytesThrowDeserializationandtestEmptyInputThrowsInvalidParameteronly check that some error is thrown, but their names (andTransactionDecoder.decode's documented contract of.deserializationfor malformed bytes /.invalidParameterfor empty input) imply verifying the specific case. As written, a regression that throws the wrong error type would pass silently.🧪 Proposed fix
func testMalformedBytesThrowDeserialization() { var bytes = fixtureData bytes.append(contentsOf: [0xDE, 0xAD]) // trailing garbage - XCTAssertThrowsError(try TransactionDecoder.decode(bytes, network: .testnet)) - XCTAssertThrowsError(try TransactionDecoder.decode(Data([0xFF, 0xFF, 0xFF]), network: .testnet)) + XCTAssertThrowsError(try TransactionDecoder.decode(bytes, network: .testnet)) { error in + guard case PlatformWalletError.deserialization = error else { + return XCTFail("Expected .deserialization, got \(error)") + } + } + XCTAssertThrowsError(try TransactionDecoder.decode(Data([0xFF, 0xFF, 0xFF]), network: .testnet)) { error in + guard case PlatformWalletError.deserialization = error else { + return XCTFail("Expected .deserialization, got \(error)") + } + } } func testEmptyInputThrowsInvalidParameter() { - XCTAssertThrowsError(try TransactionDecoder.decode(Data(), network: .testnet)) + XCTAssertThrowsError(try TransactionDecoder.decode(Data(), network: .testnet)) { error in + guard case PlatformWalletError.invalidParameter = error else { + return XCTFail("Expected .invalidParameter, got \(error)") + } + } }🤖 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/swift-sdk/SwiftTests/SwiftDashSDKTests/TransactionDecoderTests.swift` around lines 70 - 79, The tests in TransactionDecoderTests are only asserting that TransactionDecoder.decode throws, so they can miss regressions where the wrong error is returned. Update testMalformedBytesThrowDeserialization and testEmptyInputThrowsInvalidParameter to inspect the thrown error and verify the specific TransactionDecoder error case matches the contract: malformed/trailing bytes should map to deserialization and empty input should map to invalidParameter. Use the existing TransactionDecoder.decode call sites in these tests to locate the assertions.
🤖 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/swift-sdk/SwiftTests/SwiftDashSDKTests/TransactionDecoderTests.swift`:
- Around line 58-62: The test in
TransactionDecoderTests.testTxidDisplayHexMatchesExplorerOrder contains a
tautological assertion that always passes, so remove the redundant XCTAsertEqual
on decoded.txid and keep the meaningful check against Self.fixtureTxidDisplay;
if you want additional coverage, replace it with an assertion that compares
decoded.txid to an independently derived expected value from the fixture rather
than reversing the same data twice.
- Around line 70-79: The tests in TransactionDecoderTests are only asserting
that TransactionDecoder.decode throws, so they can miss regressions where the
wrong error is returned. Update testMalformedBytesThrowDeserialization and
testEmptyInputThrowsInvalidParameter to inspect the thrown error and verify the
specific TransactionDecoder error case matches the contract: malformed/trailing
bytes should map to deserialization and empty input should map to
invalidParameter. Use the existing TransactionDecoder.decode call sites in these
tests to locate the assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16940c17-0bcc-4af7-921e-ed66b8b231c9
📒 Files selected for processing (6)
packages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet-ffi/src/tx_decode.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/transaction_decode.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/TransactionDecoder.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/TransactionDecoderTests.swift
There was a problem hiding this comment.
Code Review
Small, additive PR adding a pure transaction decoder in rs-platform-wallet plus a thin FFI/Swift wrapper. FFI memory management is correctly paired (Box::into_raw / Vec::from_raw_parts / CString round-trips), no consensus surfaces are touched, and tests cover the happy path plus trailing-garbage/coinbase/malformed cases. No blocking issues. Main suggestions cluster around the P2PKH sender-address heuristic (which infers a sender from the last scriptSig push without verifying P2PKH shape, so P2SH spends could occasionally be misattributed) and a couple of small performance/defensive tightenings.
🟡 5 suggestion(s) | 💬 2 nitpick(s)
Source: reviewers: general(opus, claude-sonnet-5, gpt-5.5[high] failed/unparseable), security-auditor(opus, claude-sonnet-5, gpt-5.5[high] failed/unparseable), rust-quality(opus, claude-sonnet-5, gpt-5.5[high] failed/unparseable), ffi-engineer(opus, claude-sonnet-5, gpt-5.5[high] failed/unparseable); verifier: opus.
🤖 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-platform-wallet/src/transaction_decode.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/transaction_decode.rs:96-111: P2PKH sender heuristic doesn't verify scriptSig shape, only the last push's length
`input_address_from_script_sig` iterates every instruction in the scriptSig and takes whichever data push occurred last, then treats it as a pubkey if it is 33 or 65 bytes. It never checks that the scriptSig actually has the P2PKH shape (exactly two pushes: signature then pubkey). For a P2SH spend the last push is the redeem script; if that redeem script happens to be 33 or 65 bytes and parses as a valid secp256k1 point (spender-controllable, only constrained by hashing to the P2SH address), the function silently returns a P2PKH address unrelated to the actual signer. The module doc claims `None` for non-P2PKH, but the implementation doesn't uphold that.
The simplest tightening is to require exactly two pushes (`sig`, `pubkey`) — anything else, including multisig P2SH redeem scripts, returns `None`. An additional guard that the first push looks like a DER-encoded ECDSA signature (starts with `0x30`, length ≤ 73) would eliminate essentially all remaining collision risk. This is not blocking for the stated CrowdNode use case (which matches on outputs, not input senders), but the failure mode — a wrong non-null address is worse than `None` — is worth closing since callers building sender-verification logic on top of `input.address` would be fooled.
- [SUGGESTION] packages/rs-platform-wallet/src/transaction_decode.rs:76-90: Unnecessary clone of every output's scriptPubkey
`decode_transaction` iterates `tx.output` by reference and clones every `script_pubkey` to populate `DecodedOutput::script_pubkey`. Since `tx.txid()` only needs `&self` and doesn't care about the order relative to consuming other fields, the txid can be computed first, after which `tx.output` (and `tx.input`) can be consumed via `into_iter()` — eliminating the per-output `ScriptBuf` clone entirely. For transactions with large scripts (OP_RETURN payloads, multisig redeem scripts) this is a real, avoidable allocation on every decode call, and this function is positioned as the primitive for scanning arbitrary persisted transactions.
- [SUGGESTION] packages/rs-platform-wallet/src/transaction_decode.rs:113-228: No test exercises a non-P2PKH scriptSig (the documented `None` path)
The only "no address" input test is `coinbase_input_has_no_address`. There is no test where the scriptSig's last push is not a valid P2PKH-shaped pubkey — for example a P2SH spend with a redeem-script push, a 3+-push scriptSig, or a 33/65-byte push that fails `PublicKey::from_slice`. These are exactly the ambiguous cases the length + parse checks are meant to filter out, and they'd pin the intended P2PKH-only contract so a future loosening of the heuristic (or the fix suggested above) has regression coverage.
In `packages/rs-platform-wallet-ffi/src/tx_decode.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/tx_decode.rs:87-100: `*out_decoded` is not zeroed on failure paths
On every error return (`check_ptr!(tx_bytes)`, the `tx_bytes_len == 0` branch, `unwrap_result_or_return!(decode_transaction(...))`), the function returns a non-Success `PlatformWalletFFIResult` without assigning to `*out_decoded`. The Swift wrapper is safe because it initializes `outDecoded = nil` and checks the result code first, but any other C/Swift caller that reuses an already-populated out pointer — or reads it before checking the result code — will observe stale data and may double-free by re-calling `platform_wallet_decoded_transaction_free` on the previous value. Convention for out-pointers is to null on entry: after `check_ptr!(out_decoded)`, do `*out_decoded = std::ptr::null_mut();`. Small change, removes a foot-gun for anyone consuming this from C.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/tx_decode.rs:30-34: Document that `DecodedTxInputFFI::address` is unauthenticated and must not be used for authorization
The per-input `address` is derived from the last scriptSig push with no signature verification tying it to the UTXO being spent. A crafter can put any 33/65-byte value in that position, so the field is trivially spoofable. The pure module doc says "best-effort / sent from", but the FFI struct comment only says the value is "recovered from a P2PKH scriptSig", which a downstream consumer (dashwallet-ios, future CrowdNode integration) could reasonably read as "the sender is P" and use for routing/authorization decisions. Strengthen the comment on `DecodedTxInputFFI::address` (and the mirrored Swift `Input.address`) to say explicitly: derived from an unauthenticated script push, safe for display or matching hints only, not for authentication/authorization. The safe matching primitive for CrowdNode-style protocols is per-output `(address, amount)`, which is already exposed and not attacker-influenceable in the same way.
| fn input_address_from_script_sig(script_sig: &ScriptBuf, network: Network) -> Option<Address> { | ||
| let mut last_push: Option<Vec<u8>> = None; | ||
| for instruction in script_sig.instructions() { | ||
| match instruction { | ||
| Ok(Instruction::PushBytes(bytes)) => last_push = Some(bytes.as_bytes().to_vec()), | ||
| Ok(_) => {} | ||
| Err(_) => return None, | ||
| } | ||
| } | ||
| let candidate = last_push?; | ||
| if candidate.len() != 33 && candidate.len() != 65 { | ||
| return None; | ||
| } | ||
| let pubkey = PublicKey::from_slice(&candidate).ok()?; | ||
| Some(Address::p2pkh(&pubkey, network)) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: P2PKH sender heuristic doesn't verify scriptSig shape, only the last push's length
input_address_from_script_sig iterates every instruction in the scriptSig and takes whichever data push occurred last, then treats it as a pubkey if it is 33 or 65 bytes. It never checks that the scriptSig actually has the P2PKH shape (exactly two pushes: signature then pubkey). For a P2SH spend the last push is the redeem script; if that redeem script happens to be 33 or 65 bytes and parses as a valid secp256k1 point (spender-controllable, only constrained by hashing to the P2SH address), the function silently returns a P2PKH address unrelated to the actual signer. The module doc claims None for non-P2PKH, but the implementation doesn't uphold that.
The simplest tightening is to require exactly two pushes (sig, pubkey) — anything else, including multisig P2SH redeem scripts, returns None. An additional guard that the first push looks like a DER-encoded ECDSA signature (starts with 0x30, length ≤ 73) would eliminate essentially all remaining collision risk. This is not blocking for the stated CrowdNode use case (which matches on outputs, not input senders), but the failure mode — a wrong non-null address is worse than None — is worth closing since callers building sender-verification logic on top of input.address would be fooled.
source: ['claude']
| let outputs = tx | ||
| .output | ||
| .iter() | ||
| .map(|txout| DecodedOutput { | ||
| address: Address::from_script(&txout.script_pubkey, network).ok(), | ||
| value_duffs: txout.value, | ||
| script_pubkey: txout.script_pubkey.clone(), | ||
| }) | ||
| .collect(); | ||
|
|
||
| Ok(DecodedTransaction { | ||
| txid: tx.txid(), | ||
| inputs, | ||
| outputs, | ||
| }) |
There was a problem hiding this comment.
🟡 Suggestion: Unnecessary clone of every output's scriptPubkey
decode_transaction iterates tx.output by reference and clones every script_pubkey to populate DecodedOutput::script_pubkey. Since tx.txid() only needs &self and doesn't care about the order relative to consuming other fields, the txid can be computed first, after which tx.output (and tx.input) can be consumed via into_iter() — eliminating the per-output ScriptBuf clone entirely. For transactions with large scripts (OP_RETURN payloads, multisig redeem scripts) this is a real, avoidable allocation on every decode call, and this function is positioned as the primitive for scanning arbitrary persisted transactions.
| let outputs = tx | |
| .output | |
| .iter() | |
| .map(|txout| DecodedOutput { | |
| address: Address::from_script(&txout.script_pubkey, network).ok(), | |
| value_duffs: txout.value, | |
| script_pubkey: txout.script_pubkey.clone(), | |
| }) | |
| .collect(); | |
| Ok(DecodedTransaction { | |
| txid: tx.txid(), | |
| inputs, | |
| outputs, | |
| }) | |
| let txid = tx.txid(); | |
| let inputs = tx | |
| .input | |
| .into_iter() | |
| .map(|txin| { | |
| let address = if txin.previous_output.is_null() { | |
| None // coinbase | |
| } else { | |
| input_address_from_script_sig(&txin.script_sig, network) | |
| }; | |
| DecodedInput { | |
| previous_output: txin.previous_output, | |
| address, | |
| } | |
| }) | |
| .collect(); | |
| let outputs = tx | |
| .output | |
| .into_iter() | |
| .map(|txout| DecodedOutput { | |
| address: Address::from_script(&txout.script_pubkey, network).ok(), | |
| value_duffs: txout.value, | |
| script_pubkey: txout.script_pubkey, | |
| }) | |
| .collect(); | |
| Ok(DecodedTransaction { | |
| txid, | |
| inputs, | |
| outputs, | |
| }) |
source: ['claude']
| pub unsafe extern "C" fn platform_wallet_decode_transaction( | ||
| tx_bytes: *const u8, | ||
| tx_bytes_len: usize, | ||
| network: FFINetwork, | ||
| out_decoded: *mut *mut DecodedTransactionFFI, | ||
| ) -> PlatformWalletFFIResult { | ||
| check_ptr!(tx_bytes); | ||
| check_ptr!(out_decoded); | ||
| if tx_bytes_len == 0 { | ||
| return PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorInvalidParameter, | ||
| "tx_bytes_len is zero", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: *out_decoded is not zeroed on failure paths
On every error return (check_ptr!(tx_bytes), the tx_bytes_len == 0 branch, unwrap_result_or_return!(decode_transaction(...))), the function returns a non-Success PlatformWalletFFIResult without assigning to *out_decoded. The Swift wrapper is safe because it initializes outDecoded = nil and checks the result code first, but any other C/Swift caller that reuses an already-populated out pointer — or reads it before checking the result code — will observe stale data and may double-free by re-calling platform_wallet_decoded_transaction_free on the previous value. Convention for out-pointers is to null on entry: after check_ptr!(out_decoded), do *out_decoded = std::ptr::null_mut();. Small change, removes a foot-gun for anyone consuming this from C.
source: ['claude']
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use dashcore::consensus::serialize; | ||
| use dashcore::hashes::Hash; | ||
| use dashcore::secp256k1::{Secp256k1, SecretKey}; | ||
| use dashcore::{TxIn, TxOut, Witness}; | ||
|
|
||
| fn test_pubkey() -> PublicKey { | ||
| let secp = Secp256k1::new(); | ||
| let sk = SecretKey::from_slice(&[0x42u8; 32]).expect("valid secret key"); | ||
| PublicKey::new(sk.public_key(&secp)) | ||
| } | ||
|
|
||
| /// One P2PKH input (spending 11..11:3), one P2PKH output of 151072 duffs, | ||
| /// one OP_RETURN output. The 151072 amount mirrors a CrowdNode signUp | ||
| /// signal so the fixture doubles as documentation. | ||
| fn synthetic_tx(network: Network) -> (Transaction, Address) { | ||
| let pubkey = test_pubkey(); | ||
| let addr = Address::p2pkh(&pubkey, network); | ||
| let script_sig = dashcore::blockdata::script::Builder::new() | ||
| .push_slice(&[0x30u8; 71]) | ||
| .push_key(&pubkey) | ||
| .into_script(); | ||
| let tx = Transaction { | ||
| version: 1, | ||
| lock_time: 0, | ||
| input: vec![TxIn { | ||
| previous_output: OutPoint { | ||
| txid: Txid::from_byte_array([0x11u8; 32]), | ||
| vout: 3, | ||
| }, | ||
| script_sig, | ||
| sequence: 0xffffffff, | ||
| witness: Witness::default(), | ||
| }], | ||
| output: vec![ | ||
| TxOut { | ||
| value: 151_072, | ||
| script_pubkey: addr.script_pubkey(), | ||
| }, | ||
| TxOut { | ||
| value: 0, | ||
| script_pubkey: ScriptBuf::new_op_return(&[0xAAu8; 4]), | ||
| }, | ||
| ], | ||
| special_transaction_payload: None, | ||
| }; | ||
| (tx, addr) | ||
| } | ||
|
|
||
| #[test] | ||
| fn decodes_outputs_with_addresses_and_values() { | ||
| let (tx, addr) = synthetic_tx(Network::Testnet); | ||
| let decoded = decode_transaction(&serialize(&tx), Network::Testnet).unwrap(); | ||
|
|
||
| assert_eq!(decoded.txid, tx.txid()); | ||
| assert_eq!(decoded.outputs.len(), 2); | ||
|
|
||
| assert_eq!(decoded.outputs[0].address.as_ref(), Some(&addr)); | ||
| assert_eq!(decoded.outputs[0].value_duffs, 151_072); | ||
| assert_eq!(decoded.outputs[0].script_pubkey, addr.script_pubkey()); | ||
| assert!( | ||
| addr.to_string().starts_with('y'), | ||
| "testnet P2PKH starts with 'y'" | ||
| ); | ||
|
|
||
| // OP_RETURN: no address, script still present. | ||
| assert!(decoded.outputs[1].address.is_none()); | ||
| assert!(!decoded.outputs[1].script_pubkey.as_bytes().is_empty()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn recovers_p2pkh_input_address_from_script_sig() { | ||
| let (tx, addr) = synthetic_tx(Network::Testnet); | ||
| let decoded = decode_transaction(&serialize(&tx), Network::Testnet).unwrap(); | ||
|
|
||
| assert_eq!(decoded.inputs.len(), 1); | ||
| assert_eq!( | ||
| decoded.inputs[0].previous_output.txid, | ||
| Txid::from_byte_array([0x11u8; 32]) | ||
| ); | ||
| assert_eq!(decoded.inputs[0].previous_output.vout, 3); | ||
| assert_eq!(decoded.inputs[0].address.as_ref(), Some(&addr)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn network_changes_rendered_addresses() { | ||
| let (tx, testnet_addr) = synthetic_tx(Network::Testnet); | ||
| let decoded = decode_transaction(&serialize(&tx), Network::Mainnet).unwrap(); | ||
| let rendered = decoded.outputs[0].address.as_ref().unwrap().to_string(); | ||
| assert_ne!(rendered, testnet_addr.to_string()); | ||
| assert!(rendered.starts_with('X'), "mainnet P2PKH starts with 'X'"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn coinbase_input_has_no_address() { | ||
| let (mut tx, _) = synthetic_tx(Network::Testnet); | ||
| tx.input[0].previous_output = OutPoint::null(); | ||
| tx.input[0].script_sig = ScriptBuf::new(); | ||
| let decoded = decode_transaction(&serialize(&tx), Network::Testnet).unwrap(); | ||
| assert!(decoded.inputs[0].address.is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn trailing_garbage_is_an_error() { | ||
| let (tx, _) = synthetic_tx(Network::Testnet); | ||
| let mut bytes = serialize(&tx); | ||
| bytes.extend_from_slice(&[0xDE, 0xAD]); | ||
| assert!(decode_transaction(&bytes, Network::Testnet).is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn garbage_bytes_are_an_error() { | ||
| assert!(decode_transaction(&[0xFFu8; 16], Network::Testnet).is_err()); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: No test exercises a non-P2PKH scriptSig (the documented None path)
The only "no address" input test is coinbase_input_has_no_address. There is no test where the scriptSig's last push is not a valid P2PKH-shaped pubkey — for example a P2SH spend with a redeem-script push, a 3+-push scriptSig, or a 33/65-byte push that fails PublicKey::from_slice. These are exactly the ambiguous cases the length + parse checks are meant to filter out, and they'd pin the intended P2PKH-only contract so a future loosening of the heuristic (or the fix suggested above) has regression coverage.
source: ['claude']
| /// Sender address recovered from a P2PKH scriptSig, or null when the | ||
| /// input is coinbase / non-P2PKH / unparseable. Owned by the parent | ||
| /// structure — freed by `platform_wallet_decoded_transaction_free`. | ||
| pub address: *mut c_char, | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Document that DecodedTxInputFFI::address is unauthenticated and must not be used for authorization
The per-input address is derived from the last scriptSig push with no signature verification tying it to the UTXO being spent. A crafter can put any 33/65-byte value in that position, so the field is trivially spoofable. The pure module doc says "best-effort / sent from", but the FFI struct comment only says the value is "recovered from a P2PKH scriptSig", which a downstream consumer (dashwallet-ios, future CrowdNode integration) could reasonably read as "the sender is P" and use for routing/authorization decisions. Strengthen the comment on DecodedTxInputFFI::address (and the mirrored Swift Input.address) to say explicitly: derived from an unauthenticated script push, safe for display or matching hints only, not for authentication/authorization. The safe matching primitive for CrowdNode-style protocols is per-output (address, amount), which is already exposed and not attacker-influenceable in the same way.
source: ['claude']
| fn input_address_from_script_sig(script_sig: &ScriptBuf, network: Network) -> Option<Address> { | ||
| let mut last_push: Option<Vec<u8>> = None; | ||
| for instruction in script_sig.instructions() { | ||
| match instruction { | ||
| Ok(Instruction::PushBytes(bytes)) => last_push = Some(bytes.as_bytes().to_vec()), | ||
| Ok(_) => {} | ||
| Err(_) => return None, | ||
| } | ||
| } | ||
| let candidate = last_push?; | ||
| if candidate.len() != 33 && candidate.len() != 65 { | ||
| return None; | ||
| } | ||
| let pubkey = PublicKey::from_slice(&candidate).ok()?; | ||
| Some(Address::p2pkh(&pubkey, network)) | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: last_push reallocates on every push instead of holding a borrow
The loop allocates a fresh Vec<u8> for every push instruction, discarding the previous allocation, only to keep the last one. Since PushBytes borrows from the underlying ScriptBuf, you can hold &[u8] for the loop duration and only inspect it at the end. Not a hot-path concern, but easy to fix and avoids O(n_pushes) allocations.
| fn input_address_from_script_sig(script_sig: &ScriptBuf, network: Network) -> Option<Address> { | |
| let mut last_push: Option<Vec<u8>> = None; | |
| for instruction in script_sig.instructions() { | |
| match instruction { | |
| Ok(Instruction::PushBytes(bytes)) => last_push = Some(bytes.as_bytes().to_vec()), | |
| Ok(_) => {} | |
| Err(_) => return None, | |
| } | |
| } | |
| let candidate = last_push?; | |
| if candidate.len() != 33 && candidate.len() != 65 { | |
| return None; | |
| } | |
| let pubkey = PublicKey::from_slice(&candidate).ok()?; | |
| Some(Address::p2pkh(&pubkey, network)) | |
| } | |
| fn input_address_from_script_sig(script_sig: &ScriptBuf, network: Network) -> Option<Address> { | |
| let mut last_push: Option<&[u8]> = None; | |
| for instruction in script_sig.instructions() { | |
| match instruction { | |
| Ok(Instruction::PushBytes(bytes)) => last_push = Some(bytes.as_bytes()), | |
| Ok(_) => {} | |
| Err(_) => return None, | |
| } | |
| } | |
| let candidate = last_push?; | |
| if candidate.len() != 33 && candidate.len() != 65 { | |
| return None; | |
| } | |
| let pubkey = PublicKey::from_slice(candidate).ok()?; | |
| Some(Address::p2pkh(&pubkey, network)) | |
| } |
source: ['claude']
| public struct DecodedTransaction: Sendable, Equatable { | ||
| public struct Input: Sendable, Equatable { | ||
| /// Previous output's txid in consensus (internal) byte order. | ||
| public let prevTxid: Data | ||
| /// Previous output's index. | ||
| public let prevVout: UInt32 | ||
| /// Sender address recovered from a P2PKH scriptSig; nil for | ||
| /// coinbase / non-P2PKH / unparseable script sigs. | ||
| public let address: String? | ||
| } | ||
|
|
||
| public struct Output: Sendable, Equatable { | ||
| /// Destination address for standard scripts (P2PKH / P2SH); nil for | ||
| /// non-standard scripts (OP_RETURN, bare multisig, …). | ||
| public let address: String? | ||
| /// Output value in duffs. | ||
| public let valueDuffs: UInt64 | ||
| /// Raw scriptPubKey bytes. | ||
| public let scriptPubkey: Data | ||
| } | ||
|
|
||
| /// Transaction id in consensus (internal) byte order — reverse for | ||
| /// explorer-style display (see `txidDisplayHex`). | ||
| public let txid: Data | ||
| public let inputs: [Input] | ||
| public let outputs: [Output] | ||
|
|
||
| /// Explorer-style (reversed, hex) rendering of `txid`. | ||
| public var txidDisplayHex: String { | ||
| txid.reversed().map { String(format: "%02x", $0) }.joined() | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Input.prevTxid lacks a display-hex helper analogous to txidDisplayHex
txid gets a txidDisplayHex computed property that reverses to explorer order, but Input.prevTxid (same consensus/internal byte-order convention per the FFI doc comment) has no parallel helper. Callers matching a stored transaction's inputs against an explorer-copied txid string will silently produce a byte-reversed mismatch. Given the CrowdNode use case involves matching known txids, exposing a symmetric prevTxidDisplayHex (or a shared helper) would prevent a common footgun. Pure ergonomics/consistency note.
source: ['claude']
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #3981 +/- ##
=============================================
- Coverage 87.18% 52.54% -34.64%
=============================================
Files 2632 11 -2621
Lines 327563 1707 -325856
=============================================
- Hits 285592 897 -284695
+ Misses 41971 810 -41161
🚀 New features to boost your workflow:
|
|
Opened a focused helper PR for the CodeRabbit test feedback: #3993 It removes the tautological txid assertion and pins the expected |
2 participants
Issue being fixed or feature implemented
Host apps consuming the platform-wallet FFI (dashwallet-ios mid-migration off DashSync) need to match arbitrary persisted transactions against external addresses and exact amounts — e.g. the CrowdNode on-chain API protocol: "did a transaction pay
apiOffset + codeduffs to the CrowdNode address?" and "did CrowdNode reply with amount X to my account address?".The wallet layer persists every transaction's raw bytes, but only materializes TXO rows for the wallet's own outputs, and
AccountTransactionEntryFFIcarries no per-output data. There is currently no way to evaluate a per-output(address, amount)predicate from either the FFI or the Swift SDK — payments to external addresses (CrowdNode's) have no addressable representation at all. This blocks porting CrowdNode's on-chain response tracking (and several tx-history readers) off DashSync.What was done?
Adds a pure, stateless transaction decoder with a thin FFI + Swift wrapper on top:
rs-platform-wallet(pure logic, no FFI, nounsafe) —src/transaction_decode.rs:decode_transaction(tx_bytes, network) -> Result<DecodedTransaction, …>— consensus-decodes raw tx bytes (rejecting trailing bytes) into:txid,(Option<Address> via Address::from_script, value_duffs, ScriptBuf)—Noneaddress for non-standard scripts (OP_RETURN etc.),(OutPoint, Option<Address>)— sender address recovered best-effort from P2PKH scriptSigs (last data push parsed as a public key),Nonefor coinbase/P2SH/unparseable.rs-platform-wallet-ffi—src/tx_decode.rs:platform_wallet_decode_transaction(...)/platform_wallet_decoded_transaction_free(...)— thin C-ABI marshalling only, delegating toplatform_wallet::decode_transaction(mirrors howmnemonic_words.rsforwards tokey_wallet::Mnemonic). Marshals the decoded structure into#[repr(C)]structs + paired free. 3 boundary tests (C round-trip, null/empty params, null-free safety) — the logic coverage lives in the pure crate.swift-sdk—Sources/SwiftDashSDK/PlatformWallet/TransactionDecoder.swift:TransactionDecoder.decode(_ txData: Data, network: Network) throws -> DecodedTransaction— typed structs,PlatformWalletErrormapping,defer-freed FFI memory,txidDisplayHexhelper. 7 unit tests sharing the same fixture.How Has This Been Tested?
cargo test -p platform-wallet transaction_decode— 6 pure-logic tests.cargo test -p platform-wallet-ffi tx_decode— 3 FFI boundary tests; full crate suite green (no regressions).swift build+swift test --filter TransactionDecoderTests— 7 wrapper tests against the regeneratedDashSDKFFIheader.Breaking Changes
None — purely additive (new pure module, new FFI symbols, new Swift wrapper).
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit