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

Shield fixes #354

Merged
merged 13 commits into from
May 1, 2024
Merged

Shield fixes #354

merged 13 commits into from
May 1, 2024

Conversation

Duddino
Copy link
Member

@Duddino Duddino commented Apr 22, 2024

Abstract

Various shield related fixes/improvements.
First commit: Remove sapling prover loading when syncing. It will be downloaded as needed (when creating a shield tx). Add target to getShieldUTXOs, this considerably speeds up shielding when using big wallets
Second commit: Throw directly instead of recreating the error
Third commit: Fix error when restoring wallet when doing a shield tx. Introduced by #325

Testing

  • Assert that shield txs work as expected
  • Try restoring the wallet when doing a shield tx transaction

Copy link

netlify bot commented Apr 22, 2024

Deploy Preview for cheery-moxie-4f1121 ready!

Name Link
🔨 Latest commit 37c7ffc
🔍 Latest deploy log https://app.netlify.com/sites/cheery-moxie-4f1121/deploys/66325ed958c4a9000890d0dd
😎 Deploy Preview https://deploy-preview-354--cheery-moxie-4f1121.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@JSKitty JSKitty requested a review from panleone April 24, 2024 15:22
@JSKitty JSKitty added Bug This is either a bugfix (PR) or a bug (issue). Enhancement New feature or request labels Apr 24, 2024
@JSKitty
Copy link
Member

JSKitty commented Apr 24, 2024

A quick middle-of-testing suggestion: Given the one-time param loading is now at the beginning of a Shield spend: the first Shield Tx is obviously very slow and sticks at the start of the progress bar for a long time.

Perhaps we should add this loading stage to the text section of the progress bar, then flip back to Creating SHIELD transaction... once it's done? This makes it more clear rather than the user staring at an immobile progress bar for 5 minutes with slow internet (such is my own case, lol).

Example text during param loading (inspect element):
image

@JSKitty
Copy link
Member

JSKitty commented Apr 26, 2024

After the latest commits, the progress bar seems non-functional now - but other than that, this is perfect and the params text is also great to have. After this is fixed, getting my tACK.

I tested that it doesn't progress anymore by creating 10x 1-PIV SHIELD notes sent from Public, ensuring there were plenty of notes to process, then spent all of them back to Public: the progress bar sat at 0 for a full minute, then bounced to 100% without any granularity.

image

image

Copy link
Member

@panleone panleone left a comment

Choose a reason for hiding this comment

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

Unlocking the wallet is very slow with this PR.
Apart from that everything works fine and the bug is indeed solved

@@ -116,9 +116,15 @@ getEventEmitter().on('shield-sync-status-update', (str, finished) => {
getEventEmitter().on(
'shield-transaction-creation-update',
async (percentage, finished) => {
if (percentage === 0.0 && !finished) {
Copy link
Member

Choose a reason for hiding this comment

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

In this way at each shield tx it displays "loading shield parameters".

I guess for the moment it's fine since it is only a log to the user, in the future it can be improved

@@ -1063,7 +1063,7 @@ export class Wallet {
}

const periodicFunction = setInterval(async () => {
const percentage = 5 + (await this.#shield.getTxStatus()) * 95;
const percentage = await this.#shield.getTxStatus();
Copy link
Member

Choose a reason for hiding this comment

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

why did you change this line? It was so to make the loading look faster

@panleone panleone mentioned this pull request May 1, 2024
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 37c7ffc

Only note is the noticeably slower first-unlock during a Shield Tx, which is dependent on internet speed during the .wasm loading and thus blocks the UI for considerable time - perhaps in a new PR though, merging this is more urgent atm.

Copy link
Member

@panleone panleone left a comment

Choose a reason for hiding this comment

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

tACK 37c7ffc, agree that other changes can be done in another PR

@JSKitty JSKitty merged commit 6d9dfa5 into PIVX-Labs:master May 1, 2024
5 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug This is either a bugfix (PR) or a bug (issue). Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants