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

fix: fix invalid signature verification #289

Merged

Conversation

edisontim
Copy link
Contributor

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Adapted the format of the signature to something more representative and fixed an issue with signatures not being verified correctly. Added some test cases that were generated by the javascript lib Noble

Does this introduce a breaking change?

  • Yes
  • No

Other information

@edisontim edisontim requested a review from 0xLucqs as a code owner April 1, 2024 14:18
Copy link
Collaborator

@0xLucqs 0xLucqs left a comment

Choose a reason for hiding this comment

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

I have no clue what's happening here, can you add some context plz

src/math/src/ed25519.cairo Outdated Show resolved Hide resolved
src/math/src/tests/ed25519_test.cairo Outdated Show resolved Hide resolved
src/math/src/ed25519.cairo Outdated Show resolved Hide resolved
@edisontim
Copy link
Contributor Author

The format I was using before for the signature as well as for the public key was very inconvenient.
I also had a bug where I was verifying the r_sign and s_sign part of the signature in big-endian when they should've been in little-endian. In my tests this was working but using a signature generated from a standard library this wouldn't work out of the box, so changed that as well. Noticed it after @AbdelStark tried using this function.

src/math/src/ed25519.cairo Outdated Show resolved Hide resolved
@0xLucqs
Copy link
Collaborator

0xLucqs commented Apr 4, 2024

otherwise looks good

src/math/src/ed25519.cairo Show resolved Hide resolved
@0xLucqs 0xLucqs merged commit bd822fb into keep-starknet-strange:main Apr 5, 2024
3 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants