Skip to content
This repository was archived by the owner on Oct 31, 2024. It is now read-only.

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

Closed
wants to merge 17 commits into from
Closed

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

wants to merge 17 commits into from

Conversation

s1na
Copy link
Collaborator

@s1na s1na commented Mar 13, 2019

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
Copy link

Coverage Status

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

@holgerd77
Copy link
Member

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

@s1na
Copy link
Collaborator Author

s1na commented Mar 25, 2019

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.

@s1na s1na changed the title [WIP] Modulo after operations [WIP] Rewrite in typescript, add more methods Mar 25, 2019
@s1na s1na mentioned this pull request Mar 26, 2019
@holgerd77
Copy link
Member

Googled a bit and came along this EIP from @axic and follow up/related work, will leave here for reference: ethereum/EIPs#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
Copy link
Collaborator Author

s1na commented Apr 10, 2019

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
Copy link
Member

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

@s1na
Copy link
Collaborator Author

s1na commented Apr 10, 2019

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

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

I still have to update the readme too.

@holgerd77
Copy link
Member

😀😛 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 😛).

@s1na s1na mentioned this pull request Apr 16, 2019
@holgerd77 holgerd77 closed this Oct 31, 2024
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.

3 participants