[2847] Bug in token balance #2866
+136
−104
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,18 @@ | ||
| import React, { useEffect } from 'react'; | ||
| import PropTypes from 'prop-types'; | ||
| import { StyleSheet, View, Text } from 'react-native'; | ||
| import { colors, fontStyles } from '../../../styles/common'; | ||
| import { strings } from '../../../../locales/i18n'; | ||
| import { connect } from 'react-redux'; | ||
| import ActionView from '../ActionView'; | ||
| import { renderFromTokenMinimalUnit } from '../../../util/number'; | ||
| import { toLowerCaseEquals } from '../../../util/general'; | ||
| import TokenImage from '../../UI/TokenImage'; | ||
| import Device from '../../../util/device'; | ||
| import Engine from '../../../core/Engine'; | ||
| import URL from 'url-parse'; | ||
| import AnalyticsV2 from '../../../util/analyticsV2'; | ||
| import useTokenBalance from '../../hooks/useTokenBalance'; | ||
|
|
||
| const styles = StyleSheet.create({ | ||
| root: { | ||
|
|
@@ -78,44 +80,29 @@ const styles = StyleSheet.create({ | |
| } | ||
| }); | ||
|
|
||
| const WatchAssetRequest = ({ | ||
| suggestedAssetMeta, | ||
| contractBalances, | ||
| currentPageInformation, | ||
| selectedAddress, | ||
| onCancel, | ||
| onConfirm | ||
| }) => { | ||
| useEffect( | ||
| () => async () => { | ||
| const { TokensController } = Engine.context; | ||
| typeof suggestedAssetMeta !== undefined && (await TokensController.rejectWatchAsset(suggestedAssetMeta.id)); | ||
| }, | ||
| [suggestedAssetMeta] | ||
| ); | ||
|
|
||
| const getAnalyticsParams = () => { | ||
| try { | ||
| const { asset } = suggestedAssetMeta; | ||
| const { NetworkController } = Engine.context; | ||
| const { chainId, type } = NetworkController?.state?.provider || {}; | ||
| const url = new URL(currentPageInformation?.url); | ||
|
|
||
| return { | ||
| token_address: asset?.address, | ||
| token_symbol: asset?.symbol, | ||
|
|
@@ -130,92 +117,115 @@ class WatchAssetRequest extends PureComponent { | |
| } | ||
| }; | ||
|
|
||
| const onConfirmPress = async () => { | ||
| const { TokensController } = Engine.context; | ||
| await TokensController.acceptWatchAsset(suggestedAssetMeta.id); | ||
| AnalyticsV2.trackEvent(AnalyticsV2.ANALYTICS_EVENTS.TOKEN_ADDED, getAnalyticsParams()); | ||
| onConfirm && onConfirm(); | ||
| }; | ||
|
|
||
| const { asset } = suggestedAssetMeta; | ||
|
|
||
| const address = Object.keys(contractBalances).find(key => toLowerCaseEquals(key, asset.address)); | ||
| let [balance] = useTokenBalance(asset.address, selectedAddress); | ||
|
|
||
| try { | ||
| balance = renderFromTokenMinimalUnit(address ? contractBalances[address] : balance, asset.decimals); | ||
| } catch (e) { | ||
| balance = 0; | ||
| } | ||
|
|
||
| return ( | ||
| <View style={styles.root}> | ||
| <View style={styles.titleWrapper}> | ||
| <Text style={styles.title} onPress={this.cancelSignature}> | ||
| {strings('watch_asset_request.title')} | ||
| </Text> | ||
| </View> | ||
| <ActionView | ||
| cancelTestID={'request-signature-cancel-button'} | ||
| confirmTestID={'request-signature-confirm-button'} | ||
| cancelText={strings('watch_asset_request.cancel')} | ||
| confirmText={strings('watch_asset_request.add')} | ||
| onCancelPress={onCancel} | ||
| onConfirmPress={onConfirmPress} | ||
| > | ||
| <View style={styles.children}> | ||
| <View style={styles.addMessage}> | ||
| <Text style={styles.signText}>{strings('watch_asset_request.message')}</Text> | ||
| </View> | ||
|
|
||
| <View style={styles.tokenInformation}> | ||
| <View style={styles.tokenInfo}> | ||
| <View style={styles.infoTitleWrapper}> | ||
| <Text style={styles.infoTitle}>{strings('watch_asset_request.token')}</Text> | ||
| </View> | ||
|
|
||
| <View style={styles.infoToken}> | ||
| <View style={styles.token}> | ||
| <View style={styles.identicon}> | ||
| <TokenImage | ||
| asset={{ | ||
| ...asset, | ||
| logo: asset.image | ||
| }} | ||
| logoDefined | ||
| /> | ||
| </View> | ||
| <Text style={styles.text}>{asset.symbol}</Text> | ||
| </View> | ||
| </View> | ||
| </View> | ||
|
|
||
| <View style={styles.tokenInfo}> | ||
| <View style={styles.infoTitleWrapper}> | ||
| <Text style={styles.infoTitle}>{strings('watch_asset_request.balance')}</Text> | ||
| </View> | ||
|
|
||
| <View style={styles.infoBalance}> | ||
| <Text style={styles.text}> | ||
| {balance} {asset.symbol} | ||
| </Text> | ||
| </View> | ||
| </View> | ||
| </View> | ||
| </View> | ||
| </ActionView> | ||
| </View> | ||
| ); | ||
| }; | ||
|
|
||
| WatchAssetRequest.propTypes = { | ||
|
There was a problem hiding this comment. @gantunesr Not a blocker, but just wanted to bring up that There was a problem hiding this comment. @Cal-L I'll open another PR for the next release to convert this component to TS, for the moment I would merged as it is. |
||
| /** | ||
| * Callback triggered when this message signature is rejected | ||
| */ | ||
| onCancel: PropTypes.func, | ||
| /** | ||
| * Callback triggered when this message signature is approved | ||
| */ | ||
| onConfirm: PropTypes.func, | ||
| /** | ||
| * Token object | ||
| */ | ||
| suggestedAssetMeta: PropTypes.object, | ||
| /** | ||
| * Current public address | ||
| */ | ||
| selectedAddress: PropTypes.string, | ||
| /** | ||
| * Object containing token balances in the format address => balance | ||
| */ | ||
| contractBalances: PropTypes.object, | ||
| /** | ||
| * Object containing current page title, url, and icon href | ||
| */ | ||
| currentPageInformation: PropTypes.object | ||
| }; | ||
|
|
||
| const mapStateToProps = state => ({ | ||
| contractBalances: state.engine.backgroundState.TokenBalancesController.contractBalances, | ||
| selectedAddress: state.engine.backgroundState.PreferencesController.selectedAddress, | ||
| provider: state.engine.backgroundState.NetworkController.provider | ||
| }); | ||
|
|
||
| export default connect(mapStateToProps)(WatchAssetRequest); | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| import { useState, useEffect, Dispatch, SetStateAction } from 'react'; | ||
| import Engine from '../../core/Engine'; | ||
| import { BN } from '@metamask/controllers'; | ||
|
|
||
| /** | ||
| * Hook to handle the balance of ERC20 tokens | ||
| * @param {String} tokenAddress Token contract address | ||
| * @param {String} userAddress Public address which holds the token | ||
| * @returns {Handlers} Handlers `[balance, loading, error]` | ||
|
There was a problem hiding this comment. Since this is TypeScript, we could remove the types from the TSDocs. For returns, I wasn't really sure what There was a problem hiding this comment. Fixed, thanks for the reference |
||
| */ | ||
|
|
||
| const useTokenBalance = (tokenAddress: string, userAddress: string): [any, boolean, boolean] => { | ||
| // This hook should be only used with ERC20 tokens | ||
| const [balance, setBalance]: [BN | null, Dispatch<SetStateAction<BN | null>>] = useState<BN | null>(null); | ||
| const [loading, setLoading]: [boolean, Dispatch<SetStateAction<boolean>>] = useState<boolean>(true); | ||
| const [error, setError]: [boolean, Dispatch<SetStateAction<boolean>>] = useState<boolean>(false); | ||
| const { TokenBalancesController }: any = Engine.context; | ||
|
|
||
| const fetchBalance = async (tokenAddress: string, userAddress: string): Promise<void> => { | ||
| TokenBalancesController.getBalanceOf(tokenAddress, userAddress) | ||
| .then((balance: BN) => setBalance(balance)) | ||
| .catch(() => setError(true)) | ||
| .finally(() => setLoading(false)); | ||
| }; | ||
|
|
||
| useEffect(() => { | ||
| fetchBalance(tokenAddress, userAddress); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [tokenAddress, userAddress]); | ||
|
|
||
| return [balance, loading, error]; | ||
| }; | ||
|
|
||
| export default useTokenBalance; | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@gantunesr This will refer to line 129->136: What do you think about ditching
contractBalancessince based on the logic and what we've observed, it doesn't seem to be a 100% reliable source (Since it's possible for balances to not exist in the object). Based on your implementation,useTokenBalanceis already called every time the function is rendered and we can just rely on that for now. This gives us one less condition to check/worry about and it'll clean up the code. The only downside is that we may not be showing existing contract balances right away, but I think that's totally fine. As for what happens if the token balances call fails: It looks like you've exposed a loading and error state inuseTokenBalance, which would give us control over the UI.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.
Agreed, I was thinking to pass the balance value of
contractBalancesto the hook and only fetch the new balance if the value wasundefinedbut in a second thought, maybe is better to removecontractBalancesas a dependence.