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

feat: support toBigInt method #356

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Dunqing
Copy link

@Dunqing Dunqing commented Aug 1, 2023

close: #352

@MikeMcl
Copy link
Owner

MikeMcl commented Aug 5, 2023

Nice work but more test cases are required. In the documentation you add the following example,

y = new BigNumber('45987349857634085409857349856430985')
y.toBigInt()                    // 45987349857634085409857349856430985n

but your implementation of toBigInt actually returns a BigInt with value 45987349857634086842492183761649664 because you are converting the BigNumber's value to a JS number using +.

A correct implementation would be the following. Note that I use toFixed because BigInt does not handle exponential notation.

P.toBigInt = function () { 
  if (!this.isInteger()) throw Error(bignumberError + 'Not an integer: ' + valueOf(this));
  return BigInt(this.toFixed());
};

or, if we want to allow BigInts to be formed from non-integers, and to return null for NaN and Infinity

P.toBigInt = function () {
  return this.isFinite() ? BigInt(this.integerValue(1).toFixed()) : null;
};

where the 1 is BigNumber.ROUND_DOWN, but it could be left out if we wanted the current global ROUNDING_MODE setting to be used when rounding non-integers, or we could allow the option of passing a rounding mode as an argument.

The most efficient implementation would be the following. Like the BigInt constructor, it always throws on non-integers. It is easy for a user to call integerValue before toBigInt if required anyway.

P.toBigInt = function () {
  if (!this.isInteger()) throw Error(bignumberError + 'Not an integer: ' + valueOf(this));
  var str = coeffToString(this.c);
  for (var i = this.e + 1 - str.length; i > 0; i--) str += '0';
  return BigInt(this.s < 0 ? '-' + str : str);
};

Okay, so this PR needs your implementation of toBigInt to be replaced with the one directly above and more tests to be added.

@Dunqing
Copy link
Author

Dunqing commented Aug 7, 2023

Thanks for taking the time here. This is a great implementation. I will replace it later

@Dunqing
Copy link
Author

Dunqing commented Aug 7, 2023

I replaced and added some tests. Do we need more test cases? what tests need to add?

@MikeMcl
Copy link
Owner

MikeMcl commented Sep 4, 2023

No, it's all good. I'm still not 100% on including this because up to now this library uses ECMAScript 3 (ES3) features only and I am not sure I want to break that convention. I'll leave this PR open for now. Thanks for your efforts.

@Dunqing
Copy link
Author

Dunqing commented Sep 5, 2023

I'm still not 100% on including this because up to now this library uses ECMAScript 3 (ES3) features only and I am not sure I want to break that convention.

We could determine if the current environment supports BigInt or else throw an error so that we can support the modern browser without affecting ES3 users. do you think that would work?

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

Successfully merging this pull request may close these issues.

[Feature Request]: Can we support toBigInt?
2 participants