Skip to content

Commit

Permalink
I feel better variable naming would look like
Browse files Browse the repository at this point in the history 
this.

@Divide-By-0 any comments while I didn't start to apply it more widely?
  • Loading branch information
@skaunov
skaunov committed Oct 16, 2023
1 parent 7485a24 commit 251fba6
Showing 1 changed file with 16 additions and 15 deletions.
31 changes: 16 additions & 15 deletions rust-arkworks/src/lib.rs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
mod error;
mod hash_to_curve;
#[cfg(test)]
mod tests;

pub mod sig {
use crate::error::CryptoError;
Expand Down Expand Up @@ -50,22 +48,22 @@ pub mod sig {
}

fn compute_c_v1<P: SWModelParameters>(
g: &GroupAffine<P>,
generator: &GroupAffine<P>,
pk: &GroupAffine<P>,
h: &GroupAffine<P>,
nul: &GroupAffine<P>,
g_r: &GroupAffine<P>,
z: &GroupAffine<P>,
hash1: &GroupAffine<P>,
nullifier: &GroupAffine<P>,
r_point: &GroupAffine<P>,
hash1_r: &GroupAffine<P>,
) -> Output<Sha256> {
// Compute c = sha512([g, pk, h, nul, g^r, z])
let g_bytes = affine_to_bytes::<P>(g);
let pk_bytes = affine_to_bytes::<P>(pk);
let h_bytes = affine_to_bytes::<P>(h);
let nul_bytes = affine_to_bytes::<P>(nul);
let g_r_bytes = affine_to_bytes::<P>(g_r);
let z_bytes = affine_to_bytes::<P>(z);

let c_preimage_vec = [g_bytes, pk_bytes, h_bytes, nul_bytes, g_r_bytes, z_bytes].concat();
let c_preimage_vec = [
affine_to_bytes::<P>(generator),
affine_to_bytes::<P>(pk),
affine_to_bytes::<P>(hash1),
affine_to_bytes::<P>(nullifier),
affine_to_bytes::<P>(r_point),
affine_to_bytes::<P>(hash1_r)
].concat();

Sha256::digest(c_preimage_vec.as_slice())
}
Expand Down Expand Up @@ -267,3 +265,6 @@ pub mod sig {
}
}
}

#[cfg(test)]
mod tests;

9 comments on commit 251fba6

@Divide-By-0
Copy link
Member

@Divide-By-0 Divide-By-0 commented on 251fba6 Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Seems like a good start. I don't think hash1 or hash1_r are particularly more insightful; i like your point that hash1 and hash2 are bad names, can we improve that somehow? Also g and g_r are standard for generator, and generator and r_point seem inconsistent (i.e. maybe every* point should be labeled point or not)

@skaunov
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Divide-By-0 , Thanks for swift reply!

absolutely! =) just a mild improvement over hash while settling that in the issue

I don't think hash1 or hash1_r are particularly more insightful;

here I don't have a fine suggestion
maybe use full names like hash_to_point and "SHA256"?

hash1 and hash2 are bad names, can we improve that somehow?

yep, I guess it's my personal tendency to avoid terms in code which are that unhelpful with full-text search

g and g_r are standard for generator, and generator...

so, just leave g_r then? I thought r_scalar and r_point could be convenient
also didn't quite get second part in the parenthesis =(

r_point seem inconsistent (i.e. maybe very point should be labeled point or not)

thanks again for replying
I really feel like sort things out iteratively before further steps and "scaling" this along codebase
so further comments and approve are very welcome!

@Divide-By-0
Copy link
Member

Choose a reason for hiding this comment

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

maybe use full names like hash_to_point and "SHA256"?

Hey, great thoughts. hash_to_curve is standard so that should be fine, we shouldn't enshrine sha256 in terminology since some i.e. Mina uses Poseidon. Maybe hash_to_scalar for that one?

yep, I guess it's my personal tendency to avoid terms in code which are that unhelpful with full-text search

Good point, I like that.

so, just leave g_r then? I thought r_scalar and r_point could be convenient

Got it, I actually like that distinguishment. maybe rename g -> g_point and r -> r_point, and put r_scalar where relevant?

I really feel like sort things out iteratively before further steps and "scaling" this along codebase

agreed

@skaunov
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct me if I'm wrong: for systems with different tech we'll need to add suitable implementations. In this case verbal naming will be even more helpful: hash_sha256 and hash_poseidon immediately communicate quite some info.

... we shouldn't enshrine sha256 in terminology since some i.e. Mina uses Poseidon. ...

@skaunov
Copy link
Collaborator Author

@skaunov skaunov commented on 251fba6 Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Btw, do you now get notifications here when not mentioned? @Divide-By-0

@Divide-By-0
Copy link
Member

@Divide-By-0 Divide-By-0 commented on 251fba6 Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Btw, do you now get notifications here when not mentioned? @Divide-By-0

Yup.

correct me if I'm wrong

Sounds good. Note that their poseidon hash and poseidon hash to curve are technically both poseidon hashes so might be ambiguous

@skaunov
Copy link
Collaborator Author

@skaunov skaunov commented on 251fba6 Oct 19, 2023 via email

Choose a reason for hiding this comment

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

@Divide-By-0
Copy link
Member

Choose a reason for hiding this comment

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

One pass? What do you mean

@skaunov
Copy link
Collaborator Author

@skaunov skaunov commented on 251fba6 Oct 19, 2023 via email

Choose a reason for hiding this comment

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

Please sign in to comment.