Skip to content

wallet: Use defined purposes instead of inlining #26858

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

wallet: Use defined purposes instead of inlining #26858

wants to merge 1 commit into from

Conversation

aureleoules
Copy link
Contributor

Based on this comment #26761 (comment).

This PR stores the currently inlined address purposes as constants and use them accordingly.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 9, 2023 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #bitcoin-core/gui/650 (Add Import to Wallet GUI by KolbyML)
  • #27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)
  • #27217 (wallet: Replace use of purpose strings with an enum by achow101)
  • #26840 (refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc by KolbyML)
  • #26836 (wallet: finish addressbook encapsulation by furszy)
  • #26728 (wallet: Have the wallet store the key for automatically generated descriptors by achow101)
  • #26699 (wallet, gui: bugfix, getAvailableBalance skips selected coins by furszy)
  • #26008 (wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets by achow101)
  • #25907 (wallet: rpc to add automatically generated descriptors by achow101)
  • #25620 (wallet: Introduce AddressBookManager by furszy)
  • #25297 (wallet: group independent db writes on single batched db transaction by furszy)
  • #22341 (rpc: add getxpub by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot added the Wallet label Jan 9, 2023
furszy
furszy reviewed Jan 9, 2023
View reviewed changes
@aureleoules aureleoules force-pushed the 2023-01-purpose-refactor branch from 5c71192 to 4510c03 Compare January 9, 2023 16:35
furszy
furszy reviewed Jan 9, 2023
View reviewed changes
@DrahtBot DrahtBot mentioned this pull request Jan 9, 2023
Closed
@achow101
Copy link
Member

achow101 commented Jan 9, 2023

I don't think interfaces/wallet.h is the correct place for these constants to live, better would be perhaps wallet/wallet.h or wallet/walletutil.h. Additionally, there are some places in the GUI code that use the hardcoded strings, these should use the new constants too. The string constants could also be put inside of a AddressBookPurpose (or similar) namespace, in the same way that DBKeys are.

This was referenced Jan 10, 2023
Merged
Closed
Closed
Closed
@aureleoules aureleoules force-pushed the 2023-01-purpose-refactor branch 2 times, most recently from 6845729 to fe167bb Compare January 10, 2023 08:26
@aureleoules
Copy link
Contributor Author

I moved the constants to src/wallet/wallet.h inside the new AddressBookPurposes namespace.

Additionally, there are some places in the GUI code that use the hardcoded strings, these should use the new constants too.

This should be a separate pull request in https://github.com/bitcoin-core/gui once this one is merged?

This was referenced Jan 10, 2023
Closed
Closed
@achow101
Copy link
Member

This should be a separate pull request in https://github.com/bitcoin-core/gui once this one is merged?

I think it's fine to do those at the same time here.

This was referenced Jan 10, 2023
Closed
Closed
@aureleoules aureleoules force-pushed the 2023-01-purpose-refactor branch 2 times, most recently from 0300b5f to 7ed198c Compare January 13, 2023 12:11
This was referenced Jan 13, 2023
Merged
Merged
Merged
@aureleoules aureleoules force-pushed the 2023-01-purpose-refactor branch from 7ed198c to 2670c92 Compare January 17, 2023 12:43
furszy
furszy reviewed Jan 20, 2023
View reviewed changes
Copy link
Member

@furszy furszy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I don't think that we should continue adding wallet.h dependency on any GUI widget/view. It breaks the layers division structure.
The GUI should be as abstracted as possible from the wallet/node internals (this is why all the interfaces and models classes exist).

Would suggest to move the strings to a separate file, or at least to walletutil.h so the required backend dependency is much smaller.
With wallet.h dependency the GUI can access the wallet context (calling the static function GetWallet()), and with it access any wallet directly, breaking the entire requirement of requesting data through the different layers of the system (in other words, could by-pass the need to call the interface/model methods to get backend data which is not desirable for the system architecture).

@DrahtBot DrahtBot mentioned this pull request Feb 21, 2023
Closed
2 tasks
@DrahtBot DrahtBot mentioned this pull request Feb 27, 2023
Merged
@DrahtBot DrahtBot mentioned this pull request Mar 7, 2023
Merged
@DrahtBot DrahtBot mentioned this pull request Mar 21, 2023
Merged
@aureleoules
Copy link
Contributor Author

Closing in favor of #27217.

@aureleoules aureleoules deleted the 2023-01-purpose-refactor branch March 26, 2023 00:12
@bitcoin bitcoin locked and limited conversation to collaborators Mar 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@achow101 achow101 achow101 left review comments

+1 more reviewer

@furszy furszy furszy left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aureleoules @DrahtBot @achow101 @furszy