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

chore(TransactionFeedV2): Cleanup stand by transactions in Transaction Feed V2 #6146

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sviderock
Copy link
Contributor

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

Backwards compatibility

Yes

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

  • Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.76%. Comparing base (720ae00) to head (9a5e4f3).
Report is 5 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           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     
Files with missing lines Coverage Δ
src/transactions/actions.ts 100.00% <100.00%> (ø)
src/transactions/feed/TransactionFeedV2.tsx 89.36% <100.00%> (+1.08%) ⬆️
src/transactions/reducer.ts 86.17% <100.00%> (+0.96%) ⬆️

... and 65 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 720ae00...9a5e4f3. Read the comment docs.

Copy link
Member

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

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

Thanks! 👍

Comment on lines 290 to 307
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]
Copy link
Member

Choose a reason for hiding this comment

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

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:

@sviderock
Copy link
Contributor Author

@jeanregisser updated

@sviderock sviderock changed the title chore(feed): Cleanup stand by transactions in Transaction Feed V2 chore(TransactionFeedV2): Cleanup stand by transactions in Transaction Feed V2 Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants