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

dnum.from Incorrect number from String() converting to scientific notation #3

Closed
greg-schrammel opened this issue Mar 10, 2023 · 7 comments

Comments

@greg-schrammel
Copy link

in the from method
String(value) converts expoents greater than 21 or less than -6 to scientific notation, which makes it not pass the regex in the next line

I was trying something like multiply(price, 10 ** -8, 18)
It's not a big issue since divide(price, 10 ** Math.abs(expo), 18) should do the same

a possible solution could be something like this, I'm not sure of the implications or if this should be handled in the lib, if you want I can look further and open a pr

  value = String(value);
  
  if (value.includes('e')) value = Number(value).toFixed(20)
  // or value = typeof value === 'number' ? value.toFixed(20) : String(value)

  if (!value.match(NUM_RE)) {
    throw new Error(`Incorrect number: ${value}`);
  }

It took some time today to figure what I was doing wrong here haha

Number.toString

Scientific notation is used if the radix is 10 and the number's magnitude (ignoring sign) is greater than or equal to 10^21 or less than 10-6

@greg-schrammel
Copy link
Author

got here because that's how the pyth api is setup btw https://docs.pyth.network/pythnet-price-feeds/best-practices

@bpierre
Copy link
Owner

bpierre commented Mar 11, 2023

Oh good catch! Yes we should definitely handle this better. I’ve been doing some tests and noticed that even toFixed() can produce scientific notation numbers:

(10 ** 20).toFixed(20) // "100000000000000000000.00000000000000000000"
(10 ** 21).toFixed(20) // "1e+21"

From Number.toFixed:

If the absolute value of numObj is greater or equal to 1021, this method uses the same algorithm as Number.prototype.toString() and returns a string in exponential notation. toFixed() returns "Infinity", "NaN", or "-Infinity" if the value of numObj is non-finite.

I see two paths:

  • We try to get it right, maybe by using something like https://github.com/shrpne/from-exponential which seems complete. It would make the library a little bit heavier but not by much.
  • We detect the scientific notation and add a specific error message to let consumers know about the exact problem and how to solve it. An issue I see with this solution is that the error might not appear during the development phase but later.

Also I am thinking that it might be good to add some warning in the README about the limitations of using numbers as an input, compared to strings / bigints / dnums.

What do you think?

@greg-schrammel
Copy link
Author

I think ethers throws a numeric underflow(?) well this lib users probably are already avoiding numbers so a warning on readme and custom error message may be enough
but if it's easily fixable I don't see why not, from-exponential is tinyy

@greg-schrammel
Copy link
Author

looks like from-exponential loses precision 😢
Screenshot 2023-03-14 at 17 35 57

@bpierre
Copy link
Owner

bpierre commented Mar 15, 2023

Oh too bad, yes then I guess the simplest way to handle it would be a custom error for now. Let me know if you want to open a PR for it, otherwise I’ll do it later this week :)

@bpierre
Copy link
Owner

bpierre commented May 15, 2023

Fixed 680f4ad

@bpierre bpierre closed this as completed May 15, 2023
@bpierre
Copy link
Owner

bpierre commented May 16, 2023

@greg-schrammel I changed my mind and decided to use from-exponential rather than throwing an error. The reasoning is that Number is always prone to precision loss so it doesn’t change that.

https://github.com/bpierre/dnum/releases/tag/v2.7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants