refactor: rename FFI context structs to match key-wallet naming#612
Merged
QuantumExplorer merged 2 commits intoApr 1, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughA cohesive refactor in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #612 +/- ##
=============================================
+ Coverage 67.40% 67.44% +0.04%
=============================================
Files 320 320
Lines 67946 67949 +3
=============================================
+ Hits 45800 45830 +30
+ Misses 22146 22119 -27
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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/FFI_API.md (1)
856-863:⚠️ Potential issue | 🟡 MinorThese generated docs still describe the old contract.
Both entries now take
FFITransactionContextType, but the surrounding text is still stale, andwallet_check_transactionstill documents removed outputs likeinputs_spent_outandnew_address_out. Since this file is auto-generated, please update the source rustdoc onkey-wallet-ffi/src/transaction_checking.rsandkey-wallet-ffi/src/transaction.rsand regenerate the API docs.Also applies to: 1320-1327
🤖 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 856 - 863, Update the autogenerated docs by fixing the rustdoc comments in key-wallet-ffi/src/transaction_checking.rs and key-wallet-ffi/src/transaction.rs to reflect the new function signatures: document that both managed_wallet_check_transaction and wallet_check_transaction now accept FFITransactionContextType, remove references to removed outputs like inputs_spent_out and new_address_out, and update the safety/description text to match the current FFITransactionCheckResult shape (including noting affected_accounts must be freed by transaction_check_result_free); then regenerate the FFI_API.md so the generated docs and descriptions match the actual function signatures and result fields.key-wallet-ffi/src/types.rs (1)
841-923:⚠️ Potential issue | 🟠 MajorThis public rename needs the example and PR metadata updated too.
FFITransactionContextchanges meaning from enum to struct here, butkey-wallet-ffi/examples/check_transaction.c:15-24still declaresFFITransactionContextas the enum. That leaves the bundled example out of sync with the exported FFI surface, andrefactor:does not fully signal the downstream rename/breaking change. Please update the example in the same PR and call the API break out explicitly.As per coding guidelines, "Check whether the PR title prefix allowed in the pr-title.yml workflow accurately describes the changes."
🤖 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 841 - 923, The exported FFI type FFITransactionContext was changed from an enum to a struct but the bundled C example (check_transaction.c) still treats it as an enum and must be updated; modify the example to construct and read the new struct form (use FFITransactionContext.mempool(), in_block(), in_chain_locked_block() semantics from the Rust API via the FFI helpers or adapt the C code to access .context_type and .block_info), and also update PR metadata/pr title to explicitly declare an API-breaking change (replace the "refactor:" prefix with a breaking-aware prefix and adjust any pr-title.yml entries or PR description to reflect the breaking rename) so downstream consumers know the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@key-wallet-ffi/FFI_API.md`:
- Around line 856-863: Update the autogenerated docs by fixing the rustdoc
comments in key-wallet-ffi/src/transaction_checking.rs and
key-wallet-ffi/src/transaction.rs to reflect the new function signatures:
document that both managed_wallet_check_transaction and wallet_check_transaction
now accept FFITransactionContextType, remove references to removed outputs like
inputs_spent_out and new_address_out, and update the safety/description text to
match the current FFITransactionCheckResult shape (including noting
affected_accounts must be freed by transaction_check_result_free); then
regenerate the FFI_API.md so the generated docs and descriptions match the
actual function signatures and result fields.
In `@key-wallet-ffi/src/types.rs`:
- Around line 841-923: The exported FFI type FFITransactionContext was changed
from an enum to a struct but the bundled C example (check_transaction.c) still
treats it as an enum and must be updated; modify the example to construct and
read the new struct form (use FFITransactionContext.mempool(), in_block(),
in_chain_locked_block() semantics from the Rust API via the FFI helpers or adapt
the C code to access .context_type and .block_info), and also update PR
metadata/pr title to explicitly declare an API-breaking change (replace the
"refactor:" prefix with a breaking-aware prefix and adjust any pr-title.yml
entries or PR description to reflect the breaking rename) so downstream
consumers know the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8960d1a9-d3f3-49e1-994b-a1bc895491e2
📒 Files selected for processing (7)
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.rs
Contributor
There was a problem hiding this comment.
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/examples/check_transaction.c (2)
81-93:⚠️ Potential issue | 🔴 CriticalCall sites also need to be updated to match the corrected signature.
Once the function declaration is fixed to use
FFIBlockInfo, the call sites must be updated. For example:Example fix for mempool transaction check (lines 81-93)
+ FFIBlockInfo mempool_block_info = {0}; // Zeroed for mempool bool success = wallet_check_transaction( wallet, - network, tx_bytes, tx_len, Mempool, // Transaction is in mempool - 0, // No block height for mempool tx - NULL, // No block hash for mempool tx - 0, // No timestamp + mempool_block_info, false, // Don't update wallet state &result, &error );Example fix for confirmed transaction check (lines 110-122)
uint8_t block_hash[32] = { /* ... block hash ... */ }; + FFIBlockInfo confirmed_block_info = { + .height = 650000, + .hash = { /* copy block_hash */ }, + .timestamp = 1234567890 + }; + memcpy(confirmed_block_info.hash, block_hash, 32); + success = wallet_check_transaction( wallet, - network, tx_bytes, tx_len, InBlock, // Transaction is in a block - 650000, // Block height - block_hash, // Block hash - 1234567890, // Unix timestamp + confirmed_block_info, true, // Update wallet state &result, &error );Also applies to: 110-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/examples/check_transaction.c` around lines 81 - 93, The call sites must be updated to match the corrected wallet_check_transaction signature that now takes an FFIBlockInfo pointer; create an FFIBlockInfo variable (e.g., FFIBlockInfo block_info) and populate its fields for mempool checks (set .source = Mempool, .height = 0, .hash = NULL, .timestamp = 0) and for confirmed checks (set .source = Block, .height = <block_height>, .hash = <block_hash>, .timestamp = <timestamp>), then pass &block_info as the block info argument to wallet_check_transaction instead of separate height/hash/timestamp parameters; update all call sites (the mempool example around wallet_check_transaction and the confirmed-transaction example) to use this pattern and keep passing &result and &error as before.
43-55:⚠️ Potential issue | 🔴 CriticalCritical FFI signature mismatch will cause undefined behavior.
The extern declaration does not match the actual Rust
wallet_check_transactionsignature intransaction.rs:
- Extra
networkparameter (line 45): The Rust function has noFFINetworkparameter.- Block info structure mismatch (lines 49-51): The Rust function expects a single
block_info: FFIBlockInfostruct, but this C code declares three separate parameters (block_height,block_hash,timestamp).- Timestamp type mismatch: The Rust struct uses
u32for timestamp, notu64.This parameter misalignment at the FFI boundary will cause arguments to be interpreted incorrectly, leading to undefined behavior at runtime.
Proposed fix to match actual Rust FFI signature
First, add the
FFIBlockInfostruct definition:+typedef struct { + uint32_t height; + uint8_t block_hash[32]; + uint32_t timestamp; +} FFIBlockInfo; + // External function declarationsThen fix the function declaration:
extern bool wallet_check_transaction( void* wallet, - FFINetwork network, const uint8_t* tx_bytes, size_t tx_len, FFITransactionContextType context_type, - uint32_t block_height, - const uint8_t* block_hash, // 32 bytes if not null - uint64_t timestamp, + FFIBlockInfo block_info, bool update_state, FFITransactionCheckResult* result_out, FFIError* error );Update the call sites accordingly (lines 81-93 and 110-122).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/examples/check_transaction.c` around lines 43 - 55, The extern declaration for wallet_check_transaction is misaligned with the Rust FFI: remove the extra FFINetwork parameter, replace the separate block_height/block_hash/timestamp parameters with a single FFIBlockInfo parameter (define the FFIBlockInfo struct in this C file to match the Rust layout) and change the timestamp field/type to uint32_t to match Rust's u32; keep the remaining parameters (void* wallet, FFITransactionContextType context_type, bool update_state, FFITransactionCheckResult* result_out, FFIError* error) in the same order and update all local call sites that pass block_height/block_hash/timestamp to instead construct and pass an FFIBlockInfo instance when calling wallet_check_transaction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@key-wallet-ffi/examples/check_transaction.c`:
- Around line 81-93: The call sites must be updated to match the corrected
wallet_check_transaction signature that now takes an FFIBlockInfo pointer;
create an FFIBlockInfo variable (e.g., FFIBlockInfo block_info) and populate its
fields for mempool checks (set .source = Mempool, .height = 0, .hash = NULL,
.timestamp = 0) and for confirmed checks (set .source = Block, .height =
<block_height>, .hash = <block_hash>, .timestamp = <timestamp>), then pass
&block_info as the block info argument to wallet_check_transaction instead of
separate height/hash/timestamp parameters; update all call sites (the mempool
example around wallet_check_transaction and the confirmed-transaction example)
to use this pattern and keep passing &result and &error as before.
- Around line 43-55: The extern declaration for wallet_check_transaction is
misaligned with the Rust FFI: remove the extra FFINetwork parameter, replace the
separate block_height/block_hash/timestamp parameters with a single FFIBlockInfo
parameter (define the FFIBlockInfo struct in this C file to match the Rust
layout) and change the timestamp field/type to uint32_t to match Rust's u32;
keep the remaining parameters (void* wallet, FFITransactionContextType
context_type, bool update_state, FFITransactionCheckResult* result_out,
FFIError* error) in the same order and update all local call sites that pass
block_height/block_hash/timestamp to instead construct and pass an FFIBlockInfo
instance when calling wallet_check_transaction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2872d86f-7d70-4928-b918-dd6a34dc7f27
📒 Files selected for processing (1)
key-wallet-ffi/examples/check_transaction.c
QuantumExplorer
approved these changes
Apr 1, 2026
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
2 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.
Summary by CodeRabbit