-
Notifications
You must be signed in to change notification settings - Fork 0
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
Full transaction format with signer integration #74
Changes from 22 commits
1c72ad7
d32ce25
9c2089c
34f3fee
e549cd6
171e5f9
c3fe52c
c44fd5f
97adddb
6c042d5
c29f5bd
b38f785
fca49f3
52b2854
8015e35
3003e72
79c18c5
54616bc
31fc954
609fa94
69e1990
0242e4c
09cfc7e
578f520
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,5 +20,13 @@ pub trait CryptoSigner { | |
signature_bytes: U, | ||
) -> Result<(), CryptoError>; | ||
|
||
#[cfg(feature = "tx")] | ||
fn verify_tx(&self, tx: kairos_tx::asn::Transaction) -> Result<(), CryptoError>; | ||
#[cfg(feature = "tx")] | ||
fn sign_tx_payload( | ||
&self, | ||
payload: kairos_tx::asn::SigningPayload, | ||
) -> Result<kairos_tx::asn::Transaction, CryptoError>; | ||
|
||
Comment on lines
+25
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't love feature gated parts of traits. I also don't like having a trait with a single impl, but these issues can be addressed in followup at a much latter date. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Created follow up task - #86. |
||
fn to_public_key(&self) -> Result<Vec<u8>, CryptoError>; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ pub use rasn::types::{Integer, OctetString}; | |
use num_traits::cast::ToPrimitive; | ||
use rasn::types::AsnType; | ||
use rasn::{Decode, Encode}; | ||
use sha2::Digest; | ||
|
||
#[derive(AsnType, Encode, Decode, Debug, Clone)] | ||
#[rasn(delegate)] | ||
|
@@ -21,6 +22,12 @@ impl From<PublicKey> for Vec<u8> { | |
} | ||
} | ||
|
||
impl From<Vec<u8>> for PublicKey { | ||
fn from(value: Vec<u8>) -> Self { | ||
PublicKey(OctetString::copy_from_slice(&value)) | ||
} | ||
} | ||
|
||
impl From<&[u8]> for PublicKey { | ||
fn from(value: &[u8]) -> Self { | ||
PublicKey(OctetString::copy_from_slice(value)) | ||
|
@@ -33,6 +40,40 @@ impl<const N: usize> From<&[u8; N]> for PublicKey { | |
} | ||
} | ||
|
||
#[derive(AsnType, Encode, Decode, Debug, Clone)] | ||
#[rasn(delegate)] | ||
pub struct Signature(pub(crate) OctetString); | ||
|
||
// Converts an ASN.1 decoded signature into raw byte representation. | ||
impl From<Signature> for Vec<u8> { | ||
fn from(value: Signature) -> Self { | ||
value.0.into() | ||
} | ||
} | ||
|
||
impl From<Vec<u8>> for Signature { | ||
fn from(value: Vec<u8>) -> Self { | ||
Signature(OctetString::copy_from_slice(&value)) | ||
} | ||
} | ||
|
||
#[derive(AsnType, Encode, Decode, Debug, Clone)] | ||
#[rasn(delegate)] | ||
pub struct PayloadHash(pub(crate) OctetString); | ||
|
||
// Converts an ASN.1 decoded payload hash into raw byte representation. | ||
impl From<PayloadHash> for Vec<u8> { | ||
fn from(value: PayloadHash) -> Self { | ||
value.0.into() | ||
} | ||
} | ||
|
||
impl From<&[u8; 32]> for PayloadHash { | ||
fn from(value: &[u8; 32]) -> Self { | ||
PayloadHash(OctetString::copy_from_slice(value)) | ||
} | ||
} | ||
|
||
#[derive(AsnType, Encode, Decode, Debug, Clone)] | ||
#[rasn(delegate)] | ||
pub struct Amount(pub(crate) Integer); | ||
|
@@ -82,6 +123,54 @@ impl From<u64> for Nonce { | |
} | ||
} | ||
|
||
#[derive(AsnType, Encode, Decode, Debug)] | ||
#[non_exhaustive] | ||
pub struct Transaction { | ||
pub public_key: PublicKey, // NOTE: Field name can be different than defined in schema, as only **order** is crucial | ||
pub payload: SigningPayload, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand that you can compute the PayloadHash, I'm just wondering where the according computation happens? Or should it be added as a field here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for catching it, I forgot to add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Computing |
||
pub hash: PayloadHash, | ||
pub algorithm: SigningAlgorithm, | ||
pub signature: Signature, | ||
} | ||
|
||
impl Transaction { | ||
/// Wraps full transaction data for storing. | ||
/// | ||
/// CAUTION: This method does NOT perform validity checks - please use | ||
/// `kairos-crypto::sign_tx_payload()` to construct it safely. | ||
pub fn new( | ||
public_key: impl Into<PublicKey>, | ||
payload: SigningPayload, | ||
hash: impl Into<PayloadHash>, | ||
algorithm: SigningAlgorithm, | ||
signature: Signature, | ||
) -> Self { | ||
Self { | ||
public_key: public_key.into(), | ||
payload, | ||
hash: hash.into(), | ||
algorithm, | ||
signature, | ||
} | ||
} | ||
|
||
pub fn der_encode(&self) -> Result<Vec<u8>, TxError> { | ||
rasn::der::encode(self).map_err(TxError::EncodeError) | ||
} | ||
|
||
pub fn der_decode(value: impl AsRef<[u8]>) -> Result<Self, TxError> { | ||
rasn::der::decode(value.as_ref()).map_err(TxError::DecodeError) | ||
} | ||
} | ||
|
||
#[derive(AsnType, Encode, Decode, Debug, PartialEq, Copy, Clone)] | ||
#[rasn(enumerated)] | ||
#[non_exhaustive] | ||
pub enum SigningAlgorithm { | ||
CasperSecp256k1 = 0, | ||
CasperEd25519 = 1, | ||
} | ||
|
||
#[derive(AsnType, Encode, Decode, Debug)] | ||
#[non_exhaustive] | ||
pub struct SigningPayload { | ||
|
@@ -130,6 +219,15 @@ impl SigningPayload { | |
pub fn der_decode(value: impl AsRef<[u8]>) -> Result<Self, TxError> { | ||
rasn::der::decode(value.as_ref()).map_err(TxError::DecodeError) | ||
} | ||
|
||
// Computes the hash for a transaction. | ||
// Hash is obtained from payload by computing sha256 of DER encoded ASN.1 data. | ||
pub fn hash(&self) -> Result<[u8; 32], TxError> { | ||
let data = self.der_encode()?; | ||
let tx_hash: [u8; 32] = sha2::Sha256::digest(data).into(); | ||
|
||
Ok(tx_hash) | ||
} | ||
} | ||
|
||
#[derive(AsnType, Encode, Decode, Debug)] | ||
|
@@ -206,6 +304,22 @@ impl Withdrawal { | |
} | ||
} | ||
|
||
impl TryFrom<&[u8]> for Transaction { | ||
type Error = TxError; | ||
|
||
fn try_from(value: &[u8]) -> Result<Self, Self::Error> { | ||
Transaction::der_decode(value) | ||
} | ||
} | ||
|
||
impl TryFrom<Transaction> for Vec<u8> { | ||
type Error = TxError; | ||
|
||
fn try_from(value: Transaction) -> Result<Self, Self::Error> { | ||
value.der_encode() | ||
} | ||
} | ||
|
||
impl TryFrom<&[u8]> for SigningPayload { | ||
type Error = TxError; | ||
|
||
|
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 don't see a benefit to this
CryptoSigner
trait. It should instead be replaced with asign
andverify
generic function.It also does too much at this point.
from_private_key_file
andfrom_public_key
should be removed and replaced withFrom
trait implementationsto_public_key
can be extracted to aFrom
trait implementation too.sign
andverify
could be extracted in a trait, but I would be surprised that there is nothing like this already somewhere.Instead of all these steps, I would remove the
CryptoSigner
trait completely, and implement a generic function that says "if these traits are implemented ..." I will produce either this signed result or an error.sign_tx
could moreover be an alias for callingsign(tx.into())
. If the ASN1 type would have a respectiveFrom
trait implementation. And would moreover not require us to introduce atx
feature.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 would prefer if
from_private_key_file
remain a method, not aFrom
impl.I don't like
From
/Into
doing IO.I agree we don't need the
CryptoSigner
trait, since there is only one implementation just keep theSigner
struct.Currently this implementation signs transaction hashes.
I have my doubts about signing transaction hashes rather than transactions bodies.
It is more general, but I suspect it's a lot of overhead in a zkVM on simple transfer/withdraw payloads.
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.
You have to hash the input before signing; signing algorithms take 255-bit input (for secp256k1) or 256-bit input (for ed25519). See https://crypto.stackexchange.com/a/59203