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.
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
Add TokenAmount #757
Add TokenAmount #757
Changes from 1 commit
f87be84
8da3f1a
c15f5b7
fb6bcbe
b9f947d
9736c81
5431650
d7cf58b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should we return a copy of the JSBI object instead?
Or maybe do a quick availability check for BigInt existing natively?
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 would avoid returning a different type implicitly: JSBI and BigInt have different APIs and we should be able to rely on one of them from the other side.
What about adding a
.amountJsbi()
method? I’d like to keep.amount()
forBigInt
only, as it is the only one we should use eventually.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.
Oh I had assumed JSBI and BigInt were as close as possible API-wise.
What do you think about just returning a string here instead as the default API? I think we'll still be using strings to represent BNs for a while until we've all moved to BigInt... this will make it less confusing / failure-prone if we wanted to transfer a TokenAmount into one of the other BN libraries (especially web3 / ethers—they always accept strings, but the BN types are a mishmash).
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.
We could, but the downside is that we will have to break compatibility at some point if we want to keep using
amount()
to return a BigInt. Something I like aboutamountString()
is that it is just verbose enough to push users towards usingamount()
as soon as they can 😄What do you think about using a
asString
boolean parameter?amount()
would return aBigInt
whileamount(true)
would return aString
.About BigInt more generally, I think the most important thing would be to get support in Safari, because we can’t use BigInt at all without it, even as an intermediary step. I am not as worried for libraries support, because users can convert the type themselves before feeding it to their library. I’m also not too worried about libraries adding support for it: they all support string integers already, so supporting BigInt only requires a
String(value)
on the parameter.For Safari, it seems that support for BigInt is coming in the next version 🎉🎉🎉 https://developer.apple.com/safari/technology-preview/release-notes/
Ethers.js v5 will have full support for BigInt: ethers-io/ethers.js#594 (comment)
Web3.js seems to have plans to support BigInt, but it doesn’t seem to be done yet.
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'd honestly default to returning strings as the "base denominator" BN type until we see massive user adoption of BigInts, just so people can fall into the "pit of success" today.
I greatly prefer this over forcing users to learn how to use a new type / look up why their toolchain doesn't support BigInts natively and have to do a string cast. This should be easy.
I really don't think the string serialization into a BigInt will cost that much; if someone is serializing that many strings, they probably have much bigger performance problems than the small waste of us doing a roundtrip back/forth BigInts/strings.
As you can tell, I'm not convinced we will want to do this in the next two years. Is there a reason that makes you think exposing the BigInt type is going to be preferred in that time frame?
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 it’s also a valid choice, let’s do this for now then. We have a bunch of other things returning strings rather than BigInt (e.g.
useWallet()
), so we could wait and move them all at the same time whenever it makes sense.Two years seems really far to me if Safari support is right around the corner, but time will tell! I anticipate a quick adoption of BigInt in the web3 ecosystem once Safari supports it, at least as input values. In I might be wrong, but I think it provides clear benefits:
amount = 38n * 10n ** 18n
vs.amount = (new BN('38')).mul((new BN('10')).pow(new BN('18'))
.Intl.NumberFormat
.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.
Done in https://github.com/aragon/aragon-ui/pull/757/commits/5431650d51665a1fed0c8acf47d78e4d6b474d02