Skip to content
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

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 101 additions & 0 deletions src/utils/TokenAmount.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
/* global BigInt */

import JSBI from 'jsbi'
import { toJsbi } from './math'
import { formatTokenAmount } from './format'

class TokenAmount {
#amount
#decimals
#symbol

/**
* Create a TokenAmount.
* @param {BigInt|string|number} amount The amount as an integer (e.g. in Wei for Ether).
* @param {BigInt|string|number} decimals The token decimals (e.g. 18 for Ether).
* @param {string} options.symbol The token symbol (e.g. ETH for Ether).
*/
constructor(amount, decimals, { symbol = '' } = {}) {
amount = toJsbi(amount)
decimals = toJsbi(decimals)

if (JSBI.lessThan(decimals, 0)) {
throw new Error('TokenAmount: decimals cannot be negative')
}

this.#amount = amount
this.#decimals = decimals
this.#symbol = symbol
}

/**
* Get the amount as an integer (e.g. in Wei for Ether).
* @returns {BigInt}
*/
amount() {
return BigInt(this.#amount.toString())
Copy link
Contributor

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?

Copy link
Contributor Author

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() for BigInt only, as it is the only one we should use eventually.

Copy link
Contributor

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).

Copy link
Contributor Author

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 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.

Copy link
Contributor

@sohkai sohkai Apr 14, 2020

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.


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?

Copy link
Contributor Author

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 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 ** 18n vs. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

/**
* Get the amount as an integer (e.g. in Wei for Ether).
* @returns {string}
*/
amountString() {
bpierre marked this conversation as resolved.
Show resolved Hide resolved
return this.#amount.toString()
}

/**
* Get the decimals of the token.
* @returns {number}
*/
decimals() {
return this.#decimals.toNumber()
}

/**
* Formats the token amount for display purposes.
* @param {string} options.displaySign Whether to display the sign or not.
* @param {string} options.displaySymbol Whether to display the token symbol or not.
* @param {string} options.digits The number of digits to appear after the decimal point.
* @returns {string}
*/
format({ sign = false, symbol = false, digits = 2 } = {}) {
bpierre marked this conversation as resolved.
Show resolved Hide resolved
return formatTokenAmount(this.#amount, this.#decimals, {
digits,
sign,
symbol: symbol ? this.#symbol : '',
})
}

/**
* Returns an object to be serialized by JSON.stringify().
* @returns {object}
*/
toJSON() {
return {
amount: this.#amount.toString(),
decimals: this.#decimals.toString(),
symbol: this.#symbol,
}
}

/**
* Instanciate a new TokenAmount from the data serialized by JSON.stringify().
* @returns {object}
*/
static fromJSON(jsonData) {
try {
const { amount, decimals, symbol } = JSON.parse(jsonData)
if (amount === undefined || decimals === undefined) {
throw new Error()
}
return new TokenAmount(amount, decimals, { symbol })
} catch (err) {
throw new Error(
'The data passed to TokenAmount.fromJSON() seems incorrect or incomplete.'
)
}
}
}

export default TokenAmount
60 changes: 60 additions & 0 deletions src/utils/TokenAmount.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import TokenAmount from './TokenAmount'

describe('TokenAmount', () => {
test('should instanciate from an amount expressed as a Number', () => {
expect(new TokenAmount(91234, 4).format()).toEqual('9.12')
})

test('should throw if decimals are negative', () => {
expect(() => {
return new TokenAmount(91234, -1)
}).toThrow()
})
})

describe('TokenAmount#amount()', () => {
test('should export the amount as a BigInt', () => {
expect(new TokenAmount('9381295879707883945', 18).amount()).toEqual(
9381295879707883945n
)
})
})

describe('TokenAmount#amountString()', () => {
test('should export the amount as a String', () => {
expect(new TokenAmount('9381295879707883945', 18).amountString()).toEqual(
'9381295879707883945'
)
})
})

describe('TokenAmount#toJSON()', () => {
test('should serialize properly', () => {
expect(new TokenAmount('9381295879707883945', 18).toJSON()).toEqual({
amount: '9381295879707883945',
decimals: '18',
symbol: '',
})
expect(JSON.stringify(new TokenAmount('9381295879707883945', 18))).toEqual(
JSON.stringify({
amount: '9381295879707883945',
decimals: '18',
symbol: '',
})
)
})
})

describe('TokenAmount.fromJSON()', () => {
test('should deserialize properly', () => {
expect(
TokenAmount.fromJSON(
JSON.stringify(new TokenAmount('9381295879707883945', 18))
).toJSON()
).toEqual({
amount: '9381295879707883945',
decimals: '18',
symbol: '',
})
})
})
8 changes: 4 additions & 4 deletions src/utils/format.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import JSBI from 'jsbi'
import { NO_BREAK_SPACE } from './characters'
import { divideRoundBigInt } from './math'
import { divideRoundBigInt, toJsbi } from './math'

/**
* Formats an integer based on a limited range.
Expand Down Expand Up @@ -65,9 +65,9 @@ export function formatTokenAmount(
decimals,
{ digits = 2, symbol = '', displaySign = false } = {}
) {
amount = JSBI.BigInt(String(amount))
decimals = JSBI.BigInt(String(decimals))
digits = JSBI.BigInt(String(digits))
amount = toJsbi(amount)
decimals = toJsbi(decimals)
digits = toJsbi(digits)

if (JSBI.lessThan(decimals, 0)) {
throw new Error('formatTokenAmount(): decimals cannot be negative')
Expand Down
10 changes: 10 additions & 0 deletions src/utils/math.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
import JSBI from 'jsbi'

/**
* Converts BigInt-like values into JSBI objects.
*
* @param {JSBI.BigInt|BigInt|string|number} value The value to convert.
* @returns {JSBI.BigInt}
*/
export function toJsbi(value) {
return JSBI.BigInt(String(value))
}

/**
* Re-maps a number from one range to another.
*
Expand Down