-
Notifications
You must be signed in to change notification settings - Fork 5
feat: define Signed<M, A>
message
#321
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
Signed<M, S>
message
fn verify(&self, pub_key: HexBinary, msg: impl AsRef<[u8]>) -> Result<(), Error>; | ||
} | ||
|
||
impl Verifier for Signature<SpendAuth> { |
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 added this example impl to showcase what a Verifier
implementation might look like and to explain why the verify
method has that particular signature. Once the changes are approved, I’ll remove this along with the decaf377-rdsa
dependency.
Side note: one reason we introduced this Signed
type was to avoid the orphan rule, where we’d need to implement the Quartz trait (in our case, HasUserData
) for a foreign type. But it seems not avoidable. Even with this Signed
type, we’d still need to wrap the signature in our usecase as a newtype so can implement this Verifier
trait. Unless there’s another approach I’ve missed?
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.
Yeah, I think you're right 😓 , the newtype is unavoidable. Ig the other option is to have a third generic field in the Signed
struct that implements the verifier e.g. ->
#[derive(Clone, Debug, PartialEq)]
pub struct Signed<M, S, V = EcdsaSigVerifier> { // Provide a PenumbraSigVerifier too
msg: M,
sig: S,
verifier: PhantomData<V>,
}
Haven't thought this through but it might be more restrictive if we just used PhantomData
especially if the verifier needed its own state. I think what you implemented is better.
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.
Yeah, that makes sense. if the verifier ends up needing internal state, that could be limiting.
BTW, we can keep this PR open for a bit before merging, just to see how everything plays out on the prime side after integrating this signed message, if that makes sense?
Signed<M, S>
messageSigned<M, A>
message
While working on the epoch-based clearing issue, I experimented with this new message format to see what it could handle. Mainly to (1) limit who can request a clearing, and (2) tell apart signed requests from the enclave vs. those from admins. Turns out, we can't just use That said, after some trial and internal discussions, the implementation got a few updates. I added an So as for this PR, I think we’re good to go now, unless there’s anything else? |
Closes: #320