-
Notifications
You must be signed in to change notification settings - Fork 79
Eth <> Wei + ERC20 Formatting functions for Utils.cs #10
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
Conversation
If this isn't done: 1) some errors could never be handled on the unity end and break the app 2) returning "default" may be confused with an actual result Best bet is to throw so the task handler can check if task is faulted and handle as they wish.
Ok this should be good now |
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.
looks good! just a couple of comments
Assets/Thirdweb/Scripts/Utils.cs
Outdated
{ | ||
BigInteger weiBigInt = 0; | ||
if (!BigInteger.TryParse(wei, out weiBigInt)) | ||
return "INVALID_ARGUMENTS"; |
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.
probably want to throw here instead no?
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.
Up to you, wasn't sure what's best
Assets/Thirdweb/Scripts/Utils.cs
Outdated
if (!BigInteger.TryParse(wei, out weiBigInt)) | ||
return "INVALID_ARGUMENTS"; | ||
double eth = (double)weiBigInt / DECIMALS_18; | ||
return eth.ToString(); |
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.
how about just calling FormatERC20
here? that way we can expose decimalsToDisplay
as an optional param on ToEth
too which is super useful
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.
Yep good point!
Here we go! |
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.
amazing, thank you!
ToWei, ToEth and FormatERC20 functions added.