Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Split out bn128 helpers into a separate package #9147

Closed
axic opened this issue Jul 17, 2018 · 3 comments
Closed

Split out bn128 helpers into a separate package #9147

axic opened this issue Jul 17, 2018 · 3 comments
Labels
F8-enhancement 🎊 An additional feature request. M4-core ⛓ Core client code / Rust. M5-dependencies 🖇 Dependencies. P7-nicetohave 🐕 Issue is worth doing eventually. Q2-easy 💃 Can be fixed by copy and pasting from StackOverflow.
Milestone

Comments

@axic
Copy link

axic commented Jul 17, 2018

This is kind of a feature request.

Currently the precompiled contracts are implemented in ethcore/builtin. The bn128 ecadd/ecmul/ecpairing precompiles are implemented there as well, using the bn crate as the underlying library.

In ethereumjs-vm the same underlying library is used and part of the code is duplicated as a wrapper library rustbn.js.

Now additionally we are experimented with writing the precompiles as contracts in the ewasm project and as a result we need the very same wrapper, hence we have split out the above code from builtin.rs into a crate of its own: https://github.com/ewasm/ethereum-bn128.rs

It can now be used by both rustbn.js and the contract.

This works well, but it seems like a bad idea to maintain a fork and such would like to invite the maintainers to consider splitting out that code into a crate maintained in this org and used by the Parity client too.

Would that be considered? Are there are any reasons against it? Would you accept a PR to do this?

@NikVolf
Copy link
Contributor

NikVolf commented Jul 18, 2018

Hi axic!

Sure, we will accept the pr, no reason against it.

But the code in ethcore/builtin is merely deserialization and serialization of points and field members, all the work is done in bn itself, what do you want to split out exactly?

@axic
Copy link
Author

axic commented Jul 18, 2018

If you can have a look at https://github.com/ewasm/ethereum-bn128.rs/blob/master/src/lib.rs it has the parts we need to be split: the (de)serialisation and the test cases.

I'd assume these internal helpers would then be called by the Builtin
implementation for the said precompiles.

The gas calculation could also go in there, but not necessarily.

@debris debris added F8-enhancement 🎊 An additional feature request. Q2-easy 💃 Can be fixed by copy and pasting from StackOverflow. labels Jul 19, 2018
@5chdn 5chdn added this to the 2.3 milestone Sep 25, 2018
@5chdn 5chdn added P7-nicetohave 🐕 Issue is worth doing eventually. M4-core ⛓ Core client code / Rust. M5-dependencies 🖇 Dependencies. labels Sep 25, 2018
@5chdn 5chdn modified the milestones: 2.3, 2.4 Oct 29, 2018
@5chdn 5chdn modified the milestones: 2.4, 2.5 Jan 10, 2019
@5chdn 5chdn modified the milestones: 2.5, 2.6 Feb 21, 2019
@soc1c soc1c modified the milestones: 2.6, 2.7 Apr 2, 2019
@debris
Copy link
Collaborator

debris commented Jul 7, 2019

we just split builtins into it's own crate (#10850). for now, further splits are not planned

@debris debris closed this as completed Jul 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
F8-enhancement 🎊 An additional feature request. M4-core ⛓ Core client code / Rust. M5-dependencies 🖇 Dependencies. P7-nicetohave 🐕 Issue is worth doing eventually. Q2-easy 💃 Can be fixed by copy and pasting from StackOverflow.
Projects
None yet
Development

No branches or pull requests

5 participants