Feat/live 3629 add dex providers #1291
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
π¦ Changeset detectedLatest commit: 92b03b1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
common
sarneijim
requested review from
Justkant,
hedi-edelbloute and
chabroA
and removed request for
chabroA,
Justkant and
hedi-edelbloute
Screenshots: β
There are no changes in the screenshots for this PR. If this is expected, you are good to go. |
Codecov ReportBase: 43.84% // Head: 45.91% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #1291 +/- ##
===========================================
+ Coverage 43.84% 45.91% +2.06%
===========================================
Files 609 642 +33
Lines 25501 27157 +1656
Branches 6947 7250 +303
===========================================
+ Hits 11180 12468 +1288
- Misses 14275 14634 +359
- Partials 46 55 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. β View full report at Codecov. |
|
We need to disable the 'Exchange' button when nothing valid is selected @sarneijim. Otherwise we can proceed and cause a crash Screen.Recording.2022-09-16.at.11.49.22.mov |
|
There was a problem hiding this comment.
The overall idea is good. Basically what you want to do is handle the static list of DEX providers on the "UI" side for now and only on LLD.
But you need to handle dex static providers differently than centralised / backend provided providers to avoid added complexity and risk. For now you are mixing them in some sort of generic logic when they are actually totally different entities / concepts.
| {decentralizedSwapAvailable && | ||
| DEX_PROVIDERS.map((rate, index) => ( | ||
| <Rate | ||
| key={rate.rateId || index} |
There was a problem hiding this comment.
There is no rateId for your dex providers objects. You should rather use provider name rate.provider (assuming we have only one entry per dex provider which is the case today).
Also, using index as key should be avoided, especially when the order of items may change in the array (which is the case here since centralised rates are dynamic and are updated every 30 seconds).
cf. here for more details on that -> https://reactjs.org/docs/lists-and-keys.html#keys
Also, and for the same reasons, you should change the key property in the above centralised rate component (line 145).
Not all centralised rates have rateId, you could then construct the unique key for a centralised rate using a combination of the ExchangeRate provider and tradeMethod property, like:
key={`${rate.provider}-${rate.tradeMethod}`}| {centralized && ( | ||
| <Box> | ||
| <Box style={{ height: "19.5px", justifyContent: "center" }}> | ||
| <Price | ||
| withEquality | ||
| withIcon={false} | ||
| from={fromCurrency} | ||
| to={toCurrency} | ||
| rate={value.magnitudeAwareRate} | ||
| fontWeight="600" | ||
| /> | ||
| </Box> | ||
| <Text fontSize={3} color="palette.text.shade40"> | ||
| <Trans | ||
| i18nKey={ | ||
| value.tradeMethod === "fixed" | ||
| ? "swap2.form.rates.fixed" | ||
| : "swap2.form.rates.float" | ||
| } | ||
| /> | ||
| </Text> | ||
| </Box> | ||
| )} | ||
| </Box> | ||
| <Box alignItems="flex-end" flex={1}> | ||
| {centralized ? ( | ||
| <> | ||
| <FormattedVal | ||
| inline | ||
| fontSize={4} | ||
| val={amount} | ||
| currency={toCurrency} | ||
| unit={toCurrency?.units[0]} | ||
| showCode={true} | ||
| color="palette.text.shade100" | ||
| fontWeight="600" | ||
| /> | ||
| </Box> | ||
| <Text fontSize={3} color="palette.text.shade40"> | ||
| <Trans | ||
| i18nKey={ | ||
| value.tradeMethod === "fixed" | ||
| ? "swap2.form.rates.fixed" | ||
| : "swap2.form.rates.float" | ||
| } | ||
| <CounterValue | ||
| fontSize={3} | ||
| inline | ||
| currency={toCurrency} | ||
| value={amount} | ||
| disableRounding | ||
| showCode | ||
| color="palette.text.shade40" | ||
| /> | ||
| </> | ||
| ) : ( | ||
| <Text | ||
| fontSize={3} | ||
| color="palette.text.shade40" | ||
| style={{ width: "110px", textAlign: "right" }} | ||
| > | ||
| <Trans i18nKey={"swap.providers.noQuote"} /> | ||
| </Text> | ||
| </Box> | ||
| </Box> | ||
| <Box alignItems="flex-end" flex={1}> | ||
| <FormattedVal | ||
| inline | ||
| fontSize={4} | ||
| val={amount} | ||
| currency={toCurrency} | ||
| unit={toCurrency?.units[0]} | ||
| showCode={true} | ||
| color="palette.text.shade100" | ||
| fontWeight="600" | ||
| /> | ||
| <CounterValue | ||
| fontSize={3} | ||
| inline | ||
| currency={toCurrency} | ||
| value={amount} | ||
| disableRounding | ||
| showCode | ||
| color="palette.text.shade40" | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Your Rate component is getting quite complex and messy by trying to handle centralised and decentralised providers in the same place. Especially since both centralised and decentralised rates have different sets of props.
You should rather break it down and use composition to have 2 different end components, a CentralisedRate (or CentralisedProvider) and a DecentralisedRate (or DecentralisedProvider, because for DEX we don't actually have rate).
You could have a generic Rate component for all the common logic / style and have the centralised and decentralised ones being specialised version of this generic Rate one.
cf. https://reactjs.org/docs/composition-vs-inheritance.html
There was a problem hiding this comment.
Totally agree. Done.
| @@ -492,10 +508,9 @@ const SwapForm = () => { | |||
| )} | |||
|
|
|||
| <Box> | |||
| <Button primary disabled={!isSwapReady} onClick={onSubmit} data-test-id="exchange-button"> | |||
| <Button primary onClick={onSubmit} data-test-id="exchange-button"> | |||
There was a problem hiding this comment.
Why did you get rid of the disable prop?
Now the button is enabled even if the user can't swap (for example in the nominal state when no rate is provided or when the user needs to do login or KYC).
To enable the button when a dex provided is selected, you should rather update the isSwapReady const, which starts to become a bit convoluted / complex so it could also be a good opportunity to break it down in an aggregation os smaller individual conditions, for example:
const hasError = error || providersError || swapError;
const isLoading = swapTransaction.bridgePending ||Β exchangeRatesState.status !== "loading";
const isSwapReady = !hasError && !isLoading && (...)
There was a problem hiding this comment.
Extra change to test. This change has been already removed.
| @@ -1,5 +1,5 @@ | |||
| import { useCallback, useEffect, useReducer, useState } from "react"; | |||
| import { getExchangeRates } from ".."; | |||
| import { dex, getExchangeRates } from ".."; | |||
There was a problem hiding this comment.
Also, there is some logic duplication since you handle the list of dex providers in LLC and in LLD
There is therefore no single source of truth.
I would rather only handle this static dex list on the UI side (LLD as of today), like it was done for the original banner.
The main difference being that now you are appending these 2 states providers entries (they are actually not rates per say) to the dynamic list of providers returned by the API.
Which, by the way, seems to already be what you are doing in your Rates.index.tsx component (cf. here)
There was a problem hiding this comment.
Any logic has been removed from LLC
| @@ -100,7 +100,7 @@ const swapProviders: Record<string, SwapProviderConfig> = { | |||
| }; | |||
|
|
|||
| const getProviderConfig = (providerName: string): SwapProviderConfig => { | |||
| const res = swapProviders[providerName.toLowerCase()]; | |||
| const res = swapProviders[providerName.toLowerCase()] || {}; | |||
There was a problem hiding this comment.
This is error prone and also not really necessary once you rework / break down your Rate component since the DEXs should not require / need registration nor KYC
There was a problem hiding this comment.
Any logic has been removed from LLC
|
@sarneijim The 1inch text has been fixed but 1inch is still below Paraswap in the list. Other than that it looks look. I'll test again once Alex's comments have been addressed. |
|
@sarneijim quick question / feedback that I forgot in my first review, I didn't see the category (all / centralised / decentralised) filter present in the Figma. Is it an oversight or is it out of scope for this PR? |
Yes, it is out of scope. The filter will be addressed in the future. |
Now that rates and providers are display in the rates list, the SectionProvider is not used anymore
KYX status is not needed in the rates list section
|
Looks good to me too. Still some things to be added/fixed but this will come in future iterations:
|
π Description
Add dex provider to the provider list
Out of scope: Filter (it will be developed in the next pr, out of scope next release)
β Context
β Checklist
πΈ Demo