-
Notifications
You must be signed in to change notification settings - Fork 212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Finance: fix token fallbacks and error handling #813
Conversation
@@ -167,16 +168,20 @@ class Deposit extends React.Component { | |||
} | |||
|
|||
const [tokenSymbol, tokenDecimals] = await Promise.all([ | |||
getTokenSymbol(api, address), | |||
token.decimals().toPromise(), | |||
getTokenSymbol(api, address).catch(() => {}), |
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.
Super nitpicky but should we return explicit values (null
) rather than implicit undefined
in these cases?
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.
Yeah... we should probably settle on a convention for handling errors and their "safe" default values.
In the case of the balance check above, I opted for "0" since it'll show "no balances" but we could probably use "-1" as "couldn't find balance".
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.
I've now changed it to use -1
for the balance if the balance call failed and default the symbol and name values to ''
(since they're optional strings for the token to implement) in a067c7c.
return '' | ||
} | ||
|
||
return userBalance === -1 |
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.
I think this should be '-1'
?
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.
😅 this is why I need a second pair of eyes 😄
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.
💯 💯 💯
Fixes aragon/client#734. With aragon/aragon.js#277 we now get the actual errors back from the RPC when a call goes wrong (e.g. naive `token.symbol()` on DAI) and we need to handle these explicitly. This PR converts a lot of the token fallback-related bits into simpler Promise-based code and adds explicit handlers for their error cases.
Fixes aragon/client#734.
With aragon/aragon.js#277 we now get the actual errors back from the RPC when a call goes wrong (e.g. naive
token.symbol()
on DAI) and we need to handle these explicitly.This PR converts a lot of the token fallback-related bits into simpler Promise-based code and adds explicit handlers for their error cases.