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: parse buy txid from midgard actions #6813

Merged
merged 4 commits into from
May 6, 2024
Merged

Conversation

kaladinlight
Copy link
Contributor

Description

The previous double swap logic was incorrect resulting in rune swaps polling the wrong txid against the wrong chain. This logic removes the double swap condition and instead just attempts to get the latest txid specified in the outbound transactions array. This will evaluate in three different ways:

  • Single outbound transaction is rune (asset -> rune): which will return the sellTxId and rely on the swap status for confirmation (sellTxId doesn't appear to actually be used in the downstream logic due to the hasOutboundTx variable in swapper endpoints.ts)
  • Single outbound transaction is asset (asset or rune -> assest): which will return the outbound txid for the buy chain to poll for status
  • Double swap transaction (asset -> asset): which will return sellTxId on the first rune swap outbound and rely on swap status which will be pending, and then wait for the final outbound asset tx and return the txid for the buy chain to poll status for

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

N/A

Risk

High Risk PRs Require 2 approvals

Medium - this changes transaction confirmation logic

What protocols, transaction types or contract interactions might be affected by this PR?

Thorchain Swaps

Testing

  • Ensure the following swaps confirm:
    • rune -> asset
    • asset -> rune
    • asset -> asset
    • asset -> asset streaming (results in double swap it seems which I didn't have enough funds to test myself)

Engineering

☝️

Operations

☝️

Screenshots (if applicable)

runeToAtom
atomToRune
atomToAvax

@kaladinlight kaladinlight requested a review from a team as a code owner May 3, 2024 18:07
Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Logic looks sane conceptually.

Tested at runtime and spotted no regressions in THOR swapper status polling:

  • L1 -> RUNE
image
  • RUNE -> L1
image
  • L1 -> L1
image
  • L1 - L1 streaming swap

Untested

Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Retested locally:

  • L1 -> RUNE
image
  • RUNE -> L1
image
  • L1 -> L1
image

@kaladinlight kaladinlight enabled auto-merge (squash) May 6, 2024 22:25
@kaladinlight kaladinlight merged commit 149ac8c into develop May 6, 2024
3 checks passed
@kaladinlight kaladinlight deleted the fix-parse-thor-buy-tx branch May 6, 2024 22:31
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