Add TokenAmount #757
Add TokenAmount #757
Conversation
I like leaving it as a class—that was it is and introducing a factory just to hide the export is contrived. I wonder if we could hide it under a namespace somehow, e.g. |
src/utils/TokenAmount.js
Outdated
| * @returns {BigInt} | ||
| */ | ||
| amount() { | ||
| return BigInt(this.#amount.toString()) |
There was a problem hiding this comment.
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.
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() for BigInt only, as it is the only one we should use eventually.
There was a problem hiding this comment.
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.
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 about amountString() is that it is just verbose enough to push users towards using amount() as soon as they can 😄
What do you think about using a asString boolean parameter? amount() would return a BigInt while amount(true) would return a String.
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.
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.
if we want to have amount() return a BigInt
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.
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 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.
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?
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:
- No need to import and ship one or multiple libraries to deal with big integers (on both sides, libraries and apps).
- Being able to use operators directly e.g.
amount = 38n * 10n ** 18nvs.amount = (new BN('38')).mul((new BN('10')).pow(new BN('18')). - More than operators, being able to use the rest of the language with it, e.g.
Intl.NumberFormat. - For libraries, no need to export non-standard types, provide the libraries to work with them, or have to deal with version incompatibilities (e.g. Ethers’s embedded bignumber utility).
- Stop doing conversions everywhere between the different types (not so much for the performances cost than for the additional complexity).
- On input, supporting BigInt for libraries is trivial and can be done today, even without Safari support.
- On output (assuming support in Safari is there), it would break compatibility for existing libraries. But for authors already using BigInt, first class support for it might become an important factor when choosing a library.
There was a problem hiding this comment.
|
@sohkai I forgot to answer this comment:
I agree about the nice semantics of a constructor, my main concern is really about conflicting with a potential
// factory
<Tag
background={color('#FFF').alpha(0.5)}
label={tokenAmount(amount, decimals).format()}
/>
// constructor
<Tag
background={(new Color('#FFF')).alpha(0.5)}
label={(new TokenAmount(amount, decimals)).format()}
/>Also factories are pretty common in JS, even within the language recently (Symbol(), BigInt()), so I don’t think it would be confusing for users.
This doesn’t work, because the Also we would users would still have to do things like this if import { TokenAmount } from '@aragon/ui'
import { TokenAmount as TokenAmountSomething } from '@aragon/ui/utils'
function App() {
const amount = new TokenAmountSomething(…)
return <TokenAmount amount={amount} />
}
That could be an idea yes! I guess we would also need to introduce modules like Now in case of conflict, users would still deal with similar names, but at least they wouldn’t have to rename their imports: // using utils for now because I’m not sure what could be a category for the TokenAmount class
import { utils, TokenAmount } from '@aragon/ui'
function App() {
const amount = new utils.TokenAmount(…)
return <TokenAmount amount={amount} />
} |
I believe this is the way to go, for a few reasons:
I feel such a component would be unnatural (at least for me) — I expect a component to be purely visual from an "end-user" standpoint, and For the reason mentioned above, I prefer the class approach; it's clear to me, and also, thinking about how I would use it, I expect to pass this newly created |
|
Closing for now, |
|
To come back to this point @Evalir:
Agreed, and I think what we're seeing internally for frontend libraries (leading to #772) is:
|
This class will help to consolidate a token amount into its own type. For now it is mostly about using its
format()method, but we can imagine adding other things in the future.We might also want to make some aragonUI components aware of it, e.g. a
<TransferAmount />component could be used to represent the transferred amounts in the Finance app, and acceptTokenAmountinstances.About this, any thoughts on exporting a class to use with
new TokenAmount()vs. atokenAmount()function that would do the same thing? Since this is primarily a components library, I was thinking that we might want to avoid exporting names that might conflict with component names in the future.