Skip to content
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.

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

[GUI][Refactoring] Constrain direct wallet access from the GUI in a single place #2293

Merged
merged 12 commits into from
Apr 16, 2021

Conversation

random-zebra
Copy link

First step for the multiwallet implementation.
We have several places in the GUI code, where we freely access pwalletMain.
Such global should be used exclusively in pivx-qt.cpp to initialize the internal pointer of WalletModel. All other calls to the wallet from the GUI should pass through the model.

Bonus: In the process, kill multisend with fire.

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Code reviewed up until 3ffd730 , left few comments.

Other than that, looking good ☕ . One step closer to the multi-process goal :).

src/qt/walletmodel.cpp Outdated Show resolved Hide resolved
src/qt/walletmodel.cpp Outdated Show resolved Hide resolved
src/qt/optionsmodel.cpp Outdated Show resolved Hide resolved
src/qt/walletmodel.cpp Outdated Show resolved Hide resolved
src/qt/pivx/settings/settingswalletoptionswidget.cpp Outdated Show resolved Hide resolved
@random-zebra random-zebra force-pushed the 202104_qt_walletmodel branch 2 times, most recently from 7f71823 to a6b14f6 Compare April 10, 2021 13:32
@furszy
Copy link

furszy commented Apr 11, 2021

Checked c3f6a7d and is looking quite good ☕ .

Have made some changes to it so we can get rid off the friend class declaration furszy@71bec54. Better to not allow private/protected members access/modifications from outside of the object and get a bit better class responsibilities division :).
Feel free to squash it inside that same commit, no need to cherry-pick it.

@random-zebra
Copy link
Author

Squashed @furszy's patch, with a little edit, inside 9638333.
Also rebased on master, fixing minor conflict after #2253 merge.

furszy
furszy previously approved these changes Apr 12, 2021
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

nice work! ☕☕
Code review ACK 9d7e545

Will try it a bit now.

@furszy
Copy link

furszy commented Apr 13, 2021

Needs rebase

`getBalances` is meant to be used only for the internal polling update
process. Now that `processBalanceChangeInternal` has been encapsulated,
there is no more need to expose `getBalances`.
The rest of the GUI should be using `GetWalletBalances`, which returns
cached data.
- encapsulate SST settings, and save only in wallet DB
- emit signal when SST changes (to sync GUI with RPC)
multisend is disabled since long ago, and it won't be re-entroduced in
its current form. Remove all related dead code, including the GUI
widgets, which have also lots of direct accesses to pwalletMain.
@random-zebra
Copy link
Author

Rebased.

random-zebra added a commit to random-zebra/PIVX that referenced this pull request Apr 14, 2021
… inside SaplingOperation

b1e1f4e [Trivial] Initialize wallet pointers to nullptr (random-zebra)
c4853b1 [Refactoring] Remove all pwalletMain direct access in sapling_operation (random-zebra)
80054f7 [Refactoring] Initialize txbuilder inside sapling_operation (random-zebra)
74a1592 [Cleanup] Remove un-used setTxBuilder in sapling_operation (random-zebra)

Pull request description:

  This complements PIVX-Project#2293, trying to achieve the same goal on the sapling code (remove all rogue accesses to `pwalletMain`).
  Save a pointer to a wallet inside `SaplingOperation`, initialized in the constructor, and passed directly to the `TransactionBuilder` (which needs a keystore to produce signatures for the transparent inputs).

ACKs for top commit:
  furszy:
    ACK b1e1f4e
  Fuzzbawls:
    ACK b1e1f4e

Tree-SHA512: 96de28f3b97edee34171c815c5d9f74c826caed025b60dadd6f5ec66ee020f912f61d99d707e95573d5537c2d0151078d8bd06cffff45753b754d927bf8c9fdb
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

re-utACK 012de60 after rebase.

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK 012de60

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] GUI isn't updating changes using the setstakesplitthreshold console command
3 participants