Skip to content
This repository has been archived by the owner on Jan 11, 2022. It is now read-only.

Add bigint support #85

Merged
merged 2 commits into from
Apr 30, 2020
Merged

Add bigint support #85

merged 2 commits into from
Apr 30, 2020

Conversation

wemeetagain
Copy link
Contributor

The logic for number encoding works fine for bigints.

  • Add bigint to Input type
  • Add typeof check to allow bigint encoding to share number encoding code path
  • Add simple unit test to verify bigint encoding

@alcuadrado
Copy link
Member

Hey @wemeetagain! Thanks for this contribution.

While this is backward compatible, its tests won't run in Node 8. We are currently trying to define a policy about which versions of Node to support, so we may not be able to merge it just yet. You can read more about it here.

@wemeetagain
Copy link
Contributor Author

Okydoky, it looks like the consensus is to wait until Node 8 goes out of maintenance (Dec 31) to put any sort of policy in motion?

Feel free to close / ignore this PR until the time is right.

@axic
Copy link
Member

axic commented Apr 5, 2020

@alcuadrado would it make sense to support some kind of shim for this?

@alcuadrado
Copy link
Member

AFAIK there's no way to shim native bigints. Maybe a partial shim can be implemented for this tests though.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.654% when pulling d1138f9 on ChainSafe:add-bigint into f8394b3 on ethereumjs:master.

@wemeetagain
Copy link
Contributor Author

bump :)
I added a switch to skip the BigInt test in CI for older versions of node.

@holgerd77
Copy link
Member

Sorry, my bigint experience is pretty limited. Won't this cause major problems for people who want to use this library within a browser context?

@wemeetagain
Copy link
Contributor Author

This change is meant to be backwards compatible. The only change to the generated js should be the additional typeof check, a condition which will just return false for browsers without bigint support.

The intToHex and intToBuffer functions work without modifications with both number and bigint inputs, reason being the syntax for arithmetic operations is the same for number vs bigint.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Thanks @wemeetagain, this looks good, will merge now.

Is this just generally useful contribution, or do you need this as some basis for further work and should we do a release (patch level I would guess?) on this?

@holgerd77 holgerd77 merged commit b89c0c7 into ethereumjs:master Apr 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants