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

Upstream openmina changes #2750

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sebastiencs
Copy link

@sebastiencs sebastiencs commented Nov 8, 2024

@dannywillems This contains the diffs we use for openmina
Merging this will allow us to depend directly on your master branch, without having to maintain our own fork

cc @adonagy @binier , some commits belongs to you

@@ -93,6 +93,13 @@ impl Keypair {
pub fn to_hex(&self) -> String {
hex::encode(self.to_bytes())
}

/// Performs scalar multiplication with the secret key
Copy link
Member

Choose a reason for hiding this comment

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

Opinion: I would inline it in the OpenMina codebase, see #2755. Curious to see the other team members opinions.

@@ -19,6 +19,16 @@ impl Signature {
pub fn new(rx: BaseField, s: ScalarField) -> Self {
Self { rx, s }
}

/// Dummy signature
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@@ -232,6 +232,12 @@ pub struct CompressedPubKey {
pub is_odd: bool,
}

impl fmt::Debug for CompressedPubKey {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.write_fmt(format_args!("{}", self.into_address()))
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

/// Mimic OCaml `point[0]`
///
/// Short for `first`
pub fn fst(&self) -> Evals {
Copy link
Member

Choose a reason for hiding this comment

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

We should change the Caml side instead. It is pretty ugly if we do point[0] in OCaml.

Copy link
Member

Choose a reason for hiding this comment

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

See openmina/openmina#890. I'll keep in mind to change in Pickles.

@@ -207,7 +207,7 @@ where
};

// TODO: Switch to commit_evaluations for all index polys
VerifierIndex {
Arc::new(VerifierIndex {
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if it requires changes in wasm stubs. I'll have a look later.

Copy link
Author

Choose a reason for hiding this comment

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

@dannywillems Did you have time to look into this ?

Copy link
Member

Choose a reason for hiding this comment

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

@volhovm is working on merging develop into master and use master in MinaProtocol/Mina. We should have an idea if it breaks when this is done.

Copy link
Member

Choose a reason for hiding this comment

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

Work happening here. I'll come back on this PR when !2868 is merged (with o1js/mina related PR).

@sebastiencs
Copy link
Author

Just removed dde0cdd from the PR.
Now waiting on:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants