wallet: Use defined purposes instead of inlining #26858

pull aureleoules wants to merge 1 commits into bitcoin:master from aureleoules:2023-01-purpose-refactor changing 10 files +38 −22

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.
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.
aureleoules force-pushed on Jan 10, 2023
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?
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.
aureleoules force-pushed on Jan 13, 2023
aureleoules force-pushed on Jan 13, 2023
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).
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

Contributors

Labels

Linked (view graph)

#26761 wallet: fully migrate address book entries for watchonly/solvable wallets

wallet: Use defined purposes instead of inlining #26858

Reviews

Conflicts