search

plume-sig / zk-nullifier-sig

Implementation of PLUME: nullifier friendly signature scheme on ECDSA
MIT License
128 stars 22 forks source link

Solve #69 (Tests improvements) #87

Closed skaunov closed 5 months ago

skaunov commented 5 months ago

I suggest to squash the commits while merging.

Divides tests into unit and integration.

Clean and structure info it provided.

Disjoints mod helpers from the release; along the road where pastes and adapts "one-liners" where it relied on the actual code under the testing (preserves used function calls indication on the best efforts as comments, they're good to be removed on a next iteration).

Divide-By-0 commented 5 months ago

Circom checks fail?

skaunov commented 5 months ago

Should be out of bounds running long as usual. :shrug: Of course I didn't touch it, only one crate here.

On Thu, Feb 1 2024 at 09:12:03 AM -0800, Yush G @.***> wrote:

Circom checks fail?

— Reply to this email directly, view it on GitHub https://github.com/plume-sig/zk-nullifier-sig/pull/87#issuecomment-1921814916, or unsubscribe https://github.com/notifications/unsubscribe-auth/APXLOT5DJI4WRCB6BJROXBTYRPEGHAVCNFSM6AAAAABCUVGANWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRRHAYTIOJRGY. You are receiving this because you authored the thread.Message ID: @.***>

Divide-By-0 commented 5 months ago

Should be out of bounds running long as usual. 🤷 Of course I didn't touch it, only one crate here. … On Thu, Feb 1 2024 at 09:12:03 AM -0800, Yush G @.> wrote: Circom checks fail? — Reply to this email directly, view it on GitHub <#87 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/APXLOT5DJI4WRCB6BJROXBTYRPEGHAVCNFSM6AAAAABCUVGANWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRRHAYTIOJRGY. You are receiving this because you authored the thread.Message ID: @.>

Can we still correct that test timeout though? I can't pass a failing test. Github actions should work.

skaunov commented 5 months ago

@Divide-By-0 , I just run locally when see that something is changed in <./circuits/circom>. \ Do they offer plans for running longer tasks? It takes around 20 min. IIRC? \ Another way could be moving <./circuits/circom> to a separate repo, so it won't run unless it's changed.

Divide-By-0 commented 5 months ago

@Divide-By-0 , I just run locally when see that something is changed in <./circuits/circom>. Do they offer plans for running longer tasks? It takes around 20 min. IIRC? Another way could be moving <./circuits/circom> to a separate repo, so it won't run unless it's changed.

I'd recommend either using a self-hosted runner on my GCP account (I can add you) or triggering a modal.com run to test instead (I can cover that, it's dirt cheap).

Divide-By-0 commented 5 months ago

should add that it's also needed to

  • convert tests to integration,
  • clean hex strings assertions for further comprehensibilty. \ (This well might demote AsRef issue to a nice to have thing.)

Can we add these to some issues?

skaunov commented 5 months ago

should add that it's also needed to

  • convert tests to integration,
  • clean hex strings assertions for further comprehensibilty. (This well might demote AsRef issue to a nice to have thing.)

Can we add these to some issues?

This PR did both! (cross-linking for consistency: https://github.com/plume-sig/zk-nullifier-sig/issues/84#issuecomment-1929013072)