refactor: move core tx building and signing logic into the key-wallet-manager crate#727
refactor: move core tx building and signing logic into the key-wallet-manager crate#727
Conversation
📝 WalkthroughWalkthroughThe transaction building logic is being refactored from the FFI layer into the ChangesTransaction Building Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
key-wallet-ffi/src/transaction.rs (1)
134-156:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Return tuple not destructured;
fee_outis never written.
manager.build_and_sign_transactionreturnsResult<(Transaction, u64), WalletError>. The tuple is assigned totransaction, but thenconsensus::serialize(&transaction)attempts to serialize the tuple(Transaction, u64)instead of just theTransaction. Additionally, thefee_outparameter (documented to receive the fee on success) is never written.Proposed fix
- let transaction = unwrap_or_return!( + let (transaction, fee) = unwrap_or_return!( manager.build_and_sign_transaction( &wallet_id, account_index, outputs, FeeRate::new(fee_per_kb) ), error ); // Serialize the transaction let serialized = consensus::serialize(&transaction); let size = serialized.len(); let boxed = serialized.into_boxed_slice(); let tx_bytes = Box::into_raw(boxed) as *mut u8; *tx_bytes_out = tx_bytes; *tx_len_out = size; + *fee_out = fee; (*error).clean(); true🤖 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 `@key-wallet-ffi/src/transaction.rs` around lines 134 - 156, The code incorrectly treats the Result from manager.build_and_sign_transaction as a single Transaction; destructure the result into (transaction, fee) (e.g., let (transaction, fee) = unwrap_or_return!(manager.build_and_sign_transaction(...), error);), then call consensus::serialize(&transaction) (not the tuple), set *fee_out = fee (convert to expected type if necessary) before cleaning the error, and keep assignments to *tx_bytes_out and *tx_len_out as-is so tx/len and fee are all written on success.
🧹 Nitpick comments (4)
key-wallet-manager/src/error.rs (1)
102-106: 💤 Low valueConsider removing the redundant prefix in the error conversion.
The
Displayimpl forWalletError::TransactionBuild(line 57) already prefixes messages with"Transaction build failed: ". This conversion adds another prefix"Transaction building error: ", resulting in messages like"Transaction build failed: Transaction building error: <actual error>".Suggested fix
impl From<BuilderError> for WalletError { fn from(err: BuilderError) -> Self { - WalletError::TransactionBuild(format!("Transaction building error: {}", err)) + WalletError::TransactionBuild(err.to_string()) } }🤖 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 `@key-wallet-manager/src/error.rs` around lines 102 - 106, The From<BuilderError> for WalletError impl is adding a redundant prefix; update the impl for From<BuilderError> so it constructs WalletError::TransactionBuild using only the BuilderError message (e.g., err.to_string()) instead of prepending "Transaction building error: ", so the existing Display for WalletError::TransactionBuild remains the sole prefix provider.key-wallet-ffi/src/transaction.rs (1)
130-131: 💤 Low valueRedundant nested
unsafeblock.The function is already declared
unsafe extern "C", so the innerunsafeblock at line 130 is unnecessary.Suggested simplification
- unsafe { - manager_ref.runtime.block_on(async { + manager_ref.runtime.block_on(async { // ... function body ... - }) - } + })🤖 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 `@key-wallet-ffi/src/transaction.rs` around lines 130 - 131, The inner unsafe block around the call to manager_ref.runtime.block_on(async { ... }) is redundant because the enclosing function is already declared unsafe extern "C"; remove that inner `unsafe { ... }` wrapper and directly call `manager_ref.runtime.block_on(async { ... })` (preserving the async closure body) so the function remains unsafe but without nested unsafe blocks; locate this in the function containing the `manager_ref.runtime.block_on` call in transaction.rs.key-wallet-manager/src/lib.rs (2)
957-960: 💤 Low valueMove
HashMapimport to the top of the file.The
use std::collections::HashMapstatement inside the function body is unconventional. While it works, placing imports at module scope improves readability and follows Rust conventions.🤖 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 `@key-wallet-manager/src/lib.rs` around lines 957 - 960, The local use statement for HashMap should be moved to module scope: remove the inline `use std::collections::HashMap;` inside the function and add `use std::collections::HashMap;` with the other top-level imports so the code that creates `let mut address_to_path: HashMap<dashcore::Address, key_wallet::DerivationPath> = HashMap::new();` remains unchanged; this keeps imports conventional and improves readability.
897-904: 💤 Low valueMethod only supports BIP44 accounts; consider documenting this limitation or accepting account type preference.
The method hardcodes
AccountTypePreference::BIP44for change address (line 907) and only queriesstandard_bip44_accounts(line 920). Users with BIP32-only accounts cannot use this method.If this is intentional, consider adding documentation. If broader support is needed, the method signature could accept an
AccountTypePreferenceparameter.🤖 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 `@key-wallet-manager/src/lib.rs` around lines 897 - 904, The build_and_sign_transaction method currently forces BIP44-only flow (AccountTypePreference::BIP44 and standard_bip44_accounts), which excludes BIP32-only wallets; either document this limitation on WalletManager::build_and_sign_transaction or make it accept an AccountTypePreference parameter and branch accordingly: add an account_type_pref: AccountTypePreference argument to build_and_sign_transaction, use that value instead of AccountTypePreference::BIP44 when requesting a change address, and replace queries to standard_bip44_accounts with the account list lookup that matches the chosen AccountTypePreference (and update any helper calls that enumerate accounts or derive change addresses), and update all call sites to pass the desired preference.
🤖 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.
Inline comments:
In `@key-wallet-manager/src/lib.rs`:
- Around line 969-985: The closure passed to tx_builder.select_inputs currently
calls secp256k1::Secp256k1::new() for every UTXO which is expensive; create a
single Secp256k1 instance outside the closure (e.g., let secp =
secp256k1::Secp256k1::new();) and capture/reuse it inside the closure, leaving
the rest of the logic (address_to_path.get(&utxo.address),
root_xpriv.to_extended_priv_key(wallet.network), derived_xpriv =
root_ext_priv.derive_priv(&secp, path).ok() and returning
Some(derived_xpriv.private_key)) unchanged.
---
Outside diff comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 134-156: The code incorrectly treats the Result from
manager.build_and_sign_transaction as a single Transaction; destructure the
result into (transaction, fee) (e.g., let (transaction, fee) =
unwrap_or_return!(manager.build_and_sign_transaction(...), error);), then call
consensus::serialize(&transaction) (not the tuple), set *fee_out = fee (convert
to expected type if necessary) before cleaning the error, and keep assignments
to *tx_bytes_out and *tx_len_out as-is so tx/len and fee are all written on
success.
---
Nitpick comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 130-131: The inner unsafe block around the call to
manager_ref.runtime.block_on(async { ... }) is redundant because the enclosing
function is already declared unsafe extern "C"; remove that inner `unsafe { ...
}` wrapper and directly call `manager_ref.runtime.block_on(async { ... })`
(preserving the async closure body) so the function remains unsafe but without
nested unsafe blocks; locate this in the function containing the
`manager_ref.runtime.block_on` call in transaction.rs.
In `@key-wallet-manager/src/error.rs`:
- Around line 102-106: The From<BuilderError> for WalletError impl is adding a
redundant prefix; update the impl for From<BuilderError> so it constructs
WalletError::TransactionBuild using only the BuilderError message (e.g.,
err.to_string()) instead of prepending "Transaction building error: ", so the
existing Display for WalletError::TransactionBuild remains the sole prefix
provider.
In `@key-wallet-manager/src/lib.rs`:
- Around line 957-960: The local use statement for HashMap should be moved to
module scope: remove the inline `use std::collections::HashMap;` inside the
function and add `use std::collections::HashMap;` with the other top-level
imports so the code that creates `let mut address_to_path:
HashMap<dashcore::Address, key_wallet::DerivationPath> = HashMap::new();`
remains unchanged; this keeps imports conventional and improves readability.
- Around line 897-904: The build_and_sign_transaction method currently forces
BIP44-only flow (AccountTypePreference::BIP44 and standard_bip44_accounts),
which excludes BIP32-only wallets; either document this limitation on
WalletManager::build_and_sign_transaction or make it accept an
AccountTypePreference parameter and branch accordingly: add an
account_type_pref: AccountTypePreference argument to build_and_sign_transaction,
use that value instead of AccountTypePreference::BIP44 when requesting a change
address, and replace queries to standard_bip44_accounts with the account list
lookup that matches the chosen AccountTypePreference (and update any helper
calls that enumerate accounts or derive change addresses), and update all call
sites to pass the desired preference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f78ef05-f68e-4877-b2a5-173fca126d46
📒 Files selected for processing (3)
key-wallet-ffi/src/transaction.rskey-wallet-manager/src/error.rskey-wallet-manager/src/lib.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #727 +/- ##
=============================================
- Coverage 70.85% 70.82% -0.03%
=============================================
Files 319 319
Lines 68184 68207 +23
=============================================
- Hits 48313 48309 -4
- Misses 19871 19898 +27
|
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
It should be in key wallet and not key wallet manager. (Maybe a convenience method in key wallet manager though).
|
But there is no struct in key-wallet that knows Wallet and ManagedWalletInfo at the same time, only the Wallet manager does |
| // Get the wallet's root extended private key for signing | ||
| let root_xpriv = match &wallet.wallet_type { | ||
| WalletType::Mnemonic { | ||
| root_extended_private_key, | ||
| .. | ||
| } => root_extended_private_key, | ||
| WalletType::Seed { | ||
| root_extended_private_key, | ||
| .. | ||
| } => root_extended_private_key, | ||
| WalletType::ExtendedPrivKey(root_extended_private_key) => root_extended_private_key, | ||
| _ => { | ||
| return Err(WalletError::TransactionBuild( | ||
| "Cannot sign with watch-only wallet".to_string(), | ||
| )); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The ios wallet is external signable, so this is incorrect.
2 participants
I want this logic accesible to all rust consumers (eg: platform-wallet), it doesn't make sense at all to have logic in the FFI side
Summary by CodeRabbit