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

feat: process txs as they arrive #810

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

r4mmer
Copy link
Member

@r4mmer r4mmer commented Jan 10, 2025

Context

Nowadays we run the method to process the wallet full history every time a new tx arrives in the ws. Wallets with many transactions/addresses usually take a long time (0.5s) to process their entire history. There are cases where the wallet returns and error because of insufficient funds because it's processing the history of a recent transaction that has just arrived when the user wants to send a new tx.

Acceptance Criteria

  • Process only the single tx that arrives in the ws (including delete the utxos that are being spent in this tx).
  • If this tx is an update and the voided state has changed, then we run the full process history, just to make this easier.
  • While this processing is being done we change the wallet state to PROCESSING, so users know that they must wait until creating the next tx.

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.

Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 91.48936% with 4 lines in your changes missing coverage. Please review.

Project coverage is 80.75%. Comparing base (c06d3ce) to head (b822a04).

Files with missing lines Patch % Lines
src/utils/storage.ts 88.46% 3 Missing ⚠️
src/new/wallet.js 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #810      +/-   ##
==========================================
+ Coverage   80.68%   80.75%   +0.07%     
==========================================
  Files          85       86       +1     
  Lines        6549     6594      +45     
  Branches     1419     1430      +11     
==========================================
+ Hits         5284     5325      +41     
- Misses       1252     1256       +4     
  Partials       13       13              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pedroferreira1 pedroferreira1 self-assigned this Jan 13, 2025
@andreabadesso andreabadesso self-requested a review January 14, 2025 19:41
Comment on lines +1350 to +1363
if (metadataChanged) {
// Process the full history because
// this tx changed the voided state
// XXX in the future we should be able to handle this
// voidness alone, without processing everything
// but for now it's an isolated case and it's easier
await this.storage.processHistory();
} else if (isNewTx) {
// Process this single transaction.
// Handling new metadatas and deleting utxos that are not available anymore
await this.storage.processNewTx(newTx);
}
// restore previous state
this.state = previousState;
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment semi-related to line number

Code looks ok, but should we add tests for this new mechanism?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress (Done)
Development

Successfully merging this pull request may close these issues.

3 participants