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

feat: hide native assets in asset selection modals for limit orders #8121

Merged
merged 11 commits into from
Nov 21, 2024

Conversation

woodenfurniture
Copy link
Member

@woodenfurniture woodenfurniture commented Nov 13, 2024

Description

Hides native assets in asset selection for limit orders, as native assets are not supported.

Fixes 2 production bugs:

  • [phantom wallet only] Solana should not be shown in chain drop-down menus, but in prod it appears.
  • Chains for the "buy" asset which not supported by the wallet should appear in the chain drop-down on the right, but in prod they are disabled.

Issue (if applicable)

closes #6203

Risk

High Risk PRs Require 2 approvals

Moderate risk as this makes some minor refactors of the trade asset selection components.

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

Testing

Thoroughly test asset selection, focusing on trade input and less so on limit orders - trades are user facing and limit orders are behind a feature flag.

Spot Trade

  • Native assets should still be allowed
  • [phantom wallet] Solana should not appear in the list of assets or chains (prod shows it in the chain drop-down on the right which is a bug)
  • Chains for the "buy" asset which not supported by the wallet should appear in the chain drop-down on the right (prod disables them which is a bug), and should prompt for a manual receive address

Limit order

  • Native assets should be hidden
  • [phantom wallet] Solana should not appear in the list of assets or chains
  • Chains for the "buy" asset which not supported by the wallet should appear in the chain drop-down on the right, and should prompt for a manual receive address

Engineering

Operations

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

Screenshots (if applicable)

Native assets hidden for limit order asset selection:

[phantom wallet] Solana should not appear in the list of assets or chains
Prod:

This PR:

Chains for the "buy" asset which not supported by the wallet should appear in the chain drop-down on the right.
Prod:

This PR:

@woodenfurniture woodenfurniture changed the base branch from develop to limit-orders-redux November 13, 2024 22:08
@woodenfurniture woodenfurniture marked this pull request as ready for review November 13, 2024 23:58
@woodenfurniture woodenfurniture requested a review from a team as a code owner November 13, 2024 23:58
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.

Looks beautiful conceptually - found two issues however from a functional perspective (native asset buys should be available for limit, and Solana related assets are still displayed for limit) as well as one conceptual issue (which may warrant some discussion as to what's our actual intent here).

See testing below.

https://gist.github.com/gomesalexandre/1f8a17be1710e38cc98bf7bfe6e3d9c6

Base automatically changed from limit-orders-redux to develop November 20, 2024 00:35
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, Solana asset selection looks sane again!

Spotted one bug re: native assets selection for native being broken with the main unfiltered by chain view for limit orders, and also something odd which may or may not be a prod bug re: "My Assets" list not being available when filtering by chain, although it is also present in swapper too, and in develop as well currently.

Spot

Phantom Solana selection ✅

  • Solana asset selection is back! 🎉

image

image

image

Chain selection for unsupported buy assets

  • Still available ✅

image

image

  • And still filtered on the sell side ✅

image

  • Still available ✅

Native asset selection

  • Still still happy ✅

image

image

Limit

Native asset selection is disabled

  • Sell side ✅

image

  • Buy side is available 🚫

While native assets are available for search after filtering out by a given chain (e.g filtering by Ethereum chain makes ETH available for search), native assets are not available on

  • initial search (without chain filtered)
  • popular assets list after filtering by a given chain (but without search terms just yet, i.e these should be available as popular asset)
  • additionally, "My Assets" i.e user held balance list, isn't available after filtering by chain and is only available on the main unfiltered view, which is also present in swapper in develop atm

See screenshots below, and Jam which should make it more obvious: https://jam.dev/c/b111cf87-4a45-4d0c-8a9d-7840f3619dc7

image

image

image

image

image

image

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.

Retested locally as a subset of previous testing

  • Native assets selection is available for limit orders buy side
image image image
  • Native assets selection is not available for limit orders sell side
image image image
  • "My Assets" is available on chain-filtered view 🎉
image
  • Swapper is still happy (e2e testing of assets/chain selection)

https://jam.dev/c/835ab941-4c2b-4525-a5ff-f9f3b76b29c5

@gomesalexandre gomesalexandre merged commit d404b27 into develop Nov 21, 2024
3 checks passed
@gomesalexandre gomesalexandre deleted the limit-orders-no-native branch November 21, 2024 09:32
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.

limit orders cannot be used starting from native assets
2 participants