feat: improve limit order ui to be more human readable #8970

NeOMakinG · 2025-03-07T15:21:49Z

Description

Added market price next to the price information reach as well as human readable sentence
Added fiat value switch to every inputs, authorize writing on any inputs
I did move the expiry to the bottom overview
Added a big human readable sentence explaining every amount
Allowed zero out input so it's easier for users to write in the inputs

Issue (if applicable)

closes #8868

Risk

Medium (limit order/swapper but mainly presentational)

High Risk PRs Require 2 approvals

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

Testing

Try to play with the limit order settings, see if informations are accurate
Try to submit limit orders, see if informations are right

Engineering

n/a

Operations

n/a

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

Screenshots (if applicable)

gomesalexandre

Tested locally:

Natural language explainer is missing 🚫

Market is showed as dotted but does nothing on click ❓

Or rather it does, but the behaviour is not obvious since the UX is the same as the "When 1 FOX is worth" which has a totally different behaviour.

Could we perhaps use the same logic as CoW when if this is already the market rate (we probably want to allow for a very small 0.x% deviation when implementing this because of prorate shenanigans), then this is not dotted and a no-op on click? See CoW: https://jam.dev/c/d0be44b5-c9d3-49d2-a7bf-7f048e1f8bc1

Clicking the "When " is confusing ❓

See https://jam.dev/c/100e36d6-3d04-4302-929f-bd6ac65d2ec7

Although it is a product q., as a user, I find it even more confusing now to have two buttons doing the exact same, especially with the missing explainer atm.

Perhaps we could only keep the dotted amount as a trigger, add an asset icon and perhaps a switch icon, and remove the right-side switch?

Fiat switch on receive amount is a dead click, and inputting either crypto/fiat doesn't do anything (although it is possible to remove some chars and produce a buggy behaviour) 🚫

https://jam.dev/c/c7f190e4-0901-4cbe-9299-4084193b3973

@NeOMakinG this should:

prorate fiat amount on click (required)
ideally, also able to enter a receive amount either in crypto/fiat and calculate other values as derived

Expiry looks sane ✅

0 is a valid input ✅

RateGasRow is working as it should on limit ✅ (showing the delta)

gomesalexandre · 2025-03-10T10:16:14Z

src/components/MultiHopTrade/components/LimitOrder/components/LimitOrderBuyAsset.tsx

-        // Disable account selection when user set a manual receive address
        isAccountSelectionHidden={Boolean(manualReceiveAddress)}
-        isReadOnly={true}


blocking: no bueno, able to get into a state where manual receive address is entered and account selection is still possible, despite manual address still taking over

https://jam.dev/c/eb2b3b3d-58b4-42a2-a07e-65bea31375ff

The behavior looks ok, you can actually select the from address, but the to address shouldn't be another account as you specified an external address, and if you remove the external address, the account should be selectable again!

https://jam.dev/c/abe5fdc1-19a8-49bd-aeb4-cac6ede04be7 LGTM!

src/components/MultiHopTrade/components/RateGasRow.tsx

.../MultiHopTrade/components/SharedTradeInput/SharedTradeInputFooter/SharedTradeInputFooter.tsx

NeOMakinG · 2025-03-10T16:03:01Z

@gomesalexandre for the asset switching, could we maybe not block this PR and wait for @shapeshift/product to give their input if we want to modify it?

gomesalexandre

Gucci ✅

Values look good, minus the market one on the opposite direction, and the obvious rate discrepancy against CoW which was not introduced by this PR, see below

https://jam.dev/c/aa93cabb-1e89-44a7-8d6f-217fd27f1edc

Derivation from buy side input looks good 🐐

And clicking market even sets it back to market, v. noice ser

https://jam.dev/c/65d6771f-2eb2-4293-8e09-2eba2dff18db

Hot takes 🧑‍🚒

Crypto/fiat mode sync is inconsistent with spot and feels unnatural to me, though not saying one option is better than the other, but rather we shouldn't go with it for the time being at least because something something consistency

https://jam.dev/c/9a26c740-4a23-4073-a6b5-073a6b929fe0

Market is not clickable when in default mode, but then becomes clickable when in the opposite direction. We should probably use some heuristics here (e.g default vs. custom input value, or perhaps some deviation tolerance) to ensure it stays disabled in both modes. This may well be an actual bug though, see below.

https://jam.dev/c/dbcee3d7-48af-49ca-8f82-6dc279968048

Not introduced by this PR, but limit inherits the URL of spot while not doing anything with it. To-be-fixed in the big boi swapper routing refactor

CoW has some logic of using the opposite default direction to say 1 = when dealing with stablecoin sells, which does indeed feel more natural, as you're usually aware of the price of a given asset in fiat terms, in which stablecoins are indexed. Should we do the same here?

https://jam.dev/c/e1ff5674-857c-4661-a024-30a59018275c

We should probably trim 0s padding in the explainer (and anywhere relevant)

Unrelated to this PR, but we seem to leverage market data/quotes caching and never refresh?

https://jam.dev/c/98dd1254-1397-429c-b959-39a55f6760f3

Unrelated to this PR, but we seem to consistently get rates wrong, which actually sounds like a critical issue when you're doing limit orders and care about execution reliability. I know there is some weird logic re: CoW quotes, we should probably capture this as an issue and revisit ASAP

https://jam.dev/c/84097246-4dd9-47e8-bd71-5cc02f40b553

FWIW, CoW does not rely on the quote to display the rate, but only to make you able to actually proceed, which should be a good hint re: what we're doing wrong here

https://jam.dev/c/114f8a53-fa6b-45a7-80de-f3a854221fe8

Blocking 🚫

Market is wrong in the opposite direction, which may or may not explain the disabled issue above

https://jam.dev/c/bba3dc8c-f3ec-4de2-a5cf-2b110d1ac909

Inputs are janky, probably as a result of the added 0-handling. Note they're also kind-of-janky on prod, but even more so now that users are able to input a buy amount with other amounts derived

https://jam.dev/c/9a951b51-bb65-426e-97fe-b0c27356b0cf

Note, this is not happening in spot. Not sure what was the rationale re: zero but we should probably reconsider that, unless we're confident we can make the inputs reliable 🙏🏽

<RateGasRow /> doesn't sync with the direction

https://jam.dev/c/b9d1fe99-e068-4cea-b337-31747a1914fc

gomesalexandre · 2025-03-11T16:28:08Z

src/assets/translations/en/main.json

+    "explanation": {
+      "priceReaches": {
+        "prefix": "When",
+        "middle": "'s price reaches"
+      },
+      "orderWill": {
+        "prefix": ", your limit order will automatically sell"
+      },
+      "ensuring": {
+        "prefix": ", ensuring you receive at least",
+        "suffix": "."
+      }
+    },


extremely nitpick: this won't generalise to every language syntax but hey

gomesalexandre · 2025-03-11T16:31:28Z

src/components/MultiHopTrade/components/LimitOrder/components/LimitOrderBuyAsset.tsx

@@ -34,6 +39,7 @@ export type LimitOrderBuyAssetProps = {
  isLoading: boolean
  selectedChainId?: ChainId | 'All'
  onSelectedChainIdChange?: (chainId: ChainId | 'All') => void
+  onChangeIsInputtingFiatSellAmount: (isInputtingFiatSellAmount: boolean) => void


extremely nitpick: perhaps less verbose here i.eisFiatInput / isSellAssetFiatInput

can ditch the sell terminology as it's impossible to trigger fiat input for receive, since there's no input for receive... yet. If you think we're going to implement (which we probably should tbh), then feel free to keep it!

syntax is backwards here and should be onXyzChange, which is even harder to visually grep with the verbosy namy

tl;dr feel free to rename this to onIsInputtingFiatSellAmountChange and call it a day

gomesalexandre · 2025-03-11T16:32:59Z

src/components/MultiHopTrade/components/LimitOrder/components/LimitOrderBuyAsset.tsx

    const manualReceiveAddress = useAppSelector(selectManualReceiveAddress)
+    const assetMarketData = useAppSelector(state =>


nitpick: buy/asset implied since this component is called LimitOrderBuyAsset

Suggested change

const assetMarketData = useAppSelector(state =>

const marketData = useAppSelector(state =>

gomesalexandre · 2025-03-11T16:33:25Z

src/components/MultiHopTrade/components/LimitOrder/components/LimitOrderBuyAsset.tsx

+        // Always process the input change, even if value is 0
+        if (sellAmountCryptoPrecision && Number(sellAmountCryptoPrecision) > 0) {
+          // If value is 0 or empty, set a zero rate
+          if (!value || Number(value) === 0) {


bnOrZero(value).isZero() ?

Also wondering if we ever need this branch? i.e 0 would work just the same in the regular flow as we already bnOrZero() it below?

gomesalexandre · 2025-03-11T17:20:39Z

src/components/MultiHopTrade/components/LimitOrder/components/LimitOrderConfig.tsx

-    // onChange will send us the formatted value
-    // To get around this we need to get the value from the onChange using a ref
-    // Now when the preset price mode buttons are clicked the onChange will not fire


gomesalexandre · 2025-03-11T17:30:05Z

src/components/MultiHopTrade/components/LimitOrder/components/LimitOrderFooter.tsx

+
+  const handleChangeExpiryOption = useCallback(
+    (newExpiry: string | string[]) => {
+      setExpiry(newExpiry as ExpiryOption)


q: given ExpiryOption is an enum of string values, do we want to paranoia throw if typeof newExpiry !== 'string'?

gomesalexandre · 2025-03-11T17:30:39Z

src/components/MultiHopTrade/components/RateGasRow.tsx

    onClick,
  }) => {
    const translate = useTranslate()
    const { isOpen, onToggle } = useDisclosure()

+    const deltaPercentagePercentage = useMemo(() => {


gomesalexandre · 2025-03-11T17:31:22Z

src/components/MultiHopTrade/components/RateGasRow.tsx

+    const deltaPercentagePercentage = useMemo(() => {
+      if (!deltaPercentage) return null
+
+      if (deltaPercentage.isGreaterThan(0.01) || deltaPercentage.isLessThan(-0.01)) {


gt(0) / lt(0) / isZero() and remove this branch?

gomesalexandre · 2025-03-11T17:36:02Z

src/components/MultiHopTrade/components/RateGasRow.tsx

@@ -23,6 +24,7 @@ type RateGasRowProps = {
  swapperName: SwapperName | undefined
  swapSource: SwapSource | undefined
  networkFeeFiatUserCurrency: string | undefined
+  deltaPercentage?: BigNumber | null


Nitpick: pass me as string which is always the better, more serializable option

.../MultiHopTrade/components/SharedTradeInput/SharedTradeInputFooter/SharedTradeInputFooter.tsx

NeOMakinG added 2 commits March 7, 2025 16:21

feat: improve limit order ui

1295b57

fix: finish human readable sentence

c748d59

NeOMakinG changed the title ~~feat: improve limit order ui~~ feat: improve limit order ui to be more human readable Mar 10, 2025

NeOMakinG marked this pull request as ready for review March 10, 2025 09:57

NeOMakinG requested a review from a team as a code owner March 10, 2025 09:57

gomesalexandre self-requested a review March 10, 2025 10:02

gomesalexandre requested changes Mar 10, 2025

View reviewed changes

fix: revert comment removal

8bed7bc

NeOMakinG force-pushed the limit-ui-changes branch from e36b23e to 8bed7bc Compare March 10, 2025 12:01

NeOMakinG added 2 commits March 10, 2025 16:50

fix: review feedbacks

368e0e7

fix: review feedbacks

995f7d7

gomesalexandre requested changes Mar 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve limit order ui to be more human readable #8970

feat: improve limit order ui to be more human readable #8970

NeOMakinG commented Mar 7, 2025 •

edited

Loading

gomesalexandre left a comment

gomesalexandre Mar 10, 2025

NeOMakinG Mar 10, 2025

gomesalexandre Mar 11, 2025

NeOMakinG commented Mar 10, 2025

gomesalexandre left a comment

gomesalexandre Mar 11, 2025

gomesalexandre Mar 11, 2025

gomesalexandre Mar 11, 2025

gomesalexandre Mar 11, 2025

gomesalexandre Mar 11, 2025

gomesalexandre Mar 11, 2025

gomesalexandre Mar 11, 2025

gomesalexandre Mar 11, 2025

gomesalexandre Mar 11, 2025

gomesalexandre Mar 11, 2025

		const manualReceiveAddress = useAppSelector(selectManualReceiveAddress)
		const assetMarketData = useAppSelector(state =>

	const assetMarketData = useAppSelector(state =>
	const marketData = useAppSelector(state =>

feat: improve limit order ui to be more human readable #8970

Are you sure you want to change the base?

feat: improve limit order ui to be more human readable #8970

Conversation

NeOMakinG commented Mar 7, 2025 • edited Loading

Description

Issue (if applicable)

Risk

Testing

Engineering

Operations

Screenshots (if applicable)

gomesalexandre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NeOMakinG commented Mar 10, 2025

gomesalexandre left a comment

Choose a reason for hiding this comment

Gucci ✅

Hot takes 🧑‍🚒

Blocking 🚫

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NeOMakinG commented Mar 7, 2025 •

edited

Loading