[2847] Bug in token balance #2866
Conversation
gantunesr
added
type-bug
gantunesr
changed the title
mobularay
removed
the
needs-dev-review
cortisiko
added
QA in Progress
|
Issue 1 If I were to remove a token from my wallet then re-add it, the token value shows as “0” to reproduce:
|
cortisiko
added
QA'd - Issues Found
cortisiko
added
QA Passed
sethkfman
removed
the
needs-dev-review
| ); | ||
| }; | ||
|
|
||
| WatchAssetRequest.propTypes = { |
There was a problem hiding this comment.
@gantunesr Not a blocker, but just wanted to bring up that propTypes here are never exposed when consuming this component because this component is wrapped in a HOC caused by connect(). We'll eventually want to convert this component to TypeScript, which will fix this scenario. Totally up to you if you want to convert it now or leave it for the next person who touches this.
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.
| <View style={styles.infoTitleWrapper}> | ||
| <Text style={styles.infoTitle}>{strings('watch_asset_request.token')}</Text> | ||
| </View> | ||
| const address = Object.keys(contractBalances).find(key => toLowerCaseEquals(key, asset.address)); |
There was a problem hiding this comment.
@gantunesr This will refer to line 129->136: What do you think about ditching contractBalances since 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, useTokenBalance is 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 in useTokenBalance, which would give us control over the UI.
There was a problem hiding this comment.
Agreed, I was thinking to pass the balance value of contractBalances to the hook and only fetch the new balance if the value was undefined but in a second thought, maybe is better to remove contractBalances as a dependence.
| * 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.
Ref: https://github.com/microsoft/tsdoc/tree/master/tsdoc
For returns, I wasn't really sure what Handlers meant in this context. We could do something like:
@returns Array that consists of [balance, loading, error] to be more explicit.
There was a problem hiding this comment.
Fixed, thanks for the reference
| <Text style={styles.infoTitle}>{strings('watch_asset_request.token')}</Text> | ||
| </View> | ||
| try { | ||
| balance = renderFromTokenMinimalUnit(balance, asset.decimals); |
There was a problem hiding this comment.
Let's move this try catch logic into the renderFromTokenMinimalUnit function and have it return a fallback balance of 0.
cc: @gantunesr
Bug related to a case sensitive match between the keys (address) associated with the token balance.
0x514910771bf9ca656af840dff83e8264ecf123nm != 0x514910771BF9Ca656af840dff83E8264EcF123NMFix: Case insensitive key match.