fix: review comments from #25 (#86)

Author: varex83

crates/charon-cluster/src/definition.rs

@@ -324,11 +324,11 @@ impl Definition {
         };
 
         def.validator_addresses = fee_recipient_addresses
-            .into_iter()
-            .enumerate()
-            .map(|(i, address)| ValidatorAddresses {
-                fee_recipient_address: address,
-                withdrawal_address: withdrawal_addresses[i].clone(),
+            .iter()
+            .zip(withdrawal_addresses.iter())
+            .map(|(fee, wa)| ValidatorAddresses {
+                fee_recipient_address: fee.clone(),
+                withdrawal_address: wa.clone(),
             })
             .collect();
 

crates/charon-cluster/src/eip712sigs.rs

@@ -134,26 +134,28 @@ fn digest_eip712(
 ) -> Result<Vec<u8>> {
     let chain_id = fork_version_to_chain_id(definition.fork_version.as_ref())?;
 
-    let mut data = TypedData {
+    let fields = typ
+        .fields
+        .iter()
+        .map(|f| Field {
+            name: f.field.to_string(),
+            field_type: f.field_type.to_string(),
+            value: (f.value_func)(definition, operator),
+        })
+        .collect::<Vec<_>>();
+
+    let data = TypedData {
         domain: Domain {
             name: "Obol".to_string(),
             version: "1".to_string(),
             chain_id,
         },
         primary_type: Type {
             name: typ.primary_type.to_string(),
-            fields: vec![],
+            fields,
         },
     };
 
-    for field in typ.fields.iter() {
-        data.primary_type.fields.push(Field {
-            name: field.field.to_string(),
-            field_type: field.field_type.to_string(),
-            value: (field.value_func)(definition, operator),
-        });
-    }
-
     let digest = hash_typed_data(&data).map_err(EIP712Error::FailedToHashTypedData)?;
 
     Ok(digest)

crates/charon-eth2/src/eip712.rs

@@ -38,17 +38,17 @@ pub struct Field {
     /// Name of the field.
     pub name: String,
     /// Type of the field.
-    pub field_type: String,
+    pub field_type: Primitive,
     /// Value of the field.
     pub value: Value,
 }
 
 impl Field {
     /// Creates a new field.
-    pub fn new(name: String, field_type: String, value: Value) -> Self {
+    pub fn new(name: impl Into<String>, field_type: impl Into<String>, value: Value) -> Self {
         Self {
-            name,
-            field_type,
+            name: name.into(),
+            field_type: field_type.into(),
             value,
         }
     }
@@ -156,9 +156,9 @@ fn encode_field(field: &Field) -> Result<Vec<u8>> {
     match (field.field_type.as_str(), &field.value) {
         (PRIMITIVE_STRING, Value::String(s)) => Ok(keccak_hash(s.as_bytes())?),
         (PRIMITIVE_UINT256, Value::Number(n)) => {
-            let mut bytes = [0u8; 32];
+            let mut bytes = vec![0u8; 32];
             bytes[24..].copy_from_slice(&n.to_be_bytes());
-            Ok(bytes.to_vec())
+            Ok(bytes)
         }
         _ => Err(Eip712Error::UnsupportedFieldType {
             field_type: field.field_type.clone(),

crates/charon-eth2/src/enr.rs

@@ -5,7 +5,10 @@ use charon_k1util::{self, self as k1util, SIGNATURE_LEN_WITHOUT_V};
 use k256::{PublicKey, SecretKey, elliptic_curve};
 use sha3::{Digest, Keccak256};
 
-use crate::rlp::{RlpError, decode_bytes_list, encode_bytes_list};
+use crate::{
+    rlp::{RlpError, decode_bytes_list, encode_bytes_list},
+    utils,
+};
 
 /// The key for the secp256k1 public key in the ENR.
 pub const KEY_SECP256K1: &str = "secp256k1";
@@ -154,45 +157,27 @@ impl Record {
     ///
     /// Returns None if the IP address is not set.
     pub fn ip(&self) -> Option<Ipv4Addr> {
-        self.kvs.get(KEY_IP).and_then(|value| {
-            if value.len() != 4 {
-                return None;
-            }
-
-            let mut bytes = [0u8; 4];
-            bytes.copy_from_slice(value);
-            Some(Ipv4Addr::from(bytes))
-        })
+        let value = self.kvs.get(KEY_IP)?;
+        let bytes: [u8; 4] = value.as_slice().try_into().ok()?;
+        Some(Ipv4Addr::from(bytes))
     }
 
     /// Returns the TCP port of the record.
     ///
     /// Returns None if the TCP port is not set.
     pub fn tcp(&self) -> Option<u16> {
-        self.kvs.get(KEY_TCP).and_then(|value| {
-            if value.len() != 2 {
-                return None;
-            }
-
-            let mut bytes = [0u8; 2];
-            bytes.copy_from_slice(value);
-            Some(u16::from_be_bytes(bytes))
-        })
+        let value = self.kvs.get(KEY_TCP)?;
+        let bytes = value.as_slice().try_into().ok()?;
+        Some(u16::from_be_bytes(bytes))
     }
 
     /// Returns the UDP port of the record.
     ///
     /// Returns None if the UDP port is not set.
     pub fn udp(&self) -> Option<u16> {
-        self.kvs.get(KEY_UDP).and_then(|value| {
-            if value.len() != 2 {
-                return None;
-            }
-
-            let mut bytes = [0u8; 2];
-            bytes.copy_from_slice(value);
-            Some(u16::from_be_bytes(bytes))
-        })
+        let value = self.kvs.get(KEY_UDP)?;
+        let bytes = value.as_slice().try_into().ok()?;
+        Some(u16::from_be_bytes(bytes))
     }
 }
 
@@ -249,9 +234,11 @@ impl TryFrom<&str> for Record {
             kvs: HashMap::new(),
         };
 
-        for i in (2..elements.len()).step_by(2) {
-            let key = String::from_utf8_lossy(&elements[i]).to_string();
-            let value = elements[i.wrapping_add(1)].clone();
+        for pair in elements.chunks_exact(2).skip(1) {
+            let [key, value] = pair else {
+                unreachable!("Expected even number of elements");
+            };
+            let key = String::from_utf8_lossy(key).to_string();
 
             if record.kvs.contains_key(&key) {
                 return Err(RecordError::DuplicateKey(key));
@@ -262,11 +249,11 @@ impl TryFrom<&str> for Record {
             match key.as_str() {
                 KEY_SECP256K1 => {
                     record.public_key = Some(
-                        PublicKey::from_sec1_bytes(&value).map_err(RecordError::Secp256k1Error)?,
+                        PublicKey::from_sec1_bytes(value).map_err(RecordError::Secp256k1Error)?,
                     );
                 }
                 KEY_ID => {
-                    let value_str = String::from_utf8_lossy(&value).to_string();
+                    let value_str = String::from_utf8_lossy(value).to_string();
                     if value_str != VAL_ID {
                         return Err(RecordError::InvalidFormat(
                             InvalidFormatError::NonV4IdentitySchemeNotSupported,
@@ -277,21 +264,15 @@ impl TryFrom<&str> for Record {
             }
         }
 
-        if record.public_key.is_none() {
+        let Some(public_key) = record.public_key else {
             return Err(RecordError::InvalidFormat(
                 InvalidFormatError::PublicKeyNotSet,
             ));
-        }
+        };
 
         let encoded_elements = encode_bytes_list(&elements[1..]);
 
-        verify(
-            &record
-                .public_key
-                .unwrap_or_else(|| unreachable!("Public key expected to be set")),
-            &record.signature,
-            &encoded_elements,
-        )?;
+        verify(&public_key, &record.signature, &encoded_elements)?;
 
         Ok(record)
     }
@@ -307,8 +288,9 @@ pub(crate) fn sign(
     let digest = hasher.finalize();
 
     let signature = k1util::sign(private_key, &digest).map_err(RecordError::FailedToSign)?;
-    let mut signature_without_v = [0u8; SIGNATURE_LEN_WITHOUT_V];
-    signature_without_v.copy_from_slice(&signature[..SIGNATURE_LEN_WITHOUT_V]);
+    let signature_without_v = signature[..SIGNATURE_LEN_WITHOUT_V]
+        .try_into()
+        .expect("SIGNATURE_LEN_WITHOUT_V < SIGNATURE_LEN");
 
     Ok(signature_without_v)
 }
@@ -335,7 +317,7 @@ pub(crate) fn encode_elements(signature: &[u8], kvs: &HashMap<String, Vec<u8>>)
     keys.sort();
 
     // Start with sequence number = 0
-    let mut elements: Vec<Vec<u8>> = vec![to_big_endian(0)];
+    let mut elements: Vec<Vec<u8>> = vec![utils::to_big_endian(0)];
 
     for key in keys {
         elements.push(key.as_bytes().to_vec());
@@ -349,18 +331,9 @@ pub(crate) fn encode_elements(signature: &[u8], kvs: &HashMap<String, Vec<u8>>)
     encode_bytes_list(&elements)
 }
 
-/// Converts an integer to big-endian bytes without leading zeros.
-fn to_big_endian(mut n: u64) -> Vec<u8> {
-    let mut result = Vec::new();
-    while n > 0 {
-        result.insert(0, (n & 0xFF) as u8);
-        n >>= 8;
-    }
-    result
-}
-
 #[cfg(test)]
 mod tests {
+    use crate::utils;
     use charon_testutil::random::generate_insecure_k1_key;
     use k256::{
         Secp256k1,
@@ -491,7 +464,7 @@ mod tests {
             keys.sort();
 
             // Start with sequence number = 0
-            let mut elements: Vec<Vec<u8>> = vec![to_big_endian(0), to_big_endian(0)];
+            let mut elements: Vec<Vec<u8>> = vec![utils::to_big_endian(0), utils::to_big_endian(0)];
 
             for key in keys {
                 elements.push(key.as_bytes().to_vec());

crates/charon-eth2/src/lib.rs

@@ -15,3 +15,6 @@ pub mod eip712;
 
 /// Network utilities.
 pub mod network;
+
+/// Utilities.
+pub(crate) mod utils;

crates/charon-eth2/src/rlp.rs

@@ -1,5 +1,7 @@
 use thiserror::Error;
 
+use crate::utils;
+
 /// An error that can occur when decoding or encoding RLP data.
 #[derive(Debug, Error)]
 pub enum RlpError {
@@ -132,7 +134,7 @@ fn encode_length(length: usize, offset: u8) -> Vec<u8> {
         ];
     }
 
-    let big_endian = to_big_endian(length);
+    let big_endian = utils::to_big_endian(length);
     let prefix = u8::try_from(big_endian.len())
         .expect("big_endian.len() is always less than 256")
         .wrapping_add(offset)
@@ -141,17 +143,6 @@ fn encode_length(length: usize, offset: u8) -> Vec<u8> {
     [vec![prefix], big_endian].concat()
 }
 
-/// Converts an integer to big-endian byte representation without leading zeros.
-fn to_big_endian(mut value: usize) -> Vec<u8> {
-    let mut bytes = Vec::new();
-    while value > 0 {
-        bytes.push(u8::try_from(value % 256).expect("value % 256 is always less than 256"));
-        value >>= 8;
-    }
-    bytes.reverse();
-    bytes
-}
-
 /// Decodes a big-endian integer from a byte slice at the given offset and
 /// length.
 fn from_big_endian(bytes: &[u8], offset: usize, length: usize) -> Result<usize> {

crates/charon-eth2/src/utils.rs

@@ -0,0 +1,8 @@
+/// Converts an integer to big-endian byte representation without leading zeros.
+pub(crate) fn to_big_endian(value: usize) -> Vec<u8> {
+    value
+        .to_be_bytes()
+        .into_iter()
+        .skip_while(|x| *x == 0)
+        .collect()
+}

crates/charon-k1util/src/k1util.rs

@@ -41,10 +41,8 @@ pub enum K1UtilError {
     InvalidSignature(ecdsa::Error),
 
     /// The hash length is invalid.
-    #[error("The hash length is invalid: expected {expected}, actual {actual}")]
+    #[error("The hash length is invalid: expected {K1_HASH_LEN}, actual {actual}")]
     InvalidHashLength {
-        /// The expected hash length.
-        expected: usize,
         /// The actual hash length.
         actual: usize,
     },
@@ -95,10 +93,7 @@ pub fn public_key_from_libp2p(pk: &Libp2pPublicKey) -> Result<PublicKey> {
 /// or 1.
 pub fn sign(key: &SecretKey, hash: &[u8]) -> Result<[u8; SIGNATURE_LEN]> {
     if hash.len() != K1_HASH_LEN {
-        return Err(K1UtilError::InvalidHashLength {
-            expected: K1_HASH_LEN,
-            actual: hash.len(),
-        });
+        return Err(K1UtilError::InvalidHashLength { actual: hash.len() });
     }
 
     let mut hash_bytes = [0u8; K1_HASH_LEN];
@@ -142,10 +137,7 @@ pub fn verify_64(pubkey: &PublicKey, hash: &[u8], sig: &[u8]) -> Result<bool> {
     }
 
     if hash.len() != K1_HASH_LEN {
-        return Err(K1UtilError::InvalidHashLength {
-            expected: K1_HASH_LEN,
-            actual: hash.len(),
-        });
+        return Err(K1UtilError::InvalidHashLength { actual: hash.len() });
     }
 
     let signature = Signature::from_slice(sig).map_err(K1UtilError::InvalidSignature)?;
@@ -163,10 +155,7 @@ pub fn verify_64(pubkey: &PublicKey, hash: &[u8], sig: &[u8]) -> Result<bool> {
 /// Recover recovers the public key from a signature.
 pub fn recover(hash: &[u8], sig: &[u8]) -> Result<PublicKey> {
     if hash.len() != K1_HASH_LEN {
-        return Err(K1UtilError::InvalidHashLength {
-            expected: K1_HASH_LEN,
-            actual: hash.len(),
-        });
+        return Err(K1UtilError::InvalidHashLength { actual: hash.len() });
     }
 
     if sig.len() != SIGNATURE_LEN {
@@ -176,7 +165,11 @@ pub fn recover(hash: &[u8], sig: &[u8]) -> Result<PublicKey> {
         });
     }
 
-    let recovery_byte = sig[K1_REC_IDX];
+    let mut recovery_byte = sig[K1_REC_IDX];
+
+    if recovery_byte == 27 || recovery_byte == 28 {
+        recovery_byte = recovery_byte.wrapping_sub(27);
+    }
 
     let signature =
         Signature::from_slice(&sig[..SIGNATURE_LEN - 1]).map_err(K1UtilError::InvalidSignature)?;