-
Notifications
You must be signed in to change notification settings - Fork 46
Add bridge form #724
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
Add bridge form #724
Conversation
2c88d2e to
2d7eb48
Compare
|
📦 build.zip [updated at Apr 10, 3:54:04 PM UTC] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed ✅
| import React from 'react'; | ||
| import { animated, useTransition } from '@react-spring/web'; | ||
|
|
||
| export function SlidingRectangle({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this component doesn't work :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably because the animation works before image is loaded and shown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this didn't work before, so you don't need to fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to the "tech debt" list, lets fix this in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhm, what exactly isn't working? It seems to work for me in both Swap and Bridge forms
| direction, | ||
| }); | ||
|
|
||
| invariant( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check has been already made in config.ts file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! updated ✅
| }) { | ||
| const { currency } = useCurrency(); | ||
|
|
||
| const asset = name === 'receiveInput' ? receiveAsset : spendAsset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be it is the right time to deprecate receiveInput editing?
This feature is not well-supported and backend suggests not to use it if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't aware that it isn't well-supported... It works perfectly in a swap form. Did you mean it isn't well supported only for bridge? Or in swap as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not well supported under the hood :(
For bridge it is not supported at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I don't allow editing receiveInput in bridge, but regarding removing support for editing receiveInput in SwapForm... maybe lets discuss separately?
src/ui/shared/requests/useQuotes.ts
Outdated
| if (receiveChainInput) { | ||
| searchParams.append('output_chain', receiveChainInput); | ||
| } | ||
| if (slippage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slippage has been already defined on line 123 + there should be if (slippage != null) to allow 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, good catch, we always have slippagePercent (since we always have default slippage), so there is no need for this condition at all, updated ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just a copy of existing file with another props? Then we shouldn't forget to replace the previous on with this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated ✅
| color: var(--neutral-500); | ||
| } | ||
|
|
||
| .reverseButton:focus .spinArrow { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This style triggers animation on focus, not on click. Which works with click but not with keyboard navigation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I copied this part from webapp as is :)
I just removed :focus for now
| @@ -0,0 +1,181 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This state should be changed with the new pending animation. It was merged some time ago so may be is worth using here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated ✅
| title: string; | ||
| value: string; | ||
| networks: Networks | null; | ||
| openDialog: () => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onClick or onDialogOpen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have openDialog everywhere else (I mean in all variants of the DisclosureButton), so I decided to keep it as is
| map[position.asset.asset_code] = position; | ||
| } | ||
|
|
||
| return { sorted, map }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map: Object.fromEntries(sorted.map(position => ([position.asset.asset_code, position])));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, not important :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code block used the for loop before I extracted the function, so I left this unchanged,
you think its worth changing to Object.fromEntries + .map? the loop should be more efficient afaik
| receiverAddressInput?: string | null; | ||
| showReceiverAddressInput?: boolean; | ||
|
|
||
| gas?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gas is not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed gas field for now ✅
| queryFn: () => getBridgeTokens({ inputChain, outputChain, direction }), | ||
| suspense: false, | ||
| retry: false, | ||
| enabled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be it is better to type here enabled: Boolean(enabled) && Boolean(inputChain) && Boolean(outputChain)?
P.S. it is probably good that you restricted enabled no be not-undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, good idea, updated ✅
| export class TransactionsApiError extends HTTPError { | ||
| static interpretError(parsedError: ParsedHttpError) { | ||
| if (parsedError.detail && parsedError.detail.length > 0) { | ||
| const [{ msg }] = parsedError.detail; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see that this was taken from web app. Then let's hope it works correctly :))
| Bridge | ||
| </UIText> | ||
| <span> | ||
| {isLoading && !quote ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we allow user to see and use quote when the loading is not done yet?
May be we need to add such check here and to the main form?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it the point to show the progression of quotes? But we don't allow submit during isLoading
| transaction={currentTransaction} | ||
| from={address} | ||
| chain={spendChain} | ||
| paymasterEligible={false} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add // TODO: here to enable this one in the next releases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added ✅
|
|
||
| const positions = positionsResponse?.data ?? null; | ||
|
|
||
| const defaultFormValues = getDefaultFormValues({ networks, positions }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's wrap this with useMemo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated ✅
| network.id in positionDistribution | ||
| } | ||
| onChangeSourceChain={(value) => | ||
| setUserFormState((state) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand right that this can be replaced with handleChange?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, thanks! updated ✅
| })) | ||
| } | ||
| onChangeDestinationChain={(value) => | ||
| setUserFormState((state) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
5aaac0e to
6667062
Compare
| 'spendInput', | ||
| 'chainInput', | ||
| ]); | ||
| const { receiveAsset, spendAsset } = swapView; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these 2 vars can be extracted on line:28 :)
Not critical though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated ✅
src/ui/pages/BridgeForm/LabeledNetworkSelect/LabeledNetworkSelect.tsx
Outdated
Show resolved
Hide resolved
| > | ||
| <QuestionIcon | ||
| role="decoration" | ||
| role="presentation" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to presentation, idk what that "decoration" role means, it is "presentation" in other places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome thanks
| explorerUrl={ | ||
| hash ? networks.getExplorerTxUrlByName(spendChain, hash) : undefined | ||
| } | ||
| explorerUrl={explorerUrl ?? explorerFallbackUrl} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as we discussed It makes more sense to display links like this when briding (instead of explorer link to the "send" (first/source) tx of only), but in case there is no explorer link available we still might want to show that fallback link
| <UIText color="var(--primary)" kind="small/accent"> | ||
| {quote?.estimated_seconds | ||
| ? `~${formatSeconds(Number(quote.estimated_seconds))}` | ||
| {quote?.seconds_estimation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
estimated_seconds is deprecated, long live seconds_estimation
| const bridgeTransaction = useMemo( | ||
| () => (selectedQuote ? getQuoteTx(selectedQuote) : null), | ||
| [selectedQuote] | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I extracted this getQuoteTx fn, looks like it doesn't make much sense to prepare that transaction data structure inside the useQuotes hook, it is like a separate concern...
| enough_allowance?: boolean; | ||
| enough_balance: boolean; | ||
| slippage_type?: 'normal' | 'zero-slippage'; | ||
| estimated_seconds?: string | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deprecated
| output_amount_min: string; | ||
|
|
||
| token_spender: string; | ||
| exchanges: ExchangeWithName[] | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find this field in the API docs and we don't use it
| input_asset_id: params.inputAssetId, | ||
| output_asset_id: params.outputAssetId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input_token and output_token parameters are deprecated and replaced with those having the _asset_id suffix
| outputAssetId: receiveTokenInput, | ||
| inputPosition: spendPosition, | ||
| outputPosition: receivePosition, | ||
| sort: 'amount', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default we sort by the amount to prioritize the best quote
0b2610d to
c21d6ae
Compare
| width: 16, | ||
| height: 16, | ||
| width: 20, | ||
| height: 20, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is 20x20 in design
c21d6ae to
e4b170e
Compare
| address, | ||
| userSlippage, | ||
| primaryInput, | ||
| primaryInput: primaryInput ?? 'spend', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was an assumption for the primaryInput to be of type 'spend' anyway, let's make it explicit (less undefined types)
e4b170e to
5f89015
Compare
b8f9340 to
4183422
Compare
| const { refetch: refetchQuotes } = quotesData; | ||
|
|
||
| const defaultQuote = quotesData.quotes?.[0] ?? null; | ||
| // TODO: add support for quote selection, useState |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will have a hard merge...
| position: 'relative', | ||
| width: 20, | ||
| height: 20, | ||
| // overflow: 'hidden', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deleted ✅ thanks
| transaction: bridgeTransaction, | ||
| quote, | ||
| refetch: refetchQuotes, | ||
| } = quotesData; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh, I think, I'll move quote selection logic inside quotesData when it is done. Just to avoid doubling the logic in swap/bridge ¯\_(ツ)_/¯
src/ui/shared/requests/useQuotes.ts
Outdated
| if (params.refetchHash) { | ||
| searchParams.append('refetchId', params.refetchHash.toString()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed ✅ I don't even remember where I get this lines from
src/ui/shared/requests/useQuotes.ts
Outdated
| maxFee?: string; | ||
| sourceId?: string; | ||
| sort?: QuoteSortType; | ||
| refetchHash?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
src/ui/shared/requests/useQuotes.ts
Outdated
| const { slippagePercent } = getSlippageOptions({ | ||
| chain, | ||
| userSlippage, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be oneline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
Uh oh!
There was an error while loading. Please reload this page.