From db1b9e88dd4199960a0a0238959939f5c7b5057c Mon Sep 17 00:00:00 2001 From: Smuu <18609909+Smuu@users.noreply.github.com> Date: Thu, 19 Dec 2024 13:48:04 +0100 Subject: [PATCH] feat: apply coderabbit review 1st round Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com> --- crates/cli/src/main.rs | 53 ++++++++++------------ crates/keys/src/signing_keys.rs | 10 ++-- crates/node_types/prover/src/prover/mod.rs | 7 ++- crates/tests/src/lib.rs | 8 ++-- 4 files changed, 35 insertions(+), 43 deletions(-) diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index a4eee750..efac8965 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -37,17 +37,14 @@ async fn main() -> std::io::Result<()> { ) })?; - let verifying_key_algorithm = config.verifying_key_algorithm.as_str(); + let verifying_key_algorithm = validate_algorithm(&config.verifying_key_algorithm)?; - if verifying_key_algorithm.is_empty() { - return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, "verifying key algorithm is required")); - } - - if !["ed25519", "secp256k1", "secp256r1"].contains(&verifying_key_algorithm) { - return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, "invalid verifying key algorithm")); - } - - let prover_vk = VerifyingKey::from_algorithm_and_bytes(verifying_key_algorithm, config.verifying_key.unwrap().as_bytes()).unwrap(); + let prover_vk = VerifyingKey::from_algorithm_and_bytes( + verifying_key_algorithm, + config.verifying_key.unwrap().as_bytes(), + ).map_err(|e| std::io::Error::new( + std::io::ErrorKind::InvalidData, format!("invalid prover verifying key: {}", e), + ))?; Arc::new(LightClient::new(da, celestia_config, Some(prover_vk))) } @@ -72,14 +69,7 @@ async fn main() -> std::io::Result<()> { .get_signing_key() .map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e.to_string()))?; - let verifying_key_algorithm = config.verifying_key_algorithm.as_str(); - if verifying_key_algorithm.is_empty() { - return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, "verifying key algorithm is required")); - } - - if !["ed25519", "secp256k1", "secp256r1"].contains(&verifying_key_algorithm) { - return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, "invalid verifying key algorithm")); - } + let verifying_key_algorithm = validate_algorithm(&config.verifying_key_algorithm)?; let signing_key = SigningKey::from_algorithm_and_bytes(verifying_key_algorithm, signing_key_chain.as_bytes()).unwrap(); let verifying_key = signing_key.verifying_key(); @@ -128,16 +118,7 @@ async fn main() -> std::io::Result<()> { .get_signing_key() .map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e.to_string()))?; - // error if config.verifying_key_algorithm.as_str() is not present in config or invalid - if config.verifying_key_algorithm.is_empty() { - return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, "verifying key algorithm is required")); - } - - let verifying_key_algorithm = config.verifying_key_algorithm.as_str(); - - if !["ed25519", "secp256k1", "secp256r1"].contains(&verifying_key_algorithm) { - return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, "invalid verifying key algorithm")); - } + let verifying_key_algorithm = validate_algorithm(&config.verifying_key_algorithm)?; let signing_key = SigningKey::from_algorithm_and_bytes(verifying_key_algorithm, signing_key_chain.as_bytes()).unwrap(); @@ -149,10 +130,10 @@ async fn main() -> std::io::Result<()> { "prover verifying key not found", ) }) - .and_then(|vk| VerifyingKey::from_algorithm_and_bytes(verifying_key_algorithm, vk.as_bytes()).map_err(|_| { + .and_then(|vk| VerifyingKey::from_algorithm_and_bytes(verifying_key_algorithm, vk.as_bytes()).map_err(|e| { std::io::Error::new( std::io::ErrorKind::InvalidData, - "invalid prover verifying key", + format!("invalid prover verifying key: {}", e), ) }))?; @@ -178,3 +159,15 @@ async fn main() -> std::io::Result<()> { node.start().await.map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e.to_string())) } + +fn validate_algorithm(algorithm: &str) -> Result<&str, std::io::Error> { + if algorithm.is_empty() { + return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, "verifying key algorithm is required")); + } + + if !["ed25519", "secp256k1", "secp256r1"].contains(&algorithm) { + return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, "invalid verifying key algorithm")); + } + + Ok(algorithm) +} diff --git a/crates/keys/src/signing_keys.rs b/crates/keys/src/signing_keys.rs index d7bb74b8..7b3f511b 100644 --- a/crates/keys/src/signing_keys.rs +++ b/crates/keys/src/signing_keys.rs @@ -31,12 +31,12 @@ impl SigningKey { SigningKey::Secp256r1(Secp256r1SigningKey::random(&mut OsRng)) } - pub fn new_with_algorithm(algorithm: &str) -> Self { + pub fn new_with_algorithm(algorithm: &str) -> Result { match algorithm { - "ed25519" => SigningKey::Ed25519(Box::new(Ed25519SigningKey::new(OsRng))), - "secp256k1" => SigningKey::Secp256k1(Secp256k1SigningKey::new(&mut OsRng)), - "secp256r1" => SigningKey::Secp256r1(Secp256r1SigningKey::random(&mut OsRng)), - _ => panic!("Unexpected key algorithm for SigningKey: {}", algorithm), + "ed25519" => Ok(SigningKey::Ed25519(Box::new(Ed25519SigningKey::new(OsRng)))), + "secp256k1" => Ok(SigningKey::Secp256k1(Secp256k1SigningKey::new(&mut OsRng))), + "secp256r1" => Ok(SigningKey::Secp256r1(Secp256r1SigningKey::random(&mut OsRng))), + _ => bail!("Unexpected key algorithm for SigningKey: '{}'. Expected one of: ed25519, secp256k1, secp256r1", algorithm), } } diff --git a/crates/node_types/prover/src/prover/mod.rs b/crates/node_types/prover/src/prover/mod.rs index a7a5b7c4..eb2fabf6 100644 --- a/crates/node_types/prover/src/prover/mod.rs +++ b/crates/node_types/prover/src/prover/mod.rs @@ -282,10 +282,9 @@ impl Prover { } // TODO: Issue #144 - match epoch.verify_signature(self.cfg.verifying_key.clone()) { - Ok(_) => trace!("valid signature for epoch {}", epoch.height), - Err(e) => panic!("invalid signature in epoch {}: {:?}", epoch.height, e), - } + epoch.verify_signature(&self.cfg.verifying_key) + .with_context(|| format!("Invalid signature in epoch {}", epoch.height))?; + trace!("valid signature for epoch {}", epoch.height); let prev_commitment = self.db.get_commitment(¤t_epoch)?; diff --git a/crates/tests/src/lib.rs b/crates/tests/src/lib.rs index 595fb937..e82f222e 100644 --- a/crates/tests/src/lib.rs +++ b/crates/tests/src/lib.rs @@ -47,7 +47,7 @@ async fn test_light_client_prover_talking() -> Result<()> { let bridge_da_layer = Arc::new(CelestiaConnection::new(&bridge_cfg, None).await.unwrap()); let lc_da_layer = Arc::new(CelestiaConnection::new(&lc_cfg, None).await.unwrap()); let db = setup_db(); - let signing_key = SigningKey::new_with_algorithm(algorithm); + let signing_key = SigningKey::new_with_algorithm(algorithm)?; let pubkey = signing_key.verifying_key(); let prover_cfg = prism_prover::Config { @@ -80,7 +80,7 @@ async fn test_light_client_prover_talking() -> Result<()> { let mut transaction_builder = TransactionBuilder::new(); let register_service_req = - transaction_builder.register_service_with_random_keys(algorithm, "test_service").commit(); + transaction_builder.register_service_with_random_keys(algorithm, "test_service")?.commit(); let mut i = 0; @@ -93,7 +93,7 @@ async fn test_light_client_prover_talking() -> Result<()> { for _ in 0..num_new_accounts { let random_user_id = format!("{}@gmail.com", i); let new_acc = transaction_builder - .create_account_with_random_key_signed(algorithm, random_user_id.as_str(), "test_service") + .create_account_with_random_key_signed(algorithm, random_user_id.as_str(), "test_service")? .commit(); match prover.clone().validate_and_queue_update(new_acc).await { Ok(_) => { @@ -112,7 +112,7 @@ async fn test_light_client_prover_talking() -> Result<()> { .map_or("Could not find random account id", |id| id.as_str()); let update_acc = - transaction_builder.add_random_key_verified_with_root(algorithm, acc_id).commit(); + transaction_builder.add_random_key_verified_with_root(algorithm, acc_id)?.commit(); match prover.clone().validate_and_queue_update(update_acc).await { Ok(_) => (),