fix: refactor CertsError

.config/rust-f3.dic

@@ -12,6 +12,7 @@ ChainSafe/M
 CLI
 crypto
 deserialize/D
+enum
 EOF
 Merkle
 MiB

Makefile

@@ -8,7 +8,7 @@ test:
 	cargo test --all-features
 
 doc:
-	cargo doc
+	cargo doc --all-features --no-deps
 	cp ./build/vendored-docs-redirect.index.html target/doc/index.html
 
 bench:

certs/src/error.rs

@@ -1,71 +1,104 @@
 // Copyright 2019-2024 ChainSafe Systems
 // SPDX-License-Identifier: Apache-2.0, MIT
 
-use filecoin_f3_gpbft::{ActorId, CborError, GPBFTError};
+use std::borrow::Cow;
+
+use filecoin_f3_gpbft::{ActorId, CborError, Cid, GPBFTError, Phase};
 use thiserror::Error;
 
 #[derive(Error, Debug, PartialEq)]
 pub enum CertsError {
+    /// Error when the chain is empty.
     #[error("empty chain")]
     EmptyChain,
 
+    /// Error when the power table delta is empty.
     #[error("empty power delta")]
     EmptyPowerDelta,
 
-    #[error("invalid justification: {0}")]
-    InvalidJustification(String),
+    /// Error when the justification vote phase doesn't match the expected phase
+    #[error("invalid justification vote phase: expected {expected}, got {actual}")]
+    InvalidJustificationVotePhase { expected: Phase, actual: Phase },
+
+    /// Error when the justification vote value chain is invalid
+    #[error("invalid justification vote value chain: {0}")]
+    InvalidJustificationVoteValueChain(Cow<'static, str>),
 
+    /// Error when getting a decision for bottom for a instance.
     #[error("got a decision for bottom for instance {0}")]
     BottomDecision(u64),
 
+    /// Error when the power table delta is invalid.
     #[error("invalid power table delta: {0}")]
-    InvalidPowerTableDelta(String),
+    InvalidPowerTableDelta(Cow<'static, str>),
 
+    /// Error when serialization fails.
     #[error("serialization error: {0}")]
-    SerializationError(String),
+    SerializationError(Cow<'static, str>),
 
-    #[error("expected instance {expected}, found instance {found}")]
-    InstanceMismatch { expected: u64, found: u64 },
+    /// Error when the instance doesn't match the expected instance.
+    #[error("expected instance {expected}, found instance {actual}")]
+    InstanceMismatch { expected: u64, actual: u64 },
 
-    #[error("invalid round: expected 0, got {0}")]
-    InvalidRound(u64),
+    /// Error when the round is invalid.
+    #[error("invalid round: expected {expected}, got {actual}")]
+    InvalidRound { expected: u64, actual: u64 },
 
+    /// Error when the diff is not sorted by participant ID.
     #[error("diff {0} not sorted by participant ID")]
     UnsortedDiff(usize),
 
+    /// Error when the power table delta is empty for a participant.
     #[error("diff {0} contains an empty delta for participant {1}")]
     EmptyDelta(usize, ActorId),
 
+    /// Error when the power delta for a participant is non-positive.
     #[error("diff {0} includes a new entry with a non-positive power delta for participant {1}")]
     NonPositivePowerDeltaForNewEntry(usize, ActorId),
 
+    /// Error when the power table delta includes an unchanged key.
     #[error("diff {0} delta for participant {1} includes an unchanged key")]
     UnchangedKey(usize, ActorId),
 
+    /// Error when the new key removes all storage power for a participant.
     #[error("diff {0} removes all power for participant {1} while specifying a new key")]
     RemovesAllPowerWithNewKey(usize, ActorId),
 
+    /// Error when the storage power is negative for a participant.
     #[error("diff {0} resulted in negative power for participant {1}")]
     NegativePower(usize, ActorId),
 
+    /// Error when the finality certificate is invalid for a certain instance.
     #[error("invalid finality certificate at instance {0}: {1}")]
     InvalidFinalityCertificate(u64, GPBFTError),
 
+    /// Error when the finality certificate is empty for a certain instance.
     #[error("empty finality certificate for instance {0}")]
     EmptyFinalityCertificate(u64),
 
+    /// Error that occurs when the power diff from a finality certificate doesn't match the expected value.
+    ///
+    /// # Fields
+    /// * `instance` - The instance identifier for which the power diff mismatch occurred
+    /// * `expected` - The expected power diff CID
+    /// * `actual` - The actual power diff CID that was received
+    ///
+    /// Note: `Box<Cid>` is used instead of `Cid` to reduce the overall size of the error enum
+    /// and suppress "largest variant" warnings.
     #[error("base tipset does not match finalized chain at instance {0}")]
     BaseTipsetMismatch(u64),
 
+    /// Error when the power diff is incorrect.
     #[error(
-        "incorrect power diff from finality certificate for instance {instance}: expected {expected:?}, got {got:?}"
+        "incorrect power diff from finality certificate for instance {instance}: expected {expected}, got {actual}"
     )]
     IncorrectPowerDiff {
         instance: u64,
-        expected: String,
-        got: String,
+        expected: Box<Cid>,
+        actual: Box<Cid>,
     },
 
+    /// Error when encoding fails.
     #[error("cbor encoding error")]
     EncodingError(#[from] CborError),
 }

certs/src/lib.rs

@@ -94,14 +94,18 @@ impl FinalityCertificate {
     /// # Returns
     /// A Result containing the new `FinalityCertificate` if successful
     pub fn new(power_delta: PowerTableDiff, justification: &Justification) -> Result<Self> {
-        if justification.vote.step != Phase::Decide {
-            return Err(CertsError::InvalidJustification(
-                justification.vote.step.to_string(),
-            ));
+        if justification.vote.phase != Phase::Decide {
+            return Err(CertsError::InvalidJustificationVotePhase {
+                expected: Phase::Decide,
+                actual: justification.vote.phase,
+            });
         }
 
         if justification.vote.round != 0 {
-            return Err(CertsError::InvalidRound(justification.vote.round));
+            return Err(CertsError::InvalidRound {
+                expected: 0,
+                actual: justification.vote.round,
+            });
         }
 
         if justification.vote.value.is_empty() {
@@ -295,7 +299,7 @@ pub fn validate_finality_certificates<'a>(
         if cert.gpbft_instance != next_instance {
             return Err(CertsError::InstanceMismatch {
                 expected: next_instance,
-                found: cert.gpbft_instance,
+                actual: cert.gpbft_instance,
             });
         }
 
@@ -326,8 +330,8 @@ pub fn validate_finality_certificates<'a>(
         if cert.supplemental_data.power_table != power_table_cid {
             return Err(CertsError::IncorrectPowerDiff {
                 instance: cert.gpbft_instance,
-                expected: cert.supplemental_data.power_table.to_string(),
-                got: power_table_cid.to_string(),
+                expected: power_table_cid.into(),
+                actual: cert.supplemental_data.power_table.into(),
             });
         }
 
@@ -365,7 +369,7 @@ mod tests {
     use filecoin_f3_gpbft::{Cid, Payload};
     use std::str::FromStr;
 
-    fn create_mock_justification(step: Phase, cid: &str) -> Justification {
+    fn create_mock_justification(phase: Phase, cid: &str) -> Justification {
         let base_tipset = Tipset {
             epoch: 1,
             key: vec![1, 2, 3],
@@ -386,7 +390,7 @@ mod tests {
             vote: Payload {
                 instance: 1,
                 round: 0,
-                step,
+                phase,
                 supplemental_data: SupplementalData {
                     commitments: keccak_hash::H256::zero(),
                     power_table: Cid::from_str(cid).unwrap(),
@@ -447,7 +451,10 @@ mod tests {
         assert!(result.is_err());
         assert_eq!(
             result.unwrap_err(),
-            CertsError::InvalidJustification(Phase::Commit.to_string())
+            CertsError::InvalidJustificationVotePhase {
+                expected: Phase::Decide,
+                actual: Phase::Commit
+            }
         );
     }
 
@@ -460,7 +467,13 @@ mod tests {
 
         let result = FinalityCertificate::new(power_delta, &justification);
         assert!(result.is_err());
-        assert_eq!(result.unwrap_err(), CertsError::InvalidRound(1));
+        assert_eq!(
+            result.unwrap_err(),
+            CertsError::InvalidRound {
+                expected: 0,
+                actual: 1
+            }
+        );
     }
 
     // It makes no sense that ECChain can be empty. Perhaps this warrants a discussion.

gpbft/src/lib.rs

@@ -36,7 +36,7 @@ pub use fvm_ipld_bitfield::BitField;
 pub use fvm_ipld_encoding::{Error as CborError, to_vec as to_vec_cbor};
 pub use num_bigint::{BigInt, Sign};
 pub use num_traits::Zero;
-use strum_macros::Display;
+use strum_macros::{Display, IntoStaticStr};
 
 pub use crate::chain::{Cid, ECChain, cid_from_bytes};
 pub use types::{ActorId, NetworkName, PubKey, StoragePower};
@@ -66,7 +66,7 @@ pub struct SupplementalData {
 
 /// Represents the different phases of the GPBFT consensus protocol
 #[repr(u8)]
-#[derive(Display, Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+#[derive(Display, IntoStaticStr, Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
 #[strum(serialize_all = "UPPERCASE")]
 pub enum Phase {
     /// This phase marks the beginning of a new consensus round. During this phase,
@@ -95,7 +95,7 @@ pub struct Payload {
     /// GPBFT round number
     pub round: u64,
     /// Current phase of the GPBFT protocol
-    pub step: Phase,
+    pub phase: Phase,
     /// Additional data related to this consensus instance
     pub supplemental_data: SupplementalData,
     /// The agreed-upon value for this instance
@@ -115,14 +115,14 @@ impl Payload {
     pub fn new(
         instance: u64,
         round: u64,
-        step: Phase,
+        phase: Phase,
         supplemental_data: SupplementalData,
         value: ECChain,
     ) -> Self {
         Payload {
             instance,
             round,
-            step,
+            phase,
             supplemental_data,
             value,
         }
@@ -147,7 +147,7 @@ mod tests {
     fn test_payload_new() {
         let instance = 1;
         let round = 2;
-        let step = Phase::Commit;
+        let phase = Phase::Commit;
         let supplemental_data = SupplementalData {
             commitments: keccak_hash::H256::zero(),
             power_table: Cid::default(),
@@ -157,14 +157,14 @@ mod tests {
         let payload = Payload::new(
             instance,
             round,
-            step,
+            phase,
             supplemental_data.clone(),
             value.clone(),
         );
 
         assert_eq!(payload.instance, instance);
         assert_eq!(payload.round, round);
-        assert_eq!(payload.step, step);
+        assert_eq!(payload.phase, phase);
         assert_eq!(payload.supplemental_data, supplemental_data);
         assert_eq!(payload.value, value);
     }