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

fix: push notification registration latency on PinScreen #423

Merged
merged 5 commits into from
Feb 6, 2024

Conversation

alexruzenhack
Copy link
Contributor

@alexruzenhack alexruzenhack commented Feb 1, 2024

Acceptance Criteria

  • fix: PinScreen dismiss response
  • fix: exits script after dispatch failure effect
  • fix: replace BLOCKED to DENIED enum value
  • fix: push notification authorization logic

PinScreen response latency now in miliseconds:

pinscreen-response-latency.mp4

Closes: #415


Performance Profiling

We have 2 major offenders:

  • generateAccessDataFromSeed consuming ~4s
  • generateCreateWalletAuthData consuming ~14s

Under generateCreateWalletAuthData we have other 3 offenders:

  • getXPrivKeyFromSeed using ~4s
  • getXPubKeyFromSeed using ~4s
  • getAuthXPubKeyFromSeed using ~4s

Both getXPubKeyFromSeed and getAuthXPubKeyFromSeed calls getXPrivKeyFromSeed under the hood, which took almost all execution time. Therefore the real offender for this group is getXPrivKeyFromSeed.

image

Wallet start

Both methods generateAccessDataFromSeed and generateCreateWalletAuthData are called from wallet start. We start an instance of HathorWalletServiceWallet when a user is operating a fullnode's wallet facade.

There are 4 options to try to solve or reduce the effect of this latency, either:

  1. We implement a common wallet API to mount an authentication payload that can be used to call wallet-service API without the need to instantiate HathorWalletServiceWallet.
  2. We store the HathorWalletServiceWallet instance once it is started for the first time, which can decrease the latency for subsequent interactions with push notification operations.
  3. We remove support of push notification for users operating a fullnode's wallet facade and make it a feature strict to wallet-service users; which would allow us to remove this logic to instantiate a HathorWalletServiceWallet.
  4. We implement a lightweight wallet just for the purpose to call wallet-service APIs.

We can discuss other possibilities, these ones were the ones I identified quickly without thinking too much.

Note

PM me to get the profiler.


Security Checklist

  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

@alexruzenhack alexruzenhack force-pushed the fix/push-confirmation-latency branch from 7c597a8 to 4f0d4ec Compare February 1, 2024 01:34
@alexruzenhack alexruzenhack changed the base branch from dev to master February 1, 2024 01:34
@alexruzenhack alexruzenhack self-assigned this Feb 1, 2024

// Delay 300ms to resume script execution in the next loop.
// This solution liberates the PinScreen to dismiss.
yield delay(300);
Copy link
Member

Choose a reason for hiding this comment

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

Does it work if you use delay(0)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I will test.

@alexruzenhack alexruzenhack merged commit 7c9f831 into master Feb 6, 2024
2 checks passed
@alexruzenhack alexruzenhack deleted the fix/push-confirmation-latency branch February 6, 2024 23:00
@alexruzenhack alexruzenhack mentioned this pull request Mar 8, 2024
6 tasks
@alexruzenhack alexruzenhack mentioned this pull request May 9, 2024
alexruzenhack added a commit that referenced this pull request May 15, 2024
* fix: PinScreen dismiss response

* This fix doesn't solve the problem of long latency, however it decreases latency perception for users.

* fix: exits script after dispatch failure effect

* fix: replace BLOCKED to DENIED enum value

* fix: push notification authorization logic

* chore: extract delay from race
alexruzenhack added a commit that referenced this pull request May 15, 2024
* fix: PinScreen dismiss response

* This fix doesn't solve the problem of long latency, however it decreases latency perception for users.

* fix: exits script after dispatch failure effect

* fix: replace BLOCKED to DENIED enum value

* fix: push notification authorization logic

* chore: extract delay from race
This was referenced May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Pin Screen taking time to confirm Push Notification registration
3 participants