chore(TransactionFeedV2): Cleanup stand by transactions in Transaction Feed V2 #6146
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6146 +/- ##
========================================
Coverage 88.75% 88.76%
========================================
Files 727 727
Lines 30752 30776 +24
Branches 5624 5630 +6
========================================
+ Hits 27295 27319 +24
- Misses 3259 3414 +155
+ Partials 198 43 -155
... and 65 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| function cleanupStandByTransactions() { | ||
| const confirmedPaginationTransactions = | ||
| data?.transactions | ||
| ?.filter((tx) => tx.status !== TransactionStatus.Pending) | ||
| .map((tx) => tx.transactionHash) || [] | ||
|
|
||
| const standByTransactionsToRemove: string[] = [] | ||
| for (const tx of standByTransactions.confirmed) { | ||
| if (confirmedPaginationTransactions.includes(tx.transactionHash)) { | ||
| standByTransactionsToRemove.push(tx.transactionHash) | ||
| } | ||
| } | ||
|
|
||
| if (standByTransactionsToRemove.length) { | ||
| dispatch(removeStandByTransactions(standByTransactionsToRemove)) | ||
| } | ||
| }, | ||
| [data?.transactions, standByTransactions] |
There was a problem hiding this comment.
Would it be possible to move this logic to the reducer?
Acting on the successful fetched transactions action.
This way we follow more closely Redux best practices:
|
@jeanregisser updated |
sviderock
changed the title
| const standByInRange = standBy.filter((tx) => tx.timestamp >= min && tx.timestamp <= max) | ||
| const standByInRange = standByTransactions.filter((tx) => { | ||
| const inRange = tx.timestamp >= min && tx.timestamp <= max | ||
| const newTransaction = currentCursor === FIRST_PAGE_TIMESTAMP && tx.timestamp > max |
There was a problem hiding this comment.
With the change to how the cursor is structured in blockchain-api (using the b64-encoded Zerion next page URL), it seems like the cursor semantics in the wallet need to change accordingly, right?
There was a problem hiding this comment.
@jophish Yes, all the changes to work with the new API will be addressed with the separate PR. This bunch of PRs were worked on with the prototype branch.
| if (data?.transactions.length) { | ||
| dispatch(removeDuplicatedStandByTransactions(data.transactions)) | ||
| } |
There was a problem hiding this comment.
Instead of using an effect and a new action, can we react on the RTK-Query API fetched action?
Directly at the reducer level.
There was a problem hiding this comment.
@jeanregisser Will investigate and get back to you on this!
There was a problem hiding this comment.
We chatted with @sviderock and agreed to try to move the effect for cleaning up to the reducer, in a follow up PR.
Approving now so PRs can get merged one by one.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
…n Feed V2 (#6146) ### Description 4th PR of RET-1207. This implements the cleanup of confirmed stand by transactions that are already present in the feed. Currently, we keep confirmed transactions in `standByTransactions` even if they are present in the pagination data. This is unnecessary as it takes extra space from the persisted storage. ### Test plan - changed the test that checks merging of pages with stand by transactions - added test to check if the stand by transaction was correctly removed if it appeared in the pagination data ### Related issues - Relates to RET-1207 ### Backwards compatibility Yes ### Network scalability If a new NetworkId and/or Network are added in the future, the changes in this PR will: - [x] Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)
Description
4th PR of RET-1207. This implements the cleanup of confirmed stand by transactions that are already present in the feed. Currently, we keep confirmed transactions in
standByTransactionseven if they are present in the pagination data. This is unnecessary as it takes extra space from the persisted storage.Test plan
Related issues
Backwards compatibility
Yes
Network scalability
If a new NetworkId and/or Network are added in the future, the changes in this PR will: