search

scaffold-eth / scaffold-eth-2

Open source forkable Ethereum dev stack
https://scaffoldeth.io
MIT License
1.19k stars 745 forks source link

Add use effect on Balance component for the price #856

Closed carletex closed 1 month ago

carletex commented 1 month ago

The usdMode flag wasn't working on our Balance component, since the price takes some time to load and we were always setting displayUsdMode to false on the first render (price > 0 condition)

You can test in page.tsx 

/* This will always show the Balance in ETH mode, even if we say usdMode */
<Balance address={connectedAddress} usdMode />

This PR adds the missing useEffect (as in EtherInput).

rin-st commented 1 month ago

Fix works great!

But I think we need to consider rewriting a bit the logic of the Balance and EtherInput components (in another pull request). Because usdMode prop is basically doesn't mean real mode inside the component because of internal toggling logic. I'm thinking about two options

  1. Rename usdMode to defaultUsdMode and use that value as default in useState. Remove useEffect. (I prefer this)
  2. Remove inner usd/eth logic. Use only usdMode prop (or mode)

Yes, with that options we can't change mode of let's say group of EtherInputs, but not sure we need it.

Also, who remembers why we added that price > 0 condition? Looks like it's redundant

carletex commented 1 month ago

Thanks for the review @rin-st !

who remembers why we added that price > 0 condition? Looks like it's redundant

The USD mode only makes sense when we have a price for the native currency. If it's loading (or if the fetch fails) it doesn't make sense to show the USD mode... since you won't be able to convert to the native currency (which is what we want for any onchain operation / show balance, etc). So I think that was the reason.

Rename usdMode to defaultUsdMode and use that value as default in useState. Remove useEffect. (I prefer this)

This will only work if we ignore my previous comment (the price condition). I agree that defaultUsdMode might be a better name.

Remove inner usd/eth logic. Use only usdMode prop (or mode)

I don't like this one since it'd require people to do their logic for the basic toggle USD / ETH.

I'm not sure if these components need a refactor, but like you said, it should probably happen in another PR.

I just created this one as a hotfix, to tackle a bug that is currently happening on main (and tried to be consistent with other components)

rin-st commented 1 month ago

Thank you for explanation!

I just created this one as a hotfix, to tackle a bug that is currently happening on main (and tried to be consistent with other components)

Agree, lgtm!

rin-st commented 1 month ago

Thanks for your answer Shiv, makes sense. But I don't understand how component will work ifdefaultUsdMode prop changes? I think we need to tinker with that component and find the best solution in the process. I see one problem with naming: If prop named defaultUsdMode it should affect only default inner value, and not change it later if prop value changes. (See point 1) https://github.com/scaffold-eth/scaffold-eth-2/pull/856#issuecomment-2138012845 For me with current logic it's better to leave prop name as is.