-
Notifications
You must be signed in to change notification settings - Fork 127
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
Closed
Add TokenAmount #757
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
f87be84
Add TokenAmount
bpierre 8da3f1a
Merge branch 'master' into token-amount
bpierre c15f5b7
New formatTokenAmount() API
bpierre fb6bcbe
Throw if decimals are negative + add a toJsbi() utility
bpierre b9f947d
Merge branch 'master' into token-amount
bpierre 9736c81
Ethers => Ether
bpierre 5431650
Move from returning a BigInt to a string integer
bpierre d7cf58b
Use toJsBi()
bpierre File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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()) | ||
} | ||
|
||
/** | ||
* 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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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: '', | ||
}) | ||
}) | ||
}) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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