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

Elliptic curves utilities refactory #2068

Merged
merged 12 commits into from
Oct 31, 2023

Conversation

davxy
Copy link
Member

@davxy davxy commented Oct 27, 2023

  • Usage the new published arkworks-extensions crates.
    Hooks are internally defined to jump into the proper host functions.
  • Conditional compilation of each curve (gated by feature with curve name)
  • Separation in smaller host functions sets, divided by curve (fits nicely with prev point)

Runtime dev should not care that much about host functions and hooks as these are internally defined and he
can use the library as it'd use Arkworks upstream in his runtimes.


Please be aware that this library is mostly designed for internal usage within crates residing in the polkadot-sdk workspace, such as Bandersnatch. This is to avoid potential problems, as demonstrated in issues like #2013.

Devs of pallets outside the workspace can utilize this library or the stand alone crates available on crates.io and maintained in arkworks-substrate repository if they like.
Stand alone crates essentially offer the same functionality as the curve modules found here.

@davxy davxy self-assigned this Oct 27, 2023
@davxy davxy added the T0-node This PR/Issue is related to the topic “node”. label Oct 27, 2023
Copy link
Contributor

@michalkucharczyk michalkucharczyk left a comment

Choose a reason for hiding this comment

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

I did not dive into the logic, implementation details.
Overall it looks good.

I did not notice any test, maybe we should have some, (at least for utils)?

@bkchr
Copy link
Member

bkchr commented Oct 31, 2023

Devs of pallets outside the workspace can utilize this library or the stand alone crates available on crates.io and maintained in arkworks-substrate repository if they like.

So these crates here are just a verbatim copy?

Comment on lines +18 to +19
//! Elliptic curves which are mostly compatible with *Arkworks* library
//! mostly useful in non-native contexts.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe?

Suggested change
//! Elliptic curves which are mostly compatible with *Arkworks* library
//! mostly useful in non-native contexts.
//! This crate offers elliptic curves functionality which is mostly compatible with *Arkworks* library.
//! It is primarily designed for use in non-native contexts.

also wouid be nice to have link to Arkworks.

Comment on lines +21 to +25
//! The definitions make use of host functions to offload the non-native
//! computational environment from the some of the most computationally
//! expensive operations by internally leveraging the
//! [arkworks-extensions](https://github.com/paritytech/arkworks-extensions)
//! library.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//! The definitions make use of host functions to offload the non-native
//! computational environment from the some of the most computationally
//! expensive operations by internally leveraging the
//! [arkworks-extensions](https://github.com/paritytech/arkworks-extensions)
//! library.
//! The implementation make use of host functions to offload the non-native
//! environment from some of the most computationally
//! expensive operations. It internally leverages the
//! [arkworks-extensions](https://github.com/paritytech/arkworks-extensions)
//! library.

@davxy
Copy link
Member Author

davxy commented Oct 31, 2023

So these crates here are just a verbatim copy?

@bkchr Not really verbatim (but in practice, at the moment, yes). Here we leverage some tiny encode/decode shared functions.

After this is merged arkworks-substrate will be refactored to just expose (re-export) the stuff we are implementing here (0 logic there). So the crates there will act as some kind of convenience facade.

The arkworks-substrate crates are handy if someone wants to include the single crates individually and thus more closely mimics what you'd do when using upstream Arkworks.

@davxy davxy merged commit c38aae6 into paritytech:master Oct 31, 2023
139 of 140 checks passed
@davxy davxy mentioned this pull request Oct 31, 2023
@davxy davxy deleted the davxy-integrate-ark-defs branch December 1, 2023 15:47
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
- Usage the new published
[arkworks-extensions](https://github.com/paritytech/arkworks-extensions)
crates.
  Hooks are internally defined to jump into the proper host functions.
- Conditional compilation of each curve (gated by feature with curve
name)
- Separation in smaller host functions sets, divided by curve (fits
nicely with prev point)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
Status: done
Development

Successfully merging this pull request may close these issues.

4 participants