doc, refactor(apps/hermes/server): add code guidelines, enable extra clippy lints

README.md

@@ -56,6 +56,8 @@ Use the [Conventional Commits](https://www.conventionalcommits.org) format for y
 In the PR description, please include a summary of the changes and any relevant context. Also, please make sure
 to update the package versions following the [Semantic Versioning](https://semver.org/) rules.
 
+See also: [Code guidelines](doc/code-guidelines.md)
+
 ### Releases
 
 The repository has several CI workflows that automatically release new versions of the various components when a new Github release is published.

apps/hermes/server/Cargo.toml

@@ -74,3 +74,46 @@ panic                  = 'abort'
 
 [profile.dev]
 panic                  = 'abort'
+
+
+[lints.rust]
+unsafe_code = "deny"
+
+[lints.clippy]
+# See [Rust code guidelines](../../doc/rust-code-guidelines.md)
+
+wildcard_dependencies = "deny"
+
+collapsible_if = "allow"
+collapsible_else_if = "allow"
+
+allow_attributes_without_reason = "warn"
+
+# Panics
+expect_used = "warn"
+fallible_impl_from = "warn"
+indexing_slicing = "warn"
+panic = "warn"
+panic_in_result_fn = "warn"
+string_slice = "warn"
+todo = "warn"
+unchecked_duration_subtraction = "warn"
+unreachable = "warn"
+unwrap_in_result = "warn"
+unwrap_used = "warn"
+
+# Correctness
+cast_lossless = "warn"
+cast_possible_truncation = "warn"
+cast_possible_wrap = "warn"
+cast_sign_loss = "warn"
+collection_is_never_read = "warn"
+match_wild_err_arm = "warn"
+path_buf_push_overwrite = "warn"
+read_zero_byte_vec = "warn"
+same_name_method = "warn"
+suspicious_operation_groupings = "warn"
+suspicious_xor_used_as_pow = "warn"
+unused_self = "warn"
+used_underscore_binding = "warn"
+while_float = "warn"

apps/hermes/server/build.rs

@@ -13,6 +13,7 @@ fn main() {
     // Build the wormhole and google protobufs using Rust's prost_build crate.
     // The generated Rust code is placed in the OUT_DIR (env var set by cargo).
     // `network/wormhole.rs` then includes the generated code into the source while compilation is happening.
+    #[allow(clippy::expect_used, reason = "failing at build time is fine")]
     tonic_build::configure()
         .build_server(false)
         .compile(

apps/hermes/server/src/api.rs

@@ -140,7 +140,7 @@ where
     // Initialize Axum Router. Note the type here is a `Router<State>` due to the use of the
     // `with_state` method which replaces `Body` with `State` in the type signature.
     let app = Router::new();
-    #[allow(deprecated)]
+    #[allow(deprecated, reason = "serving deprecated API endpoints")]
     let app = app
         .merge(SwaggerUi::new("/docs").url("/docs/openapi.json", ApiDoc::openapi()))
         .route("/", get(rest::index))

apps/hermes/server/src/api/rest.rs

@@ -119,6 +119,7 @@ where
     }
 }
 #[cfg(test)]
+#[allow(clippy::unwrap_used, reason = "tests")]
 mod tests {
     use {
         super::*,

apps/hermes/server/src/api/rest/get_vaa_ccip.rs

@@ -51,15 +51,16 @@ pub async fn get_vaa_ccip<S>(
 where
     S: Aggregates,
 {
+    let data: [u8; 40] = *params.data;
     let price_id: PriceIdentifier = PriceIdentifier::new(
-        params.data[0..32]
+        data[0..32]
             .try_into()
             .map_err(|_| RestError::InvalidCCIPInput)?,
     );
     validate_price_ids(&state, &[price_id], false).await?;
 
     let publish_time = UnixTimestamp::from_be_bytes(
-        params.data[32..40]
+        data[32..40]
             .try_into()
             .map_err(|_| RestError::InvalidCCIPInput)?,
     );

apps/hermes/server/src/api/types.rs

@@ -396,6 +396,7 @@ impl Display for AssetType {
 }
 
 #[cfg(test)]
+#[allow(clippy::unwrap_used, reason = "tests")]
 mod tests {
     use super::*;
 

apps/hermes/server/src/config.rs

@@ -14,7 +14,10 @@ mod wormhole;
 #[command(author = crate_authors!())]
 #[command(about = crate_description!())]
 #[command(version = crate_version!())]
-#[allow(clippy::large_enum_variant)]
+#[allow(
+    clippy::large_enum_variant,
+    reason = "performance is not a concern for config"
+)]
 pub enum Options {
     /// Run the Hermes Price Service.
     Run(RunOptions),

apps/hermes/server/src/config/cache.rs

@@ -14,5 +14,5 @@ pub struct Options {
     #[arg(long = "cache-size-slots")]
     #[arg(env = "CACHE_SIZE_SLOTS")]
     #[arg(default_value = "1600")]
-    pub size_slots: u64,
+    pub size_slots: usize,
 }

apps/hermes/server/src/main.rs

@@ -1,5 +1,5 @@
 use {
-    anyhow::Result,
+    anyhow::{Context, Result},
     clap::{CommandFactory, Parser},
     futures::future::join_all,
     lazy_static::lazy_static,
@@ -53,9 +53,13 @@ async fn init() -> Result<()> {
             // Listen for Ctrl+C so we can set the exit flag and wait for a graceful shutdown.
             spawn(async move {
                 tracing::info!("Registered shutdown signal handler...");
-                tokio::signal::ctrl_c().await.unwrap();
-                tracing::info!("Shut down signal received, waiting for tasks...");
-                let _ = EXIT.send(true);
+                match tokio::signal::ctrl_c().await {
+                    Ok(()) => {
+                        tracing::info!("Shut down signal received, waiting for tasks...");
+                        let _ = EXIT.send(true);
+                    }
+                    Err(err) => tracing::warn!("failed to register shutdown signal handler: {err}"),
+                }
             });
 
             // Spawn all worker tasks, and wait for all to complete (which will happen if a shutdown
@@ -83,8 +87,8 @@ async fn init() -> Result<()> {
                         let defaults = arg
                             .get_default_values()
                             .iter()
-                            .map(|v| v.to_str().unwrap())
-                            .collect::<Vec<_>>()
+                            .map(|v| v.to_str().context("non-utf8 default arg value"))
+                            .collect::<Result<Vec<_>>>()?
                             .join(",");
 
                         println!(

apps/hermes/server/src/network/wormhole.rs

@@ -43,7 +43,10 @@ impl std::fmt::Display for GuardianSet {
 
 /// BridgeData extracted from wormhole bridge account, due to no API.
 #[derive(borsh::BorshDeserialize)]
-#[allow(dead_code)]
+#[allow(
+    dead_code,
+    reason = "we have to deserialize all fields but we don't use all of them"
+)]
 pub struct BridgeData {
     pub guardian_set_index: u32,
     pub last_lamports: u64,
@@ -52,7 +55,10 @@ pub struct BridgeData {
 
 /// BridgeConfig extracted from wormhole bridge account, due to no API.
 #[derive(borsh::BorshDeserialize)]
-#[allow(dead_code)]
+#[allow(
+    dead_code,
+    reason = "we have to deserialize all fields but we don't use all of them"
+)]
 pub struct BridgeConfig {
     pub guardian_set_expiration_time: u32,
     pub fee: u64,
@@ -75,7 +81,11 @@ pub struct GuardianSetData {
 ///
 /// The following module structure must match the protobuf definitions, so that the generated code
 /// can correctly reference modules from each other.
-#[allow(clippy::enum_variant_names)]
+#[allow(
+    clippy::enum_variant_names,
+    clippy::allow_attributes_without_reason,
+    reason = "generated code"
+)]
 mod proto {
     pub mod node {
         pub mod v1 {

apps/hermes/server/src/serde.rs

@@ -17,13 +17,16 @@ pub mod hex {
         R: FromHex,
         <R as hex::FromHex>::Error: std::fmt::Display,
     {
-        let s: String = Deserialize::deserialize(d)?;
-        let p = s.starts_with("0x") || s.starts_with("0X");
-        let s = if p { &s[2..] } else { &s[..] };
-        hex::serde::deserialize(s.into_deserializer())
+        let full: String = Deserialize::deserialize(d)?;
+        let hex = full
+            .strip_prefix("0x")
+            .or_else(|| full.strip_prefix("0X"))
+            .unwrap_or(&full);
+        hex::serde::deserialize(hex.into_deserializer())
     }
 
     #[cfg(test)]
+    #[allow(clippy::unwrap_used, reason = "tests")]
     mod tests {
         use serde::Deserialize;
 

apps/hermes/server/src/state.rs

@@ -56,7 +56,7 @@ struct State {
 
 pub fn new(
     update_tx: Sender<AggregationEvent>,
-    cache_size: u64,
+    cache_size: usize,
     benchmarks_endpoint: Option<Url>,
     readiness_staleness_threshold: Duration,
     readiness_max_allowed_slot_lag: Slot,
@@ -87,7 +87,7 @@ pub mod test {
     };
 
     pub async fn setup_state(
-        cache_size: u64,
+        cache_size: usize,
     ) -> (Arc<impl Aggregates>, Receiver<AggregationEvent>) {
         let (update_tx, update_rx) = tokio::sync::broadcast::channel(1000);
         let state = super::new(update_tx, cache_size, None, Duration::from_secs(30), 10);

apps/hermes/server/src/state/aggregate.rs

@@ -1,3 +1,4 @@
+use anyhow::Context;
 use log::warn;
 #[cfg(test)]
 use mock_instant::{SystemTime, UNIX_EPOCH};
@@ -164,8 +165,9 @@ pub struct AccumulatorMessages {
 }
 
 impl AccumulatorMessages {
+    #[allow(clippy::cast_possible_truncation, reason = "intended truncation")]
     pub fn ring_index(&self) -> u32 {
-        (self.slot % self.ring_size as u64) as u32
+        (self.slot % u64::from(self.ring_size)) as u32
     }
 }
 
@@ -492,7 +494,12 @@ where
         let state_data = self.into().data.read().await;
         let price_feeds_metadata = PriceFeedMeta::retrieve_price_feeds_metadata(self)
             .await
-            .unwrap();
+            .unwrap_or_else(|err| {
+                tracing::error!(
+                    "unexpected failure of PriceFeedMeta::retrieve_price_feeds_metadata(self): {err}"
+                );
+                Vec::new()
+            });
 
         let current_time = SystemTime::now();
 
@@ -529,8 +536,8 @@ where
                 latest_completed_unix_timestamp: state_data.latest_completed_update_time.and_then(
                     |t| {
                         t.duration_since(UNIX_EPOCH)
-                            .map(|d| d.as_secs() as i64)
                             .ok()
+                            .and_then(|d| d.as_secs().try_into().ok())
                     },
                 ),
                 price_feeds_metadata_len: price_feeds_metadata.len(),
@@ -547,7 +554,11 @@ fn build_message_states(
     let wormhole_merkle_message_states_proofs =
         construct_message_states_proofs(&accumulator_messages, &wormhole_merkle_state)?;
 
-    let current_time: UnixTimestamp = SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs() as _;
+    let current_time: UnixTimestamp = SystemTime::now()
+        .duration_since(UNIX_EPOCH)?
+        .as_secs()
+        .try_into()
+        .context("timestamp overflow")?;
 
     accumulator_messages
         .raw_messages
@@ -663,7 +674,12 @@ where
         ));
     } else {
         // Use the publish time from the first end message
-        end_messages[0].message.publish_time() - window_seconds as i64
+        end_messages
+            .first()
+            .context("no messages found")?
+            .message
+            .publish_time()
+            - i64::try_from(window_seconds).context("window size overflow")?
     };
     let start_time = RequestTime::FirstAfter(start_timestamp);
 
@@ -807,6 +823,12 @@ fn calculate_twap(start_message: &TwapMessage, end_message: &TwapMessage) -> Res
 }
 
 #[cfg(test)]
+#[allow(
+    clippy::unwrap_used,
+    clippy::indexing_slicing,
+    clippy::cast_possible_wrap,
+    reason = "tests"
+)]
 mod test {
     use {
         super::*,
@@ -892,11 +914,11 @@ mod test {
     ) -> PriceFeedMessage {
         PriceFeedMessage {
             feed_id: [seed; 32],
-            price: seed as _,
-            conf: seed as _,
+            price: seed.into(),
+            conf: seed.into(),
             exponent: 0,
-            ema_conf: seed as _,
-            ema_price: seed as _,
+            ema_conf: seed.into(),
+            ema_price: seed.into(),
             publish_time,
             prev_publish_time,
         }
@@ -1239,7 +1261,7 @@ mod test {
                         PriceIdentifier::new([100; 32]),
                         PriceIdentifier::new([200; 32])
                     ],
-                    RequestTime::FirstAfter(slot as i64),
+                    RequestTime::FirstAfter(slot.into()),
                 )
                 .await
                 .is_err());
@@ -1631,6 +1653,7 @@ mod test {
     }
 }
 #[cfg(test)]
+#[allow(clippy::unwrap_used, reason = "tests")]
 /// Unit tests for the core TWAP calculation logic in `calculate_twap`
 mod calculate_twap_unit_tests {
     use super::*;

apps/hermes/server/src/state/aggregate/metrics.rs

@@ -106,7 +106,12 @@ impl Metrics {
 
         // Clear out old slots
         while self.first_observed_time_of_slot.len() > MAX_SLOT_OBSERVATIONS {
-            let oldest_slot = *self.first_observed_time_of_slot.keys().next().unwrap();
+            #[allow(clippy::expect_used, reason = "len checked above")]
+            let oldest_slot = *self
+                .first_observed_time_of_slot
+                .keys()
+                .next()
+                .expect("first_observed_time_of_slot is empty");
             self.first_observed_time_of_slot.remove(&oldest_slot);
         }
     }

apps/hermes/server/src/state/aggregate/wormhole_merkle.rs

@@ -142,6 +142,13 @@ pub fn construct_update_data(mut messages: Vec<RawMessageWithMerkleProof>) -> Re
 }
 
 #[cfg(test)]
+#[allow(
+    clippy::unwrap_used,
+    clippy::cast_possible_wrap,
+    clippy::panic,
+    clippy::indexing_slicing,
+    reason = "tests"
+)]
 mod test {
     use {
         super::*,
@@ -175,6 +182,7 @@ mod test {
     }
 
     #[test]
+
     fn test_construct_update_data_works_on_mixed_slot_and_big_size() {
         let mut messages = vec![];