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

Feat ecdsa sigs #104

Closed
wants to merge 2 commits into from
Closed

Feat ecdsa sigs #104

wants to merge 2 commits into from

Conversation

samlaf
Copy link
Collaborator

@samlaf samlaf commented Jan 25, 2024

We're currently developing ecdsa versions of everything:

Leaving this as a draft until I've finished incorporating these changes into the ecdsa version of inc-sq, but would still appreciate a review on the current structural decisions in this PR. Main things are:

  • split avsregistry clients and aggregation service into ecdsa and bls ones
    • ideally we'd only have one aggregation service, possibly using generics, that could do both ecdsa and bls aggregation, but I just wanted something quick for now, so opted to duplicate a lot of the code. There are a lot of changes though, so don't think merging them would be that easy... we can prob get a junior dev to clean this up eventually (if we think its worth it)
  • utils and types are getting ugly with the duplicate ecdsa and bls names, so please check those and suggest any better alternative for organizing these things

@samlaf samlaf marked this pull request as draft January 25, 2024 19:44
break up services into bls and ecdsa

create ecdsa SignMsg function - more of a placeholder for now

generated bindings for ecdsaSignatureChecker

updated middleware contracts again (after rebasing on master) and updated bindings

renamed bls/bls_aggregation -> bls/aggregation

updated middleware contracts and generated bindings

some updates to ecdsa types

ecdsa avs registry service tests pass

no more compilation errors - mocks generated - ecdsa agg tests still failing

tests are passing
Comment on lines 10 to 17
// SignMsg is a wrapper around geth's crypto.Sign function which adds 27 to the v value
// see https://github.com/ethereum/go-ethereum/issues/28757#issuecomment-1874525854
// and https://twitter.com/pcaversaccio/status/1671488928262529031
// Currently all of our contracts expect the v value to be 27 or 28, which is why we add it here.
// VerifySignature below thus subtracts 27 from the v value before verifying the signature.
// TODO(samlaf): if and when we figure out some avs contracts use the 0/1 v scheme, or the eip-155 scheme,
// then we should generalize this function to sign based on a scheme passed as argument.
func SignMsg(msg []byte, privateKey *ecdsa.PrivateKey) ([]byte, error) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is an issue that I'm unsure how to deal with, so arranged it like this for now, but if anyone has ideas on how to better deal with this, would be good to know.

@shrimalmadhur
Copy link
Collaborator

Closing it because it is outdated

@MegaRedHand MegaRedHand deleted the feat-ecdsa-sigs branch February 17, 2025 15:09
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.

2 participants