Skip to content

Conversation

@jimmychu0807
Copy link

No description provided.

Copy link
Collaborator

@ArtiomTr ArtiomTr left a comment

Choose a reason for hiding this comment

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

one small moment: when you add precompile block to previous ones, e.g.:

// RISC0
#[cfg(all(target_os = "zkvm", target_vendor = "risc0"))]
{ /* ... */ }

#[cfg(all(target_os = "zkvm", target_vendor = "zisk"))]
{ /* your code */ }

// SP1
#[cfg(all(target_os = "zkvm", target_vendor = "sp1"))]
{ /* ... */ }

can you please move your patch to the bottom, and include comment:

// RISC0
#[cfg(all(target_os = "zkvm", target_vendor = "risc0"))]
{ /* ... */ }

- #[cfg(all(target_os = "zkvm", target_vendor = "zisk"))]
- { /* your code */ }

// SP1
#[cfg(all(target_os = "zkvm", target_vendor = "sp1"))]
{ /* ... */ }

+ // Zisk
+ #[cfg(all(target_os = "zkvm", target_vendor = "zisk"))]
+ { /* your code */ }

thank you 🙏


#[inline]
#[cfg(all(target_os = "zkvm", target_vendor = "zisk"))]
pub fn mul_r_inv(&self) -> Fp {
Copy link
Collaborator

Choose a reason for hiding this comment

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

better not to add publicly-available functions, that are zkvm-specific. If you need this function outside this file, you can make it pub(crate), to expose only to current crate.


#[inline]
#[cfg(all(target_os = "zkvm", target_vendor = "zisk"))]
pub fn mul_r_inv(&self) -> Fp2 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as previous one, should be private or pub(crate)

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.

2 participants