Exchange Quote Picker #4534

Jon-edge · 2023-10-19T17:26:40Z

Before final cleanup commit:

After final (WIP) cleanup commit:

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

Yes
No

Dependencies

none

Requirements

If you have made any visual changes to the GUI. Make sure you have:

Tested on iOS device
Tested on Android device
Tested on small-screen device (iPod Touch)
Tested on large-screen device (tablet)

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1205453208664797

paullinator · 2023-10-20T23:56:17Z

src/actions/CryptoExchangeActions.tsx

-  const feeDenominatedAmount = await fromWallet.nativeToDenomination(feeNativeAmount, request.fromWallet.currencyInfo.currencyCode)
-  const feeFiatAmountRaw = parseFloat(convertCurrency(state, request.fromWallet.currencyInfo.currencyCode, fromWallet.fiatCurrencyCode, feeDenominatedAmount))
-  const feeFiatAmount = formatNumber(feeFiatAmountRaw || 0, { toFixed: 2 })
+  const feeCryptoAmount = div(feeNativeAmount, feeDenomination.multiplier, DECIMAL_PRECISION)


feeDenomination is the display denom. You can't use this in convertCurrency. The nativeToDenomination works because (while poorly named) it returns the exchangeAmount

Mainly changed it just so my earlier commit would build - where I use this method in the scene selector. Needed this to become synchronous for that commit to work.

The intention is to later clean all this up to use common utility methods.

Since I don't even use this on the scene anymore by the last cleanup commit, I'll revert

paullinator · 2023-10-20T23:59:35Z

src/actions/CryptoExchangeActions.tsx

  return async (dispatch, getState) => {
    const state = getState()
    const { account } = state.core
    dispatch({ type: 'START_SHIFT_TRANSACTION' })

-    const { fromDisplayAmount, quote, request, fee, fromFiat, fromTotalFiat, toDisplayAmount, toFiat } = swapInfo
-    const { isEstimate, fromNativeAmount, toNativeAmount, networkFee, pluginId, expirationDate } = quote
+    const { fromDisplayAmount, fee, fromFiat, fromTotalFiat, toDisplayAmount, toFiat } = getSwapInfo(state, quote)


Seems like an unnecessary refactor to pull out getSwapInfo. Updating the fiat exchange rate after the user approved the quote will possibly save different fiat info in the metadata than what the user originally saw. I would prefer the old behavior

This entire commit was unnecessary work

There were a lot of motivations.

Work towards removing all these old guiXXX types, which are often tied into unnecessary redux and relied on a lot of duplicated handling for crypto/fiat numbers that were often inconsistent.

At least take a step towards unifying the handling for crypto/fiat amount displays for this feature. I implemented a lot of common display/conversion handling functionality with the intention of using towards this very purpose. Going to take a bit more work to get rid of all the crypto/fiat amount display handling in favor of common utils, hence leaving a lot of this here in getSwapInfo.

I probably could have broken up the commits better, but by the final commit at least the scenes and components can use the crypto/fiat hooks + utilities as intended by that design, and the api for the components becomes simple and makes sense for what the components are displaying. In the future we can unify crypto/fiat calcs for actions/reducers as well.

This is crazy overly complicated, prone to making mistakes (evidenced in this PR) and missing mistakes in reviews if we keep needing these giant blocks of conversion code every time we need to deal with simple fiat and crypto amounts.

If the only concern was fiat exchange rates, I'd argue that the new implementation is better.

In the old implementation, worse case scenario, the user is saving minute-old prices + the time it takes to execute. Sure, they're guaranteed to save what they see, but what they see isn't really the true price at that time.

In the new implementation, worse-case scenario for saved fiat price metadata is only the price delta in the time it takes the quote to execute.

EDIT: Actually I don't see anything in CryptoExchangeActions about saving amountFiat metadata... It's just falling back to historical data as far as I can tell?

paullinator · 2023-10-21T00:05:24Z

src/components/scenes/CryptoExchangeQuoteScene.tsx

@@ -154,12 +152,7 @@ export const CryptoExchangeQuoteScene = (props: Props) => {
              walletId={request.toWallet.id}
              walletName={getWalletName(request.toWallet)}
            />
-            <TouchableOpacity style={styles.pluginRowPoweredByRow} onPress={handlePoweredByTap}>
-              <EdgeText style={styles.footerText}>{lstrings.plugin_powered_by_space + ' '}</EdgeText>


footerText not used.

paullinator · 2023-10-21T00:19:16Z

src/components/scenes/CryptoExchangeQuoteScene.tsx

+    return (
+      <TouchableOpacity
+        onPress={() => {
+          navigation.replace('exchangeQuote', {


Instead of a full navigation.replace, would have been simpler to make selectedQuote a useState which you then change with a setSelectedQuote

Line 48 could be

const { initialSelectedQuote, quotes, onApprove } = route.params

Then

const [ selectedQuote, setSelectedQuote ] = useState(initialSelectedQuote)

paullinator · 2023-10-21T00:19:22Z

src/components/scenes/CryptoExchangeQuoteScene.tsx

+    return (
+      <TouchableOpacity
+        onPress={() => {
+          navigation.replace('exchangeQuote', {


Instead of a full navigation.replace, would have been simpler to make selectedQuote a useState which you then change with a setSelectedQuote

Line 48 could be

const { selectedQuote: initialSelectedQuote, quotes, onApprove } = route.params

Then

const [ selectedQuote, setSelectedQuote ] = useState(initialSelectedQuote)

paullinator · 2023-10-21T00:21:49Z

src/components/data/row/SwapProviderRow.tsx

+  const tokenId = isToQuote ? fromTokenId : toTokenId
+  const nativeAmount = isToQuote ? fromNativeAmount : toNativeAmount
+
+  const toAmount = useCryptoText({ wallet, tokenId, nativeAmount })


Misnomer. Might be a 'from' or 'to'. Should be exchangeAmount

Made it a bit more literal, exchange already has multiple meanings in our codebase

I'll accept the update, but exchangeAmount has a very specific meaning. I might agree that it's not the best term but we do use it consistently.

Jon-edge · 2023-10-24T01:28:14Z

Fixed unit tests on the last commit

src/components/themed/ExchangeQuoteComponent.tsx

paullinator · 2023-10-25T19:35:25Z

src/components/themed/ExchangeQuoteComponent.tsx

  })

+  const totalFiatText = `${getSymbolFromCurrency(fromIsoFiatCurrencyCode)}${formatFiatString({ fiatAmount: add(feeFiatAmount, fromFiatAmount) })}`


revert to using no symbol

This seemed to have been implemented with the intention of being a one-stop source for all GUI display requirements of the EdgeSwapQuote. However, as implemented, this prevents fiat exchange rates from ever being updated since it only gets set once upon initial receipt of the quote. Update this to be a utility function of an EdgeSwapQuote, remove some unecessary redux.

Minor adjustments made: use of FastImage + resizeMode. Text was not updated to EdgeText due to unresolved styling differences.

- Simplify props. All this is doing is visual representation of one side of a quote. - Make use of new hooks, utility methods, and common components. - Enforce visual consistency. Also has a beneficial side effect of removing more redux dependencies.

Jon-edge force-pushed the jon/exchange-quote-picker branch 5 times, most recently from 2af7400 to dcae8ce Compare October 20, 2023 23:03

paullinator requested changes Oct 21, 2023

View reviewed changes

Jon-edge force-pushed the jon/exchange-quote-picker branch 4 times, most recently from d9c1508 to ab2bc69 Compare October 24, 2023 01:26

paullinator approved these changes Oct 24, 2023

View reviewed changes

Jon-edge force-pushed the jon/exchange-quote-picker branch 2 times, most recently from 6eebf8d to 4df8fd8 Compare October 25, 2023 03:54

paullinator approved these changes Oct 25, 2023

View reviewed changes

Jon-edge added 5 commits October 25, 2023 17:15

Convert CryptoExchangeQuoteScene into a functional component

a7bc612

Refactor and add PoweredByCard

2af3db2

Minor adjustments made: use of FastImage + resizeMode. Text was not updated to EdgeText due to unresolved styling differences.

Implement provider list modal, display multiple quotes

7dfc65e

Refactor ExchangeQuote Component

49ef0f1

- Simplify props. All this is doing is visual representation of one side of a quote. - Make use of new hooks, utility methods, and common components. - Enforce visual consistency. Also has a beneficial side effect of removing more redux dependencies.

Jon-edge force-pushed the jon/exchange-quote-picker branch from 4df8fd8 to 49ef0f1 Compare October 26, 2023 00:35

Jon-edge merged commit afcd2b6 into develop Oct 26, 2023
1 of 2 checks passed

Jon-edge deleted the jon/exchange-quote-picker branch October 26, 2023 00:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exchange Quote Picker #4534

Exchange Quote Picker #4534

Jon-edge commented Oct 19, 2023 •

edited

Loading

paullinator Oct 20, 2023

Jon-edge Oct 24, 2023 •

edited

Loading

paullinator Oct 20, 2023

paullinator Oct 21, 2023

Jon-edge Oct 24, 2023 •

edited

Loading

Jon-edge Oct 24, 2023 •

edited

Loading

paullinator Oct 21, 2023

paullinator Oct 21, 2023

paullinator Oct 21, 2023

paullinator Oct 21, 2023

Jon-edge Oct 24, 2023 •

edited

Loading

paullinator Oct 24, 2023

Jon-edge commented Oct 24, 2023

paullinator Oct 25, 2023

		})

		const totalFiatText = `${getSymbolFromCurrency(fromIsoFiatCurrencyCode)}${formatFiatString({ fiatAmount: add(feeFiatAmount, fromFiatAmount) })}`

Exchange Quote Picker #4534

Exchange Quote Picker #4534

Conversation

Jon-edge commented Oct 19, 2023 • edited Loading

CHANGELOG

Dependencies

Requirements

Choose a reason for hiding this comment

Jon-edge Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jon-edge Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Jon-edge Oct 24, 2023 • edited Loading

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

Jon-edge Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jon-edge commented Oct 24, 2023

Choose a reason for hiding this comment

Jon-edge commented Oct 19, 2023 •

edited

Loading

Jon-edge Oct 24, 2023 •

edited

Loading

Jon-edge Oct 24, 2023 •

edited

Loading

Jon-edge Oct 24, 2023 •

edited

Loading

Jon-edge Oct 24, 2023 •

edited

Loading