From c0034ec26f56f653e6a978f721da11bf61cb7bce Mon Sep 17 00:00:00 2001 From: Alex Su Date: Wed, 20 Jul 2022 15:17:00 +1000 Subject: [PATCH 1/2] custom error enums for state and token methods --- fil_fungible_token/src/token/errors.rs | 71 --------------- fil_fungible_token/src/token/mod.rs | 48 +++++++--- fil_fungible_token/src/token/state.rs | 120 ++++++++++++------------- 3 files changed, 94 insertions(+), 145 deletions(-) delete mode 100644 fil_fungible_token/src/token/errors.rs diff --git a/fil_fungible_token/src/token/errors.rs b/fil_fungible_token/src/token/errors.rs deleted file mode 100644 index c74b6f09..00000000 --- a/fil_fungible_token/src/token/errors.rs +++ /dev/null @@ -1,71 +0,0 @@ -use std::{error::Error, fmt::Display}; - -use fvm_ipld_hamt::Error as HamtError; -use fvm_shared::address::Address; - -#[derive(Debug)] -pub enum RuntimeError { - AddrNotFound(Address), -} - -impl Display for RuntimeError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - RuntimeError::AddrNotFound(_) => write!(f, "Address not found"), - } - } -} - -impl Error for RuntimeError {} - -#[derive(Debug)] -pub enum StateError {} - -impl Display for StateError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "State error") - } -} - -impl Error for StateError {} - -#[derive(Debug)] -pub enum ActorError { - AddrNotFound(Address), - Arithmetic(String), - IpldState(StateError), - IpldHamt(HamtError), - RuntimeError(RuntimeError), -} - -impl Display for ActorError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - ActorError::AddrNotFound(e) => write!(f, "{}", e), - ActorError::Arithmetic(e) => write!(f, "{}", e), - ActorError::IpldState(e) => write!(f, "{}", e), - ActorError::IpldHamt(e) => write!(f, "{}", e), - ActorError::RuntimeError(e) => write!(f, "{}", e), - } - } -} - -impl Error for ActorError {} - -impl From for ActorError { - fn from(e: StateError) -> Self { - Self::IpldState(e) - } -} - -impl From for ActorError { - fn from(e: HamtError) -> Self { - Self::IpldHamt(e) - } -} - -impl From for ActorError { - fn from(e: RuntimeError) -> Self { - ActorError::RuntimeError(e) - } -} diff --git a/fil_fungible_token/src/token/mod.rs b/fil_fungible_token/src/token/mod.rs index 6091fc30..c4f11c9b 100644 --- a/fil_fungible_token/src/token/mod.rs +++ b/fil_fungible_token/src/token/mod.rs @@ -1,20 +1,27 @@ -pub mod errors; pub mod receiver; mod state; mod types; -use std::ops::Neg; -use self::state::TokenState; +use self::state::{StateError, TokenState}; pub use self::types::*; -use anyhow::bail; -use anyhow::Ok; -use anyhow::Result; use cid::Cid; use fvm_ipld_blockstore::Blockstore as IpldStore; use fvm_shared::econ::TokenAmount; use fvm_shared::ActorID; use num_traits::Signed; +use std::ops::Neg; +use thiserror::Error; + +#[derive(Error, Debug)] +pub enum TokenError { + #[error("error in underlying state {0}")] + State(#[from] StateError), + #[error("invalid negative: {0}")] + InvalidNegative(String), +} + +type Result = std::result::Result; /// Library functions that implement core FRC-??? standards /// @@ -44,7 +51,7 @@ where /// Constructs the token state tree and saves it at a CID pub fn init_state(&self) -> Result { let init_state = TokenState::new(&self.bs)?; - init_state.save(&self.bs) + Ok(init_state.save(&self.bs)?) } /// Helper function that loads the root of the state tree related to token-accounting @@ -60,7 +67,10 @@ where /// The mint amount must be non-negative or the method returns an error pub fn mint(&self, initial_holder: ActorID, value: TokenAmount) -> Result<()> { if value.is_negative() { - bail!("value of mint was negative {}", value); + return Err(TokenError::InvalidNegative(format!( + "mint amount {} cannot be negative", + value + ))); } // Increase the balance of the actor and increase total supply @@ -92,7 +102,7 @@ where pub fn balance_of(&self, holder: ActorID) -> Result { // Load the HAMT holding balances let state = self.load_state(); - state.get_balance(&self.bs, holder) + Ok(state.get_balance(&self.bs, holder)?) } /// Gets the allowance between owner and spender @@ -116,7 +126,10 @@ where delta: TokenAmount, ) -> Result { if delta.is_negative() { - bail!("value of delta was negative {}", delta); + return Err(TokenError::InvalidNegative(format!( + "increase allowance delta {} cannot be negative", + delta + ))); } let mut state = self.load_state(); @@ -138,7 +151,10 @@ where delta: TokenAmount, ) -> Result { if delta.is_negative() { - bail!("value of delta was negative {}", delta); + return Err(TokenError::InvalidNegative(format!( + "decrease allowance delta {} cannot be negative", + delta + ))); } let mut state = self.load_state(); @@ -186,7 +202,10 @@ where value: TokenAmount, ) -> Result { if value.is_negative() { - bail!("cannot burn a negative amount"); + return Err(TokenError::InvalidNegative(format!( + "burn amount {} cannot be negative", + value + ))); } let mut state = self.load_state(); @@ -235,7 +254,10 @@ where value: TokenAmount, ) -> Result<()> { if value.is_negative() { - bail!("cannot transfer a negative amount"); + return Err(TokenError::InvalidNegative(format!( + "transfer amount {} cannot be negative", + value + ))); } let mut state = self.load_state(); diff --git a/fil_fungible_token/src/token/state.rs b/fil_fungible_token/src/token/state.rs index 6e4c40be..5997c1c0 100644 --- a/fil_fungible_token/src/token/state.rs +++ b/fil_fungible_token/src/token/state.rs @@ -1,15 +1,12 @@ -use anyhow::anyhow; -use anyhow::bail; -use anyhow::Result; use cid::multihash::Code; use cid::Cid; - use fvm_ipld_blockstore::Block; use fvm_ipld_blockstore::Blockstore as IpldStore; use fvm_ipld_encoding::tuple::*; use fvm_ipld_encoding::Cbor; use fvm_ipld_encoding::CborStore; use fvm_ipld_encoding::DAG_CBOR; +use fvm_ipld_hamt::Error as HamtError; use fvm_ipld_hamt::Hamt; use fvm_shared::bigint::bigint_ser; use fvm_shared::bigint::bigint_ser::BigIntDe; @@ -17,9 +14,39 @@ use fvm_shared::bigint::Zero; use fvm_shared::econ::TokenAmount; use fvm_shared::ActorID; use num_traits::Signed; +use thiserror::Error; const HAMT_BIT_WIDTH: u32 = 5; +#[derive(Error, Debug)] +pub enum StateError { + #[error("ipld hamt error: {0}")] + IpldHamt(#[from] HamtError), + #[error("missing state at cid: {0}")] + MissingState(Cid), + #[error("underlying serialization error: {0}")] + Serialization(String), + #[error( + "negative balance caused by subtracting {delta:?} from {owner:?}'s balance of {balance:?}" + )] + NegativeBalance { + owner: ActorID, + balance: TokenAmount, + delta: TokenAmount, + }, + #[error( + "{spender:?} attempted to utilise {delta:?} of allowance {allowance:?} set by {owner:?}" + )] + InsufficentAllowance { + owner: ActorID, + spender: ActorID, + allowance: TokenAmount, + delta: TokenAmount, + }, +} + +type Result = std::result::Result; + /// Token state IPLD structure #[derive(Serialize_tuple, Deserialize_tuple, PartialEq, Clone, Debug)] pub struct TokenState { @@ -37,9 +64,8 @@ pub struct TokenState { /// /// This is a simple wrapper of state and in general does not account for token protocol level /// checks such as ensuring necessary approvals are enforced during transfers. This is left for the -/// caller to handle. However, some invariants such as enforcing non-negative balances, allowances -/// and total supply are enforced. Furthermore, this layer returns errors if any of the underlying -/// arithmetic overflows. +/// caller to handle. However, some invariants such as non-negative balances, allowances and total +/// supply are enforced. /// /// Some methods on TokenState require the caller to pass in a blockstore implementing the Clone /// trait. It is assumed that when cloning the blockstore implementation does a "shallow-clone" @@ -63,8 +89,8 @@ impl TokenState { // Load the actor state from the state tree. match bs.get_cbor::(cid) { Ok(Some(state)) => Ok(state), - Ok(None) => Err(anyhow!("no state at this cid {:?}", cid)), - Err(err) => Err(anyhow!("failed to get state: {}", err)), + Ok(None) => Err(StateError::MissingState(*cid)), + Err(err) => Err(StateError::Serialization(err.to_string())), } } @@ -72,7 +98,7 @@ impl TokenState { pub fn save(&self, bs: &BS) -> Result { let serialized = match fvm_ipld_encoding::to_vec(self) { Ok(s) => s, - Err(err) => return Err(anyhow!("failed to serialize state: {:?}", err)), + Err(err) => return Err(StateError::Serialization(err.to_string())), }; let block = Block { codec: DAG_CBOR, @@ -80,7 +106,7 @@ impl TokenState { }; let cid = match bs.put(Code::Blake2b256, &block) { Ok(cid) => cid, - Err(err) => return Err(anyhow!("failed to store initial state: {:}", err)), + Err(err) => return Err(StateError::Serialization(err.to_string())), }; Ok(cid) } @@ -126,11 +152,11 @@ impl TokenState { // if the new_balance is negative, return an error if new_balance.is_negative() { - bail!( - "resulting balance was negative adding {:?} to {:?}", - delta, - balance.unwrap_or(&BigIntDe(TokenAmount::zero())).0 - ); + return Err(StateError::NegativeBalance { + balance: new_balance, + delta: delta.clone(), + owner, + }); } balance_map.set(owner, BigIntDe(new_balance.clone()))?; @@ -144,30 +170,19 @@ impl TokenState { &self, bs: &BS, ) -> Result> { - match Hamt::::load_with_bit_width( + Ok(Hamt::::load_with_bit_width( &self.balances, (*bs).clone(), HAMT_BIT_WIDTH, - ) { - Ok(map) => Ok(map), - Err(err) => return Err(anyhow!("failed to load balances hamt: {:?}", err)), - } + )?) } /// Increase the total supply by the specified value /// - /// The requested amount must be non-negative. - /// Returns an error if the total supply overflows, else returns the new total supply - pub fn increase_supply(&mut self, value: &TokenAmount) -> Result { - let new_supply = self.supply.checked_add(value).ok_or_else(|| { - anyhow!( - "Overflow when adding {} to the total_supply of {}", - value, - self.supply - ) - })?; - self.supply = new_supply.clone(); - Ok(new_supply) + /// The requested amount must be non-negative. Returns the new total supply + pub fn increase_supply(&mut self, value: &TokenAmount) -> Result<&TokenAmount> { + self.supply += value; + Ok(&self.supply) } /// Get the allowance that an owner has approved for a spender @@ -296,25 +311,17 @@ impl TokenState { return Ok(current_allowance); } - let new_allowance = current_allowance.checked_sub(value).ok_or_else(|| { - anyhow!( - "Overflow when subtracting {} from {}'s allowance of {}", - value, - owner, - current_allowance - ) - })?; - - if new_allowance.is_negative() { - return Err(anyhow!( - "Attempted to use {} of {}'s tokens from {}'s allowance of {}", - value, + if current_allowance.lt(value) { + return Err(StateError::InsufficentAllowance { owner, spender, - current_allowance - )); + allowance: current_allowance, + delta: value.clone(), + }); } + let new_allowance = current_allowance - value; + // TODO: helper function to set a new allowance and flush hamts let owner_allowances = self.get_owner_allowance_map(bs, owner)?; // to reach here, allowance must have been previously non zero; so safe to assume the map exists @@ -353,12 +360,11 @@ impl TokenState { /// /// Gets a HAMT with CIDs linking to other HAMTs fn get_allowances_map(&self, bs: &BS) -> Result> { - Hamt::::load_with_bit_width( + Ok(Hamt::::load_with_bit_width( &self.allowances, (*bs).clone(), HAMT_BIT_WIDTH, - ) - .map_err(|e| anyhow!("failed to load base allowances map {}", e)) + )?) } } @@ -405,13 +411,9 @@ mod test { let actor: ActorID = 1; // can't decrease from zero - let err = state + state .change_balance_by(bs, actor, &BigInt::from(-1)) .unwrap_err(); - assert_eq!( - err.to_string(), - "resulting balance was negative adding -1 to 0" - ); let balance = state.get_balance(bs, actor).unwrap(); assert_eq!(balance, BigInt::zero()); @@ -419,13 +421,9 @@ mod test { state .change_balance_by(bs, actor, &BigInt::from(50)) .unwrap(); - let err = state + state .change_balance_by(bs, actor, &BigInt::from(-100)) .unwrap_err(); - assert_eq!( - err.to_string(), - "resulting balance was negative adding -100 to 50" - ); } #[test] From 9ef84c36e736aabd8043de9d7200c642f578c007 Mon Sep 17 00:00:00 2001 From: Alex Su Date: Thu, 21 Jul 2022 10:59:39 +1000 Subject: [PATCH 2/2] reworded error message to make it more clear --- fil_fungible_token/src/token/state.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/fil_fungible_token/src/token/state.rs b/fil_fungible_token/src/token/state.rs index 5997c1c0..44a5cf61 100644 --- a/fil_fungible_token/src/token/state.rs +++ b/fil_fungible_token/src/token/state.rs @@ -26,9 +26,7 @@ pub enum StateError { MissingState(Cid), #[error("underlying serialization error: {0}")] Serialization(String), - #[error( - "negative balance caused by subtracting {delta:?} from {owner:?}'s balance of {balance:?}" - )] + #[error("negative balance caused by changing {owner:?}'s balance of {balance:?} by {delta:?}")] NegativeBalance { owner: ActorID, balance: TokenAmount,