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

Refactor: remove geth dependency #440

Merged
merged 16 commits into from
Feb 6, 2023
Merged

Refactor: remove geth dependency #440

merged 16 commits into from
Feb 6, 2023

Conversation

yelhousni
Copy link
Contributor

@yelhousni yelhousni commented Jan 20, 2023

Use gnark-crypto instead of geth/crypto for secp256k1 and ecdsa.
This needs Consensys/gnark-crypto#310 to be merged first.

@yelhousni yelhousni requested a review from ivokub January 20, 2023 11:52
@yelhousni yelhousni marked this pull request as draft January 20, 2023 12:00
@gbotrel gbotrel added this to the v0.8.0 milestone Jan 23, 2023
@yelhousni yelhousni marked this pull request as ready for review January 24, 2023 13:58
@yelhousni
Copy link
Contributor Author

needs to be updated after Consensys/gnark-crypto#313 and Consensys/gnark-crypto#314 get merged in gnark-cypto.

@ivokub
Copy link
Collaborator

ivokub commented Feb 1, 2023

So, right now there is a slight problem - when we sign with gnark-crypto, as a signature we get a []byte, but it is difficult to get R and S from the signature. We would have to instantiate fr-specific ecdsa.Signature, unmarshal it from bytes and assign from it. It involves a type-switch, but I think it is ok.

I have some ideas and will try to push the changes.

Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

Rebased on top of develop, added a few more changes (avoid explicit conversion to *big.Int, rename package import, update go.mod).

Previously I totally overthought it. I think the simple approach is better (read signature directly into secp256k1 ECDSA signature struct and get R and S from there).

As far as it seems then the failing tests are due to MiMC hashing. Imo are unrelated to this PR.

@gbotrel gbotrel merged commit 1097a17 into develop Feb 6, 2023
@gbotrel gbotrel deleted the refactor/geth-dependency branch February 6, 2023 15:28
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.

3 participants