refactor with pinocchio framework - #2
Conversation
samkim-crypto
left a comment
samkim-crypto
left a comment
There was a problem hiding this comment.
Looking good. Some high-level comments below to start.
There was a problem hiding this comment.
I think the API will stabilize after a couple of iterations, so let's update the README file only after the API is stabilized. In particular, I think we should move away from the offset processing and just interpret the public key, signature, and message data directly from instruction data.
|
|
||
| /// Stateless, zero-allocation Ed25519 verifier. | ||
| #[derive(Debug, Clone, Copy, Default)] | ||
| pub struct Ed25519Verifier; |
There was a problem hiding this comment.
In the future, we should add a bunch of configuration options. For example, I am thinking we should support different variations organized in https://hdevalence.ca/blog/2020-10-04-its-25519am/). But let's do this in follow-ups since this PR is already quite big.
samkim-crypto
left a comment
samkim-crypto
left a comment
There was a problem hiding this comment.
Okay, looks good! I think this PR can be merged and then we can clean things up a bit in follow-up PRs.
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Just a few post-merge comments, feel free to address whenever
| /// Initializes a new verifier. | ||
| pub const fn new() -> Self { | ||
| Self | ||
| } |
There was a problem hiding this comment.
nit: I'm always a fan of less code rather than more code, and this doesn't seem to give very much
| @@ -0,0 +1,27 @@ | |||
| [package] | |||
| name = "solana-ed25519-verify" | |||
| version = "4.0.0" | |||
| #[cfg(not(any(target_os = "solana", target_arch = "bpf")))] | ||
| pub use instruction::sign_message; | ||
| #[cfg(all( | ||
| feature = "bincode", | ||
| not(any(target_os = "solana", target_arch = "bpf")) | ||
| ))] | ||
| pub use instruction::{ | ||
| new_ed25519_instruction_with_signature, offsets_to_ed25519_instruction, | ||
| try_new_ed25519_instruction_with_signature, try_offsets_to_ed25519_instruction, | ||
| }; | ||
| pub use instruction::{ | ||
| Ed25519SignatureOffsets, CURRENT_INSTRUCTION_INDEX, DATA_START, PUBKEY_SERIALIZED_SIZE, | ||
| SIGNATURE_OFFSETS_SERIALIZED_SIZE, SIGNATURE_OFFSETS_START, SIGNATURE_SERIALIZED_SIZE, | ||
| extern crate std; |
There was a problem hiding this comment.
It doesn't look this is used anywhere anymore, except in tests -- can we remove it?
| @@ -0,0 +1,24 @@ | |||
| extern crate alloc; | |||
There was a problem hiding this comment.
nit: is this line needed since it's already enabled in lib.rs?
There was a problem hiding this comment.
nit: can we call this file instruction.rs? That'll line up better with other clients
| }; | ||
|
|
||
| /// Constructs an on-chain instruction to invoke `solana-ed25519-program`. | ||
| pub fn ed25519_verify_instruction( |
| let public_key = instruction_data[..PUBKEY_SERIALIZED_SIZE] | ||
| .try_into() | ||
| .map_err(|_| ProgramError::InvalidInstructionData)?; | ||
| let signature = instruction_data[SIGNATURE_OFFSET..MESSAGE_OFFSET] | ||
| .try_into() | ||
| .map_err(|_| ProgramError::InvalidInstructionData)?; |
There was a problem hiding this comment.
nit: since there's a length check already, we should be able to unwrap these
| @@ -1,7 +1,6 @@ | |||
| RUST_TOOLCHAIN_NIGHTLY = nightly-2026-01-22 | |||
| SOLANA_CLI_VERSION = v3.1.10 | |||
| SBF_ARCH = v2 | |||
There was a problem hiding this comment.
nit for future work: v2 is getting removed, so we should use v3
3 participants
This PR does the following refactoring:
ed25519-verifylib and an exampleprogramto invoke the lib