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(swap): ensure long running cross chain swaps eventually shows completed status #5704

Merged
merged 8 commits into from
Aug 5, 2024

Conversation

kathaypacific
Copy link
Collaborator

Description

This PR fixes a bug where pending cross chains swaps remain pending forever. The root cause is the query helper reconciles already received transactions with newly received transactions by preferring the already received transaction. This means that previously received pending swaps are not updated when the same transactions is received again with the completed status.

This is a bug that affects not only cross chain swaps (we should always prefer the newest information in case other fields in the transaction have changed.

Test plan

Cross chains swaps can be settled correctly

Related issues

n/a

Backwards compatibility

Y

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)

transactionHash: transaction.transactionHash || '',
block: '',
fees: [],
...transaction, // in case the transaction already has the above (e.g. cross chain swaps), use the real values
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this small change ensures the received pending cross chain swaps can display the fees in the tx details

Copy link
Contributor

Choose a reason for hiding this comment

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

can we cover this new behavior with a test to ensure new TX data overrides old data?

Copy link

codecov bot commented Aug 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.92%. Comparing base (04e04e8) to head (bf8f786).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             main    #5704    +/-   ##
========================================
  Coverage   86.92%   86.92%            
========================================
  Files         782      782            
  Lines       32315    32315            
  Branches     5339     5633   +294     
========================================
+ Hits        28090    28091     +1     
+ Misses       4175     3984   -191     
- Partials       50      240   +190     
Files Coverage Δ
src/transactions/feed/TransactionFeed.tsx 83.78% <ø> (ø)
src/transactions/feed/queryHelper.ts 93.82% <100.00%> (ø)
src/transactions/reducer.ts 76.31% <ø> (+0.87%) ⬆️

... and 78 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 04e04e8...bf8f786. Read the comment docs.

Copy link
Contributor

@jophish jophish left a comment

Choose a reason for hiding this comment

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

LGTM, just small comments

)
transactionsWithoutDuplicatedHash.sort((a, b) => {
const transactionsByTxHash: { [txHash: string]: TokenTransaction } = {}
existingTxs.forEach((transaction) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can do

[...existingTxs, ...incomingTxs].forEach((transaction) => {
    transactionsByTxHash[transaction.transactionHash] = transaction
  })

to save some space

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TY, updated!

transactionHash: transaction.transactionHash || '',
block: '',
fees: [],
...transaction, // in case the transaction already has the above (e.g. cross chain swaps), use the real values
Copy link
Contributor

Choose a reason for hiding this comment

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

can we cover this new behavior with a test to ensure new TX data overrides old data?

@kathaypacific kathaypacific added this pull request to the merge queue Aug 5, 2024
Merged via the queue into main with commit 8499480 Aug 5, 2024
16 checks passed
@kathaypacific kathaypacific deleted the kathy/fix-x-chain-misc branch August 5, 2024 12:12
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.

None yet

2 participants