From b294a81bb28a261f88c0045ed965036fb7c71148 Mon Sep 17 00:00:00 2001 From: Francesco Date: Wed, 18 Dec 2024 15:52:56 +0100 Subject: [PATCH 01/13] Fix TX signature V for legacy TXs --- src/did/src/transaction.rs | 86 +++++++++++++++++++++++++++++++++++++- 1 file changed, 84 insertions(+), 2 deletions(-) diff --git a/src/did/src/transaction.rs b/src/did/src/transaction.rs index e010812f..8adef52c 100644 --- a/src/did/src/transaction.rs +++ b/src/did/src/transaction.rs @@ -2,6 +2,7 @@ use std::borrow::Cow; use std::rc::Rc; use std::str::FromStr; +use alloy::consensus::transaction::to_eip155_value; use alloy::consensus::{ SignableTransaction, Transaction as TransactionTrait, TxEip1559, TxEip2930, TxLegacy, }; @@ -365,11 +366,15 @@ impl From for alloy::consensus::TxEnvelope { impl From for Transaction { fn from(tx: alloy::rpc::types::Transaction) -> Self { - let signature: Signature = (*tx.inner.signature()).into(); + + let signature = tx.inner.signature(); + let signature_v = signature.v(); + let signature: Signature = (*signature).into(); match tx.inner { alloy::consensus::TxEnvelope::Legacy(signed) => { let inner_tx = signed.tx(); + let signature_v = to_eip155_value(signature_v, inner_tx.chain_id()); Self { hash: (*signed.hash()).into(), nonce: inner_tx.nonce.into(), @@ -386,7 +391,7 @@ impl From for Transaction { block_number: tx.block_number.map(Into::into), transaction_index: tx.transaction_index.map(Into::into), from: tx.from.into(), - v: signature.v, + v: (signature_v as u64).into(), r: signature.r, s: signature.s, transaction_type: Some(TRANSACTION_TYPE_LEGACY.into()), @@ -1350,6 +1355,10 @@ mod test { let (re_hash, re_rlp) = tx_from_rlp.slow_hash(); assert_eq!(hash, re_hash); assert_eq!(rlp, re_rlp); + + assert_eq!(transaction.s, tx_from_rlp.s); + assert_eq!(transaction.r, tx_from_rlp.r); + assert_eq!(transaction.v, tx_from_rlp.v); } let transaction_to_value = serde_json::to_value(transaction).unwrap(); @@ -1547,4 +1556,77 @@ mod test { // If signature S field exceeds the limit, it should return an error. Signature::check_malleability(&(s + U256::from(1u64))).unwrap_err(); } + + #[test] + fn test_tx_rlp_encoding_should_preserve_signature_v() { + let mut tx = Transaction { + gas_price: Some(U256::from(1u64)), + transaction_type: Some(0u64.into()), + v: 27u64.into(), + r: U256::from(1u64), + s: U256::from(1u64), + ..Default::default() + }; + + tx.hash = tx.slow_hash().0; + + let rlp = tx.rlp_encoded_2718(); + let tx_from_rlp = Transaction::from_rlp_2718(&mut rlp.as_ref()).unwrap(); + + assert_eq!(tx, tx_from_rlp); + } + + #[test] + fn test_tx_rlp_encoding_should_preserve_signature_v_for_eip1559() { + + // This TX is from testnet block 5,169,760 (probably already reverted). + // It contains a legacy transaction with a wrong signature V value due to unneeded normalization of the value. + let tx = r#" + { + "blockHash": "0xaf2e9d3584c10c1a82500911027fe64edb892d36c650d68dcd063293cc08e638", + "blockNumber": "0x4ee260", + "chainId": "0x56b29", + "from": "0xb0e5863d0ddf7e105e409fee0ecc0123a362e14b", + "gas": "0xb71b00", + "gasPrice": "0xe", + "hash": "0x647bef21f7b58209d202e92d719ad5670aee3fb9a7bc70ddc5245fd8889e2e11", + "input": "0x", + "nonce": "0xf9ce30", + "r": "0x1d811034f7f6940c4271b3a1b6c7d479ae23fd765e80e90c40c98901b0d6f48a", + "s": "0x2d04ad2494a29743e6c8b85825ea799d71f75ebf2cfcc72efa0315fe901a1568", + "to": "0x36c4054a98366caef1abb11469b9519e24b3cb78", + "transactionIndex": "0x0", + "type": "0x0", + "v": "0x1", + "value": "0xde0b6b3a7640000" + } + "#; + + let transaction: Transaction = serde_json::from_str(tx).unwrap(); + + assert_eq!(transaction.hash, transaction.slow_hash().0); + + + // from/to rlp + { + let (hash, rlp) = transaction.slow_hash(); + let tx_from_rlp = Transaction::from_rlp_2718(&mut rlp.as_ref()).unwrap(); + + // rlp decoded TX should have the hash set + assert_eq!(hash, tx_from_rlp.hash); + + let (re_hash, re_rlp) = tx_from_rlp.slow_hash(); + assert_eq!(hash, re_hash); + assert_eq!(rlp, re_rlp); + + assert_eq!(transaction.s, tx_from_rlp.s); + assert_eq!(transaction.r, tx_from_rlp.r); + + // This is the expected V value, it corresponds to: (chain_id * 2) + 35 + signature_y_parity + let expected_v = (355113u64 * 2) + 35 + 1; + + assert_eq!(expected_v, tx_from_rlp.v.as_u64()); + } + + } } From f4dd3d9b4c7fb1d55d1dae1011a67e39e9108705 Mon Sep 17 00:00:00 2001 From: Francesco Date: Wed, 18 Dec 2024 16:52:12 +0100 Subject: [PATCH 02/13] Rework signature to primitive_signature conv --- src/did/src/transaction.rs | 53 +++++++++++++---------------- src/eth-signer/src/sign_strategy.rs | 24 +++++++------ 2 files changed, 37 insertions(+), 40 deletions(-) diff --git a/src/did/src/transaction.rs b/src/did/src/transaction.rs index 8adef52c..38deb1f6 100644 --- a/src/did/src/transaction.rs +++ b/src/did/src/transaction.rs @@ -187,6 +187,7 @@ impl Signature { .map_err(|err| EvmError::SignatureError(format!("{err:?}")))?; Ok(recovered_from.into()) } + } impl TryFrom for alloy::primitives::PrimitiveSignature { @@ -209,16 +210,6 @@ impl TryFrom<&Signature> for alloy::primitives::PrimitiveSignature { } } -impl From for Signature { - fn from(value: alloy::primitives::PrimitiveSignature) -> Self { - Self { - v: U64::from(value.v() as u64), - r: value.r().into(), - s: value.s().into(), - } - } -} - impl Signature { /// Upper limit for signature S field. /// See comment to `Signature::check_malleability()` for more details. @@ -366,15 +357,12 @@ impl From for alloy::consensus::TxEnvelope { impl From for Transaction { fn from(tx: alloy::rpc::types::Transaction) -> Self { - - let signature = tx.inner.signature(); - let signature_v = signature.v(); - let signature: Signature = (*signature).into(); - + match tx.inner { alloy::consensus::TxEnvelope::Legacy(signed) => { let inner_tx = signed.tx(); - let signature_v = to_eip155_value(signature_v, inner_tx.chain_id()); + let signature = signed.signature(); + let chain_id = inner_tx.chain_id(); Self { hash: (*signed.hash()).into(), nonce: inner_tx.nonce.into(), @@ -391,14 +379,15 @@ impl From for Transaction { block_number: tx.block_number.map(Into::into), transaction_index: tx.transaction_index.map(Into::into), from: tx.from.into(), - v: (signature_v as u64).into(), - r: signature.r, - s: signature.s, + v: (to_eip155_value(signature.v(), chain_id) as u64).into(), + r: signature.r().into(), + s: signature.s().into(), transaction_type: Some(TRANSACTION_TYPE_LEGACY.into()), } } alloy::consensus::TxEnvelope::Eip2930(signed) => { let inner_tx = signed.tx(); + let signature = signed.signature(); Self { hash: (*signed.hash()).into(), nonce: inner_tx.nonce.into(), @@ -415,14 +404,15 @@ impl From for Transaction { block_number: tx.block_number.map(Into::into), transaction_index: tx.transaction_index.map(Into::into), from: tx.from.into(), - v: signature.v, - r: signature.r, - s: signature.s, + v: (signature.v() as u64).into(), + r: signature.r().into(), + s: signature.s().into(), transaction_type: Some(TRANSACTION_TYPE_EIP2930.into()), } } alloy::consensus::TxEnvelope::Eip1559(signed) => { let inner_tx = signed.tx(); + let signature = signed.signature(); Self { hash: (*signed.hash()).into(), nonce: inner_tx.nonce.into(), @@ -439,9 +429,9 @@ impl From for Transaction { block_number: tx.block_number.map(Into::into), transaction_index: tx.transaction_index.map(Into::into), from: tx.from.into(), - v: signature.v, - r: signature.r, - s: signature.s, + v: (signature.v() as u64).into(), + r: signature.r().into(), + s: signature.s().into(), transaction_type: Some(TRANSACTION_TYPE_EIP1559.into()), } } @@ -1541,11 +1531,14 @@ mod test { alloy::primitives::U256::from(random::()), random(), ); - let roundtrip_signature = Signature::from(signature); - assert_eq!( - signature, - alloy::primitives::PrimitiveSignature::try_from(roundtrip_signature).unwrap() - ); + + let TO_DO = 0; + + // let roundtrip_signature = Signature::from(signature); + // assert_eq!( + // signature, + // alloy::primitives::PrimitiveSignature::try_from(roundtrip_signature).unwrap() + // ); } #[test] diff --git a/src/eth-signer/src/sign_strategy.rs b/src/eth-signer/src/sign_strategy.rs index 844d7fc5..823e89a0 100644 --- a/src/eth-signer/src/sign_strategy.rs +++ b/src/eth-signer/src/sign_strategy.rs @@ -98,19 +98,23 @@ impl TxSigner { &self, transaction: &mut dyn SignableTransaction, ) -> TransactionSignerResult { - match self { - Self::Local(signer) => signer.sign_transaction(transaction).await.map(Into::into), - #[cfg(feature = "ic_sign")] - Self::ManagementCanister(signer) => signer.sign_transaction(transaction).await, - } + let TO_DO = 0; + panic!("not implemented"); + // match self { + // Self::Local(signer) => signer.sign_transaction(transaction).await.map(Into::into), + // #[cfg(feature = "ic_sign")] + // Self::ManagementCanister(signer) => signer.sign_transaction(transaction).await, + // } } pub async fn sign_digest(&self, digest: [u8; 32]) -> TransactionSignerResult { - match self { - Self::Local(signer) => signer.sign_digest(digest).await.map(Into::into), - #[cfg(feature = "ic_sign")] - Self::ManagementCanister(signer) => signer.sign_digest(digest).await, - } + let TO_DO = 0; + panic!("not implemented"); + // match self { + // Self::Local(signer) => signer.sign_digest(digest).await.map(Into::into), + // #[cfg(feature = "ic_sign")] + // Self::ManagementCanister(signer) => signer.sign_digest(digest).await, + // } } pub async fn get_public_key(&self) -> TransactionSignerResult> { From ff3b6a7d0029e0d08a090b711f4e5a0d24eb7cd4 Mon Sep 17 00:00:00 2001 From: Francesco Date: Thu, 19 Dec 2024 10:57:48 +0100 Subject: [PATCH 03/13] Complete rework of Signature --- src/did/src/transaction.rs | 261 +++++++++++++++++----- src/eth-signer/src/ic_sign.rs | 29 ++- src/eth-signer/src/sign_strategy.rs | 24 +- src/eth-signer/src/transaction.rs | 42 ++-- src/register-evm-agent/src/transaction.rs | 2 +- 5 files changed, 257 insertions(+), 101 deletions(-) diff --git a/src/did/src/transaction.rs b/src/did/src/transaction.rs index 38deb1f6..c8ae9b97 100644 --- a/src/did/src/transaction.rs +++ b/src/did/src/transaction.rs @@ -1,6 +1,7 @@ use std::borrow::Cow; use std::rc::Rc; use std::str::FromStr; +use std::sync::LazyLock; use alloy::consensus::transaction::to_eip155_value; use alloy::consensus::{ @@ -172,41 +173,81 @@ impl From for BlockId { /// ECDSA signature representation #[derive(Debug, Clone, PartialEq, Eq, CandidType, Serialize, Deserialize, Default)] pub struct Signature { - pub v: U64, + pub y_parity: bool, pub r: U256, pub s: U256, } +/// Transaction type and Chain id data required to generate the Signature V value for EIP-155 +pub enum TxChainInfo { + /// A legacy transation with (EIP-155) or without (not EIP-155) chain id + LegacyTx { + chain_id: Option, + }, + /// Other transaction types + OtherTx, +} + impl Signature { + + /// Creates a new signature from the given r, s and v values. + pub fn new_from_rsv(r: U256, s: U256, v: u64) -> Result { + let y_parity = normalize_v(v) + .ok_or_else(|| EvmError::InvalidSignatureParity(format!("{}", v)))?; + Ok(Self { + r, + s, + y_parity, + }) + } + /// Recovers an [`Address`] from this signature and the given prehashed message. /// e.g.: signature.recover_from(&tx.signature_hash()) pub fn recover_from(&self, signature_hash: &H256) -> Result { - let primitive_signature = alloy::primitives::PrimitiveSignature::try_from(self)?; + let primitive_signature = alloy::primitives::PrimitiveSignature::from(self); let recovered_from = primitive_signature .recover_address_from_prehash(&signature_hash.0) .map_err(|err| EvmError::SignatureError(format!("{err:?}")))?; Ok(recovered_from.into()) } + /// Calculates the V signature value from the signature and the [TxChainInfo]. + /// The V value is calculated as follows: + /// - For legacy transactions, the V value is calculated based on the EIP-155 + /// - For other transactions, the V value is the Y parity value + pub fn v(&self, info: TxChainInfo) -> u64 { + match info { + TxChainInfo::LegacyTx { chain_id } => { + to_eip155_value(self.y_parity, chain_id) as u64 + } + TxChainInfo::OtherTx => self.y_parity as u64, + } + } } -impl TryFrom for alloy::primitives::PrimitiveSignature { - type Error = EvmError; - - fn try_from(value: Signature) -> Result { - Self::try_from(&value) +impl From for alloy::primitives::PrimitiveSignature { + fn from(value: Signature) -> Self { + alloy::primitives::PrimitiveSignature::new( + value.r.0, value.s.0, value.y_parity, + ) } } -impl TryFrom<&Signature> for alloy::primitives::PrimitiveSignature { - type Error = EvmError; +impl From<&Signature> for alloy::primitives::PrimitiveSignature { + fn from(value: &Signature) -> Self { + alloy::primitives::PrimitiveSignature::new( + value.r.0, value.s.0, value.y_parity, + ) + } +} - fn try_from(value: &Signature) -> Result { - let parity = normalize_v(value.v.0.to()) - .ok_or_else(|| EvmError::InvalidSignatureParity(format!("{}", value.v)))?; - Ok(alloy::primitives::PrimitiveSignature::new( - value.r.0, value.s.0, parity, - )) +impl From for Signature { + fn from(value: alloy::primitives::PrimitiveSignature) -> Self { + Self { + r: U256(value.r()), + s: U256(value.s()), + y_parity: value.v(), + } } } @@ -216,6 +257,12 @@ impl Signature { pub const S_UPPER_LIMIT_HEX_STR: &'static str = "0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0"; + /// Upper limit for signature S field. + /// See comment to `Signature::check_malleability()` for more details. + pub const S_UPPER_LIMIT: LazyLock = LazyLock::new(|| { + U256::from_hex_str(Self::S_UPPER_LIMIT_HEX_STR).expect("Invalid S upper limit") + }); + /// This comment copied from OpenZeppelin `ECDSA::tryRecover()` function. /// /// EIP-2 still allows signature malleability for ecrecover(). Remove this possibility and make the signature @@ -228,13 +275,11 @@ impl Signature { /// vice versa. If your library also generates signatures with 0/1 for v instead 27/28, add 27 to v to accept /// these malleable signatures as well. pub fn check_malleability(s: &U256) -> Result<(), EvmError> { - let upper_limit = U256::from_hex_str(Self::S_UPPER_LIMIT_HEX_STR)?; - if s > &upper_limit { + if s > &Self::S_UPPER_LIMIT { return Err(EvmError::TransactionSignature(format!( - "S value in transaction signature should not exceed {upper_limit}" + "S value in transaction signature should not exceed {}", Self::S_UPPER_LIMIT_HEX_STR ))); } - Ok(()) } } @@ -348,10 +393,11 @@ impl From for Transaction { } } -impl From for alloy::consensus::TxEnvelope { - fn from(value: Transaction) -> Self { - let tx: alloy::rpc::types::Transaction = value.into(); - tx.inner +impl TryFrom for alloy::consensus::TxEnvelope { + type Error = EvmError; + fn try_from(value: Transaction) -> Result { + let tx = alloy::rpc::types::Transaction::try_from(value)?; + Ok(tx.inner) } } @@ -443,18 +489,18 @@ impl From for Transaction { } } -impl From for alloy::rpc::types::Transaction { - fn from(tx: Transaction) -> Self { - let signature = Signature { - v: tx.v, - r: tx.r, - s: tx.s, - }; - let signature = alloy::primitives::PrimitiveSignature::try_from(signature).unwrap(); +impl TryFrom for alloy::rpc::types::Transaction { + + type Error = EvmError; + + fn try_from(tx: Transaction) -> Result { + let signature = Signature::new_from_rsv(tx.r, tx.s, tx.v.as_u64())?; + + let signature = alloy::primitives::PrimitiveSignature::from(signature); let tx_type = tx.transaction_type.unwrap_or_default().0.to::(); match tx_type { - TRANSACTION_TYPE_LEGACY => alloy::rpc::types::Transaction { + TRANSACTION_TYPE_LEGACY => Ok(alloy::rpc::types::Transaction { inner: TxLegacy { nonce: tx.nonce.0.to(), gas_price: tx.gas_price.map(|v| v.0.to()).unwrap_or_default(), @@ -471,8 +517,8 @@ impl From for alloy::rpc::types::Transaction { transaction_index: tx.transaction_index.map(Into::into), effective_gas_price: None, from: tx.from.into(), - }, - TRANSACTION_TYPE_EIP2930 => alloy::rpc::types::Transaction { + }), + TRANSACTION_TYPE_EIP2930 => Ok(alloy::rpc::types::Transaction { inner: TxEip2930 { nonce: tx.nonce.0.to(), gas_price: tx.gas_price.map(|v| v.0.to()).unwrap_or_default(), @@ -490,8 +536,8 @@ impl From for alloy::rpc::types::Transaction { transaction_index: tx.transaction_index.map(Into::into), effective_gas_price: None, from: tx.from.into(), - }, - TRANSACTION_TYPE_EIP1559 => alloy::rpc::types::Transaction { + }), + TRANSACTION_TYPE_EIP1559 => Ok(alloy::rpc::types::Transaction { inner: TxEip1559 { nonce: tx.nonce.0.to(), gas_limit: tx.gas.0.to(), @@ -513,31 +559,32 @@ impl From for alloy::rpc::types::Transaction { transaction_index: tx.transaction_index.map(Into::into), effective_gas_price: None, from: tx.from.into(), - }, + }), _ => { panic!("Unsupported transaction type: {}", tx_type); } } } + } impl Transaction { /// RLP encodes the transaction and recalculates the hash. /// It does not modify the transaction itself. /// It returns the calcualted hash and the RLP encoded bytes. - pub fn slow_hash(&self) -> (H256, bytes::Bytes) { - let encoded = self.rlp_encoded_2718(); - (keccak_hash(&encoded), encoded) + pub fn slow_hash(&self) -> Result<(H256, bytes::Bytes), EvmError> { + let encoded = self.rlp_encoded_2718()?; + Ok((keccak_hash(&encoded), encoded)) } /// Encode the transaction according to [EIP-2718] rules. First a 1-byte /// type flag in the range 0x0-0x7f, then the body of the transaction. - pub fn rlp_encoded_2718(&self) -> bytes::Bytes { + pub fn rlp_encoded_2718(&self) -> Result { use alloy::eips::eip2718::Encodable2718; - let alloy_transaction: alloy::consensus::TxEnvelope = self.clone().into(); + let alloy_transaction = alloy::consensus::TxEnvelope::try_from(self.clone())?; let mut buffer = BytesMut::new(); alloy_transaction.encode_2718(&mut buffer); - buffer.freeze() + Ok(buffer.freeze()) } /// Decode the transaction according to [EIP-2718] rules. @@ -1331,18 +1378,18 @@ mod test { assert_eq!( alloy::primitives::B256::from_str(&hash).unwrap(), - transaction.slow_hash().0 .0 + transaction.slow_hash().unwrap().0 .0 ); // from/to rlp { - let (hash, rlp) = transaction.slow_hash(); + let (hash, rlp) = transaction.slow_hash().unwrap(); let tx_from_rlp = Transaction::from_rlp_2718(&mut rlp.as_ref()).unwrap(); // rlp decoded TX should have the hash set assert_eq!(hash, tx_from_rlp.hash); - let (re_hash, re_rlp) = tx_from_rlp.slow_hash(); + let (re_hash, re_rlp) = tx_from_rlp.slow_hash().unwrap(); assert_eq!(hash, re_hash); assert_eq!(rlp, re_rlp); @@ -1532,13 +1579,11 @@ mod test { random(), ); - let TO_DO = 0; - - // let roundtrip_signature = Signature::from(signature); - // assert_eq!( - // signature, - // alloy::primitives::PrimitiveSignature::try_from(roundtrip_signature).unwrap() - // ); + let roundtrip_signature = Signature::from(signature); + assert_eq!( + signature, + alloy::primitives::PrimitiveSignature::try_from(roundtrip_signature).unwrap() + ); } #[test] @@ -1561,9 +1606,9 @@ mod test { ..Default::default() }; - tx.hash = tx.slow_hash().0; + tx.hash = tx.slow_hash().unwrap().0; - let rlp = tx.rlp_encoded_2718(); + let rlp = tx.rlp_encoded_2718().unwrap(); let tx_from_rlp = Transaction::from_rlp_2718(&mut rlp.as_ref()).unwrap(); assert_eq!(tx, tx_from_rlp); @@ -1597,18 +1642,18 @@ mod test { let transaction: Transaction = serde_json::from_str(tx).unwrap(); - assert_eq!(transaction.hash, transaction.slow_hash().0); + assert_eq!(transaction.hash, transaction.slow_hash().unwrap().0); // from/to rlp { - let (hash, rlp) = transaction.slow_hash(); + let (hash, rlp) = transaction.slow_hash().unwrap(); let tx_from_rlp = Transaction::from_rlp_2718(&mut rlp.as_ref()).unwrap(); // rlp decoded TX should have the hash set assert_eq!(hash, tx_from_rlp.hash); - let (re_hash, re_rlp) = tx_from_rlp.slow_hash(); + let (re_hash, re_rlp) = tx_from_rlp.slow_hash().unwrap(); assert_eq!(hash, re_hash); assert_eq!(rlp, re_rlp); @@ -1622,4 +1667,104 @@ mod test { } } + + #[test] + pub fn test_signature_v_for_legacy_eip_155_transaction_with_y_parity_true() { + // Arrange + let signature = Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64.into()).unwrap(); + assert_eq!(signature.r, 100u64.into()); + assert_eq!(signature.s, 200u64.into()); + assert_eq!(signature.y_parity, true); + + let chain_id = random::() as u64; + + // Act + let v = signature.v(TxChainInfo::LegacyTx{ chain_id: Some(chain_id)}); + + // Assert + let expected_v = 35 + (chain_id * 2) + (signature.y_parity as u64); + assert_eq!(v, expected_v); + } + + #[test] + pub fn test_signature_v_for_legacy_eip_155_transaction_with_y_parity_false() { + // Arrange + let signature = Signature::new_from_rsv(300u64.into(), 500u64.into(), 111u64.into()).unwrap(); + assert_eq!(signature.r, 300u64.into()); + assert_eq!(signature.s, 500u64.into()); + assert_eq!(signature.y_parity, false); + + let chain_id = random::() as u64; + + // Act + let v = signature.v(TxChainInfo::LegacyTx{ chain_id: Some(chain_id)}); + + // Assert + let expected_v = 35 + (chain_id * 2) + (signature.y_parity as u64); + assert_eq!(v, expected_v); + } + + #[test] + pub fn test_signature_v_for_legacy_not_eip_155_transaction_with_y_parity_true() { + // Arrange + let signature = Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64.into()).unwrap(); + assert_eq!(signature.r, 100u64.into()); + assert_eq!(signature.s, 200u64.into()); + assert_eq!(signature.y_parity, true); + + // Act + let v = signature.v(TxChainInfo::LegacyTx{ chain_id: None}); + + // Assert + let expected_v = 28; + assert_eq!(v, expected_v); + } + + #[test] + pub fn test_signature_v_for_legacy_not_eip_155_transaction_with_y_parity_false() { + // Arrange + let signature = Signature::new_from_rsv(1000u64.into(), 2000u64.into(), 101u64.into()).unwrap(); + assert_eq!(signature.r, 1000u64.into()); + assert_eq!(signature.s, 2000u64.into()); + assert_eq!(signature.y_parity, false); + + // Act + let v = signature.v(TxChainInfo::LegacyTx{ chain_id: None}); + + // Assert + let expected_v = 27; + assert_eq!(v, expected_v); + } + + #[test] + pub fn test_signature_v_for_non_legacy_transaction_with_y_parity_true() { + // Arrange + let signature = Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64.into()).unwrap(); + assert_eq!(signature.r, 100u64.into()); + assert_eq!(signature.s, 200u64.into()); + assert_eq!(signature.y_parity, true); + + // Act + let v = signature.v(TxChainInfo::OtherTx); + + // Assert + let expected_v = 1; + assert_eq!(v, expected_v); + } + + #[test] + pub fn test_signature_v_for_non_legacy_transaction_with_y_parity_false() { + // Arrange + let signature = Signature::new_from_rsv(1000u64.into(), 2000u64.into(), 101u64.into()).unwrap(); + assert_eq!(signature.r, 1000u64.into()); + assert_eq!(signature.s, 2000u64.into()); + assert_eq!(signature.y_parity, false); + + // Act + let v = signature.v(TxChainInfo::OtherTx); + + // Assert + let expected_v = 0; + assert_eq!(v, expected_v); + } } diff --git a/src/eth-signer/src/ic_sign.rs b/src/eth-signer/src/ic_sign.rs index ef2f061e..3cd69cdc 100644 --- a/src/eth-signer/src/ic_sign.rs +++ b/src/eth-signer/src/ic_sign.rs @@ -115,20 +115,25 @@ impl IcSigner { derivation_path: DerivationPath, ) -> Result { let hash = tx.signature_hash(); - let mut signature = Self + + Self .sign_digest(*hash, pubkey, key_id, derivation_path) - .await?; + .await - let v: u64 = signature.v.0.to(); + // let mut signature = Self + // .sign_digest(*hash, pubkey, key_id, derivation_path) + // .await?; - // For non-legacy transactions recovery id should be updated. - // Details: https://eips.ethereum.org/EIPS/eip-155. - signature.v = match tx.chain_id() { - Some(chain_id) => (chain_id * 2 + 35 + (v - 27)).into(), - None => v.into(), - }; + // let v: u64 = signature.v.0.to(); - Ok(signature) + // // For non-legacy transactions recovery id should be updated. + // // Details: https://eips.ethereum.org/EIPS/eip-155. + // signature.v = match tx.chain_id() { + // Some(chain_id) => (chain_id * 2 + 35 + (v - 27)).into(), + // None => v.into(), + // }; + + // Ok(signature) } /// Signs the digest using `ManagementCanister::sign_with_ecdsa()` call. @@ -166,7 +171,7 @@ impl IcSigner { let r = U256::from_be_slice(&signature_data[0..32]); let s = U256::from_be_slice(&signature_data[32..64]); - let v = recovery_id.is_y_odd() as u64 + 27; + let y_parity = recovery_id.is_y_odd(); // Signature malleability check is not required, because dfinity uses `k256` crate // as `ecdsa_secp256k1` implementation, and it takes care about signature malleability. @@ -174,7 +179,7 @@ impl IcSigner { let signature = DidSignature { r: r.into(), s: s.into(), - v: v.into(), + y_parity, }; Ok(signature) diff --git a/src/eth-signer/src/sign_strategy.rs b/src/eth-signer/src/sign_strategy.rs index 823e89a0..844d7fc5 100644 --- a/src/eth-signer/src/sign_strategy.rs +++ b/src/eth-signer/src/sign_strategy.rs @@ -98,23 +98,19 @@ impl TxSigner { &self, transaction: &mut dyn SignableTransaction, ) -> TransactionSignerResult { - let TO_DO = 0; - panic!("not implemented"); - // match self { - // Self::Local(signer) => signer.sign_transaction(transaction).await.map(Into::into), - // #[cfg(feature = "ic_sign")] - // Self::ManagementCanister(signer) => signer.sign_transaction(transaction).await, - // } + match self { + Self::Local(signer) => signer.sign_transaction(transaction).await.map(Into::into), + #[cfg(feature = "ic_sign")] + Self::ManagementCanister(signer) => signer.sign_transaction(transaction).await, + } } pub async fn sign_digest(&self, digest: [u8; 32]) -> TransactionSignerResult { - let TO_DO = 0; - panic!("not implemented"); - // match self { - // Self::Local(signer) => signer.sign_digest(digest).await.map(Into::into), - // #[cfg(feature = "ic_sign")] - // Self::ManagementCanister(signer) => signer.sign_digest(digest).await, - // } + match self { + Self::Local(signer) => signer.sign_digest(digest).await.map(Into::into), + #[cfg(feature = "ic_sign")] + Self::ManagementCanister(signer) => signer.sign_digest(digest).await, + } } pub async fn get_public_key(&self) -> TransactionSignerResult> { diff --git a/src/eth-signer/src/transaction.rs b/src/eth-signer/src/transaction.rs index 5e7da55c..614a7ccb 100644 --- a/src/eth-signer/src/transaction.rs +++ b/src/eth-signer/src/transaction.rs @@ -52,8 +52,8 @@ impl TransactionBuilder<'_, '_> { }; let alloy_signature = match self.signature { - SigningMethod::None => DidSignature::default().try_into()?, - SigningMethod::Signature(signature) => signature.try_into()?, + SigningMethod::None => DidSignature::default().into(), + SigningMethod::Signature(signature) => signature.into(), SigningMethod::SigningKey(key) => { let wallet = LocalWallet::new_with_credential( (*key).clone(), @@ -91,7 +91,6 @@ mod test { use alloy::consensus::TxEnvelope; use alloy::signers::k256::ecdsa::signature::hazmat::PrehashVerifier; use alloy::signers::utils::secret_key_to_address; - use did::U64; use super::*; @@ -110,7 +109,9 @@ mod test { }; let tx = transaction_builder.calculate_hash_and_build().unwrap(); - assert_eq!(tx.v, U64::zero()); + // eip-155 legacy TX + let expected_v = 31540u64 * 2 + 35; + assert_eq!(tx.v.as_u64(), expected_v); assert_eq!(tx.r, U256::zero()); assert_eq!(tx.s, U256::zero()); assert_eq!(tx.chain_id, Some(31540u64.into())); @@ -126,16 +127,14 @@ mod test { gas: 10_000u64.into(), gas_price: 20_000u64.into(), input: Vec::new(), - signature: SigningMethod::Signature(DidSignature { - r: 1u64.into(), - s: 2u64.into(), - v: 0u64.into(), - }), + signature: SigningMethod::Signature(DidSignature::new_from_rsv( 1u64.into(), 2u64.into(), 1u64.into()).unwrap()), chain_id: 31541, }; let tx = transaction_builder.calculate_hash_and_build().unwrap(); - assert_eq!(tx.v, U64::from(0u64)); + // eip-155 legacy TX + let expected_v = 31541u64 * 2 + 35 + 1; + assert_eq!(tx.v.as_u64(), expected_v); assert_eq!(tx.r, U256::from(1u64)); assert_eq!(tx.s, U256::from(2u64)); assert_eq!(tx.chain_id, Some(31541u64.into())); @@ -145,7 +144,7 @@ mod test { fn test_build_transaction_with_signing_key() { let key = SigningKey::random(&mut rand::thread_rng()); let from = secret_key_to_address(&key); - let chain_id = 31540; + let chain_id = 31550; let transaction_builder = TransactionBuilder { from: &from.into(), to: None, @@ -162,7 +161,7 @@ mod test { assert_eq!(tx.chain_id, Some(chain_id.into())); - let typed_tx: alloy::rpc::types::Transaction = tx.clone().into(); + let typed_tx = alloy::rpc::types::Transaction::try_from(tx.clone()).unwrap(); let signature_hash = typed_tx.inner.signature_hash(); let signature = typed_tx.inner.signature(); @@ -183,6 +182,17 @@ mod test { .verify_prehash(signature_hash.as_slice(), &signature.to_k256().unwrap()) .is_ok()); } + + // verify eip-155 signature + { + let signature = DidSignature::from(*signature); + let expected_v = signature.v(did::transaction::TxChainInfo::LegacyTx { chain_id: Some(chain_id) }); + assert_eq!(tx.v.as_u64(), expected_v); + assert_eq!(tx.r, signature.r); + assert_eq!(tx.s, signature.s); + assert_eq!(tx.chain_id, Some(chain_id.into())); + } + } #[test] @@ -234,9 +244,9 @@ mod test { chain_id, }; - let tx = alloy::rpc::types::Transaction::from( + let tx = alloy::rpc::types::Transaction::try_from( transaction_builder.calculate_hash_and_build().unwrap(), - ); + ).unwrap(); let recovered_from = tx .inner @@ -267,7 +277,7 @@ mod test { .calculate_hash_and_build() .unwrap(); - let tx_1_envelop: TxEnvelope = tx_1.clone().into(); + let tx_1_envelop = TxEnvelope::try_from(tx_1.clone()).unwrap(); let recovered_tx_1_from = tx_1_envelop.recover_signer().unwrap(); // Replaying the same transaction with different chain_id @@ -276,7 +286,7 @@ mod test { ..tx_1 }; - let tx_2_envelop: TxEnvelope = tx_2.into(); + let tx_2_envelop = TxEnvelope::try_from(tx_2.clone()).unwrap(); let recovered_tx_2_from = tx_2_envelop.recover_signer().unwrap(); assert_ne!(recovered_tx_1_from, recovered_tx_2_from); diff --git a/src/register-evm-agent/src/transaction.rs b/src/register-evm-agent/src/transaction.rs index bc73fa9f..bac7a690 100644 --- a/src/register-evm-agent/src/transaction.rs +++ b/src/register-evm-agent/src/transaction.rs @@ -62,7 +62,7 @@ impl SignTransactionArgs { let tx = self.transaction_builder(wallet, &client).await?; - let tx_bytes = alloy::rlp::encode(&alloy::rpc::types::Transaction::from(tx.clone()).inner); + let tx_bytes = alloy::rlp::encode(&alloy::rpc::types::Transaction::try_from(tx.clone())?.inner); println!("Transaction: {:#?}", tx); println!("Transaction Bytes: {:#?}", tx_bytes); From d8f5ec54c27b037cebfb0ca3066142a1d71476ca Mon Sep 17 00:00:00 2001 From: Francesco Date: Thu, 19 Dec 2024 10:58:16 +0100 Subject: [PATCH 04/13] fmt --- src/did/src/transaction.rs | 108 ++++++++++------------ src/eth-signer/src/ic_sign.rs | 3 +- src/eth-signer/src/transaction.rs | 12 ++- src/register-evm-agent/src/transaction.rs | 3 +- 4 files changed, 61 insertions(+), 65 deletions(-) diff --git a/src/did/src/transaction.rs b/src/did/src/transaction.rs index c8ae9b97..97e29e6b 100644 --- a/src/did/src/transaction.rs +++ b/src/did/src/transaction.rs @@ -180,25 +180,18 @@ pub struct Signature { /// Transaction type and Chain id data required to generate the Signature V value for EIP-155 pub enum TxChainInfo { - /// A legacy transation with (EIP-155) or without (not EIP-155) chain id - LegacyTx { - chain_id: Option, - }, + /// A legacy transation with (EIP-155) or without (not EIP-155) chain id + LegacyTx { chain_id: Option }, /// Other transaction types OtherTx, } impl Signature { - /// Creates a new signature from the given r, s and v values. pub fn new_from_rsv(r: U256, s: U256, v: u64) -> Result { - let y_parity = normalize_v(v) - .ok_or_else(|| EvmError::InvalidSignatureParity(format!("{}", v)))?; - Ok(Self { - r, - s, - y_parity, - }) + let y_parity = + normalize_v(v).ok_or_else(|| EvmError::InvalidSignatureParity(format!("{}", v)))?; + Ok(Self { r, s, y_parity }) } /// Recovers an [`Address`] from this signature and the given prehashed message. @@ -210,16 +203,14 @@ impl Signature { .map_err(|err| EvmError::SignatureError(format!("{err:?}")))?; Ok(recovered_from.into()) } - + /// Calculates the V signature value from the signature and the [TxChainInfo]. /// The V value is calculated as follows: - /// - For legacy transactions, the V value is calculated based on the EIP-155 + /// - For legacy transactions, the V value is calculated based on the EIP-155 /// - For other transactions, the V value is the Y parity value pub fn v(&self, info: TxChainInfo) -> u64 { match info { - TxChainInfo::LegacyTx { chain_id } => { - to_eip155_value(self.y_parity, chain_id) as u64 - } + TxChainInfo::LegacyTx { chain_id } => to_eip155_value(self.y_parity, chain_id) as u64, TxChainInfo::OtherTx => self.y_parity as u64, } } @@ -227,17 +218,13 @@ impl Signature { impl From for alloy::primitives::PrimitiveSignature { fn from(value: Signature) -> Self { - alloy::primitives::PrimitiveSignature::new( - value.r.0, value.s.0, value.y_parity, - ) + alloy::primitives::PrimitiveSignature::new(value.r.0, value.s.0, value.y_parity) } } impl From<&Signature> for alloy::primitives::PrimitiveSignature { fn from(value: &Signature) -> Self { - alloy::primitives::PrimitiveSignature::new( - value.r.0, value.s.0, value.y_parity, - ) + alloy::primitives::PrimitiveSignature::new(value.r.0, value.s.0, value.y_parity) } } @@ -277,7 +264,8 @@ impl Signature { pub fn check_malleability(s: &U256) -> Result<(), EvmError> { if s > &Self::S_UPPER_LIMIT { return Err(EvmError::TransactionSignature(format!( - "S value in transaction signature should not exceed {}", Self::S_UPPER_LIMIT_HEX_STR + "S value in transaction signature should not exceed {}", + Self::S_UPPER_LIMIT_HEX_STR ))); } Ok(()) @@ -403,7 +391,6 @@ impl TryFrom for alloy::consensus::TxEnvelope { impl From for Transaction { fn from(tx: alloy::rpc::types::Transaction) -> Self { - match tx.inner { alloy::consensus::TxEnvelope::Legacy(signed) => { let inner_tx = signed.tx(); @@ -490,9 +477,8 @@ impl From for Transaction { } impl TryFrom for alloy::rpc::types::Transaction { - type Error = EvmError; - + fn try_from(tx: Transaction) -> Result { let signature = Signature::new_from_rsv(tx.r, tx.s, tx.v.as_u64())?; @@ -565,7 +551,6 @@ impl TryFrom for alloy::rpc::types::Transaction { } } } - } impl Transaction { @@ -1616,7 +1601,6 @@ mod test { #[test] fn test_tx_rlp_encoding_should_preserve_signature_v_for_eip1559() { - // This TX is from testnet block 5,169,760 (probably already reverted). // It contains a legacy transaction with a wrong signature V value due to unneeded normalization of the value. let tx = r#" @@ -1644,42 +1628,43 @@ mod test { assert_eq!(transaction.hash, transaction.slow_hash().unwrap().0); + // from/to rlp + { + let (hash, rlp) = transaction.slow_hash().unwrap(); + let tx_from_rlp = Transaction::from_rlp_2718(&mut rlp.as_ref()).unwrap(); - // from/to rlp - { - let (hash, rlp) = transaction.slow_hash().unwrap(); - let tx_from_rlp = Transaction::from_rlp_2718(&mut rlp.as_ref()).unwrap(); - - // rlp decoded TX should have the hash set - assert_eq!(hash, tx_from_rlp.hash); + // rlp decoded TX should have the hash set + assert_eq!(hash, tx_from_rlp.hash); - let (re_hash, re_rlp) = tx_from_rlp.slow_hash().unwrap(); - assert_eq!(hash, re_hash); - assert_eq!(rlp, re_rlp); + let (re_hash, re_rlp) = tx_from_rlp.slow_hash().unwrap(); + assert_eq!(hash, re_hash); + assert_eq!(rlp, re_rlp); - assert_eq!(transaction.s, tx_from_rlp.s); - assert_eq!(transaction.r, tx_from_rlp.r); - - // This is the expected V value, it corresponds to: (chain_id * 2) + 35 + signature_y_parity - let expected_v = (355113u64 * 2) + 35 + 1; + assert_eq!(transaction.s, tx_from_rlp.s); + assert_eq!(transaction.r, tx_from_rlp.r); - assert_eq!(expected_v, tx_from_rlp.v.as_u64()); - } + // This is the expected V value, it corresponds to: (chain_id * 2) + 35 + signature_y_parity + let expected_v = (355113u64 * 2) + 35 + 1; + assert_eq!(expected_v, tx_from_rlp.v.as_u64()); + } } #[test] pub fn test_signature_v_for_legacy_eip_155_transaction_with_y_parity_true() { // Arrange - let signature = Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64.into()).unwrap(); + let signature = + Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64.into()).unwrap(); assert_eq!(signature.r, 100u64.into()); assert_eq!(signature.s, 200u64.into()); assert_eq!(signature.y_parity, true); - + let chain_id = random::() as u64; // Act - let v = signature.v(TxChainInfo::LegacyTx{ chain_id: Some(chain_id)}); + let v = signature.v(TxChainInfo::LegacyTx { + chain_id: Some(chain_id), + }); // Assert let expected_v = 35 + (chain_id * 2) + (signature.y_parity as u64); @@ -1689,15 +1674,18 @@ mod test { #[test] pub fn test_signature_v_for_legacy_eip_155_transaction_with_y_parity_false() { // Arrange - let signature = Signature::new_from_rsv(300u64.into(), 500u64.into(), 111u64.into()).unwrap(); + let signature = + Signature::new_from_rsv(300u64.into(), 500u64.into(), 111u64.into()).unwrap(); assert_eq!(signature.r, 300u64.into()); assert_eq!(signature.s, 500u64.into()); assert_eq!(signature.y_parity, false); - + let chain_id = random::() as u64; // Act - let v = signature.v(TxChainInfo::LegacyTx{ chain_id: Some(chain_id)}); + let v = signature.v(TxChainInfo::LegacyTx { + chain_id: Some(chain_id), + }); // Assert let expected_v = 35 + (chain_id * 2) + (signature.y_parity as u64); @@ -1707,13 +1695,14 @@ mod test { #[test] pub fn test_signature_v_for_legacy_not_eip_155_transaction_with_y_parity_true() { // Arrange - let signature = Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64.into()).unwrap(); + let signature = + Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64.into()).unwrap(); assert_eq!(signature.r, 100u64.into()); assert_eq!(signature.s, 200u64.into()); assert_eq!(signature.y_parity, true); // Act - let v = signature.v(TxChainInfo::LegacyTx{ chain_id: None}); + let v = signature.v(TxChainInfo::LegacyTx { chain_id: None }); // Assert let expected_v = 28; @@ -1723,13 +1712,14 @@ mod test { #[test] pub fn test_signature_v_for_legacy_not_eip_155_transaction_with_y_parity_false() { // Arrange - let signature = Signature::new_from_rsv(1000u64.into(), 2000u64.into(), 101u64.into()).unwrap(); + let signature = + Signature::new_from_rsv(1000u64.into(), 2000u64.into(), 101u64.into()).unwrap(); assert_eq!(signature.r, 1000u64.into()); assert_eq!(signature.s, 2000u64.into()); assert_eq!(signature.y_parity, false); // Act - let v = signature.v(TxChainInfo::LegacyTx{ chain_id: None}); + let v = signature.v(TxChainInfo::LegacyTx { chain_id: None }); // Assert let expected_v = 27; @@ -1739,7 +1729,8 @@ mod test { #[test] pub fn test_signature_v_for_non_legacy_transaction_with_y_parity_true() { // Arrange - let signature = Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64.into()).unwrap(); + let signature = + Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64.into()).unwrap(); assert_eq!(signature.r, 100u64.into()); assert_eq!(signature.s, 200u64.into()); assert_eq!(signature.y_parity, true); @@ -1755,7 +1746,8 @@ mod test { #[test] pub fn test_signature_v_for_non_legacy_transaction_with_y_parity_false() { // Arrange - let signature = Signature::new_from_rsv(1000u64.into(), 2000u64.into(), 101u64.into()).unwrap(); + let signature = + Signature::new_from_rsv(1000u64.into(), 2000u64.into(), 101u64.into()).unwrap(); assert_eq!(signature.r, 1000u64.into()); assert_eq!(signature.s, 2000u64.into()); assert_eq!(signature.y_parity, false); diff --git a/src/eth-signer/src/ic_sign.rs b/src/eth-signer/src/ic_sign.rs index 3cd69cdc..a86cd6ca 100644 --- a/src/eth-signer/src/ic_sign.rs +++ b/src/eth-signer/src/ic_sign.rs @@ -116,8 +116,7 @@ impl IcSigner { ) -> Result { let hash = tx.signature_hash(); - Self - .sign_digest(*hash, pubkey, key_id, derivation_path) + Self.sign_digest(*hash, pubkey, key_id, derivation_path) .await // let mut signature = Self diff --git a/src/eth-signer/src/transaction.rs b/src/eth-signer/src/transaction.rs index 614a7ccb..8bbd467b 100644 --- a/src/eth-signer/src/transaction.rs +++ b/src/eth-signer/src/transaction.rs @@ -127,7 +127,9 @@ mod test { gas: 10_000u64.into(), gas_price: 20_000u64.into(), input: Vec::new(), - signature: SigningMethod::Signature(DidSignature::new_from_rsv( 1u64.into(), 2u64.into(), 1u64.into()).unwrap()), + signature: SigningMethod::Signature( + DidSignature::new_from_rsv(1u64.into(), 2u64.into(), 1u64.into()).unwrap(), + ), chain_id: 31541, }; let tx = transaction_builder.calculate_hash_and_build().unwrap(); @@ -186,13 +188,14 @@ mod test { // verify eip-155 signature { let signature = DidSignature::from(*signature); - let expected_v = signature.v(did::transaction::TxChainInfo::LegacyTx { chain_id: Some(chain_id) }); + let expected_v = signature.v(did::transaction::TxChainInfo::LegacyTx { + chain_id: Some(chain_id), + }); assert_eq!(tx.v.as_u64(), expected_v); assert_eq!(tx.r, signature.r); assert_eq!(tx.s, signature.s); assert_eq!(tx.chain_id, Some(chain_id.into())); } - } #[test] @@ -246,7 +249,8 @@ mod test { let tx = alloy::rpc::types::Transaction::try_from( transaction_builder.calculate_hash_and_build().unwrap(), - ).unwrap(); + ) + .unwrap(); let recovered_from = tx .inner diff --git a/src/register-evm-agent/src/transaction.rs b/src/register-evm-agent/src/transaction.rs index bac7a690..2c520764 100644 --- a/src/register-evm-agent/src/transaction.rs +++ b/src/register-evm-agent/src/transaction.rs @@ -62,7 +62,8 @@ impl SignTransactionArgs { let tx = self.transaction_builder(wallet, &client).await?; - let tx_bytes = alloy::rlp::encode(&alloy::rpc::types::Transaction::try_from(tx.clone())?.inner); + let tx_bytes = + alloy::rlp::encode(&alloy::rpc::types::Transaction::try_from(tx.clone())?.inner); println!("Transaction: {:#?}", tx); println!("Transaction Bytes: {:#?}", tx_bytes); From 009589ff668035944baf9c5fb1ed8c0e7abeb031 Mon Sep 17 00:00:00 2001 From: Francesco Date: Thu, 19 Dec 2024 11:00:33 +0100 Subject: [PATCH 05/13] clippy --- src/did/src/transaction.rs | 52 +++++++++---------- src/eth-signer/src/ic_sign.rs | 2 +- src/eth-signer/src/sign_strategy.rs | 3 +- src/eth-signer/src/transaction.rs | 2 +- .../tests/test_canister/src/canister.rs | 2 +- 5 files changed, 30 insertions(+), 31 deletions(-) diff --git a/src/did/src/transaction.rs b/src/did/src/transaction.rs index 97e29e6b..5a686c1b 100644 --- a/src/did/src/transaction.rs +++ b/src/did/src/transaction.rs @@ -238,18 +238,18 @@ impl From for Signature { } } -impl Signature { - /// Upper limit for signature S field. - /// See comment to `Signature::check_malleability()` for more details. - pub const S_UPPER_LIMIT_HEX_STR: &'static str = - "0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0"; +/// Upper limit for signature S field. +/// See comment to `Signature::check_malleability()` for more details. +pub const SIGNATURE_S_UPPER_LIMIT_HEX_STR: &'static str = + "0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0"; - /// Upper limit for signature S field. - /// See comment to `Signature::check_malleability()` for more details. - pub const S_UPPER_LIMIT: LazyLock = LazyLock::new(|| { - U256::from_hex_str(Self::S_UPPER_LIMIT_HEX_STR).expect("Invalid S upper limit") - }); +/// Upper limit for signature S field. +/// See comment to `Signature::check_malleability()` for more details. +pub static SIGNATURE_S_UPPER_LIMIT: LazyLock = LazyLock::new(|| { + U256::from_hex_str(SIGNATURE_S_UPPER_LIMIT_HEX_STR).expect("Invalid S upper limit") +}); +impl Signature { /// This comment copied from OpenZeppelin `ECDSA::tryRecover()` function. /// /// EIP-2 still allows signature malleability for ecrecover(). Remove this possibility and make the signature @@ -262,10 +262,10 @@ impl Signature { /// vice versa. If your library also generates signatures with 0/1 for v instead 27/28, add 27 to v to accept /// these malleable signatures as well. pub fn check_malleability(s: &U256) -> Result<(), EvmError> { - if s > &Self::S_UPPER_LIMIT { + if s > &SIGNATURE_S_UPPER_LIMIT { return Err(EvmError::TransactionSignature(format!( "S value in transaction signature should not exceed {}", - Self::S_UPPER_LIMIT_HEX_STR + SIGNATURE_S_UPPER_LIMIT_HEX_STR ))); } Ok(()) @@ -1567,13 +1567,13 @@ mod test { let roundtrip_signature = Signature::from(signature); assert_eq!( signature, - alloy::primitives::PrimitiveSignature::try_from(roundtrip_signature).unwrap() + alloy::primitives::PrimitiveSignature::from(roundtrip_signature) ); } #[test] fn test_signature_malleability_check() { - let s = U256::from_hex_str(Signature::S_UPPER_LIMIT_HEX_STR).unwrap(); + let s = U256::from_hex_str(SIGNATURE_S_UPPER_LIMIT_HEX_STR).unwrap(); Signature::check_malleability(&s).unwrap(); // If signature S field exceeds the limit, it should return an error. @@ -1654,10 +1654,10 @@ mod test { pub fn test_signature_v_for_legacy_eip_155_transaction_with_y_parity_true() { // Arrange let signature = - Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64.into()).unwrap(); + Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64).unwrap(); assert_eq!(signature.r, 100u64.into()); assert_eq!(signature.s, 200u64.into()); - assert_eq!(signature.y_parity, true); + assert!(signature.y_parity); let chain_id = random::() as u64; @@ -1675,10 +1675,10 @@ mod test { pub fn test_signature_v_for_legacy_eip_155_transaction_with_y_parity_false() { // Arrange let signature = - Signature::new_from_rsv(300u64.into(), 500u64.into(), 111u64.into()).unwrap(); + Signature::new_from_rsv(300u64.into(), 500u64.into(), 111u64).unwrap(); assert_eq!(signature.r, 300u64.into()); assert_eq!(signature.s, 500u64.into()); - assert_eq!(signature.y_parity, false); + assert!(!signature.y_parity); let chain_id = random::() as u64; @@ -1696,10 +1696,10 @@ mod test { pub fn test_signature_v_for_legacy_not_eip_155_transaction_with_y_parity_true() { // Arrange let signature = - Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64.into()).unwrap(); + Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64).unwrap(); assert_eq!(signature.r, 100u64.into()); assert_eq!(signature.s, 200u64.into()); - assert_eq!(signature.y_parity, true); + assert!(signature.y_parity); // Act let v = signature.v(TxChainInfo::LegacyTx { chain_id: None }); @@ -1713,10 +1713,10 @@ mod test { pub fn test_signature_v_for_legacy_not_eip_155_transaction_with_y_parity_false() { // Arrange let signature = - Signature::new_from_rsv(1000u64.into(), 2000u64.into(), 101u64.into()).unwrap(); + Signature::new_from_rsv(1000u64.into(), 2000u64.into(), 101u64).unwrap(); assert_eq!(signature.r, 1000u64.into()); assert_eq!(signature.s, 2000u64.into()); - assert_eq!(signature.y_parity, false); + assert!(!signature.y_parity); // Act let v = signature.v(TxChainInfo::LegacyTx { chain_id: None }); @@ -1730,10 +1730,10 @@ mod test { pub fn test_signature_v_for_non_legacy_transaction_with_y_parity_true() { // Arrange let signature = - Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64.into()).unwrap(); + Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64).unwrap(); assert_eq!(signature.r, 100u64.into()); assert_eq!(signature.s, 200u64.into()); - assert_eq!(signature.y_parity, true); + assert!(signature.y_parity); // Act let v = signature.v(TxChainInfo::OtherTx); @@ -1747,10 +1747,10 @@ mod test { pub fn test_signature_v_for_non_legacy_transaction_with_y_parity_false() { // Arrange let signature = - Signature::new_from_rsv(1000u64.into(), 2000u64.into(), 101u64.into()).unwrap(); + Signature::new_from_rsv(1000u64.into(), 2000u64.into(), 101u64).unwrap(); assert_eq!(signature.r, 1000u64.into()); assert_eq!(signature.s, 2000u64.into()); - assert_eq!(signature.y_parity, false); + assert!(!signature.y_parity); // Act let v = signature.v(TxChainInfo::OtherTx); diff --git a/src/eth-signer/src/ic_sign.rs b/src/eth-signer/src/ic_sign.rs index a86cd6ca..4cf1e56d 100644 --- a/src/eth-signer/src/ic_sign.rs +++ b/src/eth-signer/src/ic_sign.rs @@ -297,7 +297,7 @@ mod tests { .unwrap(); let primitive_signature = - alloy::primitives::PrimitiveSignature::try_from(signature).unwrap(); + alloy::primitives::PrimitiveSignature::from(signature); let recovered_from = primitive_signature .recover_address_from_prehash(&tx.signature_hash()) diff --git a/src/eth-signer/src/sign_strategy.rs b/src/eth-signer/src/sign_strategy.rs index 844d7fc5..b13c162a 100644 --- a/src/eth-signer/src/sign_strategy.rs +++ b/src/eth-signer/src/sign_strategy.rs @@ -341,8 +341,7 @@ mod test { let digest = [42u8; 32]; let signature = signer.sign_digest(digest).await.unwrap(); - let recovered = alloy::primitives::PrimitiveSignature::try_from(signature) - .unwrap() + let recovered = alloy::primitives::PrimitiveSignature::from(signature) .recover_address_from_prehash(&alloy::primitives::B256::from_slice(&digest)) .unwrap(); diff --git a/src/eth-signer/src/transaction.rs b/src/eth-signer/src/transaction.rs index 8bbd467b..099cbb27 100644 --- a/src/eth-signer/src/transaction.rs +++ b/src/eth-signer/src/transaction.rs @@ -128,7 +128,7 @@ mod test { gas_price: 20_000u64.into(), input: Vec::new(), signature: SigningMethod::Signature( - DidSignature::new_from_rsv(1u64.into(), 2u64.into(), 1u64.into()).unwrap(), + DidSignature::new_from_rsv(1u64.into(), 2u64.into(), 1u64).unwrap(), ), chain_id: 31541, }; diff --git a/src/eth-signer/tests/test_canister/src/canister.rs b/src/eth-signer/tests/test_canister/src/canister.rs index 950b978e..8ff013f2 100644 --- a/src/eth-signer/tests/test_canister/src/canister.rs +++ b/src/eth-signer/tests/test_canister/src/canister.rs @@ -48,7 +48,7 @@ impl TestCanister { .await .unwrap(); - let tx = tx.into_signed(signature.try_into().unwrap()); + let tx = tx.into_signed(signature.into()); let recovered_from = tx.recover_signer().unwrap(); assert_eq!(recovered_from, from); From 7104533f1c35978cf5fc3686516089a6cec6405d Mon Sep 17 00:00:00 2001 From: Francesco Date: Thu, 19 Dec 2024 11:00:59 +0100 Subject: [PATCH 06/13] clippy --- src/did/src/transaction.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/did/src/transaction.rs b/src/did/src/transaction.rs index 5a686c1b..454ebfd0 100644 --- a/src/did/src/transaction.rs +++ b/src/did/src/transaction.rs @@ -240,7 +240,7 @@ impl From for Signature { /// Upper limit for signature S field. /// See comment to `Signature::check_malleability()` for more details. -pub const SIGNATURE_S_UPPER_LIMIT_HEX_STR: &'static str = +pub const SIGNATURE_S_UPPER_LIMIT_HEX_STR: &str = "0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0"; /// Upper limit for signature S field. From 3c85a0295c7a62bba023c9542dff80a14a89eaf1 Mon Sep 17 00:00:00 2001 From: Francesco Date: Thu, 19 Dec 2024 11:01:12 +0100 Subject: [PATCH 07/13] fmt --- src/did/src/transaction.rs | 18 ++++++------------ src/eth-signer/src/ic_sign.rs | 3 +-- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/src/did/src/transaction.rs b/src/did/src/transaction.rs index 454ebfd0..fbc2c336 100644 --- a/src/did/src/transaction.rs +++ b/src/did/src/transaction.rs @@ -1653,8 +1653,7 @@ mod test { #[test] pub fn test_signature_v_for_legacy_eip_155_transaction_with_y_parity_true() { // Arrange - let signature = - Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64).unwrap(); + let signature = Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64).unwrap(); assert_eq!(signature.r, 100u64.into()); assert_eq!(signature.s, 200u64.into()); assert!(signature.y_parity); @@ -1674,8 +1673,7 @@ mod test { #[test] pub fn test_signature_v_for_legacy_eip_155_transaction_with_y_parity_false() { // Arrange - let signature = - Signature::new_from_rsv(300u64.into(), 500u64.into(), 111u64).unwrap(); + let signature = Signature::new_from_rsv(300u64.into(), 500u64.into(), 111u64).unwrap(); assert_eq!(signature.r, 300u64.into()); assert_eq!(signature.s, 500u64.into()); assert!(!signature.y_parity); @@ -1695,8 +1693,7 @@ mod test { #[test] pub fn test_signature_v_for_legacy_not_eip_155_transaction_with_y_parity_true() { // Arrange - let signature = - Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64).unwrap(); + let signature = Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64).unwrap(); assert_eq!(signature.r, 100u64.into()); assert_eq!(signature.s, 200u64.into()); assert!(signature.y_parity); @@ -1712,8 +1709,7 @@ mod test { #[test] pub fn test_signature_v_for_legacy_not_eip_155_transaction_with_y_parity_false() { // Arrange - let signature = - Signature::new_from_rsv(1000u64.into(), 2000u64.into(), 101u64).unwrap(); + let signature = Signature::new_from_rsv(1000u64.into(), 2000u64.into(), 101u64).unwrap(); assert_eq!(signature.r, 1000u64.into()); assert_eq!(signature.s, 2000u64.into()); assert!(!signature.y_parity); @@ -1729,8 +1725,7 @@ mod test { #[test] pub fn test_signature_v_for_non_legacy_transaction_with_y_parity_true() { // Arrange - let signature = - Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64).unwrap(); + let signature = Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64).unwrap(); assert_eq!(signature.r, 100u64.into()); assert_eq!(signature.s, 200u64.into()); assert!(signature.y_parity); @@ -1746,8 +1741,7 @@ mod test { #[test] pub fn test_signature_v_for_non_legacy_transaction_with_y_parity_false() { // Arrange - let signature = - Signature::new_from_rsv(1000u64.into(), 2000u64.into(), 101u64).unwrap(); + let signature = Signature::new_from_rsv(1000u64.into(), 2000u64.into(), 101u64).unwrap(); assert_eq!(signature.r, 1000u64.into()); assert_eq!(signature.s, 2000u64.into()); assert!(!signature.y_parity); diff --git a/src/eth-signer/src/ic_sign.rs b/src/eth-signer/src/ic_sign.rs index 4cf1e56d..d7574502 100644 --- a/src/eth-signer/src/ic_sign.rs +++ b/src/eth-signer/src/ic_sign.rs @@ -296,8 +296,7 @@ mod tests { .await .unwrap(); - let primitive_signature = - alloy::primitives::PrimitiveSignature::from(signature); + let primitive_signature = alloy::primitives::PrimitiveSignature::from(signature); let recovered_from = primitive_signature .recover_address_from_prehash(&tx.signature_hash()) From 34f28df86f55c4270a18f4628034d32789881024 Mon Sep 17 00:00:00 2001 From: Francesco Date: Thu, 19 Dec 2024 11:14:48 +0100 Subject: [PATCH 08/13] remove duplication --- src/did/src/transaction.rs | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/did/src/transaction.rs b/src/did/src/transaction.rs index fbc2c336..53746c08 100644 --- a/src/did/src/transaction.rs +++ b/src/did/src/transaction.rs @@ -391,10 +391,12 @@ impl TryFrom for alloy::consensus::TxEnvelope { impl From for Transaction { fn from(tx: alloy::rpc::types::Transaction) -> Self { + let signature = tx.inner.signature(); + let signature = Signature::from(*signature); + match tx.inner { alloy::consensus::TxEnvelope::Legacy(signed) => { let inner_tx = signed.tx(); - let signature = signed.signature(); let chain_id = inner_tx.chain_id(); Self { hash: (*signed.hash()).into(), @@ -412,15 +414,14 @@ impl From for Transaction { block_number: tx.block_number.map(Into::into), transaction_index: tx.transaction_index.map(Into::into), from: tx.from.into(), - v: (to_eip155_value(signature.v(), chain_id) as u64).into(), - r: signature.r().into(), - s: signature.s().into(), + v: signature.v(TxChainInfo::LegacyTx { chain_id }).into(), + r: signature.r, + s: signature.s, transaction_type: Some(TRANSACTION_TYPE_LEGACY.into()), } } alloy::consensus::TxEnvelope::Eip2930(signed) => { let inner_tx = signed.tx(); - let signature = signed.signature(); Self { hash: (*signed.hash()).into(), nonce: inner_tx.nonce.into(), @@ -437,15 +438,14 @@ impl From for Transaction { block_number: tx.block_number.map(Into::into), transaction_index: tx.transaction_index.map(Into::into), from: tx.from.into(), - v: (signature.v() as u64).into(), - r: signature.r().into(), - s: signature.s().into(), + v: signature.v(TxChainInfo::OtherTx).into(), + r: signature.r, + s: signature.s, transaction_type: Some(TRANSACTION_TYPE_EIP2930.into()), } } alloy::consensus::TxEnvelope::Eip1559(signed) => { let inner_tx = signed.tx(); - let signature = signed.signature(); Self { hash: (*signed.hash()).into(), nonce: inner_tx.nonce.into(), @@ -462,9 +462,9 @@ impl From for Transaction { block_number: tx.block_number.map(Into::into), transaction_index: tx.transaction_index.map(Into::into), from: tx.from.into(), - v: (signature.v() as u64).into(), - r: signature.r().into(), - s: signature.s().into(), + v: signature.v(TxChainInfo::OtherTx).into(), + r: signature.r, + s: signature.s, transaction_type: Some(TRANSACTION_TYPE_EIP1559.into()), } } @@ -1651,7 +1651,7 @@ mod test { } #[test] - pub fn test_signature_v_for_legacy_eip_155_transaction_with_y_parity_true() { + pub fn test_signature_v_for_legacy_eip155_transaction_with_y_parity_true() { // Arrange let signature = Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64).unwrap(); assert_eq!(signature.r, 100u64.into()); @@ -1671,7 +1671,7 @@ mod test { } #[test] - pub fn test_signature_v_for_legacy_eip_155_transaction_with_y_parity_false() { + pub fn test_signature_v_for_legacy_eip155_transaction_with_y_parity_false() { // Arrange let signature = Signature::new_from_rsv(300u64.into(), 500u64.into(), 111u64).unwrap(); assert_eq!(signature.r, 300u64.into()); @@ -1691,7 +1691,7 @@ mod test { } #[test] - pub fn test_signature_v_for_legacy_not_eip_155_transaction_with_y_parity_true() { + pub fn test_signature_v_for_legacy_not_eip155_transaction_with_y_parity_true() { // Arrange let signature = Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64).unwrap(); assert_eq!(signature.r, 100u64.into()); @@ -1707,7 +1707,7 @@ mod test { } #[test] - pub fn test_signature_v_for_legacy_not_eip_155_transaction_with_y_parity_false() { + pub fn test_signature_v_for_legacy_not_eip155_transaction_with_y_parity_false() { // Arrange let signature = Signature::new_from_rsv(1000u64.into(), 2000u64.into(), 101u64).unwrap(); assert_eq!(signature.r, 1000u64.into()); From bd8ce9b1575dc8ccb3d26f715f22346733691c50 Mon Sep 17 00:00:00 2001 From: Francesco Date: Thu, 19 Dec 2024 12:19:09 +0100 Subject: [PATCH 09/13] Add Parity --- src/did/src/transaction.rs | 86 +++++++++++++++++++++++++++-------- src/eth-signer/src/ic_sign.rs | 20 ++------ 2 files changed, 71 insertions(+), 35 deletions(-) diff --git a/src/did/src/transaction.rs b/src/did/src/transaction.rs index 53746c08..6792ba3c 100644 --- a/src/did/src/transaction.rs +++ b/src/did/src/transaction.rs @@ -173,11 +173,40 @@ impl From for BlockId { /// ECDSA signature representation #[derive(Debug, Clone, PartialEq, Eq, CandidType, Serialize, Deserialize, Default)] pub struct Signature { - pub y_parity: bool, + pub y_parity: Parity, pub r: U256, pub s: U256, } +/// Parity of the Y coordinate of the public key +#[derive(Debug, Clone, PartialEq, Eq, CandidType, Serialize, Deserialize, Default)] +pub enum Parity { + #[default] + Even, + Odd, +} + +impl Parity { + + /// Returns the parity bool value + pub fn as_bool(&self) -> bool { + match self { + Parity::Even => false, + Parity::Odd => true, + } + } + + /// Returns a Parity from a y_parity_is_odd bool value. + pub fn from_y_parity_is_odd(y_parity_is_odd: bool) -> Self { + if y_parity_is_odd { + Parity::Odd + } else { + Parity::Even + } + } + +} + /// Transaction type and Chain id data required to generate the Signature V value for EIP-155 pub enum TxChainInfo { /// A legacy transation with (EIP-155) or without (not EIP-155) chain id @@ -189,9 +218,9 @@ pub enum TxChainInfo { impl Signature { /// Creates a new signature from the given r, s and v values. pub fn new_from_rsv(r: U256, s: U256, v: u64) -> Result { - let y_parity = + let y_parity_is_odd = normalize_v(v).ok_or_else(|| EvmError::InvalidSignatureParity(format!("{}", v)))?; - Ok(Self { r, s, y_parity }) + Ok(Self { r, s, y_parity: Parity::from_y_parity_is_odd(y_parity_is_odd) }) } /// Recovers an [`Address`] from this signature and the given prehashed message. @@ -210,21 +239,21 @@ impl Signature { /// - For other transactions, the V value is the Y parity value pub fn v(&self, info: TxChainInfo) -> u64 { match info { - TxChainInfo::LegacyTx { chain_id } => to_eip155_value(self.y_parity, chain_id) as u64, - TxChainInfo::OtherTx => self.y_parity as u64, + TxChainInfo::LegacyTx { chain_id } => to_eip155_value(self.y_parity.as_bool(), chain_id) as u64, + TxChainInfo::OtherTx => self.y_parity.as_bool() as u64, } } } impl From for alloy::primitives::PrimitiveSignature { fn from(value: Signature) -> Self { - alloy::primitives::PrimitiveSignature::new(value.r.0, value.s.0, value.y_parity) + alloy::primitives::PrimitiveSignature::new(value.r.0, value.s.0, value.y_parity.as_bool()) } } impl From<&Signature> for alloy::primitives::PrimitiveSignature { fn from(value: &Signature) -> Self { - alloy::primitives::PrimitiveSignature::new(value.r.0, value.s.0, value.y_parity) + alloy::primitives::PrimitiveSignature::new(value.r.0, value.s.0, value.y_parity.as_bool()) } } @@ -233,7 +262,7 @@ impl From for Signature { Self { r: U256(value.r()), s: U256(value.s()), - y_parity: value.v(), + y_parity: Parity::from_y_parity_is_odd(value.v()), } } } @@ -1650,13 +1679,29 @@ mod test { } } + #[test] + pub fn test_parity_to_from_bool() { + assert_eq!(Parity::from_y_parity_is_odd(true), Parity::Odd); + assert_eq!(Parity::from_y_parity_is_odd(false), Parity::Even); + + assert_eq!(Parity::Odd.as_bool(), true); + assert_eq!(Parity::Even.as_bool(), false); + + let parity = Parity::from_y_parity_is_odd(true); + assert!(parity.as_bool()); + + let parity = Parity::from_y_parity_is_odd(false); + assert!(!parity.as_bool()); + } + #[test] pub fn test_signature_v_for_legacy_eip155_transaction_with_y_parity_true() { // Arrange - let signature = Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64).unwrap(); + let signature = Signature::new_from_rsv(100u64.into(), 200u64.into(), 1u64).unwrap(); assert_eq!(signature.r, 100u64.into()); assert_eq!(signature.s, 200u64.into()); - assert!(signature.y_parity); + assert_eq!(Parity::Odd, signature.y_parity); + assert!(signature.y_parity.as_bool()); let chain_id = random::() as u64; @@ -1673,10 +1718,11 @@ mod test { #[test] pub fn test_signature_v_for_legacy_eip155_transaction_with_y_parity_false() { // Arrange - let signature = Signature::new_from_rsv(300u64.into(), 500u64.into(), 111u64).unwrap(); + let signature = Signature::new_from_rsv(300u64.into(), 500u64.into(), 0u64).unwrap(); assert_eq!(signature.r, 300u64.into()); assert_eq!(signature.s, 500u64.into()); - assert!(!signature.y_parity); + assert_eq!(Parity::Even, signature.y_parity); + assert!(!signature.y_parity.as_bool()); let chain_id = random::() as u64; @@ -1693,10 +1739,11 @@ mod test { #[test] pub fn test_signature_v_for_legacy_not_eip155_transaction_with_y_parity_true() { // Arrange - let signature = Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64).unwrap(); + let signature = Signature::new_from_rsv(100u64.into(), 200u64.into(), 28u64).unwrap(); assert_eq!(signature.r, 100u64.into()); assert_eq!(signature.s, 200u64.into()); - assert!(signature.y_parity); + assert_eq!(Parity::Odd, signature.y_parity); + assert!(signature.y_parity.as_bool()); // Act let v = signature.v(TxChainInfo::LegacyTx { chain_id: None }); @@ -1709,10 +1756,11 @@ mod test { #[test] pub fn test_signature_v_for_legacy_not_eip155_transaction_with_y_parity_false() { // Arrange - let signature = Signature::new_from_rsv(1000u64.into(), 2000u64.into(), 101u64).unwrap(); + let signature = Signature::new_from_rsv(1000u64.into(), 2000u64.into(), 27u64).unwrap(); assert_eq!(signature.r, 1000u64.into()); assert_eq!(signature.s, 2000u64.into()); - assert!(!signature.y_parity); + assert_eq!(Parity::Even, signature.y_parity); + assert!(!signature.y_parity.as_bool()); // Act let v = signature.v(TxChainInfo::LegacyTx { chain_id: None }); @@ -1728,7 +1776,8 @@ mod test { let signature = Signature::new_from_rsv(100u64.into(), 200u64.into(), 100u64).unwrap(); assert_eq!(signature.r, 100u64.into()); assert_eq!(signature.s, 200u64.into()); - assert!(signature.y_parity); + assert_eq!(Parity::Odd, signature.y_parity); + assert!(signature.y_parity.as_bool()); // Act let v = signature.v(TxChainInfo::OtherTx); @@ -1744,7 +1793,8 @@ mod test { let signature = Signature::new_from_rsv(1000u64.into(), 2000u64.into(), 101u64).unwrap(); assert_eq!(signature.r, 1000u64.into()); assert_eq!(signature.s, 2000u64.into()); - assert!(!signature.y_parity); + assert_eq!(Parity::Even, signature.y_parity); + assert!(!signature.y_parity.as_bool()); // Act let v = signature.v(TxChainInfo::OtherTx); diff --git a/src/eth-signer/src/ic_sign.rs b/src/eth-signer/src/ic_sign.rs index d7574502..b2097593 100644 --- a/src/eth-signer/src/ic_sign.rs +++ b/src/eth-signer/src/ic_sign.rs @@ -5,7 +5,7 @@ use alloy::primitives::{Address, PrimitiveSignature, SignatureError, U256}; use alloy::signers::k256::ecdsa::{RecoveryId, Signature as EcsaSignature, VerifyingKey}; use alloy::signers::utils::public_key_to_address; use candid::{CandidType, Principal}; -use did::transaction::Signature as DidSignature; +use did::transaction::{Parity, Signature as DidSignature}; use ic_canister::virtual_canister_call; use ic_exports::ic_cdk::api::call::RejectionCode; use ic_exports::ic_cdk::api::management_canister::ecdsa::{ @@ -119,20 +119,6 @@ impl IcSigner { Self.sign_digest(*hash, pubkey, key_id, derivation_path) .await - // let mut signature = Self - // .sign_digest(*hash, pubkey, key_id, derivation_path) - // .await?; - - // let v: u64 = signature.v.0.to(); - - // // For non-legacy transactions recovery id should be updated. - // // Details: https://eips.ethereum.org/EIPS/eip-155. - // signature.v = match tx.chain_id() { - // Some(chain_id) => (chain_id * 2 + 35 + (v - 27)).into(), - // None => v.into(), - // }; - - // Ok(signature) } /// Signs the digest using `ManagementCanister::sign_with_ecdsa()` call. @@ -170,7 +156,7 @@ impl IcSigner { let r = U256::from_be_slice(&signature_data[0..32]); let s = U256::from_be_slice(&signature_data[32..64]); - let y_parity = recovery_id.is_y_odd(); + let y_parity_is_odd = recovery_id.is_y_odd(); // Signature malleability check is not required, because dfinity uses `k256` crate // as `ecdsa_secp256k1` implementation, and it takes care about signature malleability. @@ -178,7 +164,7 @@ impl IcSigner { let signature = DidSignature { r: r.into(), s: s.into(), - y_parity, + y_parity: Parity::from_y_parity_is_odd(y_parity_is_odd), }; Ok(signature) From d3f96a45f2f9d0e6e34f7a134f708ccbc1618985 Mon Sep 17 00:00:00 2001 From: Francesco Date: Thu, 19 Dec 2024 12:19:18 +0100 Subject: [PATCH 10/13] fmt --- src/did/src/transaction.rs | 12 ++++++++---- src/eth-signer/src/ic_sign.rs | 1 - 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/did/src/transaction.rs b/src/did/src/transaction.rs index 6792ba3c..04b529a9 100644 --- a/src/did/src/transaction.rs +++ b/src/did/src/transaction.rs @@ -187,7 +187,6 @@ pub enum Parity { } impl Parity { - /// Returns the parity bool value pub fn as_bool(&self) -> bool { match self { @@ -204,7 +203,6 @@ impl Parity { Parity::Even } } - } /// Transaction type and Chain id data required to generate the Signature V value for EIP-155 @@ -220,7 +218,11 @@ impl Signature { pub fn new_from_rsv(r: U256, s: U256, v: u64) -> Result { let y_parity_is_odd = normalize_v(v).ok_or_else(|| EvmError::InvalidSignatureParity(format!("{}", v)))?; - Ok(Self { r, s, y_parity: Parity::from_y_parity_is_odd(y_parity_is_odd) }) + Ok(Self { + r, + s, + y_parity: Parity::from_y_parity_is_odd(y_parity_is_odd), + }) } /// Recovers an [`Address`] from this signature and the given prehashed message. @@ -239,7 +241,9 @@ impl Signature { /// - For other transactions, the V value is the Y parity value pub fn v(&self, info: TxChainInfo) -> u64 { match info { - TxChainInfo::LegacyTx { chain_id } => to_eip155_value(self.y_parity.as_bool(), chain_id) as u64, + TxChainInfo::LegacyTx { chain_id } => { + to_eip155_value(self.y_parity.as_bool(), chain_id) as u64 + } TxChainInfo::OtherTx => self.y_parity.as_bool() as u64, } } diff --git a/src/eth-signer/src/ic_sign.rs b/src/eth-signer/src/ic_sign.rs index b2097593..9c6c1cb3 100644 --- a/src/eth-signer/src/ic_sign.rs +++ b/src/eth-signer/src/ic_sign.rs @@ -118,7 +118,6 @@ impl IcSigner { Self.sign_digest(*hash, pubkey, key_id, derivation_path) .await - } /// Signs the digest using `ManagementCanister::sign_with_ecdsa()` call. From 190f07fd4f73b382bb51c47940108e6f1e825de8 Mon Sep 17 00:00:00 2001 From: Francesco Date: Thu, 19 Dec 2024 12:23:39 +0100 Subject: [PATCH 11/13] clippy --- src/did/src/transaction.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/did/src/transaction.rs b/src/did/src/transaction.rs index 04b529a9..37efda1f 100644 --- a/src/did/src/transaction.rs +++ b/src/did/src/transaction.rs @@ -1688,8 +1688,8 @@ mod test { assert_eq!(Parity::from_y_parity_is_odd(true), Parity::Odd); assert_eq!(Parity::from_y_parity_is_odd(false), Parity::Even); - assert_eq!(Parity::Odd.as_bool(), true); - assert_eq!(Parity::Even.as_bool(), false); + assert!(Parity::Odd.as_bool()); + assert!(!Parity::Even.as_bool()); let parity = Parity::from_y_parity_is_odd(true); assert!(parity.as_bool()); From ee5bfa7267ae54f7eea362f50cfee0f0f333ab69 Mon Sep 17 00:00:00 2001 From: Francesco Date: Thu, 19 Dec 2024 13:23:55 +0100 Subject: [PATCH 12/13] Add test for wrong signature creation --- src/did/src/transaction.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/did/src/transaction.rs b/src/did/src/transaction.rs index 37efda1f..7d03cb08 100644 --- a/src/did/src/transaction.rs +++ b/src/did/src/transaction.rs @@ -1807,4 +1807,11 @@ mod test { let expected_v = 0; assert_eq!(v, expected_v); } + + #[test] + pub fn test_signature_creation_should_fail_if_not_valid_v() { + assert!(Signature::new_from_rsv(100u64.into(), 200u64.into(), 2u64).is_err()); + assert!(Signature::new_from_rsv(100u64.into(), 200u64.into(), 29u64).is_err()); + assert!(Signature::new_from_rsv(100u64.into(), 200u64.into(), 32u64).is_err()); + } } From 8311f037b27f188496376bc4889f83cb1eee9976 Mon Sep 17 00:00:00 2001 From: Francesco Date: Thu, 19 Dec 2024 16:37:45 +0100 Subject: [PATCH 13/13] bump_version --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 70bf3c32..9d795bff 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,7 +23,7 @@ homepage = "https://github.com/bitfinity-network/bitfinity-evm-sdk" include = ["src/**/*", "LICENSE", "README.md"] license = "MIT" repository = "https://github.com/bitfinity-network/bitfinity-evm-sdk" -version = "0.38.0" +version = "0.39.0" [workspace.dependencies] did = { path = "src/did" }