-
Notifications
You must be signed in to change notification settings - Fork 301
feat(lazer): support ecdsa signers in solana contract #2429
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
|
||
[dependencies] | ||
pyth-lazer-protocol = "0.5.0" | ||
pyth-lazer-protocol = { path = "../../../../sdk/rust/protocol", version = "0.6.0" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you gonna change this back to getting protocol from registry before merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should leave it like this. It will use the local version when working within the workspace, and the published version will use the published dependency.
let message = LeEcdsaMessage::deserialize_slice(&message_data) | ||
.map_err(|_| ProgramError::InvalidInstructionData)?; | ||
|
||
let pubkey = secp256k1_recover( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this function validate the signature too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's walking backwards. it gives you the address that has generated the given signature. Then we should check it matches one of our trusted signers.
return Err(ProgramError::AccountDataTooSmall.into()); | ||
} | ||
if trusted_signers.len() > MAX_NUM_TRUSTED_SIGNERS { | ||
return Err(ProgramError::InvalidInstructionData.into()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to have more detailed error messages. I remember when I was debugging an error on mainnet it was really hard to figure out where exactly an error was happening because a single error message can be thrown in multiple places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Let's handle it as a separate task though.
pub const MAX_NUM_TRUSTED_SIGNERS: usize = 2; | ||
pub const SPACE_FOR_TRUSTED_SIGNERS: usize = 5; | ||
pub const EXTRA_SPACE: usize = 100; | ||
pub const SPACE_FOR_TRUSTED_ECDSA_SIGNERS: usize = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be ok for us to be more flexible here? like allow 5 trusted signers for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have enough space for 5 signers in the currently allocated space.
let message = LeEcdsaMessage::deserialize_slice(&message_data) | ||
.map_err(|_| ProgramError::InvalidInstructionData)?; | ||
|
||
let pubkey = secp256k1_recover( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's walking backwards. it gives you the address that has generated the given signature. Then we should check it matches one of our trusted signers.
5903931
to
85bedd0
Compare
Summary
Rationale
Consumers asked for it.
How has this been tested?