Skip to content

fix(wallet): fix building change signers in load_with_params#1662

Merged
evanlinjin merged 2 commits intobitcoindevkit:masterfrom
ValuedMammal:fix/wallet-change-signers
Oct 29, 2024
Merged

fix(wallet): fix building change signers in load_with_params#1662
evanlinjin merged 2 commits intobitcoindevkit:masterfrom
ValuedMammal:fix/wallet-change-signers

Conversation

@ValuedMammal
Copy link
Copy Markdown
Collaborator

@ValuedMammal ValuedMammal commented Oct 28, 2024

This fixes an issue that prevented change signers from being built when using the keymap method on LoadParams. Now we always build a SignersContainer with the internal keymap whenever a change descriptor is loaded. Note, the signer may still be "empty" if neither keymap or extract_keys is used.

Notes to the reviewers

Building a SignersContainer now happens outside of the match statement in load_with_params which simplifies the logic such that we can interpret this area of code as "initialize a value for change_descriptor and optionally extend the internal keymap, returning a LoadMismatch error if the changeset doesn't match the expected params."

Changelog notice

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

Bugfixes:

  • I've added tests to reproduce the issue which are now passing

This fixes an issue that prevented change signers from being built
when using the `keymap` method on `LoadParams`. Now we always
build a `SignersContainer` with the internal keymap whenever a
change descriptor is loaded, although the signer may still be
empty if neither `keymap` nor `extract_keys` is used.
@ValuedMammal ValuedMammal self-assigned this Oct 28, 2024
@ValuedMammal ValuedMammal added this to the 1.0.0-beta milestone Oct 28, 2024
@ValuedMammal ValuedMammal added the bug Something isn't working label Oct 28, 2024
Copy link
Copy Markdown
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b6b767f

This was my oversight. Thanks for fixing this.

@evanlinjin evanlinjin merged commit 24097d7 into bitcoindevkit:master Oct 29, 2024
@ValuedMammal ValuedMammal deleted the fix/wallet-change-signers branch October 29, 2024 14:18
@notmandatory notmandatory mentioned this pull request Dec 11, 2024
32 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants