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 opchecksig bug #223

Merged

Conversation

Copy link

vercel bot commented Sep 18, 2024

@bloomingpeach is attempting to deploy a commit to the keep-starknet-strange Team on Vercel.

A member of the Team first needs to authorize it.

@b-j-roberts
Copy link
Contributor

Hey, were you able to track down which line was giving the index out-of-bounds error? I'm curious where it was failing

@bloomingpeach
Copy link
Contributor Author

it fails here: https://github.com/keep-starknet-strange/shinigami/blob/main/packages/engine/src/signature/signature.cairo#L298

By the way I think it is not supposed to go that far with an empty pubkey, https://github.com/keep-starknet-strange/shinigami/blob/main/packages/engine/src/signature/signature.cairo#L252 I think we should at least have a check for empty pubkey before or inside this function.

@b-j-roberts
Copy link
Contributor

I see, thanks for the info! I think how you did it is a good solution.

However, I think we should also add a check here for if the pk_bytes.len != 65 and return an error, since uncompressed pub keys should always be of length 65 I think.

Would need to make parse_pub_key return an error too.

@bloomingpeach
Copy link
Contributor Author

Would you like me to include the check in this PR or should it be in another PR? @b-j-roberts

@b-j-roberts
Copy link
Contributor

Would you like me to include the check in this PR or should it be in another PR? @b-j-roberts

In this PR if you can, thanks

@bloomingpeach
Copy link
Contributor Author

@b-j-roberts please review sir

Copy link
Contributor

@b-j-roberts b-j-roberts left a comment

Choose a reason for hiding this comment

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

Sorry for taking forever to get to this, thank you! And great work as always.

@b-j-roberts b-j-roberts merged commit 196aa5c into keep-starknet-strange:main Oct 4, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment