Added a warning icon if transfer cost is higher than token market value #236
Conversation
|
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
| @@ -108,13 +114,52 @@ const App: React.FC = () => { | |||
| } | |||
| }, [balancesError]); | |||
|
|
|||
| useEffect(() => { | |||
There was a problem hiding this comment.
What do you think about creating a useWeb3 hook? for avoid passing down props to de final CurrencyCell. You can gather the chains.ts info there as well as we are going to remove it later
There was a problem hiding this comment.
I think that it's a good idea to create a useWeb3 hook, but I think that we should keep it in the App.tsx because we only want instantiate web3 and call getGasPrice only once and not in each CurrencyCell.
There was a problem hiding this comment.
+1 for a separate hook, I find that it vastly improves readability
There was a problem hiding this comment.
Could perhaps be a separate ticket @dasanra ? as TX Builder and Drain Safe can share the same hook and as @DaniSomoza mentioned me offline, we will need a context provider as well for avoid instance recreation.
There was a problem hiding this comment.
Why would we want to avoid it? In the drain-safe app, it's only used in one component and then passed down via props. To me, it looks like just moving it to a separate file
There was a problem hiding this comment.
For avoid the prop to only exist to be sent down. In this case in the Balances I think
There was a problem hiding this comment.
but the context is fine isn't it ? I mean, a context for the web3 instance and a hook to instantiate it using our getChainInfo populated with RPC url (task in progress) could be helpful for retrieving it in any level of the component tree
There was a problem hiding this comment.
done! Added useWeb3.js hook
apps/drain-safe/src/utils/chains.ts
Outdated
| [key in CHAINS]: null | ((token?: string) => string); | ||
| } = { | ||
| [CHAINS.MAINNET]: (token) => `https://mainnet.infura.io/v3/${token}`, | ||
| [CHAINS.MORDEN]: null, |
There was a problem hiding this comment.
Do we want to include chains to this enum if we don't support them?
There was a problem hiding this comment.
We are doing a copy 1:1 of what we have on TX-Builder. This is to be deprecated with #235 in both apps
| @@ -108,13 +114,52 @@ const App: React.FC = () => { | |||
| } | |||
| }, [balancesError]); | |||
|
|
|||
| useEffect(() => { | |||
There was a problem hiding this comment.
+1 for a separate hook, I find that it vastly improves readability
| <Balances | ||
| ethFiatPrice={ethFiatPrice} | ||
| gasPrice={gasPrice} | ||
| web3={web3} |
There was a problem hiding this comment.
With the useWeb3 new hook you don't really need to pass it down right?
There was a problem hiding this comment.
Yep, I decided to keep the const { web3 } = useWeb3(); and setGasPrice useState + useEffect in the App.tsx, But maybe makes sense to declare it directly in the Balances Component
There was a problem hiding this comment.
Really in CurrencyCell. In Balances you don't need i think
There was a problem hiding this comment.
but as we talked in the other thread we will need a context provider for avoid instance recreation. And we will address it in a different ticket.
There was a problem hiding this comment.
ahh right 😂 ... so let's improve it later
|
Tested and verified, top job my good Dani |
What it solves
Resolves #164
How this PR fixes it
Added a Warning Icon & Tooltip if transfer cost is higher than token market value.
Also Added a new
CurrencyCellComponentHow to test it
Screenshots