fix: refactor CertsError

Author: hanabi1224

certs/src/error.rs

@@ -1,7 +1,9 @@
 // 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)]
@@ -12,23 +14,26 @@ pub enum CertsError {
     #[error("empty power delta")]
     EmptyPowerDelta,
 
-    #[error("invalid justification: {0}")]
-    InvalidJustification(String),
+    #[error("invalid justification vote phase: expected {expected}, got {actual}")]
+    InvalidJustificationVotePhase { expected: Phase, actual: Phase },
+
+    #[error("invalid justification vote value chain: {0}")]
+    InvalidJustificationVoteValueChain(Cow<'static, str>),
 
     #[error("got a decision for bottom for instance {0}")]
     BottomDecision(u64),
 
     #[error("invalid power table delta: {0}")]
-    InvalidPowerTableDelta(String),
+    InvalidPowerTableDelta(Cow<'static, str>),
 
     #[error("serialization error: {0}")]
-    SerializationError(String),
+    SerializationError(Cow<'static, str>),
 
-    #[error("expected instance {expected}, found instance {found}")]
-    InstanceMismatch { expected: u64, found: u64 },
+    #[error("expected instance {expected}, found instance {actual}")]
+    InstanceMismatch { expected: u64, actual: u64 },
 
-    #[error("invalid round: expected 0, got {0}")]
-    InvalidRound(u64),
+    #[error("invalid round: expected {expected}, got {actual}")]
+    InvalidRound { expected: u64, actual: u64 },
 
     #[error("diff {0} not sorted by participant ID")]
     UnsortedDiff(usize),
@@ -58,12 +63,12 @@ pub enum CertsError {
     BaseTipsetMismatch(u64),
 
     #[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>, // use `Box` to suppress largest variant warning
+        actual: Box<Cid>,
     },
 
     #[error("cbor encoding error")]

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: cert.supplemental_data.power_table.into(),
+                actual: power_table_cid.into(),
             });
         }
 
@@ -386,7 +390,7 @@ mod tests {
             vote: Payload {
                 instance: 1,
                 round: 0,
-                step,
+                phase: step,
                 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
@@ -122,7 +122,7 @@ impl Payload {
         Payload {
             instance,
             round,
-            step,
+            phase: step,
             supplemental_data,
             value,
         }
@@ -164,7 +164,7 @@ mod tests {
 
         assert_eq!(payload.instance, instance);
         assert_eq!(payload.round, round);
-        assert_eq!(payload.step, step);
+        assert_eq!(payload.phase, step);
         assert_eq!(payload.supplemental_data, supplemental_data);
         assert_eq!(payload.value, value);
     }