refactor(wallet)!: remove signers and it's APIs, relying on rust-bitcoin's Psbt::sign instead.
#235
Conversation
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
oleonardolima
added
help wanted
oleonardolima
force-pushed
the
refactor/remove-signers
branch
from
6249eac to
b819375
Compare
oleonardolima
force-pushed
the
refactor/remove-signers
branch
2 times, most recently
from
5eccb29 to
e8d9c17
Compare
oleonardolima
force-pushed
the
refactor/remove-signers
branch
6 times, most recently
from
d41948b to
6b60d04
Compare
Pull Request Test Coverage Report for Build 15396776489Details
💛 - Coveralls |
- bump the `bitcoin` dependency to latest `0.32.6`. - this bump is required, in order to use the latest implementation and support of `GetKey` implementation for new `KeyRequest::XOnlyPubKey` when signing tr transactions.
- adds a new `SignerWrapper` for `KeyMap` as a test utility, with new methods: `get_wallet_signer` and `get_wallet_signer_single`. - adds an implementation of `GetKey` trait for `SignerWrapper`, in order to retrieve the private key for software signers, and successfully use `Psbt::sign` method. - the new methods added are necessary, in order to update the existing tests and examples to use `Psbt::sign` instead of `Wallet::sign`, as it further allows the signing APIs and related types to be fully removed.
- updates the existing tests which use `Wallet::sign` API, to get the signer implementation with: `get_wallet_signer_single` or `get_wallet_signer`. - FIXME: there are two tests which requires further refactoring, discussion and update, being: `test_psbt_sign_with_finalized` and `test_psbt_multiple_internalkey_signers`.
- updates `example_wallet_{electrum|esplora}` examples to use the
`SignerWrapper` implementation from test utilities through
`test-utils` feature.
- update examples to parse the used descriptors into a `KeyMap`, and
using `SignerWrapper` to sign the transaction with `Psbt::sign`
method, it still uses the `Wallet::finalize_psbt` though.
- updates the existing tests that uses the `Wallet::sign` API, to gethe signer implementation from test utilities, either: `get_wallet_signer_single` or `get_wallet_signer`. - there are a few tests which needs further refactoring, as they rely on the use of `Wallet::finalize_psbt`.
- use default empty `SignersContainer::new()` on both wallet `create_with_params` and `load_with_params`. - remove `add_signer`, `set_keymap` and `get_signers` methods. - updates `FullyNodedExport::export_wallet` to export public descriptors only, and updates it's tests accordingly. - remove test assertions for signers/keymaps creation/load. - remove both fields `signers` and `change_signers` from `Wallet`. - all usage of signers for `extract_policy` fns in `create_tx` and `policies` methods now rely on the `SignersContainer::default()`.
oleonardolima
force-pushed
the
refactor/remove-signers
branch
from
6b60d04 to
399ad6f
Compare
oleonardolima
changed the title
Psbt::sign instead.Psbt::sign instead.
oleonardolima
requested review from
notmandatory,
thunderbiscuit and
ValuedMammal
|
It's funny I was looking at the diff here when I got the notification that you had requested a review! I was like how does he know I'm looking at the files right now??? haha. Concept ACK. The My only comment is that because this is breaking, even once it's all good to go you might end up having to keep it fresh and rebase for a while because we might not be ready to merge breaking/3.0 features on |
This was referenced Jun 5, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
fixes #70
Description
It's a work in progress to remove signers from
Walletand all related APIs. Users should use their own signer implementation, which needs to implement theGetKeytrait from rust-bitcoin in order to rely on thePsbt::signmethod.Adds a reference implementation of a new
SignerWrappertype as a test utility, which is currently being used on existing tests, instead ofWallet::sign.Notes to the reviewers
Changelog notice
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
Bugfixes: