[WIP] Rewrite in typescript, add more methods

ethereumjs / fixed-bn.js

a bn.js wrapper that constrains numbers to a fixed width

7 stars 9 forks source link

[WIP] Rewrite in typescript, add more methods #8

Open s1na opened 5 years ago

s1na commented 5 years ago

This PR overrides overrides clone to return a FixedBN instance (with same width) on operations (e.g. add).

As an example I also overrode add to apply modulo when overflow occurs (as per #6). I'm not sure if this is preferable or throwing on overflow?

This also means that FixedBN should override most of the BN methods to either check bounds or do modulo, which is not ideal. Are there any better ideas?

Pinging @holgerd77 for comments.

coveralls commented 5 years ago

Coverage remained the same at 100.0% when pulling 264bf86faa7de15a595ae8ddf2a2ad7bb9fde6a6 on ops into 3082e835037df32a0cfdde8af7b50c28261cf2ff on master.

holgerd77 commented 5 years ago

I'll just take notice and leave commenting/discussion to @axic and others who are deeper involved.

s1na commented 5 years ago

Re-wrote the lib in typescript:

Restricted operands of most operations (and comparisons) to have same width (easier to deal with for now)
Operations throw on under/overflow, and are not in-place
Operations which can overflow have a corresponding mod method (e.g. mulMod, addMod) which wraps the result if it overflows instead of raising an exception
Restricted numbers to non-negative integers
The FixedWidthBN class doesn't extend BN, but is rather a wrapper. My reasoning was that, e.g. if we miss to override a method it's better to fail than to do a potentially unsafe operation
Not yet sure about the runtime penalty of the checks I've added, will have to investigate this more in future

However I don't have a strong opinion about any of them and will gladly change.

I'd appreciate if someone (maybe @axic) could have a look and see if it's generally ok so I can implement the remaining methods, add documentation and more tests.

holgerd77 commented 5 years ago

Googled a bit and came along this EIP from @axic and follow up/related work, will leave here for reference: https://github.com/ethereum/EIPs/issues/159

What is actually the current default behavior on overflow for the VM? @s1na Generally I think these two method types with the plain + mod method should be a good and flexible to choose from solution?

Another thing: do you have any link for better diff comparison?

s1na commented 5 years ago

Thanks for the link, will go through it.

Yeah so far I've come to the conclusion that we need both of these behaviours, each in some parts of the code. Having operations overflow by default seems safer and wherever wrapping is safe, the ops with explicit mod can be used.

Might be easier to directly compare the whole branch against index.js.

holgerd77 commented 5 years ago

Thanks! 😀 Is this branch now ready for review or is there still something missing?

s1na commented 5 years ago

Might be easier to directly compare the whole branch against index.js.

On second thought that wasn't a very helpful comment :laughing: Maybe I should break it into multiple PRs?

I still have to update the readme too.

holgerd77 commented 5 years ago

😀😛 If it's not too much work then it might be worth it - yes - since this is really heavy and broad changing stuff touching the fundamentals of the VM. We do lots of heavier changes atm (which is a good thing IMO) but should also not totally overrush and do some extra paths here and there where it valuably adds to transition reliability (what a sentence 😛).