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: it aint over till its over streaming swaps completion #7831

Merged
merged 6 commits into from
Oct 1, 2024

Conversation

woodenfurniture
Copy link
Member

@woodenfurniture woodenfurniture commented Sep 30, 2024

Description

In #7371, and issue was reported where streaming swaps UI were progressed to "trade complete" when streaming is still in progress (see #7371 (comment)). This PR addresses that.

Root causes:

Solutions:

  • Rewrite trade status logic to treat all stages reported by the thornode API as Pending. This prevents convoluted logic returning Confirmed state when tx is still being streamed.
  • Use thornode tx/{hash} endpoint to determine completed status for all thorchain swapper txs. This works for both streaming and non-streaming txs.

Notable changes:

  • status bar will have "indeterminate" stripes while the streaming status is being initialized (isntead of sitting at 100% green), which is more conventional ux
  • Minor updates to thorchain tx status copy

Issue (if applicable)

closes #7371

Risk

High Risk PRs Require 2 approvals

Moderate risk. This solves an issue with UI displaying the status of tx execution and does not affect user funds or ability for users to complete trades.

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

THORChain swapper, both streaming and non-streaming swaps.

Testing

  • Can perform regular trades with thorchain swapper (all chains)
  • Can perform streaming swaps from RUNE
  • Can perform streaming swaps to RUNE
  • Can perform streaming swaps from non-RUNE to non-RUNE

Engineering

Operations

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)

Screenshots (if applicable)

With outbound tx (buy asset not RUNE)
https://jam.dev/c/b9660984-a567-46bc-ba6f-e9e037d07037

Without outbound tx (buy asset is RUNE)
https://jam.dev/c/99c865a0-df13-47dd-87fc-568de47323b7

Regular trade (no streaming)
https://jam.dev/c/f64594e2-0d96-44a6-8106-b9586831fa5b

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.

Killer PR ser. Tested locally against develop, noticed one regression WRT "inbound Tx pending" state missing, which is more obvious with slow L1s (e.g UTXOs) but is happening both with RUNE and L1s ad inbound Txs.

Unable to test actual streaming swaps here, but logic looks sane to me and honours THOR docs, so happy to have ops test the streaming case (which has been confirmed working with two swaps/Jams in this PR) after this smolish regression is fixed 🙏🏽

  • DOGE -> RUNE (no outbound Tx)

https://jam.dev/c/a002e844-8036-4cd0-9214-2882e72bb25e

While swap status detection was happy after inbound, noticed we're now missing the inbound pending state i.e this guy:

Screenshot 2024-09-30 at 12 07 25 Screenshot 2024-09-30 at 12 03 55
  • RUNE -> DOGE (no L1 inbound, this is happening directly in THOR)

https://jam.dev/c/01261386-4242-4f62-9177-75b36b230c5d

Ditto, happy against develop except for inbound pending state missing

  • Streaming swap: unable to test, will defer to @shapeshift/operations

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, still noting the same issue re: "Inbound transaction pending" missed until L1 Tx is confirmed and observed.

However, all next stages perform as expected - to be retested and stamped with actual streaming swaps once fixed after getting the Foxcrypt'd seed back again

RUNE -> DOGE (non-streaming) 🚫

https://jam.dev/c/c8716f58-ba79-4764-9613-6a9f4525326e

  • Still seeing "Inbound transaction pending" missing here, though not super visible with RUNE as it's pretty fast and goes to the first stage of akschual polling in a few seconds

image

ETH -> DOGE (non-streaming) 🚫

https://jam.dev/c/c22ea5d7-1627-4436-9e95-919fb4f0edfb

  • Same issue as above

DOGE -> RUNE (fake streaming of sorts i.e low amount but streaming swap) 🚫

https://jam.dev/c/688463cf-5814-400e-99fd-3db91d653765

  • Same issue here
  • Also noticing "Swap Status" does stay indeterminate after completion, see screenshots of swap pending vs. completion
    image
    image

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.

Tested streaming swaps locally:

  • RUNE -> FOX

https://jam.dev/c/c210100a-c6f3-49ad-b54e-10cf4b82bfeb
image

  • FOX -> RUNE

https://jam.dev/c/2541cac8-c010-41cb-832d-d3efaa2ae030

Confirmed this guy does what it says on the box!

  • The spotted issue re: "fake swaps" having indeterminate progress bar is a non-blocker and can be tackled in a follow-up if we want to, though definitely edge-case.
  • Also non-blocking, as can be seen on the first jam at around 1:17, seeing some "Outbound transaction scheduled" but 1 out of 2 swaps which sounds off, and we should probably tackle as a follow-up i.e if outbound is scheduled, streaming is complete

@gomesalexandre gomesalexandre enabled auto-merge (squash) October 1, 2024 22:21
@gomesalexandre gomesalexandre merged commit a647b87 into develop Oct 1, 2024
3 checks passed
@gomesalexandre gomesalexandre deleted the fix-streaming-swap-completion branch October 1, 2024 22:23
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.

Streaming Swaps Completed State
2 participants