Skip to content

chore: initial refactor for limit orders#7912

Merged
woodenfurniture merged 13 commits intodevelopfrom
initial-refactor-limit-orders
Oct 15, 2024
Merged

chore: initial refactor for limit orders#7912
woodenfurniture merged 13 commits intodevelopfrom
initial-refactor-limit-orders

Conversation

@woodenfurniture
Copy link
Copy Markdown
Contributor

@woodenfurniture woodenfurniture commented Oct 9, 2024

Description

Refactors trade input components to allow us easy build-out of limit orders.

Likely plenty more can be done to streamline things for limit orders component-wise, but this PR is intended to be an 80:20 of value:effort so many of the known improvements are intentionally left out where the effort of changing these are one of:

  • Simpler to do at the time of building limit orders
  • Not strictly required

Issue (if applicable)

closes #7908

Risk

High Risk PRs Require 2 approvals

High risk. The changes here are theoretically straightforwards but considering the impact of getting things wrong and breaking the ability to make trades, we should review this under a "high risk" lens.

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

All trades for all protocols, namely UI transitions and interactions.

Additionally, arbitrum bridge which is not directly affected but may be indirectly affected UI-wise.

Testing

Swaps, bridges, quotes, should be EXACTLY the same as prod.

Check trades, namely UI transitions and interactions around:

  • trade input
  • quote list
  • confirming a trade
  • settings "cog" menu (custom slippage etc)
  • account selection, trading into specific account no.
  • manual receive address
  • mobile UI layout

Additionally test the arbitrum bridge which is not directly affected but may be indirectly affected UI-wise.

Engineering

Rebase was finicky, so please double check regression of https://github.com/shapeshift/web/pull/7896/files

Operations

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

Screenshots (if applicable)

A trade

https://jam.dev/c/6f6d30de-65eb-49e3-8c8d-20b621c4e283

Mobile layout unchanged

image

@woodenfurniture woodenfurniture marked this pull request as ready for review October 10, 2024 00:27
@woodenfurniture woodenfurniture requested a review from a team as a code owner October 10, 2024 00:27
@woodenfurniture woodenfurniture marked this pull request as draft October 10, 2024 00:28
@woodenfurniture woodenfurniture marked this pull request as ready for review October 10, 2024 01:21
@gomesalexandre gomesalexandre requested review from gomesalexandre and removed request for gomesalexandre October 10, 2024 11:42
Copy link
Copy Markdown
Member

@0xApotheosis 0xApotheosis left a comment

Choose a reason for hiding this comment

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

Code-wise this is looking good.

I did a regression test with this branch (left) against develop (right) and found a few small but important things:

Going to the Claim tab and back again leads to a an empty Trade/Bridge route

Screenshot 2024-10-11 at 16 19 43

End-to-end trade flow (including approval)

Screenshot 2024-10-11 at 16 23 59

Padding broken

We are missing the padding (48px) that was wrapping the Trade component

Screenshot 2024-10-11 at 16 28 43

Infinite quote loading is back, and a weird flicker in the quote list - note this happened once and I couldn't repro again, so I"m not going to pin it to this branch

Screenshot 2024-10-11 at 16 12 49
quote-list-flicker.mp4

@woodenfurniture woodenfurniture force-pushed the initial-refactor-limit-orders branch from 587b9c6 to 8272547 Compare October 13, 2024 22:21
Copy link
Copy Markdown
Member

@0xApotheosis 0xApotheosis left a comment

Choose a reason for hiding this comment

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

Retested - issues from the previous review have been resolved, looking good.

@gomesalexandre gomesalexandre self-requested a review October 14, 2024 12:30
Copy link
Copy Markdown
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.

First conceptual pass, one smolish comment re: useAccountIds() destructured vals renaming but nothing obviously standing out here, runtime pass to follow!

Copy link
Copy Markdown
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 against prod (right, note that is not develop because of current ws issues in develop endpoints) and with app env, couldn't find any regression in swapper either presentational or functional!

  • Trade input is happy (desktop)
image
  • Trade input is happy (mobile)
image
  • Claims are happy (mobile)
image
  • Claims are happy (desktop)
image
  • Quotes list (desktop)
image
  • Quotes list (mobile)
image
  • Cog settings wheel is happy

https://jam.dev/c/21445392-954b-4017-b643-39305a9afd30

  • Trade confirm/execution is happy

https://jam.dev/c/7b57952c-cd00-404b-b134-2d8eecfc667d

  • Multi-account and manual receive address swaps are happy

https://jam.dev/c/847a8b92-3d7d-4f1b-9b30-9c3b9a9508b9

@woodenfurniture woodenfurniture enabled auto-merge (squash) October 15, 2024 00:45
@woodenfurniture woodenfurniture merged commit dbe7e88 into develop Oct 15, 2024
@woodenfurniture woodenfurniture deleted the initial-refactor-limit-orders branch October 15, 2024 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Initial refactoring for limit orders

3 participants