Skip to content

refactor(wallet)!: Simplify public_descriptor(), remove redundant function#1503

Merged
notmandatory merged 1 commit intobitcoindevkit:masterfrom
gnapoli23:feat/#1501-simplify-public-descriptor
Jul 8, 2024
Merged

refactor(wallet)!: Simplify public_descriptor(), remove redundant function#1503
notmandatory merged 1 commit intobitcoindevkit:masterfrom
gnapoli23:feat/#1501-simplify-public-descriptor

Conversation

@gnapoli23
Copy link
Copy Markdown
Contributor

Fixes #1501

Description

Simplifies public_descriptor function by using get_descriptor and removes get_descriptor_for_keychain.

Notes to the reviewers

Tested with cargo test --all-features.

Changelog notice

  • Simplifies public_descriptor function and removes get_descriptor_for_keychain

Checklists

All Submissions:

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

@notmandatory notmandatory added api A breaking API change module-wallet labels Jul 5, 2024
@notmandatory notmandatory added this to the 1.0.0-alpha milestone Jul 5, 2024
Copy link
Copy Markdown
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

Looks good, one suggestion for the docs.

Comment thread crates/wallet/src/wallet/mod.rs Outdated
@notmandatory
Copy link
Copy Markdown
Member

One final thing, you need to setup a github signing key and squash and sign your commit.
see: https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

@gnapoli23
Copy link
Copy Markdown
Contributor Author

One final thing, you need to setup a github signing key and squash and sign your commit. see: https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

Sure, I thought I signed it with my yubikey but I just pushed it without signing. Going to fix in a brief.

@gnapoli23 gnapoli23 force-pushed the feat/#1501-simplify-public-descriptor branch 2 times, most recently from 4f11928 to 5668d3e Compare July 5, 2024 22:56
@gnapoli23
Copy link
Copy Markdown
Contributor Author

Done. @notmandatory

@notmandatory
Copy link
Copy Markdown
Member

notmandatory commented Jul 5, 2024

I just realized you also should update the commit message to conform to https://www.conventionalcommits.org/en/v1.0.0/.

We don't usually put github issue numbers in commit messages, so something like this:

refactor(wallet)!: remove redundant Wallet::get_descriptor_for_keychain()

Simplify Wallet::public_descriptor() and update Wallet internals to use public_descriptor() instead of get_descriptor_for_keychain().

@gnapoli23 gnapoli23 force-pushed the feat/#1501-simplify-public-descriptor branch from 5668d3e to 0fed668 Compare July 6, 2024 08:00
@gnapoli23
Copy link
Copy Markdown
Contributor Author

@notmandatory Updated following your suggestions, please let me know if something else needs to be fixed. Thanks.

@storopoli
Copy link
Copy Markdown

The commit message is still too wide, 50 chars for title and 72 for body:

Suggestion:

refactor(wallet)!: remove redundant get_descriptor_for_keychain

Simplify Wallet::public_descriptor() and update Wallet internals to use
public_descriptor() instead of get_descriptor_for_keychain().

Simplify Wallet::public_descriptor() and update Wallet internals to use
public_descriptor() instead of get_descriptor_for_keychain().
@gnapoli23 gnapoli23 force-pushed the feat/#1501-simplify-public-descriptor branch from 0fed668 to e7ec5a8 Compare July 6, 2024 11:59
@gnapoli23
Copy link
Copy Markdown
Contributor Author

Ok, please check now. @notmandatory @storopoli

Copy link
Copy Markdown
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK e7ec5a8

Thanks for getting this one wrapped up.

Copy link
Copy Markdown

@storopoli storopoli left a comment

Choose a reason for hiding this comment

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

ACK e7ec5a8

@gnapoli23
Copy link
Copy Markdown
Contributor Author

Thanks to both of you @notmandatory @storopoli . Can it be merged now?

@notmandatory notmandatory merged commit 8714e9d into bitcoindevkit:master Jul 8, 2024
@notmandatory notmandatory mentioned this pull request Jul 20, 2024
30 tasks
@notmandatory notmandatory changed the title feat(wallet): simplify public_descriptor fn and remove `get_descriptor_for_keych… refactor(wallet)!: Remove redundant function, simplify public_descriptor() Jul 20, 2024
@notmandatory notmandatory changed the title refactor(wallet)!: Remove redundant function, simplify public_descriptor() refactor(wallet)!: Simplify public_descriptor(), remove redundant function. Jul 20, 2024
@notmandatory notmandatory changed the title refactor(wallet)!: Simplify public_descriptor(), remove redundant function. refactor(wallet)!: Simplify public_descriptor(), remove redundant function Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api A breaking API change module-wallet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Simplify public_descriptor by using get_descriptor and remove redundant API function.

3 participants