feat: split trade rates and quotes final wire-up

[gomesalexandre]

Description

TODO:

• sign and broadcast button loading state and disable while final quote is loading
• Portals TBD gas
• confirmed with @reallybeard a modal on quote gotten 1 la 1inch will do the trick, implemented but needs styling check with product how the new quote should look like
• fix "Not enough xyz to cover gas" when no wallet is connected
• ensure don't referentially invalidate selected quote on wallet connect -> maybe a stretch?
• fetch "final locked quotes" at pre-Tx signing time
  • clean useGetTradequotes up
  • give it a better home, maybe, vs. it being a hook?
• ZRX no permit2
• ZRX permit2: TBD last when everything else is nice and working. permit2 should fetch "final locked quotes" at pre-permit2 signing time, vs. pre-Tx signing time for others. That's required because we can't just get random permit2 eip712 data with a dummy quote, we need the exact permit2 data to compute the hash, and any other data will revert. Furthermore, abusing the swap endpoint for the purpose of getting permit2 eip712 is a very bad practice: After obtaining the quote, the next step is to sign the Permit2 EIP-712 message. Smart contract wallets and multi-sig wallets need to sign this message securely to authorize the swap. (https://0x.org/docs/0x-swap-api/guides/smart-contract-wallet-integration#4-fetch-a-firm-quote
  )
• CoW
• Li.Fi - pay particular attention to this one as it's very dangerous. It may look like it's working but it may not. When we fetch quote vs. rate previously, we will most likely end up
• either with a different tool because of the time elapsed between the two reqs. We should ensure the same tool is used, which could be problematic if the tool isn't available in the new quote.
• or with no tool at all, in which case the confirm component will empty out
  Both states are inherently bad, and we will have to work around that
• Portals bloodbath
• THOR
• Arb Bridge

Issue (if applicable)

closes #7941

Risk

High Risk PRs Require 2 approvals

🚨 THIS 😱 RIGHT ⚠️ HERE 😤 IS 💅 LITERALLY 🙈 THE 💃 MOST 🎲 HIGHEST ⬆️ RISKIEST 💣 RISKY 🔥 RISK 🎯 THAT 😈 EVER 💅 RISKED 💀 IN 🌪️ THE 🦹 HISTORY 📚 OF 💫 RISKS 🎲 BESTIE 💅 NO 🙅‍♀️ CAP 🧢 FR 💯 FR 💯

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

Testing

• Rates should be blazingly fast
• Can now get rates for swappers without a wallet
• At pre-signing time, or whenever applicable, we get a final quote. This can mean three different things:
  • As a rule, it means that before signing the Tx, a quote will be fetched, vs. a rate previously, using wallet BIP44 params/receive addy as params
  • For ZRX with permit2, that's a bit different - it will be fetched before the permit2 signing, as the final quote is tied to permit2 eip712 data and we can't refetch a new quote after, else we would require another eip712 permit2 signing
  • For Li.Fi, we actually get an actual quote at input step (or rather, a rate with maybe receiveAddress if the wallet is connected, so a quote or rate) and always get the final one as soon as a wallet is connected. The reason for that is pretty simple, as Li.Fi tools may change in the few seconds it take to go from input to confirm, and the whole rates vs. quotes thing means Li.Fi would break most of the time if we always re-fetched at pre-signing time.
• A modal is displayed when the rate has changed to inform the user (pending @reallybeard oil)
• All txs still go through
• Funds still land on the correct address, regardless of wallet/manual receive address
• Gas is expected to sometimes be missing at input step e.g for Portals, but should eventually be there at pre-signing time

Engineering

Operations

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

Screenshots (if applicable)

[NeOMakinG]

✅ Fetching rate without being connected

https://jam.dev/c/8a85ecab-202d-48a2-b2c5-7f0b920e68ae

✅ Swap using thorchain

https://jam.dev/c/e016404c-3e9d-4551-9732-cef9f243ee36

✅ Swap using Li.Fi

✅ Swap using 0x

https://jam.dev/c/79388f55-0302-429d-8275-cd103e87843b

✅ Swap using chainflip

https://jam.dev/c/0f2ef4f0-5a19-444c-a896-53f239799eb0
To external address
https://jam.dev/c/187036cc-2792-4a32-82c6-ef21278ab970

✅ Portals

Gas weren't there before quote, then appeared, seems as promised
https://jam.dev/c/f63681c4-1ecd-4b94-b1fc-40bc0b6f0be5

Cowsap swap ✅ but gas fees ❌

Gas fees didn't even worked at quote time for some reason, is this expected @gomesalexandre ?

Everything else looks fine, I would be able to stamp after a response for the cow gas fees

[gomesalexandre]

Cowsap swap ✅ but gas fees ❌

Gas fees didn't even worked at quote time for some reason, is this expected @gomesalexandre ?

Totally expected ser @NeOMakinG! There's no Tx fees for CoW, since there is no Tx but a message signing!
Fees for CoW are only protocol fees, non-payable as a downside on the buy amount.

[0xApotheosis]

One thought on this whilst testing: in cases where we don't have a gas estimate on rate fetch (e.g. for Portals), in this branch we unfairly list that swapper above other swappers that do have a gas estimate.

In the example below LiFi is actually a much better rate overall (very low gas vs what Portals will end up being), but it ranks below Portals.

[0xApotheosis]

Next thought:

We should probably show the difference in the rate, maybe as a percentage here. This popped up every time for me when using Portals (maybe because we go from not knowing gas to knowing gas? This itself feels like a bug/undesirable behavior), and every time I had to try work out if the rate got better or worse, but I didn't know because I had no reference point.

[0xApotheosis]

Tested:

✅ Cross-account trades
✅ 0x trades (with Permit2)
✅ CoW trades
✅ Portals trades
✅ LiFi trades
✅ Rate changing modal (though see comment above, I think this is way too noisy)

❌ I was able to get into a strange state, though I'm not sure exactly how I did it. It was consistent once I got into the state though, and was able to Jam it:

https://jam.dev/c/81739f45-0a4e-4a2f-9b35-50c84ab3305d

[gomesalexandre]

❌ I was able to get into a strange state, though I'm not sure exactly how I did it. It was consistent once I got into the state though, and was able to Jam it:

jam.dev/c/81739f45-0a4e-4a2f-9b35-50c84ab3305d

Urgh, very nice catch @0xApotheosis and quite a bad one indeed, I was also able to consistently repro with ZRX (which is the odd one with the weird time of final quote fetching). Read more to dig in a proper rabbit hole.

There were a few things wrong with this one, mostly stemming from incorrect cross-account checks:

The naive solution

First of all, we were doing undefined checks instead of empty checks in useGetSwapperTradeQuoteOrRate, which, for ZRX, means that we were upserting an undefined quote, resulting in consistent early null returns in swapper. This bug is indeed specific to ZRX given the final quote flow kicks earlier. Fixed in f77e9ac

However, fixing that still made us unable to sign permit2:

We were never upserting a final quote, as we were early bailing from the getTradeQuote() endpoint - or rather, we were upserting empty (final quote) data, meaning that we would never have the final quote permit2 data required for signing.

Getting to a root cause fix... almost (but actually fixing a prod bug in the process!)

The reason why we weren't is we were checking for equal addresses, which, when using the same address as your wallet address but as a custom one, and dealing with checksum addresses (e.g copied over from MM) would fail equality checks, and result in the trade being wrongly detected as cross-account when it is in fact not.
There is actually the reverse issue on prod currently, where inputting a non-checksummed address will fail said checks and not yield a quote, see develop below using the same address as my wallet one, non-checksummed on the first, checksummed on the second:

Fixed in f8e2b7d

The actual root cause fix

Now, this obviously exposes another issue: we were able to get rates for cross-account trades, which shouldn't be possible (assuming you did provide a receive address or have a wallet connected).

You'll notice that the previous emptiness check has been reverted, which is expected. The whole reason why we ended up in such weird state in the first place was because we were able to continue with cross-account trades when we shouldn't have been able to. By fixing this, we fix the root cause that warranted the emptiness checks in the first place. Keeping the emptiness early return vs. undefined early return means that we were not anymore setting empty quote data, which in turns meant that in selectIsSwapperResponseAvailable:

web/src/state/slices/tradeQuoteSlice/selectors.ts

Lines 86 to 101 in a78a164

	// Returns a mapping from swapper name to a flag indicating whether a trade quote response is
	// available.
	export const selectIsSwapperResponseAvailable = createDeepEqualOutputSelector(
	selectTradeQuotes,
	selectEnabledSwappersIgnoringCrossAccountTrade,
	(tradeQuotes, enabledSwappers) => {
	return enabledSwappers.reduce(
	(acc, swapperName) => {
	const swapperResponse = tradeQuotes[swapperName]
	acc[swapperName] = swapperResponse !== undefined
	return acc
	},
	{} as Record<SwapperName, boolean>,
	)
	},
	)

The swapperResponse !== undefined check would return false, meaning that we would consider this response as unavailable (despite the RTK query resolving with an empty object as expected) in selectLoadingSwappers:

web/src/state/slices/tradeQuoteSlice/selectors.ts

Lines 643 to 658 in a78a164

	export const selectLoadingSwappers = createSelector(
	selectIsSwapperResponseAvailable,
	selectIsTradeQuoteApiQueryPending,
	selectTradeQuoteDisplayCache,
	(isSwapperQuoteAvailable, isTradeQuoteApiQueryPending, tradeQuoteDisplayCache) => {
	return Object.entries(isSwapperQuoteAvailable)
	.filter(
	([swapperName, isQuoteAvailable]) =>
	// only include swappers that are still fetching data
	(!isQuoteAvailable || isTradeQuoteApiQueryPending[swapperName as SwapperName]) &&
	// filter out entries that already have data - these have been loaded and are refetching
	!tradeQuoteDisplayCache.some(quoteData => quoteData.swapperName === swapperName),
	)
	.map(([swapperName, _isQuoteAvailable]) => swapperName)
	},
	)

This has all been fixed by bringing back receiveAddress in trade rates input all the way to the RTK query, so we can leverage cross-account checks at rate time, but then eventually passing undefined to the inner getTradeRate swapper methods so we have the expected correct lower-level input.

While at that, also fixed a bug with native EVM assets signing being borked for ZRX, since we correctly got final quote at pre-permit2 signing time when permit2 was required, but were never getting it for native assets (where permit2 is not required, meaning it should be fetched at pre-Tx signing time as usual)

See full regression test here including cross-account, and ensuring we do guard against cross-account detection: https://jam.dev/c/0afbf6dd-11f3-4d76-81e4-e28ccef8c802

And fix in 358c8a7

[0xApotheosis]

Nitpick, none-blocking: when we can't use a swap due to the trade being cross-account we should show the swapper in the "unavailable swappers" section (Portals & 0x in this example).

[0xApotheosis]

Re-tested with a focus on manual receive address and 0x trade permutations.

✅ Using a manual receive address that is different from the send receive address correctly does not show 0x or Portals (fine with a manual receive address with the same address - an edge case, but expected behaviour)

✅ A trade where a manual receive address is specified is correctly received by the specified address

✅ 0x permit2 trades work as expected

✅ 0x non-permit2 trades work as expected

Phew, well done ser - monster PR.

[NeOMakinG]

Smoke test quote/rate of all swappers (not E2E)

First test with portal: https://jam.dev/c/c2f08b08-cc14-4111-ad75-c2c8beb0bc49
I can click on the Sign Transaction button while the rate is not fetch yet, nitpick but could we had some loaders and skeletons everywhere while the rate is fetching?
https://jam.dev/c/3b60e0f8-5dd1-42c9-9e75-31c8b3c17288

Sometime it feels super weird but the rate expire, it should be the first time we fetch it? We shouldn't see the modal? Ex with Li.Fi: https://jam.dev/c/fa642e26-a462-49a0-aa84-fb05f3df1588 , not a blocker but it doesn't seems the best UX tbh

Manual receive address (not the same address)

https://jam.dev/c/9cec6b35-ecd7-4203-8962-a6f930a2e943

After a few minutes, using an external address, I couldn't sign a trade from BTC to ETH (https://jam.dev/c/d93e1e1f-86ac-41ed-b0aa-e631e0a7b00b)

Retrying after refresh did work

Manual receive address (same address)

https://jam.dev/c/ea3b0972-ae47-48a4-afaf-daf14ef222b3

Looks good so far, I didn't test at runtime for most of them except one or 2 with 0x, as I already tested the most in the first testing session, but very good PR