Conversation
📝 WalkthroughWalkthroughA new signer-based transaction signing method is added to managed wallets. The method validates signer capabilities, prepares unsigned transactions, derives per-input keys from the root extended private key, computes and signs sighashes, and returns the finalized signed transaction with accumulated fees. ChangesSigner-Based Transaction Signing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 #735 +/- ##
=============================================
+ Coverage 71.44% 71.51% +0.06%
=============================================
Files 320 320
Lines 68825 69094 +269
=============================================
+ Hits 49171 49411 +240
- Misses 19654 19683 +29
|
|
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
ZocoLini
changed the title
ZocoLini
force-pushed
the
feature/signer-tx-building
branch
from
1fff39b to
cec967b
Compare
github-actions
Bot
removed
the
merge-conflict
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
You can get rid of let mut address_to_path: HashMap<dashcore::Address, crate::DerivationPath> = HashMap::new(); for pool in managed_account.managed_account_type().address_pools() { for addr_info in pool.addresses.values() { address_to_path.insert(addr_info.address.clone(), addr_info.path.clone()); } }
We already have a lookup with address index /// Reverse lookup: address -> index pub address_index: HashMap<Address, u32>, that is O(1).
Using that + /// Base derivation path for this pool
pub base_path: DerivationPath, you can more easily get the path for any address.
Mirrors `ManagedWalletInfo::build_and_sign_transaction` but delegates every input signature to an external `Signer`, so hosts backing an `ExternalSignable` wallet (hardware/remote signer) never see the underlying private keys. Same input/output and fee-calculation shape as the soft path; signs via pre-computed legacy P2PKH sighashes and requires `SignerMethod::Digest`. Adds 5 unit tests covering: invalid account index, signer without digest support, happy-path end-to-end (UTXO-funded wallet, recipient output present, every input signed), insufficient funds, and network-mismatched output rejection.
ZocoLini
force-pushed
the
feature/signer-tx-building
branch
from
cec967b to
fd1545d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
key-wallet/src/wallet/managed_wallet_info/transaction_building.rs (5)
16-16: 💤 Low valueRedundant inner
use std::collections::HashMap;after module-level import.Now that line 16 imports
HashMapat module scope, the inneruse std::collections::HashMap;insidebuild_and_sign_transaction(line 78) is redundant and can be removed.♻️ Proposed cleanup
// Build a map of address -> derivation path for all addresses in the account - use std::collections::HashMap; let mut address_to_path: HashMap<dashcore::Address, crate::DerivationPath> = HashMap::new();Also applies to: 78-78
🤖 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/src/wallet/managed_wallet_info/transaction_building.rs` at line 16, Remove the redundant inner import of HashMap inside build_and_sign_transaction: since HashMap is already imported at module scope (the use std::collections::HashMap; at top), delete the inner use std::collections::HashMap; statement within the build_and_sign_transaction function to avoid duplicate imports.
181-191: 💤 Low valueFee calculation ordering differs from soft-path; verify intent.
The soft-path computes the fee after
build()(lines 115-119), choosing betweencalculate_fee()andcalculate_fee_with_extra_output()based on the built transaction. The new code computes both fees beforebuild()and then selects one. This is fine if both methods are pure with respect to builder state, but it's wasteful and creates two divergent patterns for the same conceptual operation. Consider mirroring the soft-path style for consistency, or factoring the post-build fee selection into a helper used by both paths during the planned dedup refactor noted in the PR description.🤖 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/src/wallet/managed_wallet_info/transaction_building.rs` around lines 181 - 191, The fee is being precomputed (calling calculate_fee() and calculate_fee_with_extra_output()) before tx_builder.build(), diverging from the soft-path which computes the fee after build; update this branch to mirror the soft-path: call tx_builder.build() first (use the built transaction/transaction.output to decide if an extra output was added) and then compute actual_fee by invoking calculate_fee() or calculate_fee_with_extra_output() as appropriate. Alternatively, extract the post-build fee-selection logic into a helper (e.g., compute_actual_fee_for_built_tx(tx_builder, &transaction)) and use it here and in the soft-path to avoid duplication. Ensure you reference tx_builder, build(), transaction.output, calculate_fee(), and calculate_fee_with_extra_output() when making the change.
152-156: 💤 Low valueReplace
.expect()with proper error propagation.Library code should avoid
.expect()for invariant violations. While the contract ("if change address is Some, account must be Some") is correct, returning a structuredBuilderError(e.g.,InvalidDataor a dedicated variant) keeps the public API panic-free if the invariant is ever violated by future refactors.Note: the existing soft-path on line 42 has the same pattern; if you address it here, consider mirroring the fix there too.
As per coding guidelines: "Avoid
unwrap()andexpect()in library code; use proper error types (e.g.,thiserror)".🤖 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/src/wallet/managed_wallet_info/transaction_building.rs` around lines 152 - 156, Replace the panic-inducing .expect() when fetching an account with proper error propagation: instead of calling .expect on self.accounts.standard_bip44_accounts.get(&account_index) (the code that binds managed_account in transaction_building.rs), return a BuilderError (e.g., BuilderError::InvalidData or a dedicated variant) when the account is missing and map that into the function's Result; update the function signature to propagate the error if necessary. Also mirror the same pattern used at the earlier soft-path (the similar .expect()/unwrap() on line ~42) so both places return the same BuilderError variant rather than panicking. Ensure error messages include context like "missing account for change address" to aid debugging.
700-716: ⚡ Quick winHappy-path test verifies presence of script_sigs but not their validity.
The current assertions only confirm
script_sigis non-empty. A signer implementation that returns garbage bytes (or signs over a wrong digest) would still pass this test. Consider verifying at least one signature against the prev-out script, e.g. by reconstructing the legacy sighash and callingsecp.verify_ecdsa(&msg, &sig, &pubkey)for each input. This catches digest-construction bugs end-to-end and would have caught any future regression in how script_sigs are assembled (signature byte ordering, missing sighash flag byte, wrong pubkey serialization).🤖 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/src/wallet/managed_wallet_info/transaction_building.rs` around lines 700 - 716, The test currently only checks script_sig is non-empty; update the test in transaction_building.rs to actually verify signatures by reconstructing the legacy sighash for each input and validating the signature+pubkey from the input's script_sig against the prev_out script/pubkey using secp.verify_ecdsa (or equivalent) — iterate tx.input, parse script_sig to extract the DER sig and pubkey, parse the previous output script (from the selected UTXO used to build that input), compute the sighash for that input (using the same legacy digest algorithm used in the signing path), create a secp message from that digest, and call secp.verify_ecdsa(&msg, &sig, &pubkey); fail the test if verification fails so signature construction and sighash logic (in the signer and in how script_sigs are assembled) are validated end-to-end.
198-248: 💤 Low valueSighash cache + script_sig mutation ordering is correct.
SighashCache::new(&transaction)holds an immutable borrow only within the inner block (lines 199-245);transaction.input[i].script_sig = script_sigruns only after that scope ends. All legacy SIGHASH_ALL digests are computed against the all-empty-script_sig form of the transaction, which is the correct legacy semantics. Good separation.One small contract to keep in mind: the script_sig builder pushes
pubkey.serialize()(33-byte compressed). TheSignertrait docs require compressed keys, so this is consistent — but it's a silent failure mode if a future signer implementation returns uncompressed: the produced script_sig would not match the input's P2PKH address and the transaction would be rejected at relay. Worth a defensive length check or at least a doc-level reminder when you do the dedup refactor.🤖 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/src/wallet/managed_wallet_info/transaction_building.rs` around lines 198 - 248, Check the pubkey returned by signer.sign_ecdsa(...) for compressed format before building script_sig: after receiving (sig, pubkey) from the Signer (in the loop that uses SighashCache::new(&transaction) and constructs the script via Builder::new()), verify pubkey.serialize().len() == 33 and return a BuilderError::SigningFailed with a clear message if not; this prevents silently creating an invalid P2PKH script_sig when a future Signer returns an uncompressed key. Ensure the error path is used instead of continuing to push the raw pubkey into the script_sig for transaction.input assignments.
🤖 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 `@key-wallet/src/wallet/managed_wallet_info/transaction_building.rs`:
- Line 16: Remove the redundant inner import of HashMap inside
build_and_sign_transaction: since HashMap is already imported at module scope
(the use std::collections::HashMap; at top), delete the inner use
std::collections::HashMap; statement within the build_and_sign_transaction
function to avoid duplicate imports.
- Around line 181-191: The fee is being precomputed (calling calculate_fee() and
calculate_fee_with_extra_output()) before tx_builder.build(), diverging from the
soft-path which computes the fee after build; update this branch to mirror the
soft-path: call tx_builder.build() first (use the built
transaction/transaction.output to decide if an extra output was added) and then
compute actual_fee by invoking calculate_fee() or
calculate_fee_with_extra_output() as appropriate. Alternatively, extract the
post-build fee-selection logic into a helper (e.g.,
compute_actual_fee_for_built_tx(tx_builder, &transaction)) and use it here and
in the soft-path to avoid duplication. Ensure you reference tx_builder, build(),
transaction.output, calculate_fee(), and calculate_fee_with_extra_output() when
making the change.
- Around line 152-156: Replace the panic-inducing .expect() when fetching an
account with proper error propagation: instead of calling .expect on
self.accounts.standard_bip44_accounts.get(&account_index) (the code that binds
managed_account in transaction_building.rs), return a BuilderError (e.g.,
BuilderError::InvalidData or a dedicated variant) when the account is missing
and map that into the function's Result; update the function signature to
propagate the error if necessary. Also mirror the same pattern used at the
earlier soft-path (the similar .expect()/unwrap() on line ~42) so both places
return the same BuilderError variant rather than panicking. Ensure error
messages include context like "missing account for change address" to aid
debugging.
- Around line 700-716: The test currently only checks script_sig is non-empty;
update the test in transaction_building.rs to actually verify signatures by
reconstructing the legacy sighash for each input and validating the
signature+pubkey from the input's script_sig against the prev_out script/pubkey
using secp.verify_ecdsa (or equivalent) — iterate tx.input, parse script_sig to
extract the DER sig and pubkey, parse the previous output script (from the
selected UTXO used to build that input), compute the sighash for that input
(using the same legacy digest algorithm used in the signing path), create a secp
message from that digest, and call secp.verify_ecdsa(&msg, &sig, &pubkey); fail
the test if verification fails so signature construction and sighash logic (in
the signer and in how script_sigs are assembled) are validated end-to-end.
- Around line 198-248: Check the pubkey returned by signer.sign_ecdsa(...) for
compressed format before building script_sig: after receiving (sig, pubkey) from
the Signer (in the loop that uses SighashCache::new(&transaction) and constructs
the script via Builder::new()), verify pubkey.serialize().len() == 33 and return
a BuilderError::SigningFailed with a clear message if not; this prevents
silently creating an invalid P2PKH script_sig when a future Signer returns an
uncompressed key. Ensure the error path is used instead of continuing to push
the raw pubkey into the script_sig for transaction.input assignments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4e3ed70-aced-4eea-9e6f-baeeca4ba133
📒 Files selected for processing (1)
key-wallet/src/wallet/managed_wallet_info/transaction_building.rs
|
@QuantumExplorer I applied your requested change, but I would like to mention one thing, the other transaction aren't using that method either but, right now, as I said, I am refactoring all the copy pasted logic so I will have the |
2 participants
Mirroring the asset_lock_transaction fn that uses a Signer to sign the transaction, I made something similar for standard transactions. Also added similar tests.
I am right now refactoring both transaction functions and the TransactionBuilder struct to avoid this copy pasted logic
Summary by CodeRabbit