Implement callback functionality#21
Conversation
There was a problem hiding this comment.
Thanks for the PR, I think the logic is not fully there yet but it's going into the right direction. Can you try wiring this into the example html so we have an example of the new signing methods for easier testing?
I think the end goal is that we don't need the bytes-backed signing key on the rust side anymore and can just rely on the new trait methods.
We might also want all the functions on the signing key to return result futures. Because the signing might take some time, so async is probably the best way to go about this. All operations are also relying on transports, so they are fallible as well.
| pub fn public_key_bytes(&self) -> Vec<u8> { | ||
| match &self.backend { | ||
| PrivateKeyBackend::Bytes(bytes_backend) => bytes_backend.public_key_bytes.to_vec(), | ||
| PrivateKeyBackend::Callback(_) => Vec::new(), |
There was a problem hiding this comment.
I think we should fail properly instead of returning an empty buffer here.
| /// Sign a 40-byte digest (same encoding as [`iris_ztd::Digest::to_bytes`]) with a 32-byte | ||
| /// private key. Returns 64 bytes: `c` then `s`, each 32-byte little-endian scalars. | ||
| #[wasm_bindgen(js_name = signDigestBytes)] | ||
| pub fn sign_digest_bytes( |
There was a problem hiding this comment.
I think this should not be needed as we can construct the private key from bytes on the wasm side anyways. Downstream signing should be transparent to the implementation.
h33p
left a comment
h33p
left a comment
There was a problem hiding this comment.
Change signDigest to signDigests
| if spend.accepts_signing_pubkey(&pk) { | ||
| let digest = spend.sig_hash(); | ||
| let sig = signing_key | ||
| .sign_digest(digest) |
There was a problem hiding this comment.
You should change this loop to collect all digests, then call sign_digests. This way, users don't click "yes" 20 times on a ledger, and do it only once instead.
F16oxy
force-pushed
the
feat/callback-support
branch
from
8099cf2 to
dd5221c
Compare
ko1N
left a comment
ko1N
left a comment
There was a problem hiding this comment.
Thanks, this already looks quite good! I believe what would also be nice to add is a second example to the grpc-web-demo (not necessarily a ledger example, but maybe a different way of signing via the callbacks). 🙏
| fn sign_digests<'a>( | ||
| &'a self, | ||
| digests: &'a [Digest], | ||
| ) -> impl Future<Output = Result<alloc::vec::Vec<Signature>, Self::Error>> + 'a { |
There was a problem hiding this comment.
I believe this needs to be gated by the alloc feature, otherwise it might break compilation on iris-rs with --no-default features.
| self | ||
|
|
||
| let signatures = signing_key | ||
| .sign_digests(&digests) |
There was a problem hiding this comment.
We should check if digests is empty before hitting the ledger with a potential empty signing request. This can happen if the spends do not match the given pkh. I believe this is something that could happen to legitimate users (e.g. when you plug in the wrong ledger) so we should probably propagate this as an error or at least abort early.
| } | ||
| let Some(spend) = self.spends.get_mut(&name) else { | ||
| debug_assert!(false, "signature staging used names from self.spends"); | ||
| return Err(SigningError::SignatureRejected); |
There was a problem hiding this comment.
Returning here could yield to an incomplete internal state, because some signatures might've already be applied to the spends (see 2 lines below). This could leave us in inconsistent internal state - not sure if we need to address it or if this is recoverable, but maybe its worth leaving a comment that this behavior is okay/expected.
| .sign_digests(&[digest]) | ||
| .await | ||
| .map_err(SigningError::Signer)? | ||
| .pop() |
There was a problem hiding this comment.
Maybe it would be worthwhile adding an assertion that 1 digest we provide only yields to 1 signature so if a subsequent implementation breaks this contract we fail early here.
3 participants
No description provided.