Skip to content

refactor: use BlockInfo in TransactionRecord#580

Closed
xdustinface wants to merge 1 commit intov0.42-devfrom
refactor/use-block-info-in-transaction-record
Closed

refactor: use BlockInfo in TransactionRecord#580
xdustinface wants to merge 1 commit intov0.42-devfrom
refactor/use-block-info-in-transaction-record

Conversation

@xdustinface
Copy link
Collaborator

@xdustinface xdustinface commented Mar 25, 2026

Replace three separate block-related fields with a single block_info: Option<BlockInfo> in TransactionRecord, simplify mark_confirmed to accept BlockInfo directly, and introduce FFIBlockInfo struct to replace scattered height/hash/timestamp parameters across all FFI boundaries (FFITransactionRecord, FFITransactionContextDetails, wallet_check_transaction, managed_wallet_check_transaction).

Summary by CodeRabbit

  • Refactor
    • Consolidated block information handling across transaction APIs by unifying separate block-related parameters into a single structured representation, improving API consistency and reducing parameter complexity for developers integrating with the wallet.

Replace three separate block-related fields with a single `block_info: Option<BlockInfo>`
in `TransactionRecord`, simplify `mark_confirmed` to accept `BlockInfo` directly, and
introduce `FFIBlockInfo` struct to replace scattered height/hash/timestamp parameters
across all FFI boundaries (`FFITransactionRecord`, `FFITransactionContextDetails`,
`wallet_check_transaction`, `managed_wallet_check_transaction`).
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 25, 2026

📝 Walkthrough

Walkthrough

This PR consolidates block metadata parameters across FFI and core wallet layers by replacing separate block_height, block_hash, and timestamp fields with a unified FFIBlockInfo/BlockInfo structure. Updates affect transaction checking functions, record data structures, and related test infrastructure.

Changes

Cohort / File(s) Summary
FFI API Specifications
key-wallet-ffi/FFI_API.md
Updated function signatures for managed_wallet_check_transaction and wallet_check_transaction to replace three block parameters with single FFIBlockInfo argument.
FFI Type Definitions
key-wallet-ffi/src/types.rs
Introduced new FFIBlockInfo struct with height, block_hash, and timestamp fields; added transaction_context_from_ffi helper; refactored FFITransactionContextDetails to use block_info field and updated constructors; removed unsafe block_info_from_ffi function.
FFI Transaction Functions
key-wallet-ffi/src/transaction_checking.rs, key-wallet-ffi/src/transaction.rs
Updated managed_wallet_check_transaction and wallet_check_transaction signatures to accept FFIBlockInfo; simplified context construction by delegating to transaction_context_from_ffi.
FFI Data Structures
key-wallet-ffi/src/managed_account.rs
Refactored FFITransactionRecord to replace height, block_hash, timestamp fields with single block_info: FFIBlockInfo field; updated population logic in managed_core_account_get_transactions.
FFI Tests
key-wallet-ffi/src/wallet_manager_tests.rs
Updated test_wallet_manager_process_transaction to construct FFITransactionContextDetails using new helper constructors with FFIBlockInfo values.
Core Wallet Data Model
key-wallet/src/managed_account/transaction_record.rs
Replaced separate height, block_hash, timestamp fields in TransactionRecord with consolidated block_info: Option<BlockInfo>; refactored constructor and confirmation methods accordingly; added height() accessor.
Core Wallet Logic
key-wallet/src/managed_account/mod.rs
Updated UTXO creation and transaction confirmation to use consolidated block_info field; simplified block data extraction and passing.
Core Wallet Tests
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_router/tests/routing.rs, key-wallet/src/transaction_checking/wallet_checker.rs
Updated test constructors, assertions, and block info creation to use new consolidated structure; added/updated helper function for test block info construction.
Core Wallet Test Utilities
key-wallet/src/transaction_checking/transaction_router/tests/helpers.rs
Added test_block_info(height: u32) -> BlockInfo helper function to centralize test block metadata construction.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

ready-for-review

Suggested reviewers

  • QuantumExplorer
  • ZocoLini

Poem

🐰 Block metadata bundled tight,
Heights and hashes unified in sight,
One struct holds what three once bore,
Cleaner APIs, less noise to explore! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: consolidating block-related fields in TransactionRecord into a single BlockInfo field.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/use-block-info-in-transaction-record

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@xdustinface xdustinface marked this pull request as draft March 25, 2026 04:37
@codecov
Copy link

codecov bot commented Mar 25, 2026

Codecov Report

❌ Patch coverage is 80.00000% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.84%. Comparing base (2f34963) to head (9ba033f).

Files with missing lines Patch % Lines
key-wallet-ffi/src/types.rs 61.11% 14 Missing ⚠️
key-wallet-ffi/src/transaction.rs 0.00% 2 Missing ⚠️
key-wallet-ffi/src/transaction_checking.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##           v0.42-dev     #580      +/-   ##
=============================================
+ Coverage      66.73%   66.84%   +0.10%     
=============================================
  Files            313      313              
  Lines          64948    64922      -26     
=============================================
+ Hits           43346    43398      +52     
+ Misses         21602    21524      -78     
Flag Coverage Δ
core 75.13% <ø> (ø)
ffi 39.28% <58.13%> (+0.79%) ⬆️
rpc 28.28% <ø> (ø)
spv 81.05% <ø> (-0.05%) ⬇️
wallet 66.15% <100.00%> (-0.04%) ⬇️
Files with missing lines Coverage Δ
key-wallet-ffi/src/managed_account.rs 47.88% <100.00%> (+3.96%) ⬆️
key-wallet/src/managed_account/mod.rs 48.88% <100.00%> (-0.57%) ⬇️
...y-wallet/src/managed_account/transaction_record.rs 100.00% <100.00%> (ø)
...-wallet/src/transaction_checking/wallet_checker.rs 96.52% <100.00%> (-0.02%) ⬇️
key-wallet-ffi/src/transaction.rs 0.00% <0.00%> (ø)
key-wallet-ffi/src/transaction_checking.rs 1.69% <0.00%> (+0.03%) ⬆️
key-wallet-ffi/src/types.rs 67.55% <61.11%> (+8.35%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
key-wallet-ffi/src/managed_account.rs (1)

663-670: ⚠️ Potential issue | 🟠 Major

Version this exported FFI record change.

Lines 669-670 move three public members under block_info, so downstream bindings that currently read record.height, record.block_hash, or record.timestamp now need a coordinated contract change. Please keep the legacy shape for one compatibility window or introduce a versioned FFITransactionRecordV2/managed_core_account_get_transactions_v2 instead of mutating the existing exported type in place.
As per coding guidelines, "Handle memory safety carefully at FFI boundaries".

🤖 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 663 - 670, The FFI struct
layout was changed by moving height/block_hash/timestamp under block_info, which
breaks existing consumers; restore backward compatibility by retaining the
original exported layout or introduce a versioned type and API: create
FFITransactionRecordV2 (with block_info) and a new accessor function
managed_core_account_get_transactions_v2 that returns V2 while keeping the
existing FFITransactionRecord and managed_core_account_get_transactions
unchanged for one compatibility window; update any internal conversion code to
populate both shapes and document the new symbol names (FFITransactionRecord,
FFITransactionRecordV2, block_info, managed_core_account_get_transactions_v2) so
downstream bindings can migrate safely and memory safety at the FFI boundary is
preserved.
key-wallet-ffi/FFI_API.md (1)

852-862: ⚠️ Potential issue | 🟠 Major

Regenerate the FFI docs from the new API contract.

Lines 1306-1310 still describe the removed inputs_spent_out / addresses_used_out / new_balance_out / new_address_* outputs, and neither section explains the new context_type + block_info + update_state contract or the FFIBlockInfo layout. For a public FFI surface, that leaves callers without the safety guidance they need to construct these arguments correctly. Because this file is auto-generated, the fix likely belongs in the Rust doc comments/generator rather than here.

Also applies to: 1300-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 852 - 862, The generated FFI docs
still mention removed outputs (inputs_spent_out, addresses_used_out,
new_balance_out, new_address_*) and lack the new parameter contract for
managed_wallet_check_transaction; update the Rust doc comments/generator to
regenerate FFI_API.md so the managed_wallet_check_transaction entry documents
the new parameters (FFITransactionContext `context_type`, the full FFIBlockInfo
layout and field meanings, and the `update_state` semantics) and remove
references to the deleted outputs, and ensure safety guidance mentions how to
construct/validate `tx_bytes`, `result_out` and `error` and that the
affected_accounts array must be freed via transaction_check_result_free.
🤖 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/managed_account.rs`:
- Around line 728-730: Add a unit test that calls
managed_core_account_get_transactions and asserts the FFIBlockInfo behavior for
mempool vs confirmed records: create or mock one mempool transaction record and
one confirmed transaction record, call managed_core_account_get_transactions (or
the helper that produces ffi Record structs), and assert that the mempool
record's block_info equals FFIBlockInfo::empty() while the confirmed record's
block_info equals FFIBlockInfo::from(...) with the expected populated fields;
place the test in managed_account.rs alongside other tests to lock down the new
boundary behavior.

In `@key-wallet-ffi/src/types.rs`:
- Around line 48-60: The current transaction_context_from_ffi function
constructs confirmed contexts from a zeroed FFIBlockInfo, which can create
invalid BlockInfo; change transaction_context_from_ffi(signature) to return a
Result<TransactionContext, <appropriate error type>> and validate that for
FFITransactionContext::InBlock and ::InChainLockedBlock the provided block_info
is not the placeholder (use FFIBlockInfo::empty() or equivalent check); if it is
empty return an error instead of creating
TransactionContext::InBlock/::InChainLockedBlock with a fake block, and update
all callers to propagate or handle the Result accordingly to avoid silently
accepting zeroed block metadata at the FFI boundary.

---

Outside diff comments:
In `@key-wallet-ffi/FFI_API.md`:
- Around line 852-862: The generated FFI docs still mention removed outputs
(inputs_spent_out, addresses_used_out, new_balance_out, new_address_*) and lack
the new parameter contract for managed_wallet_check_transaction; update the Rust
doc comments/generator to regenerate FFI_API.md so the
managed_wallet_check_transaction entry documents the new parameters
(FFITransactionContext `context_type`, the full FFIBlockInfo layout and field
meanings, and the `update_state` semantics) and remove references to the deleted
outputs, and ensure safety guidance mentions how to construct/validate
`tx_bytes`, `result_out` and `error` and that the affected_accounts array must
be freed via transaction_check_result_free.

In `@key-wallet-ffi/src/managed_account.rs`:
- Around line 663-670: The FFI struct layout was changed by moving
height/block_hash/timestamp under block_info, which breaks existing consumers;
restore backward compatibility by retaining the original exported layout or
introduce a versioned type and API: create FFITransactionRecordV2 (with
block_info) and a new accessor function managed_core_account_get_transactions_v2
that returns V2 while keeping the existing FFITransactionRecord and
managed_core_account_get_transactions unchanged for one compatibility window;
update any internal conversion code to populate both shapes and document the new
symbol names (FFITransactionRecord, FFITransactionRecordV2, block_info,
managed_core_account_get_transactions_v2) so downstream bindings can migrate
safely and memory safety at the FFI boundary is preserved.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1a80e8b1-64d3-440a-8e3b-9158b0dc18b0

📥 Commits

Reviewing files that changed from the base of the PR and between 2f34963 and 9ba033f.

📒 Files selected for processing (13)
  • key-wallet-ffi/FFI_API.md
  • key-wallet-ffi/src/managed_account.rs
  • key-wallet-ffi/src/transaction.rs
  • key-wallet-ffi/src/transaction_checking.rs
  • key-wallet-ffi/src/types.rs
  • key-wallet-ffi/src/wallet_manager_tests.rs
  • key-wallet/src/managed_account/mod.rs
  • key-wallet/src/managed_account/transaction_record.rs
  • key-wallet/src/tests/spent_outpoints_tests.rs
  • key-wallet/src/transaction_checking/transaction_router/tests/helpers.rs
  • key-wallet/src/transaction_checking/transaction_router/tests/identity_transactions.rs
  • key-wallet/src/transaction_checking/transaction_router/tests/routing.rs
  • key-wallet/src/transaction_checking/wallet_checker.rs

Comment on lines +728 to +730
// Copy block info
ffi_record.block_info =
record.block_info.map(FFIBlockInfo::from).unwrap_or_else(FFIBlockInfo::empty);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add a direct test for managed_core_account_get_transactions block export.

This function now emits FFIBlockInfo::empty() for mempool records and a populated FFIBlockInfo for confirmed ones, but this file still does not assert either behavior. A small unit test here would lock down the new boundary behavior.
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/managed_account.rs` around lines 728 - 730, Add a unit
test that calls managed_core_account_get_transactions and asserts the
FFIBlockInfo behavior for mempool vs confirmed records: create or mock one
mempool transaction record and one confirmed transaction record, call
managed_core_account_get_transactions (or the helper that produces ffi Record
structs), and assert that the mempool record's block_info equals
FFIBlockInfo::empty() while the confirmed record's block_info equals
FFIBlockInfo::from(...) with the expected populated fields; place the test in
managed_account.rs alongside other tests to lock down the new boundary behavior.

Comment on lines +48 to +60
/// Convert an `FFIBlockInfo` and context type to a native `TransactionContext`.
pub(crate) fn transaction_context_from_ffi(
context_type: FFITransactionContext,
block_info: &FFIBlockInfo,
) -> TransactionContext {
match context_type {
FFITransactionContext::Mempool => TransactionContext::Mempool,
FFITransactionContext::InstantSend => TransactionContext::InstantSend,
FFITransactionContext::InBlock => TransactionContext::InBlock(block_info.to_block_info()),
FFITransactionContext::InChainLockedBlock => {
TransactionContext::InChainLockedBlock(block_info.to_block_info())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Reject the zero placeholder for confirmed contexts.

FFIBlockInfo::empty() is the documented placeholder for mempool/instant-send, but transaction_context_from_ffi() will currently accept that same zeroed struct for InBlock / InChainLockedBlock and build a fake BlockInfo at height 0 with an all-zero hash. One bad FFI call can then mark a transaction as confirmed with invalid block metadata and corrupt wallet state. Returning an error here instead of an infallible TransactionContext is probably the cleanest fix.

As per coding guidelines, **/*-ffi/**/*.rs: Handle memory safety carefully at FFI boundaries.

Also applies to: 793-827

🤖 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 48 - 60, The current
transaction_context_from_ffi function constructs confirmed contexts from a
zeroed FFIBlockInfo, which can create invalid BlockInfo; change
transaction_context_from_ffi(signature) to return a Result<TransactionContext,
<appropriate error type>> and validate that for FFITransactionContext::InBlock
and ::InChainLockedBlock the provided block_info is not the placeholder (use
FFIBlockInfo::empty() or equivalent check); if it is empty return an error
instead of creating TransactionContext::InBlock/::InChainLockedBlock with a fake
block, and update all callers to propagate or handle the Result accordingly to
avoid silently accepting zeroed block metadata at the FFI boundary.

@xdustinface
Copy link
Collaborator Author

Replaced by #582

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant