61

[skaunov]

Proposed solution. Sharing early for possible reviews; will be finishing presumably tomorrow.
All done.

(Note that I decided to go with it from #66 not to merge/rebase all that renaming again 🙄 )

[skaunov]

@Divide-By-0 , btw, I'm puzzled: does the other version of the scheme takes those two additional fields only together or any of one? Test coverage and the implementation under refactoring somewhat clashes.

PS also I should check if I didn't mess the struct naming...

[Divide-By-0]

@Divide-By-0 , btw, I'm puzzled: does the other version of the scheme takes those two additional fields only together or any of one? Test coverage and the implementation under refactoring somewhat clashes.

Sorry I'm a bit confused what this means. Can you specify what the other scheme is (v2?) and what's any of one field mean? Not sure what is the scheme under implementation or what the tests cover but feel free to choose what is most intuitive and passes the circom circuit test as well.

Naming I don't know if you got everything but it seems reasonable, if tests pass then it should be fine.

There should be no package lock, please use yarn.

[skaunov]

Sorry, I was writing from Android that one. 😅

I indeed confused V1 and V2 naming -- will straight now.

the question

I tried to formulate the question, and it's still too wordy and confusing. Let me ask it with a link better.

zk-nullifier-sig/rust-k256/src/lib.rs

Line 101 in 6c487c0

r_point_optional: &Option<ProjectivePoint>,

it optionally takes r_point and hashed_to_curve and takes version (V1/V2).

So I made it quite restrictive oriented by the tests: if you supply these optional values, then your version is V1. You can't supply only one of the values, absence of values set version to V2.

But today I just thought that the logic could be different and it's ok to supply that values, verify them, but still be using V2; or supply V2 c for verification along with one of the optional values. If so, I would say these would be good to be covered by tests too.

[Divide-By-0]

Yeah, seems fine to put in those values in V2 as well, it'll make migration easier.

[skaunov]

tried such approach locally: and it doesn't look that good as it sounds =(
makes API of the function really messy

it looks much better to add additional functions to verify these V1 fields with V2 sig
if we want to cover it

other way it's a mess when that optional or not on input
and yields complex output to communicate if sig is verified ok V2, but additional V1 info is not %)

and I better keep output simple bool

[Divide-By-0]

Sorry about the delay here! Thanks for the renaming.

[skaunov]

Sorry about the delay here! Thanks for the renaming.

Cool! Though I don't remember renaming per se there, only when needed for code refactoring.