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] Decouple the wallet class #197

Merged
merged 10 commits into from
Sep 19, 2023

Conversation

panleone
Copy link
Member

@panleone panleone commented Sep 17, 2023

Abstract

The file wallet.js is a real mess: There is no actual wallet manager class and it mixes GUI, crypto functions, ledger stuff and masterKeys. This PR begins a process of refactoring the class:

  • A Wallet class has been added that at the moment incorporates all the ugly global wallet related functions that were in wallet.js. In the future Wallet will manage UTXOs, balance, addresses, masternodes...
  • MasterKey class and derivatives have been moved to a new file masterkey.js, masterkeys should have the ONLY role of managing internally the generation of new addresses.
  • Hence the global variable masterKey has been removed in favor of a global variable of type Wallet that has masterKey as a private field. At the moment I added a getter to masterKey (or the refactor would have been to big), in future PRs we should try to avoid exposing masterkey outside the wallet.
  • Ledger related functions and variables have been moved to a new file ledger.js
  • Finally a new file encoding.js has been created. It contains the low-level cryptographic functions that were previously in the wallet file.

Testing

Test that the wallet functionality are still working as expected for example:

  • Create wallet
  • Import wallet
  • Hit 'Refresh Address' in the Dashboard
  • Encrypt (+ Change Password)
  • Test Send + Receive
  • Test Contacts (Add them, Send to them, etc)

Note

As I already said the refactor is not final and I did not want to change too much lines. In future PRs the getter to masterKey and the function getDerivationPath should both be removed from the Wallet class. We should also really try to decouple the logic from the GUI

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

JSKitty commented Sep 17, 2023

Before I add the "Awaiting Review" label to this PR, the 'Testing' section is going to need to be more detailed (the Quality Control team need to understand what to test, too vague and we might not get enough tests by folks).

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.

Code-wise this is looking very clean and is a massive organisation to what we had before, I like it! 👍 🚀

I have some minor nits related to the MasterKey and Wallet classes, but many of them are out of scope to the PR (decoupling refactor) so I'll probably work on them in a different PR to keep this small and quick.

To real nits: I'm not sure cypher.js quite fits what we're using it for, maybe encoding.js would fit better? It is more En/Decoding than cyphering.

I'll get testing in the meantime. 🔧

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

To real nits: I'm not sure cypher.js quite fits what we're using it for, maybe encoding.js would fit better? It is more En/Decoding than cyphering.

totally agree

Duddino
Duddino previously approved these changes Sep 18, 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, tested wallet creation, sent to addresses + contacts and everything works

@JSKitty JSKitty added Awaiting Review This PR and/or issue is awaiting reviews before continuing. Review Reward: 10 PIV Reviewers of this Pull Request will receive a 10 PIV reward labels Sep 18, 2023
Duddino
Duddino previously approved these changes Sep 18, 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.

Other than the leftover comment, tACK

scripts/wallet.js 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 e6ddb9e

We also received positive tests from 4 Quality Control members.

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 e6ddb9e
Ledger is also working again LGTM

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 ledger works fine

@JSKitty JSKitty merged commit b7e9264 into PIVX-Labs:master Sep 19, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Review This PR and/or issue is awaiting reviews before continuing. Enhancement New feature or request Review Reward: 10 PIV Reviewers of this Pull Request will receive a 10 PIV reward
Projects
Development

Successfully merging this pull request may close these issues.

4 participants