Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Discussion: Support scientific notation in BigNumbers? #228

Closed
ricmoo opened this issue Jul 12, 2018 · 11 comments
Closed

Discussion: Support scientific notation in BigNumbers? #228

ricmoo opened this issue Jul 12, 2018 · 11 comments
Labels
enhancement New feature or improvement. major-bump Planned for the next major version bump

Comments

@ricmoo
Copy link
Member

ricmoo commented Jul 12, 2018

This came up recently, and so I thought I would open it up for discussion.

Pros:

  • Very common values are easier and less prone to bugs to enter (e.g. "1e18"); these are already available as ethers.constants.WeiPerEther, but other common values may come up too, such as 1e9 referring to wei per gwei.
  • Nice feature to have

Cons:

  • Yes another way string values can be confused for hexadecimal values (1e18 is 7704 in hex); this may be less an issue, since hexadecimal strings must begin with a 0x prefix, and currently this is already a problem if people try 19, meaning 0x19, which is 25
  • This might encourage the use of scientific notation in newer developers, which would be confused by the numeric overflow; for example bigNumberify((1e18 + 65) - (1e18 + 64)); the underflow occurs before we have a chance to catch it; this is already still an issue though
  • This may require slightly more complicated code than is immediately obvious and will require substantial testing

Anyways, please anyone else please feel free to add, augment or refute any points.

@ricmoo ricmoo added the discussion Questions, feedback and general information. label Jul 12, 2018
@mrwillis
Copy link

I think it's not necessary and might just confuse people. There is already stuff like utils.parseUnits that can take any "real world number" and convert it to an "ethereum number", i.e., times 10^18. I've seen some helper functions around that are essentially two methods toBaseUnitAmount and fromBaseUnitAmount where you specify the "real world number" and the decimals and converts it to and from the "ethereum number".

For instance, here are two great functions from the 0x wrapper: https://github.com/0xProject/0x-monorepo/blob/development/packages/web3-wrapper/src/web3_wrapper.ts

   /**
     * A unit amount is defined as the amount of a token above the specified decimal places (integer part).
     * E.g: If a currency has 18 decimal places, 1e18 or one quintillion of the currency is equivalent
     * to 1 unit.
     * @param   amount      The amount in baseUnits that you would like converted to units.
     * @param   decimals    The number of decimal places the unit amount has.
     * @return  The amount in units.
     */
    public static toUnitAmount(amount: BigNumber, decimals: number): BigNumber {
        assert.isValidBaseUnitAmount('amount', amount);
        assert.isNumber('decimals', decimals);
        const aUnit = new BigNumber(BASE_TEN).pow(decimals);
        const unit = amount.div(aUnit);
        return unit;
    }
    /**
     * A baseUnit is defined as the smallest denomination of a token. An amount expressed in baseUnits
     * is the amount expressed in the smallest denomination.
     * E.g: 1 unit of a token with 18 decimal places is expressed in baseUnits as 1000000000000000000
     * @param   amount      The amount of units that you would like converted to baseUnits.
     * @param   decimals    The number of decimal places the unit amount has.
     * @return  The amount in baseUnits.
     */
    public static toBaseUnitAmount(amount: BigNumber, decimals: number): BigNumber {
        assert.isBigNumber('amount', amount);
        assert.isNumber('decimals', decimals);
        const unit = new BigNumber(BASE_TEN).pow(decimals);
        const baseUnitAmount = amount.times(unit);
        const hasDecimals = baseUnitAmount.decimalPlaces() !== 0;
        if (hasDecimals) {
            throw new Error(`Invalid unit amount: ${amount.toString()} - Too many decimal places`);
        }
        return baseUnitAmount;
    }

I think these kinds of methods with some extra functionality like common exponentiations solve a lot of the use cases.

One thing I think would be great is a rand() function. This exists in bignumber.js but unfortunately not in bn.js, and since it's pretty hard to get at in this library, a top level function random function would be nice for generating stuff like salts.

@ricmoo
Copy link
Member Author

ricmoo commented Jul 26, 2018

I agree with you, but someone asked, so I thought I'd put it up for discussion. I like to get consensus and not just make broad-sweeping decisions. :)

For your random number, you could use:

var randomNumber = ethers.utils.bigNumberify(ethers.utils.randomBytes(32));

Or for salts, which are usually just bytes32:

var salt = ethers.utils.randomBytes(32);

:)

@mrwillis
Copy link

Wow.... that is too easy. Thank you.

@ricmoo
Copy link
Member Author

ricmoo commented Aug 27, 2018

I think we have reached a consensus (an my personal preference) to not support scientific notation in the BigNumber class.

Closing this issue, but if anyone wants to re-open it, please feel free to do so. :)

@kamescg
Copy link

kamescg commented Sep 23, 2018

It might be worthwhile to add support for scientific notation as it is currently being recommended in the "URL Format for Transaction Requests" ERC681 format.

The use of scientific notation is strongly encouraged. For example, requesting 2.014 ETH to address 0xfb6916095ca1df60bb79Ce92ce3ea74c37c5d359 would look as follows: ethereum:0xfb6916095ca1df60bb79Ce92ce3ea74c37c5d359?value=2.014e18

Depending on the adoption of the specification it would be great to easily convert the value using the ethers.utils library when processing QR codes that use that specification without creating an additional helper function.

https://github.com/ethereum/EIPs/blob/master/EIPS/eip-681.md

@ricmoo
Copy link
Member Author

ricmoo commented Sep 23, 2018

Excellent point. :)

I’ll look more into that, thanks.

(Re-opening)

@ricmoo ricmoo reopened this Sep 23, 2018
ricmoo added a commit that referenced this issue Sep 26, 2018
@ricmoo ricmoo added enhancement New feature or improvement. on-deck This Enhancement or Bug is currently being worked on. and removed discussion Questions, feedback and general information. labels Oct 3, 2018
@ricmoo
Copy link
Member Author

ricmoo commented Dec 14, 2018

(The above commit reference is wrong; it was meant to address #301)

@ricmoo ricmoo added next version and removed on-deck This Enhancement or Bug is currently being worked on. labels Feb 2, 2019
@ricmoo
Copy link
Member Author

ricmoo commented Mar 13, 2020

Any news on the adoption of EIP-681?

I think this might make sense to add as a static method to the new FixedNumber object regardless, rather than allow scientific notation in arbitrary places.

@ricmoo
Copy link
Member Author

ricmoo commented Mar 14, 2020

Also, as I try to implement this I notice the specification has unbalanced brackets... :s

I'm guessing:

[ ( "e" / "E" ) [ 1*DIGIT ] [ "+" UNIT ]

was meant to be something like:

[ ( "e" / "E" ) [ 1*DIGIT ] | [ "+" UNIT ] ]

but it still doesn't fully make sense to me. For now I'll ignore units. It will be backwards compatible once the spec is corrected.

@ricmoo ricmoo added major-bump Planned for the next major version bump and removed v5 (possible addition) labels Sep 13, 2020
@thegostep
Copy link

thegostep commented Sep 20, 2020

I am having trouble converting from bignumber.js to ethers.BigNumber for values above e21 - the ability to parse scientific numbers would be a workaround for direct casting. Any other solutions?

image

@thegostep
Copy link

Ah - it is possible to prevent exponentiation of bignumber.js by calling .string(10)

mergify bot pushed a commit to celo-org/celo-monorepo that referenced this issue Jun 2, 2021
Looks like this was related to ethers-io/ethers.js#228, but we just need to force the bignumber string to not be in scientific notation when transforming the value, and then this works as expected. Note that the outputted value in the outputted transaction will **not** be in scientific notation.

Closes #7466
tkporter pushed a commit to celo-org/celo-monorepo that referenced this issue Jul 8, 2021
Looks like this was related to ethers-io/ethers.js#228, but we just need to force the bignumber string to not be in scientific notation when transforming the value, and then this works as expected. Note that the outputted value in the outputted transaction will **not** be in scientific notation.

Closes #7466
@ethers-io ethers-io locked and limited conversation to collaborators Jan 31, 2023
@ricmoo ricmoo converted this issue into discussion #3702 Jan 31, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
enhancement New feature or improvement. major-bump Planned for the next major version bump
Projects
None yet
Development

No branches or pull requests

4 participants