refactor: store TransactionContext in TransactionRecord#582
Merged
xdustinface merged 3 commits intov0.42-devfrom Apr 1, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR consolidates block metadata (height, hash, timestamp) into unified Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #582 +/- ##
=============================================
+ Coverage 67.06% 67.15% +0.09%
=============================================
Files 320 320
Lines 67328 67367 +39
=============================================
+ Hits 45156 45243 +87
+ Misses 22172 22124 -48
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
key-wallet/src/managed_account/mod.rs (1)
377-396:⚠️ Potential issue | 🟠 MajorReturn
truewhen reprocessing mutates UTXOs.This path now sets
changedonly fromtx_record.context != context. If a gap-limit rescan reprocesses the same context butupdate_utxos()adds/removes wallet outputs, the account state changes while this still returnsfalse, which can suppress downstream balance/persistence work. Haveupdate_utxos()report whether it mutated state and OR that intochanged. As per coding guidelines "Perform atomic state updates: when adding transactions, always update all related state atomically including address usage state, UTXO set, transaction history, and balance calculations".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/managed_account/mod.rs` around lines 377 - 396, The confirm_transaction path currently only sets changed when tx_record.context != context, missing mutations from update_utxos; modify update_utxos to return a bool indicating whether it mutated UTXO/account state and in confirm_transaction capture that return value and OR it into changed (i.e., let utxos_changed = self.update_utxos(tx, account_match, context); changed |= utxos_changed), keeping the existing record_transaction and tx_record.update_context logic intact so all related state updates remain atomic.key-wallet/src/managed_account/transaction_record.rs (1)
15-23:⚠️ Potential issue | 🔴 CriticalKeep
TransactionRecordbackward-deserializable.With
serdeenabled, this replaces the persisted{height, block_hash, timestamp}layout with a requiredcontextfield. Older wallet/account bytes won't containcontext, so deserialization will fail beforeManagedCoreAccountcan rebuildspent_outpoints. Please add a compatibility layer here (customDeserialize, versioned format, or an explicit migration) before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/managed_account/transaction_record.rs` around lines 15 - 23, TransactionRecord's new required field `context` breaks deserialization of older persisted records; implement a compatibility deserializer for TransactionRecord that accepts the old shape (height, block_hash, timestamp) as optional fields and constructs a TransactionContext when `context` is missing. Concretely, add a custom Deserialize impl (or serde with #[serde(deserialize_with = "...")] for TransactionRecord) that tries to read `context` first, and if absent reads `height`, `block_hash`, `timestamp` and synthesizes a TransactionContext (using the same enum/struct constructors as TransactionContext) before returning the TransactionRecord; ensure the code references the TransactionRecord type, the TransactionContext constructor(s), and the field names `transaction`, `txid`, and `context` so older bytes remain readable and ManagedCoreAccount rebuilding logic continues to work.key-wallet-ffi/src/transaction.rs (1)
388-397:⚠️ Potential issue | 🟠 MajorFFI signature change:
wallet_check_transactionparameter consolidation.Same pattern as
managed_wallet_check_transaction- the function now acceptsblock_info: FFIBlockInfoby value instead of separateblock_height,block_hash, andtimestampparameters.This maintains consistency across the FFI API surface. Ensure downstream consumers are updated for both functions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/transaction.rs` around lines 388 - 397, The FFI signature for wallet_check_transaction was changed to take a consolidated FFIBlockInfo struct instead of separate block_height, block_hash, and timestamp parameters; update the implementation of wallet_check_transaction to accept and use the new block_info: FFIBlockInfo parameter (match how managed_wallet_check_transaction handles block_info), replace references to the old individual parameters with block_info.<field> accesses, and ensure the FFITransactionCheckResult population and any validation use block_info consistently so the API matches managed_wallet_check_transaction.key-wallet-ffi/src/transaction_checking.rs (1)
109-119:⚠️ Potential issue | 🟠 MajorFFI signature change:
managed_wallet_check_transactionparameter consolidation.The function signature changed from separate parameters (
block_height,block_hashpointer,timestamp) to a singleblock_info: FFIBlockInfopassed by value. This is an ABI-breaking change.The implementation is correct:
FFIBlockInfois#[repr(C)]andCopy, making it safe to pass by value- The 40-byte struct (4 + 32 + 4 bytes) is reasonable for stack passing
Ensure C/Swift/Kotlin callers are updated to construct and pass
FFIBlockInfoinstead of separate arguments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/transaction_checking.rs` around lines 109 - 119, The FFI changed: managed_wallet_check_transaction now takes a single FFIBlockInfo (by value) instead of separate block_height, block_hash pointer, and timestamp, which is an ABI-breaking change; update all native callers (C/Swift/Kotlin) to construct and pass an FFIBlockInfo value to managed_wallet_check_transaction, ensuring the struct layout matches the Rust definition (FFIBlockInfo is #[repr(C)] and Copy with 4+32+4 bytes) and adjust any call sites that previously passed the three separate parameters to instead populate and pass the FFIBlockInfo instance.key-wallet-ffi/src/managed_account.rs (1)
662-675:⚠️ Potential issue | 🟠 MajorABI-breaking change:
FFITransactionRecordstruct layout has changed.The struct previously had flat fields (inferred as
height,block_hash,timestampbased on the commit message "Replace separate block-related fields") that are now nested insidecontext: FFITransactionContextDetails. This changes the C struct memory layout and requires updates to FFI consumers.
- Before: Consumers accessed flat transaction block info directly
- After: Consumers must access
record->context.block_info.height, etc.Any C/Swift/Kotlin bindings reading the old field offsets will receive garbage data. Ensure:
- FFI header files are regenerated using cbindgen
- All downstream consumers (Swift, Kotlin, C) are updated to use the new nested field paths
- This breaking change is documented in release notes with migration guidance
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/managed_account.rs` around lines 662 - 675, FFITransactionRecord's memory layout changed by moving flat block fields into the nested context: FFITransactionContextDetails, so update consumers to read block info via record->context.block_info.height / .block_hash / .timestamp (or equivalent language bindings), regenerate FFI headers with cbindgen to reflect the new layout, update Swift/Kotlin/C binding access paths to the nested fields, and add a release-note entry documenting the ABI-breaking change and migration steps for downstream users.key-wallet-ffi/FFI_API.md (1)
855-862:⚠️ Potential issue | 🟠 MajorRefresh the generated prose for the new
FFIBlockInfoABI.The signatures now take
block_info, but the surrounding text is still pre-refactor. Line 859 omitscontext_type,block_info, andupdate_state, while Line 1307 still documents removed out-params instead ofresult_out. The document also never explains theFFIBlockInfofields, so callers cannot tell how to populate the new parameter. Since this file is auto-generated, the fix needs to happen in the Rust doc comments or generator inputs.Also applies to: 1303-1310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/FFI_API.md` around lines 855 - 862, Update the generated documentation in the Rust doc comments / generator inputs for the managed_wallet_check_transaction FFI: include the new parameters (context_type: FFITransactionContext, block_info: FFIBlockInfo, update_state: bool) in the prose, replace any removed out-param descriptions with the single result_out pointer, and explicitly document that result_out must be populated and freed via transaction_check_result_free; additionally add a clear description of the FFIBlockInfo struct fields (what each field means and how callers should populate them) so callers can build the new parameter. Also make the same updates to the other transaction-check doc block that still references removed out-params so both generated sections reflect the new ABI.
🧹 Nitpick comments (1)
key-wallet-ffi/src/wallet_manager_tests.rs (1)
620-689: These tests still short-circuit before the new context mapping runs.All three calls use malformed
tx_bytes, sowallet_manager_process_transaction()can fail during transaction parsing without ever touchingFFITransactionContextDetails/FFIBlockInfo. That leaves the newin_block,in_chain_locked_block, andinstant_sendpaths effectively untested. Please add at least one valid serialized transaction case per context or a direct unit test around the conversion helpers. As per coding guidelines "Write unit tests for new functionality in Rust code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/wallet_manager_tests.rs` around lines 620 - 689, The tests currently use malformed tx_bytes so wallet_manager::wallet_manager_process_transaction returns before exercising the new FFITransactionContextDetails mapping; add at least one valid serialized transaction for each context (mempool, FFITransactionContextDetails::in_block, FFITransactionContextDetails::in_chain_locked_block / instant_send) so the code reaches the context conversion path, or alternatively add direct unit tests around the conversion helpers that translate FFITransactionContextDetails/FFIBlockInfo into the internal context types; specifically update the tests that call wallet_manager_process_transaction (and/or add new tests for the conversion functions) to provide well-formed tx_bytes and assert that the correct internal context mapping/flags are produced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/src/types.rs`:
- Around line 21-29: FFIBlockInfo::empty() is being treated as a valid confirmed
block; update transaction_context_from_ffi() (and any FFI entry points that call
it) so that when the incoming FFIBlockInfo equals FFIBlockInfo::empty() you
reject it for confirmed contexts (InBlock and InChainLockedBlock) by returning
an Err/None and have to_transaction_context() and the public FFI wrappers map
that to FFIErrorCode::InvalidInput instead of creating a zero-hash confirmed
context; ensure only unconfirmed contexts accept the empty sentinel and
reference the functions transaction_context_from_ffi, to_transaction_context,
and the FFI entry points that call them when making the change.
---
Outside diff comments:
In `@key-wallet-ffi/FFI_API.md`:
- Around line 855-862: Update the generated documentation in the Rust doc
comments / generator inputs for the managed_wallet_check_transaction FFI:
include the new parameters (context_type: FFITransactionContext, block_info:
FFIBlockInfo, update_state: bool) in the prose, replace any removed out-param
descriptions with the single result_out pointer, and explicitly document that
result_out must be populated and freed via transaction_check_result_free;
additionally add a clear description of the FFIBlockInfo struct fields (what
each field means and how callers should populate them) so callers can build the
new parameter. Also make the same updates to the other transaction-check doc
block that still references removed out-params so both generated sections
reflect the new ABI.
In `@key-wallet-ffi/src/managed_account.rs`:
- Around line 662-675: FFITransactionRecord's memory layout changed by moving
flat block fields into the nested context: FFITransactionContextDetails, so
update consumers to read block info via record->context.block_info.height /
.block_hash / .timestamp (or equivalent language bindings), regenerate FFI
headers with cbindgen to reflect the new layout, update Swift/Kotlin/C binding
access paths to the nested fields, and add a release-note entry documenting the
ABI-breaking change and migration steps for downstream users.
In `@key-wallet-ffi/src/transaction_checking.rs`:
- Around line 109-119: The FFI changed: managed_wallet_check_transaction now
takes a single FFIBlockInfo (by value) instead of separate block_height,
block_hash pointer, and timestamp, which is an ABI-breaking change; update all
native callers (C/Swift/Kotlin) to construct and pass an FFIBlockInfo value to
managed_wallet_check_transaction, ensuring the struct layout matches the Rust
definition (FFIBlockInfo is #[repr(C)] and Copy with 4+32+4 bytes) and adjust
any call sites that previously passed the three separate parameters to instead
populate and pass the FFIBlockInfo instance.
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 388-397: The FFI signature for wallet_check_transaction was
changed to take a consolidated FFIBlockInfo struct instead of separate
block_height, block_hash, and timestamp parameters; update the implementation of
wallet_check_transaction to accept and use the new block_info: FFIBlockInfo
parameter (match how managed_wallet_check_transaction handles block_info),
replace references to the old individual parameters with block_info.<field>
accesses, and ensure the FFITransactionCheckResult population and any validation
use block_info consistently so the API matches managed_wallet_check_transaction.
In `@key-wallet/src/managed_account/mod.rs`:
- Around line 377-396: The confirm_transaction path currently only sets changed
when tx_record.context != context, missing mutations from update_utxos; modify
update_utxos to return a bool indicating whether it mutated UTXO/account state
and in confirm_transaction capture that return value and OR it into changed
(i.e., let utxos_changed = self.update_utxos(tx, account_match, context);
changed |= utxos_changed), keeping the existing record_transaction and
tx_record.update_context logic intact so all related state updates remain
atomic.
In `@key-wallet/src/managed_account/transaction_record.rs`:
- Around line 15-23: TransactionRecord's new required field `context` breaks
deserialization of older persisted records; implement a compatibility
deserializer for TransactionRecord that accepts the old shape (height,
block_hash, timestamp) as optional fields and constructs a TransactionContext
when `context` is missing. Concretely, add a custom Deserialize impl (or serde
with #[serde(deserialize_with = "...")] for TransactionRecord) that tries to
read `context` first, and if absent reads `height`, `block_hash`, `timestamp`
and synthesizes a TransactionContext (using the same enum/struct constructors as
TransactionContext) before returning the TransactionRecord; ensure the code
references the TransactionRecord type, the TransactionContext constructor(s),
and the field names `transaction`, `txid`, and `context` so older bytes remain
readable and ManagedCoreAccount rebuilding logic continues to work.
---
Nitpick comments:
In `@key-wallet-ffi/src/wallet_manager_tests.rs`:
- Around line 620-689: The tests currently use malformed tx_bytes so
wallet_manager::wallet_manager_process_transaction returns before exercising the
new FFITransactionContextDetails mapping; add at least one valid serialized
transaction for each context (mempool, FFITransactionContextDetails::in_block,
FFITransactionContextDetails::in_chain_locked_block / instant_send) so the code
reaches the context conversion path, or alternatively add direct unit tests
around the conversion helpers that translate
FFITransactionContextDetails/FFIBlockInfo into the internal context types;
specifically update the tests that call wallet_manager_process_transaction
(and/or add new tests for the conversion functions) to provide well-formed
tx_bytes and assert that the correct internal context mapping/flags are
produced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c04cb4e1-b1f0-4054-bea3-e4b2535e833e
📒 Files selected for processing (14)
key-wallet-ffi/FFI_API.mdkey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/transaction_checking.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/transaction_record.rskey-wallet/src/tests/spent_outpoints_tests.rskey-wallet/src/transaction_checking/transaction_context.rskey-wallet/src/transaction_checking/transaction_router/tests/helpers.rskey-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/transaction_checking/wallet_checker.rs
xdustinface
added a commit
that referenced
this pull request
Mar 25, 2026
Addresses CodeRabbit review comment on PR #582 #582 (comment)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/src/types.rs`:
- Around line 48-71: Add a #[cfg(test)] unit-test module next to
transaction_context_from_ffi that exercises all FFITransactionContext variants
using both a FFIBlockInfo::empty() and a non-empty FFIBlockInfo: assert that
transaction_context_from_ffi(FFITransactionContext::InBlock,
&FFIBlockInfo::empty()) and the InChainLockedBlock variant return None, and
assert that Mempool and InstantSend return
Some(TransactionContext::Mempool/InstantSend) even when given
FFIBlockInfo::empty(), plus validate that InBlock/InChainLockedBlock return
Some(...) for a non-empty block_info; structure as a small table-driven test to
cover accepted and rejected paths.
- Around line 31-35: FFIBlockInfo::to_block_info is currently infallible and
only rejects the exact zero-hash sentinel; change it to be fallible (return
Result<BlockInfo, Error> or Option<BlockInfo>) and validate the full invariant:
require the block_hash not equal to the all-zero byte array and require
timestamp != 0 before calling BlockInfo::new. Update all call sites (where
FFIBlockInfo is used to build confirmed TransactionContext or any "block-based"
context) to handle the failure path and avoid constructing confirmed contexts
when either hash or timestamp are zero; apply the same change for the related
conversions in the 52-71 region.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 823f9078-b257-44b4-86c3-774e8b0a02df
📒 Files selected for processing (4)
key-wallet-ffi/src/transaction.rskey-wallet-ffi/src/transaction_checking.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/wallet_manager.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- key-wallet-ffi/src/transaction_checking.rs
- key-wallet-ffi/src/transaction.rs
|
!fulltest |
dashinfraclaw
added a commit
to dashinfraclaw/platform
that referenced
this pull request
Mar 26, 2026
… TransactionRecord Update dashcore/dashcore-rpc git dependencies to 5114330ae564858803d656efba84bb4adacb2a2e for integration testing of dashpay/rust-dashcore#582.
4 tasks
dashinfraclaw
added a commit
to dashinfraclaw/kotlin-platform
that referenced
this pull request
Mar 26, 2026
… TransactionRecord Update dashcore/dashcore-rpc git dependencies to 5114330ae564858803d656efba84bb4adacb2a2e for integration testing of dashpay/rust-dashcore#582.
dashinfraclaw
added a commit
to dashinfraclaw/ferment
that referenced
this pull request
Mar 26, 2026
… TransactionRecord Update dashcore/dashcore-rpc git dependencies to 5114330ae564858803d656efba84bb4adacb2a2e for integration testing of dashpay/rust-dashcore#582.
dashinfraclaw
added a commit
to dashinfraclaw/quorum-list-server
that referenced
this pull request
Mar 26, 2026
… TransactionRecord Update dashcore/dashcore-rpc git dependencies to 5114330ae564858803d656efba84bb4adacb2a2e for integration testing of dashpay/rust-dashcore#582.
dashinfraclaw
added a commit
to dashinfraclaw/kotlin-platform
that referenced
this pull request
Mar 26, 2026
… TransactionRecord Update dashcore/dashcore-rpc git dependencies to 5114330ae564858803d656efba84bb4adacb2a2e for integration testing of dashpay/rust-dashcore#582.
dashinfraclaw
added a commit
to dashinfraclaw/kotlin-platform
that referenced
this pull request
Mar 26, 2026
… TransactionRecord Update dashcore/dashcore-rpc git dependencies to 5114330ae564858803d656efba84bb4adacb2a2e for integration testing of dashpay/rust-dashcore#582.
|
🔧 Infraclaw fulltest results for Testing rust-dashcore PR #582 (
Platform failure analysis: Updated manually after agent session interrupted. |
dashinfraclaw
added a commit
to dashinfraclaw/platform
that referenced
this pull request
Mar 26, 2026
… TransactionRecord Update dashcore/dashcore-rpc git dependencies to 5114330ae564858803d656efba84bb4adacb2a2e for integration testing of dashpay/rust-dashcore#582.
dashinfraclaw
added a commit
to dashinfraclaw/kotlin-platform
that referenced
this pull request
Mar 26, 2026
… TransactionRecord Update dashcore/dashcore-rpc git dependencies to 5114330ae564858803d656efba84bb4adacb2a2e for integration testing of dashpay/rust-dashcore#582.
dashinfraclaw
added a commit
to dashinfraclaw/quorum-list-server
that referenced
this pull request
Mar 26, 2026
… TransactionRecord Update dashcore/dashcore-rpc git dependencies to 5114330ae564858803d656efba84bb4adacb2a2e for integration testing of dashpay/rust-dashcore#582.
dashinfraclaw
added a commit
to dashinfraclaw/ferment
that referenced
this pull request
Mar 26, 2026
… TransactionRecord Update dashcore/dashcore-rpc git dependencies to 5114330ae564858803d656efba84bb4adacb2a2e for integration testing of dashpay/rust-dashcore#582.
dashinfraclaw
added a commit
to dashinfraclaw/kotlin-platform
that referenced
this pull request
Mar 26, 2026
… TransactionRecord Update dashcore/dashcore-rpc git dependencies to 5114330ae564858803d656efba84bb4adacb2a2e for integration testing of dashpay/rust-dashcore#582.
xdustinface
added a commit
that referenced
this pull request
Mar 26, 2026
Addresses CodeRabbit review comment on PR #582 #582 (comment)
xdustinface
added a commit
that referenced
this pull request
Mar 26, 2026
Addresses CodeRabbit review comment on PR #582 #582 (comment)
xdustinface
added a commit
that referenced
this pull request
Mar 26, 2026
Addresses CodeRabbit review comment on PR #582 #582 (comment)
xdustinface
force-pushed
the
refactor/store-transaction-context-in-transaction-record
branch
from
c4c0240 to
7c0acad
Compare
xdustinface
added a commit
that referenced
this pull request
Mar 26, 2026
Addresses CodeRabbit review comment on PR #582 #582 (comment)
xdustinface
added a commit
that referenced
this pull request
Mar 27, 2026
Addresses CodeRabbit review comment on PR #582 #582 (comment)
xdustinface
force-pushed
the
refactor/store-transaction-context-in-transaction-record
branch
from
bd37091 to
d8c394e
Compare
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
github-actions
bot
added
the
merge-conflict
xdustinface
added a commit
to xdustinface/rust-dashcore
that referenced
this pull request
Mar 30, 2026
Addresses CodeRabbit review comment on PR dashpay#582 dashpay#582 (comment)
xdustinface
added a commit
to xdustinface/rust-dashcore
that referenced
this pull request
Mar 30, 2026
Addresses CodeRabbit review comment on PR dashpay#582 dashpay#582 (comment)
xdustinface
added a commit
that referenced
this pull request
Mar 31, 2026
Addresses CodeRabbit review comment on PR #582 #582 (comment)
xdustinface
added a commit
that referenced
this pull request
Mar 31, 2026
Addresses CodeRabbit review comment on PR #582 #582 (comment)
xdustinface
force-pushed
the
refactor/store-transaction-context-in-transaction-record
branch
from
d8c394e to
0171606
Compare
github-actions
bot
removed
merge-conflict
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
key-wallet/src/transaction_checking/wallet_checker.rs (1)
851-853: Consider reducing repeatedblock_info().unwrap()chains in tests.Optional cleanup: bind block info once per assertion block to make failures clearer and avoid repeated unwrap calls.
♻️ Suggested test cleanup pattern
- assert_eq!(record.block_info().unwrap().block_hash, block_hash); - assert_eq!(record.block_info().unwrap().timestamp, 1700000000); + let info = record + .block_info() + .expect("Confirmed transaction should include block info"); + assert_eq!(info.block_hash, block_hash); + assert_eq!(info.timestamp, 1700000000);Also applies to: 982-984, 1021-1023, 1070-1072
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/transaction_checking/wallet_checker.rs` around lines 851 - 853, Tests repeatedly call record.block_info().unwrap(); bind the result once to a local variable (e.g., let block_info = record.block_info().unwrap();) at the top of each assertion block (in the tests in wallet_checker.rs where record is used) and then replace subsequent record.block_info().unwrap().field accesses with block_info.field to avoid repeated unwraps and make failures clearer; apply the same change to the other occurrences mentioned (the blocks around the assertions that currently use record.block_info().unwrap()).key-wallet-ffi/src/wallet_manager_tests.rs (2)
632-637: Test uses zero block hash which may mask validation gaps.The test constructs
FFIBlockInfowithblock_hash: [0u8; 32]but non-zerotimestamp. Under the current&&validation logic intransaction_context_from_ffi, this is not rejected as invalid (only the exact empty sentinel is rejected). The test passes because the transaction bytes are invalid, not because of block info validation.Using a realistic non-zero block hash would make the test more representative of actual usage:
♻️ Suggested fix
let block_context = crate::types::FFITransactionContextDetails::in_block(crate::types::FFIBlockInfo { height: 100000, - block_hash: [0u8; 32], + block_hash: [0xab; 32], // Use non-zero hash for realistic test timestamp: 1234567890, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/wallet_manager_tests.rs` around lines 632 - 637, The test constructs FFIBlockInfo with a zero block_hash which can mask validation gaps in transaction_context_from_ffi; update the block_context construction in the test (FFITransactionContextDetails::in_block with FFIBlockInfo) to use a realistic non-zero block_hash (e.g., a deterministic non-zero 32-byte array) so the block info validation is exercised rather than relying on invalid transaction bytes to fail the test.
670-677: Same zero block hash concern applies here.♻️ Suggested fix
let chain_locked_context = crate::types::FFITransactionContextDetails::in_chain_locked_block( crate::types::FFIBlockInfo { height: 100000, - block_hash: [0u8; 32], + block_hash: [0xab; 32], // Use non-zero hash for realistic test timestamp: 1234567890, }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/wallet_manager_tests.rs` around lines 670 - 677, Test uses an all-zero block hash which can be ambiguous; update the FFIBlockInfo passed to FFITransactionContextDetails::in_chain_locked_block so block_hash is a non-zero, deterministic value (e.g., a fixed test vector or filled bytes like [1u8; 32] or a specific hex-derived array) to avoid zero-hash edge cases when constructing chain_locked_context and keep tests deterministic; modify the block_hash field in the FFIBlockInfo used by chain_locked_context accordingly.key-wallet-ffi/src/types.rs (1)
550-567: Consider adding tests for partial-zero block info scenarios.The current tests cover the exact
FFIBlockInfo::empty()sentinel and fully valid block info. Adding cases for partial zeroing (e.g., zero hash with non-zero timestamp, or non-zero hash with zero timestamp) would help catch validation logic regressions.🧪 Example additional test cases
#[test] fn transaction_context_from_ffi_in_block_with_zero_hash_only() { let block_info = FFIBlockInfo { height: 1000, block_hash: [0u8; 32], // zero hash timestamp: 1700000000, // non-zero timestamp }; // With current && logic this returns Some; with || logic it would return None let result = transaction_context_from_ffi(FFITransactionContext::InBlock, &block_info); // Adjust expectation based on intended behavior } #[test] fn transaction_context_from_ffi_in_block_with_zero_timestamp_only() { let block_info = FFIBlockInfo { height: 1000, block_hash: [0xab; 32], // non-zero hash timestamp: 0, // zero timestamp }; let result = transaction_context_from_ffi(FFITransactionContext::InBlock, &block_info); // Adjust expectation based on intended behavior }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/types.rs` around lines 550 - 567, Add unit tests for partial-zero FFIBlockInfo cases to cover validation edge-cases in transaction_context_from_ffi: create tests that pass an FFIBlockInfo with (a) zero block_hash but non-zero timestamp and height, and (b) non-zero block_hash but zero timestamp with non-zero height, then call transaction_context_from_ffi for both FFITransactionContext::InBlock and ::InChainLockedBlock and assert the expected behavior (Some(TransactionContext::InBlock/ InChainLockedBlock) or None) to ensure the current empty-sentinel logic (FFIBlockInfo::empty()) doesn’t regress; name tests clearly (e.g., transaction_context_from_ffi_in_block_with_zero_hash_only and _with_zero_timestamp_only) and mirror assertions used in the existing valid/empty tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@key-wallet-ffi/src/types.rs`:
- Around line 550-567: Add unit tests for partial-zero FFIBlockInfo cases to
cover validation edge-cases in transaction_context_from_ffi: create tests that
pass an FFIBlockInfo with (a) zero block_hash but non-zero timestamp and height,
and (b) non-zero block_hash but zero timestamp with non-zero height, then call
transaction_context_from_ffi for both FFITransactionContext::InBlock and
::InChainLockedBlock and assert the expected behavior
(Some(TransactionContext::InBlock/ InChainLockedBlock) or None) to ensure the
current empty-sentinel logic (FFIBlockInfo::empty()) doesn’t regress; name tests
clearly (e.g., transaction_context_from_ffi_in_block_with_zero_hash_only and
_with_zero_timestamp_only) and mirror assertions used in the existing
valid/empty tests.
In `@key-wallet-ffi/src/wallet_manager_tests.rs`:
- Around line 632-637: The test constructs FFIBlockInfo with a zero block_hash
which can mask validation gaps in transaction_context_from_ffi; update the
block_context construction in the test (FFITransactionContextDetails::in_block
with FFIBlockInfo) to use a realistic non-zero block_hash (e.g., a deterministic
non-zero 32-byte array) so the block info validation is exercised rather than
relying on invalid transaction bytes to fail the test.
- Around line 670-677: Test uses an all-zero block hash which can be ambiguous;
update the FFIBlockInfo passed to
FFITransactionContextDetails::in_chain_locked_block so block_hash is a non-zero,
deterministic value (e.g., a fixed test vector or filled bytes like [1u8; 32] or
a specific hex-derived array) to avoid zero-hash edge cases when constructing
chain_locked_context and keep tests deterministic; modify the block_hash field
in the FFIBlockInfo used by chain_locked_context accordingly.
In `@key-wallet/src/transaction_checking/wallet_checker.rs`:
- Around line 851-853: Tests repeatedly call record.block_info().unwrap(); bind
the result once to a local variable (e.g., let block_info =
record.block_info().unwrap();) at the top of each assertion block (in the tests
in wallet_checker.rs where record is used) and then replace subsequent
record.block_info().unwrap().field accesses with block_info.field to avoid
repeated unwraps and make failures clearer; apply the same change to the other
occurrences mentioned (the blocks around the assertions that currently use
record.block_info().unwrap()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e571125-6d75-4a35-aea2-2f8ac7f6bb30
📒 Files selected for processing (17)
key-wallet-ffi/FFI_API.mdkey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/transaction_checking.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-manager/src/event_tests.rskey-wallet-manager/src/test_helpers.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/transaction_record.rskey-wallet/src/tests/spent_outpoints_tests.rskey-wallet/src/transaction_checking/transaction_context.rskey-wallet/src/transaction_checking/transaction_router/tests/helpers.rskey-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rskey-wallet/src/transaction_checking/transaction_router/tests/routing.rskey-wallet/src/transaction_checking/wallet_checker.rs
✅ Files skipped from review due to trivial changes (1)
- key-wallet-ffi/src/wallet_manager.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- key-wallet/src/transaction_checking/transaction_router/tests/helpers.rs
- key-wallet/src/tests/spent_outpoints_tests.rs
- key-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rs
- key-wallet/src/transaction_checking/transaction_context.rs
- key-wallet-ffi/src/managed_account.rs
- key-wallet/src/managed_account/mod.rs
- key-wallet-ffi/src/transaction_checking.rs
- key-wallet-ffi/FFI_API.md
- key-wallet/src/managed_account/transaction_record.rs
Replace separate block-related fields with a `context: TransactionContext` in `TransactionRecord`, preserving the full transaction context (mempool, instant-send, in-block, chain-locked). Replace `mark_confirmed`/`mark_unconfirmed` with a single `update_context` method. Introduce `FFIBlockInfo` struct and use `FFITransactionContextDetails` in `FFITransactionRecord` to expose the full context across the FFI boundary.
Addresses CodeRabbit review comment on PR #582 #582 (comment)
Addresses CodeRabbit review comment on PR #582 #582 (comment)
xdustinface
force-pushed
the
refactor/store-transaction-context-in-transaction-record
branch
from
b0def91 to
bf4b456
Compare
github-actions
bot
added
ready-for-review
ZocoLini
approved these changes
Mar 31, 2026
xdustinface
deleted the
refactor/store-transaction-context-in-transaction-record
branch
This was referenced Apr 1, 2026
shumkov
added a commit
that referenced
this pull request
Apr 10, 2026
Phase 1 (prep): - Fix AddressInfo::mark_used() hardcoded used_at=Some(0) → real timestamp - Make ManagedCoreAccount::current_timestamp() public - Add first_seen: u64 to TransactionRecord (restores pre-#582 behavior for mempool tx timestamps, needed by evo-tool wallet_transactions table) - Add helper accessors: is_instant_locked, is_chain_locked, block_height, block_hash - Derive PartialEq + Eq on TransactionRecord, InputDetail, OutputDetail (needed by changeset types) - Make Wallet::add_account() idempotent for xpub=None (deterministic derivation replays safely); still errors on explicit-xpub conflict - Add AddressPool::set_last_revealed(index) for apply() restore - Add ManagedCoreAccount::insert_utxo/remove_utxo idempotent wrappers - Add ManagedWalletInfo::find_account_by_address_mut/find_account_with_utxo_mut routing helpers for apply() Phase 2 (changeset module): - New key-wallet/src/changeset/ module with BDK-style atomic changesets - Merge trait + blanket impls for BTreeMap/BTreeSet/Option/Vec - WalletChangeSet with sub-changesets: AccountKeyChangeSet, AccountStateChangeSet, ChainChangeSet, UtxoChangeSet, TransactionChangeSet, BalanceChangeSet - Changesets carry native types directly (Utxo, TransactionRecord, AccountType) — no flattened *Entry wrappers, simpler apply logic - Merge semantics: max-wins for heights/last_revealed, sum for balance deltas, dedup for account types, extend for maps/sets Tests: 32 new (14 merge, 5 changeset, 4 address_pool, 11 changeset_support). Updated 3 existing tests that asserted add_account errors on duplicate. 478 total, all passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace separate block-related fields with a
context: TransactionContextinTransactionRecord, preserving the full transaction context (mempool, instant-send, in-block, chain-locked). Replacemark_confirmed/mark_unconfirmedwith a singleupdate_contextmethod. IntroduceFFIBlockInfostruct and useFFITransactionContextDetailsinFFITransactionRecordto expose the full context across the FFI boundary.Summary by CodeRabbit
Refactor
Tests