fix(TransactionFeedV2): Fix persisted feed storing unmerged transactions #6205
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6205 +/- ##
========================================
Coverage 88.93% 88.94%
========================================
Files 737 737
Lines 31370 31386 +16
Branches 5800 5497 -303
========================================
+ Hits 27900 27915 +15
- Misses 3425 3427 +2
+ Partials 45 44 -1
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
📸 Snapshot TestBase build not foundNo build was found for the base commit 630cf89. This is required to generate a snapshot diff for your pull request. It's possible that you created a branch off the base commit before all of the CI steps have finished processing, e.g. the one that uploads a build to our system. If that's the case, no problem! Just wait and this will eventually resolve. 🛸 Powered by Emerge Tools |
| @@ -203,7 +204,8 @@ function mergeStandByTransactionsInRange({ | |||
| return inRange || newTransaction || veryOldTransaction | |||
| }) | |||
| const deduplicatedTransactions = deduplicateTransactions([...transactions, ...standByInRange]) | |||
| const transactionsFromAllowedNetworks = deduplicatedTransactions.filter((tx) => | |||
| const sortedTransactions = sortTransactions(deduplicatedTransactions) | |||
There was a problem hiding this comment.
This was also one of the issues as initial persisted feed appeared unsorted therefore also triggering the rearrangement. It does add up an extra sorting loop, but this shouldn't be a major performance issue for now.
| @@ -427,6 +429,17 @@ export default function TransactionFeedV2() { | |||
| [newlyCompletedCrossChainSwaps] | |||
| ) | |||
|
|
|||
| useEffect( | |||
| function updatePersistedFeedFirstPage() { | |||
There was a problem hiding this comment.
if i understand this change correctly, the difference lies in that we want to store firstPageData instead of the received first page data in redux because firstPageData includes the sorted and merged standby transactions? if that is the case, aren't we storing the standby transactions potentially twice?
is it possible instead for the feedFirstPageSelector to return the first page that is merged with the standby transactions?
There was a problem hiding this comment.
- With the implementation from this PR, duplicates will be present but only for pending and failed transactions. Once pending turn to completed - they will get removed from stand by, therefore duplicates will only exist for failed ones but only for the first page of the feed anyways.
- Yes, it is possible!
There was a problem hiding this comment.
@kathaypacific Argh, it seems like if I move this to the selector - I won't be able to pass isLastPage to the mergeStandByTransactionsInRange function. The rest of un-merged stand by transactions are always pushed to the very bottom of the last page but for that we need to somehow figure out if the page is last which is impossible from the selector level.
There was a problem hiding this comment.
hum, okay. i don't have a better suggestion but am slightly concerned that these small details are causing some complexity / workarounds that we will later forget about. i can see us potentially reintroducing these bugs during cleanup initiatives. perhaps @jeanregisser has a suggestion here, or leaving a small comment with a link to your comment explanation above is sufficient for now
There was a problem hiding this comment.
@kathaypacific there are a few potential implementation changes that I have in mind but they involve a bit more time to implement as it will most likely require the overall change of managing paginationData on the side of Redux rather than on the component level. In that case, we'll have most of the logic on the reducer level and no duplication.
| @@ -427,6 +429,17 @@ export default function TransactionFeedV2() { | |||
| [newlyCompletedCrossChainSwaps] | |||
| ) | |||
|
|
|||
| useEffect( | |||
| function updatePersistedFeedFirstPage() { | |||
There was a problem hiding this comment.
hum, okay. i don't have a better suggestion but am slightly concerned that these small details are causing some complexity / workarounds that we will later forget about. i can see us potentially reintroducing these bugs during cleanup initiatives. perhaps @jeanregisser has a suggestion here, or leaving a small comment with a link to your comment explanation above is sufficient for now
|
Merging it acknowledging that we will re-visit this implementation. |
Description
While working on a different feed issue, I've noticed there's a bug in storing the first page of the feed to the persisted storage. TLDR, currently it stores "raw" transactions from pagination, which are not merged with the stand by transactions. This still sometimes causes transactions to rearrange on the first load.
This fix moves updating of the feed from to component level. It contradicts the current Redux-driven approach as it moves processing that can be managed on reducer level back to the component level but in order to make it follow that guideline we will need to:
paginatedDatato ReduxconfirmedTransactionsconstruction to ReduxpaginatedDataaway from persisted storage with an extra dependency like this or tweak the current persist settings as per their nested persist guide which might cause issues with existing persistence-related tests therefore create more friction to implement this seemingly simple fix.Initial PR review discussion made sense at the time of working on that PR but back then I overlooked the fact that I've implemented it with this very bug as I was putting the wrong
data.transactionsdata into the feed instead of the rightpaginatedData[FIRST_PAGE_CURSOR].Test plan
Existing test to check proper rehydration of the persisted feed passes.
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: