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

[Refactor] Clean up wallet related code #200

Merged
merged 8 commits into from
Sep 21, 2023

Conversation

panleone
Copy link
Member

Abstract

This is a continuation of the work started in #197 main changes are:

  • From now on Masterkey has the only role of internally generating addresses and keys without any context, all external information, for example number of generated addresses, map of known addresses, nAccount used in derivation path has been moved to the Wallet class
  • Having account info only stored in Wallet instead of spread in all files is also a first step toward multi account system.
  • I cleaned up almost all files and removed functions that should be called only internally from the Wallet class: for example getDerivationPath() and getMasterKey()
  • Wallet class has been decoupled from GUI
  • Network class takes now a Wallet object instead of a Masterkey

Overall, as you can see from the git diff, I think that this is a good clean up and from this point we should finally be able to build a good Wallet class that will also manage UTXO, masternode, balance...


Testing

Test all features related to wallet:

  • Create a new wallet
  • Check that you can send and receive funds
  • Check that you can generate new addresses
  • Check that the contact system works
  • Check that you can encrypt the wallet and change password

If any errors are found, the PR works unexpectedly, or you have viable suggestions to improve the UX or functionality of the PR, let me know!


For future contributions

While the clean up is almost completed there are still a few places that can be improved: for example

  • In bitTrx.js there is still a call to wallet.getDerivationPath()
  • the Masternode class should also be refactored and incorporated with the new Wallet class

@panleone panleone self-assigned this Sep 19, 2023
@panleone panleone added the Enhancement New feature or request label Sep 19, 2023
Copy link
Member

@Duddino Duddino left a comment

Choose a reason for hiding this comment

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

I like how the new Wallet class works, however there seems to be an exception when you open MPW, other than that I've left some minor nitpicks
image

scripts/network.js Outdated Show resolved Hide resolved
scripts/network.js Outdated Show resolved Hide resolved
scripts/wallet.js Outdated Show resolved Hide resolved
@panleone
Copy link
Member Author

implemented the review and fixed the problem on opening

Liquid369
Liquid369 previously approved these changes Sep 19, 2023
Copy link

@Liquid369 Liquid369 left a comment

Choose a reason for hiding this comment

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

tACK 893afd2
Working as per tests

Duddino
Duddino previously approved these changes Sep 20, 2023
Copy link
Member

@Duddino Duddino left a comment

Choose a reason for hiding this comment

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

tACK

scripts/wallet.js Outdated Show resolved Hide resolved
Copy link
Member

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

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

tACK ac66fcd

Tested every suggested Test area, tried new wallets, existing wallets, etc, not an issue.

This cleanup has gotten our wallet/key handling code in a much nicer state that I've been wanting for a while, really glad this was handled, on the way to a cleaner and more stable MPW! 🚀

Copy link
Member

@Duddino Duddino left a comment

Choose a reason for hiding this comment

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

tACK I like the new wallet class

Copy link

@Liquid369 Liquid369 left a comment

Choose a reason for hiding this comment

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

tACK ac66fcd

@JSKitty JSKitty merged commit 04b7d40 into PIVX-Labs:master Sep 21, 2023
1 check passed
JSKitty added a commit that referenced this pull request Sep 21, 2023
JSKitty added a commit that referenced this pull request Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants