feat(key-wallet): add async Signer trait and build_asset_lock_with_signer#661
Conversation
…gner Adds a Signer trait for delegating signing to external devices (hardware wallets, remote signers) backing a WalletType::ExternalSignable wallet. Introduces ManagedWalletInfo::build_asset_lock_with_signer, an async sibling of build_asset_lock that never touches the private keys: P2PKH input signatures and credit-output public keys are both obtained from the Signer. AssetLockResult.keys becomes an enum so both paths share the same result shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an external signer abstraction and integrates it into asset-lock construction. Introduces signer-capability enums and an async Changes
Sequence DiagramsequenceDiagram
participant Client
participant Wallet
participant Builder as AssetLockBuilder
participant Account as ManagedAccount
participant Signer as ExternalSigner
Client->>Wallet: build_asset_lock_with_signer(account, outputs, signer)
Wallet->>Builder: build tx skeleton (no input keys)
Builder->>Builder: assemble inputs & outputs
loop per credit output
Builder->>Account: peek_next_path()
Account-->>Builder: derivation path (not consumed)
Builder->>Builder: compute P2PKH sighash for input
Builder->>Signer: sign_ecdsa(path, sighash)
Signer-->>Builder: signature
Builder->>Builder: set script_sig for input
Builder->>Signer: public_key(path)
Signer-->>Builder: public_key
Builder->>Account: mark_first_pool_index_used(index)
Builder->>Builder: record (public_key, path)
end
Builder-->>Wallet: AssetLockResult { keys: Public([...]) }
Wallet-->>Client: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
Adds SignerMethod (Digest | Transaction(category)) and TransactionCategory (Classical | PlatformCredits | MasternodeLifecycle) so signers can advertise what they can actually sign. Hardware wallets that need the full transaction for on-device display cannot safely sign blind digests, and callers have to gate on capability rather than assume every signer speaks every method. The Signer trait gains supported_methods() + a default supports() convenience. build_asset_lock_with_signer — which drives signing via pre-computed P2PKH sighashes — now rejects signers that don't advertise SignerMethod::Digest before touching any UTXO state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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
🧹 Nitpick comments (2)
key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs (1)
582-746: Add a happy-path signer test.All four new
#[tokio::test]s cover error branches (empty outputs / bad account / no digest / insufficient funds) but none exercises a successful build. Without it, regressions in the sighash loop, the script_sig assembly, or theAssetLockCreditKeys::Publicordering (one entry percredit_output_fundingsin input order) won't be caught.
InMemorySigneris already capable of full end-to-end signing — a test that funds the BIP44 account with a mock UTXO and asserts:
result.keysisAssetLockCreditKeys::Public(v)withv.len() == credit_output_fundings.len(),result.transaction.output[0]is the OP_RETURN burn for the summed credit amount,- each input has a non-empty
script_sig,would close the coverage gap cheaply.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs` around lines 582 - 746, Add a new happy-path #[tokio::test] that uses test_wallet_and_info to get (wallet, mut info), creates an InMemorySigner from the wallet's RootExtendedPrivKey and Network::Testnet, calls info.build_asset_lock_with_signer(&wallet, 0, test_credit_outputs(&[amounts...]), fee, &signer).await and assert the call is Ok(result); verify result.keys matches AssetLockCreditKeys::Public(v) and v.len() == credit_output_fundings.len(); compute the summed credit amount and assert result.transaction.output[0] is the OP_RETURN burn for that sum; and assert every input in result.transaction.input has a non-empty script_sig. Use the existing helpers InMemorySigner, build_asset_lock_with_signer, test_credit_outputs, and test_wallet_and_info to implement the test.key-wallet/src/signer.rs (1)
73-75: Consider addingSend + Syncsupertrait bounds onSigner.Methods
sign_ecdsaandpublic_keytake&selfand are called withinbuild_asset_lock_with_signer(lines 451–454 and 485–488) across.awaitpoints. This requiresS: Syncat the call site. Since#[async_trait]generatesSendfutures by default, implementors effectively need both bounds anyway — making them explicit at the trait level documents the contract for downstream hardware-wallet and remote-signer implementations, providing a clearer compile-time signal instead of a cryptic "future is not Sync" error.♻️ Proposed bound
#[async_trait] -pub trait Signer { +pub trait Signer: Send + Sync { /// Error produced by the underlying signing device or service. type Error: std::fmt::Display + Send + Sync + 'static;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/signer.rs` around lines 73 - 75, The Signer trait should require Send + Sync so implementations can be referenced across await points; update the trait declaration for Signer to include the supertrait bounds Send + Sync (i.e., change "pub trait Signer" to "pub trait Signer: Send + Sync") so that implementors like hardware-wallet and remote-signer satisfy the Sync requirement used when calling Signer::sign_ecdsa and Signer::public_key from build_asset_lock_with_signer across await points.
🤖 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/src/wallet/managed_wallet_info/asset_lock_builder.rs`:
- Around line 473-490: The loop calls funding_key_account.next_path() which
consumes/advances the funding pool index before awaiting
signer.public_key(&path), so if signer.public_key fails mid-loop earlier indices
become irreversibly consumed; to fix, change the flow in the credit-output
collection: first gather non-consuming path "peeks" for each funding (introduce
or use a peek method such as next_path_peek or similar on the account returned
by resolve_funding_account) and collect the paths, then call signer.public_key
for all paths and only after all signer calls succeed commit/mark the indices
used (e.g., call a commit_path/mark_index_used method on the funding account or
an explicit finalize step); alternatively, if adding peek/commit is
unacceptable, update docs/comments near
next_path/public_key/resolve_funding_account to explicitly warn callers that
next_path can consume indices on failure so retries may leave gaps.
---
Nitpick comments:
In `@key-wallet/src/signer.rs`:
- Around line 73-75: The Signer trait should require Send + Sync so
implementations can be referenced across await points; update the trait
declaration for Signer to include the supertrait bounds Send + Sync (i.e.,
change "pub trait Signer" to "pub trait Signer: Send + Sync") so that
implementors like hardware-wallet and remote-signer satisfy the Sync requirement
used when calling Signer::sign_ecdsa and Signer::public_key from
build_asset_lock_with_signer across await points.
In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs`:
- Around line 582-746: Add a new happy-path #[tokio::test] that uses
test_wallet_and_info to get (wallet, mut info), creates an InMemorySigner from
the wallet's RootExtendedPrivKey and Network::Testnet, calls
info.build_asset_lock_with_signer(&wallet, 0,
test_credit_outputs(&[amounts...]), fee, &signer).await and assert the call is
Ok(result); verify result.keys matches AssetLockCreditKeys::Public(v) and
v.len() == credit_output_fundings.len(); compute the summed credit amount and
assert result.transaction.output[0] is the OP_RETURN burn for that sum; and
assert every input in result.transaction.input has a non-empty script_sig. Use
the existing helpers InMemorySigner, build_asset_lock_with_signer,
test_credit_outputs, and test_wallet_and_info to implement the test.
🪄 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: a758a237-997e-4efd-8bc8-025730899db4
📒 Files selected for processing (5)
key-wallet-ffi/src/transaction.rskey-wallet/src/lib.rskey-wallet/src/managed_account/mod.rskey-wallet/src/signer.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #661 +/- ##
=============================================
+ Coverage 68.12% 68.39% +0.26%
=============================================
Files 319 320 +1
Lines 67660 68023 +363
=============================================
+ Hits 46093 46522 +429
+ Misses 21567 21501 -66
|
Three changes from PR review on #661: - Signer: Send + Sync supertrait bounds. Callers use &signer methods across .await, so implementors effectively need both already — surfacing the requirement on the trait turns a cryptic "future is not Send/Sync" into an up-front bound violation. - ManagedCoreAccount gains peek_next_path / mark_first_pool_index_used, and build_asset_lock_with_signer's credit-output loop becomes peek -> signer.public_key().await -> commit. A signer failure mid- loop no longer leaks pool indices — the current funding's index stays free for a retry, and earlier iterations only advanced their pools because their signer calls already succeeded. next_path stays as a convenience wrapper around peek + mark. - New happy-path #[tokio::test]: fund a wallet account with a real UTXO at a real pool address, run build_asset_lock_with_signer end to end, assert the result carries Public(v) with v.len() == fundings.len(), assert tx.output[0] is the OP_RETURN burn for the summed credit amount, and assert every input received a non-empty script_sig from the in-memory signer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
key-wallet/src/signer.rs (1)
28-70: Make signer capability enums non-exhaustive before publishing this API.These public enums model signer/firmware capabilities and DIP transaction categories, so they are likely to grow. Without
#[non_exhaustive], adding a future method/category becomes a downstream source-breaking change for exhaustive matches.♻️ Proposed public API hardening
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +#[non_exhaustive] pub enum SignerMethod { @@ #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +#[non_exhaustive] pub enum TransactionCategory {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/signer.rs` around lines 28 - 70, Add the non_exhaustive attribute to the public enums to avoid downstream breakage when new variants are added: annotate both SignerMethod and TransactionCategory with #[non_exhaustive] so consumers cannot exhaustively match them (forcing wildcard arms) and preserve forward compatibility; update any local exhaustive matches in code you control (e.g., match arms on SignerMethod and TransactionCategory) to include a catch-all (_) where necessary or handle the compiler guidance after adding the attribute.
🤖 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/src/signer.rs`:
- Around line 83-84: The proposed change that tightens Signer::Error to require
std::error::Error is unsound for current implementations (InMemorySigner and
NoDigestSigner use String), so revert that bound change in the Signer trait
(restore type Error: std::fmt::Display + Send + Sync + 'static in signer.rs) and
instead add a short doc comment on the Signer trait/type Error recommending
implementors provide types that implement std::error::Error (or use thiserror)
for richer error chaining; optionally keep the existing defensive conversion
sites (e.g., AssetLockError::Signer(...).map_err(|e| e.to_string())) unchanged
so String-based errors remain compatible.
---
Nitpick comments:
In `@key-wallet/src/signer.rs`:
- Around line 28-70: Add the non_exhaustive attribute to the public enums to
avoid downstream breakage when new variants are added: annotate both
SignerMethod and TransactionCategory with #[non_exhaustive] so consumers cannot
exhaustively match them (forcing wildcard arms) and preserve forward
compatibility; update any local exhaustive matches in code you control (e.g.,
match arms on SignerMethod and TransactionCategory) to include a catch-all (_)
where necessary or handle the compiler guidance after adding the attribute.
🪄 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: 9e404e86-2435-4f5e-a048-19a7b9ec58ba
📒 Files selected for processing (3)
key-wallet/src/managed_account/mod.rskey-wallet/src/signer.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- key-wallet/src/managed_account/mod.rs
- key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs
Per CodeRabbit review: the Error associated type bound is intentionally the looser `Display + Send + Sync + 'static` so `String` and other Display-only types work out of the box, but implementors benefit from choosing a type that also implements std::error::Error for richer error chaining. Callers in this crate collapse the signer error to a String via Display and work with either shape.
Brings in the async Signer trait and build_asset_lock_with_signer entry point (from PR #661) so this working branch has signer support while #661 reviews. Conflict resolution: kept the Signer-path imports in asset_lock_builder.rs, and adapted the merged code to this branch's 1-arg signatures for next_receive_address / next_change_address / next_unused_with_info. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
I had a bit a chat with claude becaue i was worried about the missing gap limit maintainance. It might not be a problem but please investigate quickly before merging:
Small thing worth flagging on the address-pool bookkeeping. mark_first_pool_index_used only flips the used flag, and peek_next_path calls next_unused_with_info with NoKeySource + add_to_state: false, so it can
only return an already-generated index. A funding pool initialized with gap_limit = 20 therefore lets you build exactly 20 asset-locks before peek 21 returns Err(Error::NoKeySource) and the builder errors with
"No unused address available". Loud failure, not silent — no missed transactions, the 20 generated addresses stay watched. Same mechanics as the existing soft-wallet next_private_key path, so this PR inherits
rather than introduces the sharp edge.
In practice the ceiling may not bite, because wallet_checker.rs:140-165 marks every involved address used on a matched tx and then calls maintain_gap_limit(KeySource::Public(xpub)) on every pool of that account.
So once an asset-lock confirms and the checker sees it, the pool self-heals. Only open question is whether all_involved_addresses() enumerates the credit-output scripts inside the AssetLockPayload or just
tx.output/tx.input — credit outputs live only in the payload (tx.output is just the OP_RETURN burn), so if the classifier skips the payload, the auto-heal path never runs and the ceiling is real.
Also worth noting the standard "gap limit protects sync from missing external payments" argument doesn't apply to these account types — nobody hands out identity-funding / credit-output addresses as receive
addresses, so extending the pool here is only about not locking the wallet out of further builds.
|
Might be an issue. If so we'll solve it later. There will be a lot of tests in the end. |
2 participants
Summary
Signertrait (key-wallet/src/signer.rs) for delegating ECDSA signing and public-key lookup to external devices — hardware wallets, remote signers, anything backing aWalletType::ExternalSignablewallet. Every call takes only aDerivationPathand pre-computed sighash; private keys never cross the host boundary.ManagedWalletInfo::build_asset_lock_with_signer<S: Signer>, an async sibling ofbuild_asset_lockthat builds the unsigned tx viaTransactionBuilder, computes per-input legacy P2PKH sighashes, delegates every signature to theSigner, and records credit-output bookkeeping as(PublicKey, DerivationPath)pairs pulled from the same signer.AssetLockResult.keysinto the newAssetLockCreditKeysenum (Private(Vec<[u8;32]>)for soft wallets,Public(Vec<(PublicKey, DerivationPath)>)for signer-backed wallets) so both paths share the same result type.ManagedCoreAccount::next_path()— a key-free sibling ofnext_private_keythat advances the funding-account pool and returns only the derivation path, used by the signer path for credit-output bookkeeping.AssetLockResult.keysconsumer inkey-wallet-ffito destructure the new enum (the soft-wallet FFI entry point only accepts thePrivatevariant).Non-goals:
TransactionBuilderitself is not signer-aware yet; the signer path lives entirely inbuild_asset_lock_with_signer.WalletType::ExternalSignabledoesn't yet carry the signer — callers supply it per-build.Test plan
cargo test -p key-wallet --lib— 457 existing tests pass plus 3 new async signer tests (empty outputs rejected, invalid account index, insufficient funds), using an in-memorySignerimpl backed by a real root xpriv.cargo clippy -p key-wallet --all-targets -- -D warnings— clean.cargo build -p key-wallet-ffi— builds with the updated enum destructure.Signerimpl (out of scope for this PR).🤖 Generated with Claude Code
Summary by CodeRabbit