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

Refactoring schnorrsig #329

Closed
wants to merge 4 commits into from
Closed

Conversation

dr-orlovsky
Copy link
Contributor

As a part of rust-bitcoin/rust-bitcoin#588 discussion I propose this form of refactoring for existing multiple key and signature types:

  1. All keys, including BIP-340 specific, are moved into key module
  2. schnorrsig renamed to schnorr to have more reasonable schnorr::Signature name
  3. Old (non-schnorr) signatures are extracted into new ecdsa module, such that we can always prefix specific signature with proper type name (ecdsa::Signature and schnorr::Signature).

I understand that PR may be hard to review due to a big size of the diff, so I look for concept ACK and suggestions on how to simplify the review process

@real-or-random
Copy link
Collaborator

Are you aware of #327 ?

@dr-orlovsky
Copy link
Contributor Author

@real-or-random sorry, I missed that. Feel free to close this one if that one is considered the route forward

@real-or-random
Copy link
Collaborator

Well, I think it's not yet decided (even if we want to do something at all), but now we have two suggestions. :)

@thomaseizinger
Copy link
Contributor

I think the proposal is pretty much the same :)
In #327, I also changed around the names of the functions on Secp256k1 and a few other, minor things.

I don't mind much which of these two moves forward as long as we can land some improvement here!

@dr-orlovsky
Copy link
Contributor Author

I think #327 is better structured in terms of commit structure, has things on top (like improving function names) and more reviews already. Nice thing about mine is that it confirms that the approach taken by @thomaseizinger can be independently reproduced and such changes are needed. The fact that we did almost the same PRs independently is a good showcase that the new API design is more intuitive.

I will move this PR to a draft mode for now, such that reviewers time is not wasted on this one - and to have a comparison PR which may simplify reviewing / confirmation of the correctness of the other PR (by git difff <two branches> or something like that)

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