feat: SendProvider #2078
feat: SendProvider #2078
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| // AmountInputTypeSwitch uses a skeleton for both loading and error states | ||
| // SendAmountInputTypeSwitch uses skeleton for the loading display | ||
| // SendAmountInputTypeSwitch uses a custom error display (see loadingDisplay default) | ||
| // SendAmountInputTypeSwitch uses skeleton for the loading display but a custom error display (see loadingDisplay default) |
There was a problem hiding this comment.
the error state is called loadingDisplay?
There was a problem hiding this comment.
I think we should let exchangeRateLoading be handled by AmountInputTypeSwitch and just return early if there's an error - any reason we can't do that @brendan-defi?
There was a problem hiding this comment.
@alessey Yes, right now AmountInputTypeSwitch shows "loadingDisplay" for both loading state and exchange-rate-not-present state:
There was a problem hiding this comment.
@dschlabach I think I've taken that approach now, let me know what you think
| }, | ||
| }); | ||
| } | ||
| setIsInitialized(true); |
There was a problem hiding this comment.
yeah, good call, this is only based on eth balance right now. refactored in 0cc4028
|
|
||
| const tokenSymbol = selectedToken.symbol; | ||
| const tokenAddress = selectedToken.address; | ||
| let tokenParam: PriceQuoteToken; |
There was a problem hiding this comment.
nit: const tokenParam = selectedToken.symbol === 'ETH' ? 'ETH' : selectedToken.address; if (tokenParam) useExchangeRate()?
| statusName: 'amountChange', | ||
| statusData: { | ||
| isMissingRequiredField: true, | ||
| sufficientBalance: |
There was a problem hiding this comment.
hasSufficientBalance? may want to hoist this calculation to the top of this function
There was a problem hiding this comment.
hoisted to the top in a memo
| @@ -21,7 +26,7 @@ export type SendContextType = { | |||
|
|
|||
| // Sender Context | |||
| /** The balance of the sender's ETH wallet */ | |||
| ethBalance: number | undefined; | |||
| ethBalance: number; | |||
There was a problem hiding this comment.
should this have an undefined state to differentiate fetched and 0 from haven't fetched?
There was a problem hiding this comment.
yes, now that we are using this as the derivation scheme, we need an undefined option. updated in 0cc4028
| loadingDisplay = ( | ||
| <div className={cn(text.caption, color.foregroundMuted, 'h-[1.625rem]')}> | ||
| Exchange rate unavailable | ||
| </div> | ||
| ), |
There was a problem hiding this comment.
I think using the tags directly as default props will cause React to re-instantiate them on every render so yhou should probably extract this out to a cmponent
There was a problem hiding this comment.
updated how we handle error state
| if (errorDisplay) { | ||
| return errorDisplay; | ||
| } |
There was a problem hiding this comment.
We can check for this and return earlier in the function
There was a problem hiding this comment.
not sure i understand. the only thing earlier is an earlier return for loading state:
| classNames?: { | ||
| container?: string; | ||
| tokenName?: string; | ||
| tokenValue?: string; | ||
| fiatValue?: string; | ||
| action?: string; | ||
| }; |
There was a problem hiding this comment.
It looks like all of these classes are applied in the same element. Is it necessary to separate them in the prop definition?
There was a problem hiding this comment.
the alternative would be containerClassnames, tokenNameClassnames, etc. right?
i thought the preferred structure for multiple classNames props was an object like this.
| ...queryOptions, | ||
| }; | ||
|
|
||
| const queryKey = ['getPriceQuote', token]; |
There was a problem hiding this comment.
nit, don't need to define a variable here just use it in the hook
| const [cryptoAmount, setCryptoAmount] = useState<string | null>(null); | ||
|
|
||
| // state for transaction data | ||
| const [callData, setCallData] = useState<Call | null>(null); |
There was a problem hiding this comment.
nit - calldata is the solidity world so we should just do calldata, setCalldata
| useEffect(() => { | ||
| fetchTransactionData(); | ||
| }, [fetchTransactionData]); |
There was a problem hiding this comment.
I think you should move this into an internal util hook that uses React Query:
- It'll improve readability by moving fetching logic into its own module
- It'll be easier to handle things like refetching, cache management in case we ever need to tweak those parameters
There was a problem hiding this comment.
makes sense. does this hook belong in src/transaction?
| useEffect(() => { | ||
| const ethBalance = tokenBalances?.find((token) => token.address === ''); | ||
| if (!ethBalance || ethBalance.cryptoBalance === 0) { | ||
| setEthBalance(0); | ||
| updateLifecycleStatus({ | ||
| statusName: 'fundingWallet', | ||
| statusData: { | ||
| isMissingRequiredField: true, | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| setEthBalance( | ||
| Number( | ||
| formatUnits(BigInt(ethBalance.cryptoBalance), ethBalance.decimals), | ||
| ), | ||
| ); | ||
| updateLifecycleStatus({ | ||
| statusName: 'selectingAddress', | ||
| statusData: { | ||
| isMissingRequiredField: true, | ||
| }, | ||
| }); | ||
| }, [tokenBalances, updateLifecycleStatus]); |
There was a problem hiding this comment.
It seems like ethBalance could be derived state (it doesn't feel like it should be a side effect of tokenBalances loading in).
I wasn't sure what you were doing with the lifecycle statuses here but maybe there's a way to move them somewhere else where it's not a side effect of tokenBalances either?
What changed? Why?
getPriceQuoteusePriceHookNotes to reviewers
How has it been tested?