wallet: Use defined purposes instead of inlining #26858

aureleoules commented at 4:30 PM on January 9, 2023: member

Based on this comment #26761 (review).

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

DrahtBot commented at 4:30 PM on January 9, 2023: contributor

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 added the label Wallet on Jan 9, 2023

in src/interfaces/wallet.h:44 in 5c7119211d outdated

  40 | @@ -41,6 +41,11 @@ struct WalletContext;
  41 |  using isminefilter = std::underlying_type<isminetype>::type;
  42 |  } // namespace wallet
  43 |  
  44 | +static const std::string PURPOSE_CHANGE = "change";

furszy commented at 4:31 PM on January 9, 2023:

there isn't a "change" purpose.

aureleoules commented at 4:35 PM on January 9, 2023:

Whoops, thanks pushed.

aureleoules force-pushed on Jan 9, 2023

in src/wallet/spend.h:9 in 4510c03a09 outdated

   5 | @@ -6,6 +6,7 @@
   6 |  #define BITCOIN_WALLET_SPEND_H
   7 |  
   8 |  #include <consensus/amount.h>
   9 | +#include <interfaces/wallet.h>

furszy commented at 4:50 PM on January 9, 2023:

This introduces a circular dependency,

interfaces/wallet.h is implemented by wallet/interfaces.cpp which includes wallet/spend.h which, with this change, includes interfaces/wallet.h.

Thus why suggested to decouple it into a separate file (should check if we already have a file that serves this purpose).

achow101 commented at 9:28 PM on January 9, 2023:

wallet/spend.h doesn't even need to include this, so it could just be removed from there and not have any issues.

DrahtBot cross-referenced this on Jan 9, 2023 from issue refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc by KolbyML

achow101 commented at 9:49 PM on January 9, 2023: member

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.

DrahtBot cross-referenced this on Jan 10, 2023 from issue wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets by achow101

DrahtBot cross-referenced this on Jan 10, 2023 from issue wallet: Introduce AddressBookManager by furszy

DrahtBot cross-referenced this on Jan 10, 2023 from issue [Draft / POC] Silent Payments by w0xlt

DrahtBot cross-referenced this on Jan 10, 2023 from issue RPC/Wallet: Add "use_txids" to output of getaddressinfo by luke-jr

aureleoules force-pushed on Jan 10, 2023

aureleoules commented at 8:28 AM on January 10, 2023: member

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?

DrahtBot cross-referenced this on Jan 10, 2023 from issue wallet: Have the wallet store the key for automatically generated descriptors by achow101

DrahtBot cross-referenced this on Jan 10, 2023 from issue wallet: rpc to add automatically generated descriptors by achow101

achow101 commented at 10:43 PM on January 10, 2023: 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.

DrahtBot cross-referenced this on Jan 10, 2023 from issue wallet: group independent db writes on single batched db transaction by furszy

DrahtBot cross-referenced this on Jan 11, 2023 from issue rpc: add path to gethdkey by Sjors

aureleoules force-pushed on Jan 13, 2023

DrahtBot cross-referenced this on Jan 13, 2023 from issue clang-tidy: Force checks for headers in `src/qt` by hebasto

DrahtBot cross-referenced this on Jan 13, 2023 from issue clang-tidy: Fix `modernize-use-default-member-init` in headers and force to check all headers by hebasto

DrahtBot cross-referenced this on Jan 13, 2023 from issue wallet, gui: bugfix, getAvailableBalance skips selected coins by furszy

DrahtBot added the label Needs rebase on Jan 17, 2023

wallet: Use defined purposes instead of inlining 2670c92ede

aureleoules force-pushed on Jan 17, 2023

DrahtBot removed the label Needs rebase on Jan 17, 2023

furszy commented at 2:39 PM on January 20, 2023: member

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 cross-referenced this on Feb 21, 2023 from issue Add Import to Wallet GUI by KolbyML

DrahtBot cross-referenced this on Feb 27, 2023 from issue wallet: batch and simplify addressbook migration process by furszy

DrahtBot cross-referenced this on Mar 7, 2023 from issue wallet: Replace use of purpose strings with an enum by achow101

DrahtBot cross-referenced this on Mar 21, 2023 from issue wallet: Keep track of the wallet's own transaction outputs in memory by achow101

aureleoules commented at 12:12 AM on March 26, 2023: member

Closing in favor of #27217.

aureleoules closed this on Mar 26, 2023

aureleoules deleted the branch on Mar 26, 2023

bitcoin locked this on Mar 25, 2024