Skip to content

Refactor: remove geth dependency - #440

Merged
gbotrel merged 16 commits into
developfrom
refactor/geth-dependency
Feb 6, 2023
Merged

Refactor: remove geth dependency#440
gbotrel merged 16 commits into
developfrom
refactor/geth-dependency

Conversation

@yelhousni

@yelhousni yelhousni commented Jan 20, 2023

Copy link
Copy Markdown
Contributor

Use gnark-crypto instead of geth/crypto for secp256k1 and ecdsa.
This needs Consensys-Incorporated/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
Copy Markdown
Contributor Author

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

@ivokub

ivokub commented Feb 1, 2023

Copy link
Copy Markdown
Collaborator

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.

@ivokub ivokub left a comment

Copy link
Copy Markdown
Collaborator

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