diff --git a/Cargo.lock b/Cargo.lock index 20e16e766..cf43fa0e8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10468,6 +10468,7 @@ dependencies = [ "sp-io 38.0.0 (git+https://github.com/paritytech/polkadot-sdk?tag=polkadot-stable2409-2)", "sp-runtime 39.0.1", "sp-std 14.0.0 (git+https://github.com/paritytech/polkadot-sdk?tag=polkadot-stable2409-2)", + "thiserror 2.0.3", ] [[package]] @@ -14878,11 +14879,14 @@ version = "0.1.0" dependencies = [ "cid 0.11.1", "filecoin-proofs", + "parity-scale-codec", "primitives-proofs", "rand", + "scale-info", "sealed", "serde", "sha2 0.10.8", + "thiserror 2.0.3", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index d178f58a7..89f4796b7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -106,7 +106,7 @@ subxt = { version = "0.38.0" } subxt-signer = "0.38.0" syn = { version = "2.0.53" } tempfile = "3.10.1" -thiserror = { version = "2.0.3" } +thiserror = { version = "2.0.3", default-features = false } tokio = "1.37.0" tokio-stream = "0.1.15" tokio-util = "0.7.11" diff --git a/cli/artifacts/metadata.scale b/cli/artifacts/metadata.scale index ac54d2e16..39038327f 100644 Binary files a/cli/artifacts/metadata.scale and b/cli/artifacts/metadata.scale differ diff --git a/cli/polka-storage-provider/client/src/commands/proofs.rs b/cli/polka-storage-provider/client/src/commands/proofs.rs index b7ab30e6f..ab8525176 100644 --- a/cli/polka-storage-provider/client/src/commands/proofs.rs +++ b/cli/polka-storage-provider/client/src/commands/proofs.rs @@ -14,7 +14,7 @@ use polka_storage_proofs::{ use polka_storage_provider_common::commp::{calculate_piece_commitment, CommPError}; use primitives_commitment::{ piece::{PaddedPieceSize, PieceInfo}, - Commitment, + Commitment, CommitmentError, }; use primitives_proofs::{derive_prover_id, RegisteredPoStProof, RegisteredSealProof}; use storagext::multipair::{MultiPairArgs, MultiPairSigner}; @@ -444,8 +444,8 @@ pub enum UtilsCommandError { InvalidPieceFile(PathBuf, std::io::Error), #[error("provided invalid CommP {0}, error: {1}")] InvalidPieceCommP(String, cid::Error), - #[error("invalid piece type")] - InvalidPieceType(String, &'static str), + #[error("invalid piece type, error: {1}")] + InvalidPieceType(String, CommitmentError), #[error("file {0} is invalid CARv2 file {1}")] InvalidCARv2(PathBuf, mater::Error), #[error("no signer key was provider")] diff --git a/cli/polka-storage/storagext/Cargo.toml b/cli/polka-storage/storagext/Cargo.toml index 24f6414c0..0b6b1a554 100644 --- a/cli/polka-storage/storagext/Cargo.toml +++ b/cli/polka-storage/storagext/Cargo.toml @@ -27,7 +27,7 @@ serde_json = { workspace = true } sha2 = { workspace = true } subxt = { workspace = true, features = ["jsonrpsee", "substrate-compat"] } subxt-signer = { workspace = true, features = ["subxt"] } -thiserror = { workspace = true } +thiserror = { workspace = true, default-features = true } tokio = { workspace = true, features = ["macros", "rt-multi-thread"] } tracing = { workspace = true } tracing-subscriber = { workspace = true, features = ["env-filter"] } diff --git a/cli/polka-storage/storagext/src/runtime/mod.rs b/cli/polka-storage/storagext/src/runtime/mod.rs index e73651eb1..d759fc651 100644 --- a/cli/polka-storage/storagext/src/runtime/mod.rs +++ b/cli/polka-storage/storagext/src/runtime/mod.rs @@ -77,7 +77,7 @@ pub mod display; derive = "::serde::Serialize" ), derive_for_type( - path = "pallet_market::pallet::DealSettlementError", + path = "pallet_market::error::DealSettlementError", derive = "::serde::Serialize" ), derive_for_type( diff --git a/docs/src/architecture/pallets/market.md b/docs/src/architecture/pallets/market.md index 504db7ad5..33997aafa 100644 --- a/docs/src/architecture/pallets/market.md +++ b/docs/src/architecture/pallets/market.md @@ -211,7 +211,6 @@ The Market Pallet actions can fail with following errors: - `InsufficientFreeFunds` - Market participants do not have enough free funds. - `NoProposalsToBePublished` - `publish_storage_deals` was called with an empty list of `deals`. - `ProposalsPublishedByIncorrectStorageProvider` - Is returned when calling `publish_storage_deals` and the deals in a list are not published by the same storage provider. -- `AllProposalsInvalid` - `publish_storage_deals` call was supplied with a list of `deals` which are all invalid. - `DuplicateDeal` - There is more than one deal with this ID in the Sector. - `DealNotFound` - Tried to activate a deal that is not in the system. - `DealActivationError` - Tried to activate a deal, but data was malformed. @@ -222,6 +221,23 @@ The Market Pallet actions can fail with following errors: - Deal is not pending. - `DealsTooLargeToFitIntoSector` - Sum of all deals piece sizes for a sector exceeds sector size. The sector size is based on the registered proof type. We currently only support registered `StackedDRG2KiBV1P1` proofs, which have 2KiB sector sizes. - `TooManyDealsPerBlock` - Tried to activate too many deals at a given `start_block`. +- `StorageProviderNotRegistered` - An account tries to call `publish_storage_deals` but is not registered as a storage provider. +- `CommD` - An error occurred when trying to calculate CommD. +- `TooManyPendingDeals` - A storage provider tried to propose a deal, but there are too many pending deals. The pending deals should be activated or wait for expiry. +- `InvalidProvider` - A deal was tried to be activated by a provider which does not own it. +- `StartBlockElapsed` - A storage provider tries to activate a deal after the start block. +- `SectorExpiresBeforeDeal` - Sector containing the deal will expire before the deal is supposed to end. +- `InvalidDealState` - A deal was attempted to be activated twice. +- `DealNotPending` - A storage provider tried to activate a deal which is not in the pending proposals. +- `WrongClientSignatureOnProposal` - The client signature did not match the client's public key and data. +- `DealEndBeforeStart` - A deal was attempted to be published but the end block is before the start block and the deal is rejected. +- `DealStartExpired` - A deal was attempted to be published but the start block is in the past and the deal is rejected. +- `DealNotPublished` - A deal was attempted to be published but is not in the correct state. +- `DealDurationOutOfBounds` - A deal was attempted to be published but the duration is not between [MinDealDuration](#constants) and [MaxDealDuration](#constants). +- `InvalidPieceCid` - The deal trying to be published has an invalid piece Cid. +- `DealIsNotActive` - When a sector is being terminated but the deal state is not active. This is the result of a programmer bug. Please [report an issue](https://github.com/eigerco/polka-storage-book/issues/new) to the developers. +- `InvalidCaller` - A deal was found that does not belong to the storage provider. This is the result of a programmer bug. Please [report an issue](https://github.com/eigerco/polka-storage-book/issues/new) to the developers. +- `DealNotFound` - A deal was attempted to be fetched but could not be found. This is the result of a programmer bug. Please [report an issue](https://github.com/eigerco/polka-storage-book/issues/new) to the developers. - `UnexpectedValidationError` - `publish_storage_deals`'s core logic was invoked with a broken invariant that should be called by `validate_deals`. Please [report an issue](https://github.com/eigerco/polka-storage-book/issues/new) to the developers. - `DealPreconditionFailed` - Due to a programmer bug. Please [report an issue](https://github.com/eigerco/polka-storage-book/issues/new) to the developers. diff --git a/pallets/market/Cargo.toml b/pallets/market/Cargo.toml index 53e938c13..e17fdaf8c 100644 --- a/pallets/market/Cargo.toml +++ b/pallets/market/Cargo.toml @@ -24,6 +24,7 @@ multihash-codetable = { workspace = true, features = ["blake2b"] } primitives-commitment = { workspace = true } primitives-proofs = { workspace = true, default-features = false } scale-info = { workspace = true, default-features = false, features = ["derive"] } +thiserror = { workspace = true, default-features = false } # frame deps frame-benchmarking = { workspace = true, default-features = false, optional = true } diff --git a/pallets/market/src/error.rs b/pallets/market/src/error.rs new file mode 100644 index 000000000..1085455db --- /dev/null +++ b/pallets/market/src/error.rs @@ -0,0 +1,48 @@ +use codec::{Decode, Encode}; +use primitives_commitment::{piece::PaddedPieceSizeError, CommitmentError}; +use scale_info::TypeInfo; + +// Clone and PartialEq required because of the BoundedVec<(DealId, DealSettlementError)> +#[derive(TypeInfo, Encode, Decode, Clone, PartialEq, thiserror::Error)] +pub enum DealSettlementError { + /// The deal is going to be slashed. + #[error("DealSettlementError: Slashed Deal")] + SlashedDeal, + /// The deal last update is in the future — i.e. `last_update_block > current_block`. + #[error("DealSettlementError: Future Last Update")] + FutureLastUpdate, + /// The deal was not found. + #[error("DealSettlementError: Deal Not Found")] + DealNotFound, + /// The deal is too early to settle. + #[error("DealSettlementError: Early Settlement")] + EarlySettlement, + /// The deal has expired + #[error("DealSettlementError: Expired Deal")] + ExpiredDeal, + /// Deal is not activated + #[error("DealSettlementError: Deal Not Active")] + DealNotActive, +} + +impl core::fmt::Debug for DealSettlementError { + fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { + core::fmt::Display::fmt(self, f) + } +} + +// TODO: Implement TypeInfo for inner error so we can store them here. +// For now logging will the error will do +#[derive(TypeInfo, Encode, Decode, Clone, PartialEq, thiserror::Error)] +pub enum CommDError { + #[error("CommDError for commitment {0}")] + CommitmentError(CommitmentError), + #[error("CommDError for piece size {0}")] + PaddedPieceSizeError(PaddedPieceSizeError), +} + +impl core::fmt::Debug for CommDError { + fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { + core::fmt::Display::fmt(self, f) + } +} diff --git a/pallets/market/src/lib.rs b/pallets/market/src/lib.rs index 279a34370..b9496c4a8 100644 --- a/pallets/market/src/lib.rs +++ b/pallets/market/src/lib.rs @@ -9,6 +9,8 @@ pub use pallet::*; +mod error; + #[cfg(test)] mod mock; @@ -39,7 +41,7 @@ pub mod pallet { use primitives_commitment::{ commd::compute_unsealed_sector_commitment, piece::{PaddedPieceSize, PieceInfo}, - Commitment, + CommP, Commitment, CommitmentError, }; use primitives_proofs::{ ActiveDeal, ActiveSector, DealId, Market, RegisteredSealProof, SectorDeal, SectorNumber, @@ -49,6 +51,8 @@ pub mod pallet { use sp_arithmetic::traits::BaseArithmetic; use sp_std::vec::Vec; + use crate::error::*; + pub const LOG_TARGET: &'static str = "runtime::market"; /// Allows to extract Balance of an account via the Config::Currency associated type. @@ -268,10 +272,9 @@ pub mod pallet { ) } - fn cid(&self) -> Result { - let cid = Cid::try_from(&self.piece_cid[..]) - .map_err(|e| ProposalError::InvalidPieceCid(e))?; - Ok(cid) + fn piece_commitment(&self) -> Result, CommitmentError> { + let commitment = Commitment::from_cid_bytes(&self.piece_cid[..])?; + Ok(commitment) } } @@ -462,18 +465,12 @@ pub mod pallet { /// `publish_storage_deals` must be called by Storage Providers and it's a Provider of all of the deals. /// This error is emitted when a storage provider tries to publish deals that to not belong to them. ProposalsPublishedByIncorrectStorageProvider, - /// `publish_storage_deals` call was supplied with `deals` which are all invalid. - AllProposalsInvalid, /// `publish_storage_deals`'s core logic was invoked with a broken invariant that should be called by `validate_deals`. UnexpectedValidationError, /// There is more than 1 deal of this ID in the Sector. DuplicateDeal, /// Due to a programmer bug, bounds on Bounded data structures were incorrect so couldn't insert into them. DealPreconditionFailed, - /// Tried to activate a deal which is not in the system. - DealNotFound, - /// Tried to activate a deal, but data doesn't make sense. Details are in the logs. - DealActivationError, /// Sum of all of the deals piece sizes for a sector exceeds sector size. DealsTooLargeToFitIntoSector, /// Tried to activate too many deals at a given start_block. @@ -482,9 +479,9 @@ pub mod pallet { StorageProviderNotRegistered, /// CommD related error CommD, - } - - pub enum DealActivationError { + /// Tried to propose a deal, but there are too many pending deals. + /// The pending deals should be activated or wait for expiry. + TooManyPendingDeals, /// Deal was tried to be activated by a provider which does not own it InvalidProvider, /// Deal should have been activated earlier, it's too late @@ -495,47 +492,14 @@ pub mod pallet { InvalidDealState, /// Tried to activate a deal which is not in the Pending Proposals DealNotPending, - } - - impl core::fmt::Debug for DealActivationError { - fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { - match self { - DealActivationError::InvalidProvider => { - write!(f, "DealActivationError: Invalid Provider") - } - DealActivationError::StartBlockElapsed => { - write!(f, "DealActivationError: Start Block Elapsed") - } - DealActivationError::SectorExpiresBeforeDeal => { - write!(f, "DealActivationError: Sector Expires Before Deal") - } - DealActivationError::InvalidDealState => { - write!(f, "DealActivationError: Invalid Deal State") - } - DealActivationError::DealNotPending => { - write!(f, "DealActivationError: Deal Not Pending") - } - } - } - } - - impl core::fmt::Display for DealActivationError { - fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { - ::fmt(self, f) - } - } - - // NOTE(@th7nder,18/06/2024): - // would love to use `thiserror` but it's not supporting no_std environments yet - // `thiserror-core` relies on rust nightly feature: error_in_core - /// Errors related to [`DealProposal`] and [`ClientDealProposal`] - /// This is error does not surface externally, only in the logs. - /// Mostly used for Deal Validation [`Self::::validate_deals`]. - pub enum ProposalError { + /// Deal was not found in the [`Proposals`] table. + DealNotFound, + /// Caller is not the provider. + InvalidCaller, + /// Deal is not active + DealIsNotActive, /// ClientDealProposal.client_signature did not match client's public key and data. WrongClientSignatureOnProposal, - /// Provider of one of the deals is different than the Provider of the first deal. - DifferentProvider, /// Deal's block_start > block_end, so it doesn't make sense. DealEndBeforeStart, /// Deal's start block is in the past, it should be in the future. @@ -545,140 +509,7 @@ pub mod pallet { /// Deal's duration must be within `Config::MinDealDuration` < `Config:MaxDealDuration`. DealDurationOutOfBounds, /// Deal's piece_cid is invalid. - InvalidPieceCid(cid::Error), - /// Deal's piece_size is invalid. - InvalidPieceSize(&'static str), - /// CommD related error - CommD(&'static str), - } - - impl core::fmt::Debug for ProposalError { - fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { - match self { - ProposalError::WrongClientSignatureOnProposal => { - write!(f, "ProposalError::WrongClientSignatureOnProposal") - } - ProposalError::DifferentProvider => { - write!(f, "ProposalError::DifferentProvider") - } - ProposalError::DealEndBeforeStart => { - write!(f, "ProposalError::DealEndBeforeStart") - } - ProposalError::DealStartExpired => { - write!(f, "ProposalError::DealStartExpired") - } - ProposalError::DealNotPublished => { - write!(f, "ProposalError::DealNotPublished") - } - ProposalError::DealDurationOutOfBounds => { - write!(f, "ProposalError::DealDurationOutOfBounds") - } - ProposalError::InvalidPieceCid(_err) => { - write!(f, "ProposalError::InvalidPieceCid") - } - ProposalError::InvalidPieceSize(err) => { - write!(f, "ProposalError::InvalidPieceSize: {}", err) - } - ProposalError::CommD(err) => { - write!(f, "ProposalError::CommD: {}", err) - } - } - } - } - - impl core::fmt::Display for ProposalError { - fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { - ::fmt(self, f) - } - } - - // Clone and PartialEq required because of the BoundedVec<(DealId, DealSettlementError)> - #[derive(TypeInfo, Encode, Decode, Clone, PartialEq)] - pub enum DealSettlementError { - /// The deal is going to be slashed. - SlashedDeal, - /// The deal last update is in the future — i.e. `last_update_block > current_block`. - FutureLastUpdate, - /// The deal was not found. - DealNotFound, - /// The deal is too early to settle. - EarlySettlement, - /// The deal has expired - ExpiredDeal, - /// Deal is not activated - DealNotActive, - } - - impl core::fmt::Debug for DealSettlementError { - fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { - match self { - DealSettlementError::SlashedDeal => { - write!(f, "DealSettlementError: Slashed Deal") - } - DealSettlementError::FutureLastUpdate => { - write!(f, "DealSettlementError: Future Last Update") - } - DealSettlementError::DealNotFound => { - write!(f, "DealSettlementError: Deal Not Found") - } - DealSettlementError::EarlySettlement => { - write!(f, "DealSettlementError: Early Settlement") - } - DealSettlementError::ExpiredDeal => { - write!(f, "DealSettlementError: Expired Deal") - } - DealSettlementError::DealNotActive => { - write!(f, "DealSettlementError: Deal Not Active") - } - } - } - } - - impl core::fmt::Display for DealSettlementError { - fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { - ::fmt(self, f) - } - } - - pub enum SectorTerminateError { - /// Deal was not found in the [`Proposals`] table. - DealNotFound, - /// Caller is not the provider. - InvalidCaller, - /// Deal is not active - DealIsNotActive, - } - - impl core::fmt::Debug for SectorTerminateError { - fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { - match self { - SectorTerminateError::DealNotFound => { - write!(f, "SectorTerminateError: Deal Not Found") - } - SectorTerminateError::InvalidCaller => { - write!(f, "SectorTerminateError: Invalid Caller") - } - SectorTerminateError::DealIsNotActive => { - write!(f, "SectorTerminateError: Deal Is Not Active") - } - } - } - } - - impl core::fmt::Display for SectorTerminateError { - fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { - ::fmt(self, f) - } - } - - impl From for DispatchError { - fn from(value: SectorTerminateError) -> Self { - DispatchError::Other(match value { - SectorTerminateError::DealNotFound => "deal was not found", - SectorTerminateError::InvalidCaller => "caller is not the provider", - SectorTerminateError::DealIsNotActive => "sector contains active deals", - }) - } + InvalidPieceCid, } /// Extrinsics exposed by the pallet @@ -973,7 +804,7 @@ pub mod pallet { data: &[u8], signature: &T::OffchainSignature, signer: &T::AccountId, - ) -> Result<(), ProposalError> { + ) -> Result<(), Error> { if signature.verify(data, &signer) { return Ok(()); } @@ -989,7 +820,7 @@ pub mod pallet { ensure!( signature.verify(&*wrapped, &signer), - ProposalError::WrongClientSignatureOnProposal + Error::::WrongClientSignatureOnProposal ); Ok(()) @@ -1002,15 +833,18 @@ pub mod pallet { ) -> Result { let pieces = proposals .map(|p| { - let cid = p.cid()?; - let commitment = - Commitment::from_cid(&cid).map_err(|err| ProposalError::CommD(err))?; - let size = PaddedPieceSize::new(p.piece_size) - .map_err(|err| ProposalError::InvalidPieceSize(err))?; + let commitment = p.piece_commitment().map_err(|e| { + log::error!(target: LOG_TARGET, "compute_commd: CommitmentError {e}"); + CommDError::CommitmentError(e) + })?; + let size = PaddedPieceSize::new(p.piece_size).map_err(|e| { + log::error!(target: LOG_TARGET, "compute_commd: PaddedPieceSizeError {e:?}"); + CommDError::PaddedPieceSizeError(e) + })?; Ok(PieceInfo { size, commitment }) }) - .collect::, ProposalError>>(); + .collect::, CommDError>>(); let pieces = pieces.map_err(|err| { log::error!("error occurred while processing pieces: {:?}", err); @@ -1041,8 +875,8 @@ pub mod pallet { Self::validate_deal_can_activate(deal, provider, sector_expiry, sector_activation) .map_err(|e| { log::error!(target: LOG_TARGET, "deal {} cannot be activated, because: {:?}", *deal_id, e); - Error::::DealActivationError } - )?; + e + })?; total_deal_space += deal.piece_size; } @@ -1060,22 +894,19 @@ pub mod pallet { provider: &T::AccountId, sector_expiry: BlockNumberFor, sector_activation: BlockNumberFor, - ) -> Result<(), DealActivationError> { - ensure!( - *provider == deal.provider, - DealActivationError::InvalidProvider - ); + ) -> Result<(), Error> { + ensure!(*provider == deal.provider, Error::::InvalidProvider); ensure!( deal.state == DealState::Published, - DealActivationError::InvalidDealState + Error::::InvalidDealState ); ensure!( sector_activation <= deal.start_block, - DealActivationError::StartBlockElapsed + Error::::StartBlockElapsed ); ensure!( sector_expiry >= deal.end_block, - DealActivationError::SectorExpiresBeforeDeal + Error::::SectorExpiresBeforeDeal ); // Confirm the deal is in the pending proposals set. @@ -1086,7 +917,7 @@ pub mod pallet { let hash = Self::hash_proposal(&deal); ensure!( PendingProposals::::get().contains(&hash), - DealActivationError::DealNotPending + Error::::DealNotPending ); Ok(()) @@ -1150,45 +981,48 @@ pub mod pallet { >, provider: &T::AccountId, current_block: BlockNumberFor, - ) -> Result<(), ProposalError> { + ) -> Result<(), Error> { let encoded = Encode::encode(&deal.proposal); log::trace!(target: LOG_TARGET, "sanity_check: encoded proposal: {}", hex::encode(&encoded)); Self::validate_signature(&encoded, &deal.client_signature, &deal.proposal.client)?; - // Ensure the Piece's Cid is parsable and valid - let _ = deal.proposal.cid()?; + // piece_commitment calls Commitment::from_cid_bytes -> Commitment::from_cid checking validity. + let _ = deal.proposal.piece_commitment().map_err(|e| { + log::error!(target: LOG_TARGET, "sanity_check: Invalid piece Cid {e}"); + Error::::InvalidPieceCid + })?; ensure!( deal.proposal.provider == *provider, - ProposalError::DifferentProvider + Error::::ProposalsPublishedByIncorrectStorageProvider ); ensure!( deal.proposal.start_block < deal.proposal.end_block, - ProposalError::DealEndBeforeStart + Error::::DealEndBeforeStart ); ensure!( deal.proposal.start_block >= current_block, - ProposalError::DealStartExpired + Error::::DealStartExpired ); ensure!( deal.proposal.state == DealState::Published, - ProposalError::DealNotPublished + Error::::DealNotPublished ); let min_dur = T::MinDealDuration::get(); let deal_duration = deal.proposal.duration(); ensure!(deal_duration >= min_dur, { log::error!(target: LOG_TARGET, "deal duration too short: {deal_duration:?} < {min_dur:?}"); - ProposalError::DealDurationOutOfBounds + Error::::DealDurationOutOfBounds }); let max_dur = T::MaxDealDuration::get(); ensure!(deal_duration <= max_dur, { log::error!(target: LOG_TARGET, "deal_duration too long: {deal_duration:?} > {max_dur:?}"); - ProposalError::DealDurationOutOfBounds + Error::::DealDurationOutOfBounds }); // TODO(@th7nder,#81,18/06/2024): figure out the minimum collateral limits @@ -1230,62 +1064,80 @@ pub mod pallet { let mut total_provider_lockup: BalanceOf = Default::default(); let mut message_proposals: BoundedBTreeSet = BoundedBTreeSet::new(); + let mut valid_deals = Vec::new(); - let valid_deals = deals.into_iter().enumerate().filter_map(|(idx, deal)| { - if let Err(e) = Self::sanity_check(&deal, &provider, current_block) { - log::error!(target: LOG_TARGET, "insane deal: idx {idx}, error: {e}"); - return None; - } - - // there is no Entry API in BoundedBTreeMap - let mut client_lockup = - if let Some(client_lockup) = total_client_lockup.get(&deal.proposal.client) { - *client_lockup - } else { - Default::default() - }; - let client_fees: BalanceOf = deal.proposal.total_storage_fee()?.try_into().ok()?; - client_lockup = client_lockup.checked_add(&client_fees)?; + for (idx, deal) in deals.into_iter().enumerate() { + if let Err(e) = Self::sanity_check(&deal, &provider, current_block) { + log::error!(target: LOG_TARGET, "insane deal: idx {idx}, error: {e:?}"); + return Err(e.into()); + } - let client_balance = BalanceTable::::get(&deal.proposal.client); - if client_lockup > client_balance.free { - log::error!(target: LOG_TARGET, "invalid deal: client {:?} not enough free balance {:?} < {:?} to cover deal idx: {}", + // there is no Entry API in BoundedBTreeMap + let mut client_lockup = + if let Some(client_lockup) = total_client_lockup.get(&deal.proposal.client) { + *client_lockup + } else { + Default::default() + }; + let client_fees: BalanceOf = deal + .proposal + .total_storage_fee() + .unwrap() + .try_into() + .ok() + .unwrap(); + client_lockup = client_lockup + .checked_add(&client_fees) + .ok_or(DispatchError::Arithmetic(ArithmeticError::Overflow))?; + + let client_balance = BalanceTable::::get(&deal.proposal.client); + if client_lockup > client_balance.free { + log::error!(target: LOG_TARGET, "invalid deal: client {:?} not enough free balance {:?} < {:?} to cover deal idx: {}", deal.proposal.client, client_balance.free, client_lockup, idx); - return None; - } + return Err(Error::::InsufficientFreeFunds.into()); + } - let mut provider_lockup = total_provider_lockup; - provider_lockup = provider_lockup.checked_add(&deal.proposal.provider_collateral)?; + let mut provider_lockup = total_provider_lockup; + provider_lockup = provider_lockup + .checked_add(&deal.proposal.provider_collateral) + .ok_or(DispatchError::Arithmetic(ArithmeticError::Overflow))?; - let provider_balance = BalanceTable::::get(&deal.proposal.provider); - if provider_lockup > provider_balance.free { - log::error!(target: LOG_TARGET, "invalid deal: storage provider {:?} not enough free balance {:?} < {:?} to cover deal idx: {}", + let provider_balance = BalanceTable::::get(&deal.proposal.provider); + if provider_lockup > provider_balance.free { + log::error!(target: LOG_TARGET, "invalid deal: storage provider {:?} not enough free balance {:?} < {:?} to cover deal idx: {}", deal.proposal.provider, provider_balance.free, provider_lockup, idx); - return None; - } + return Err(Error::::InsufficientFreeFunds.into()); + } - let hash = Self::hash_proposal(&deal.proposal); - let duplicate_in_state = PendingProposals::::get().contains(&hash); - let duplicate_in_message = message_proposals.contains(&hash); - if duplicate_in_state || duplicate_in_message { - log::error!(target: LOG_TARGET, "invalid deal: cannot publish duplicate deal idx: {}", idx); - return None; - } - let mut pending = PendingProposals::::get(); - if let Err(e) = pending.try_insert(hash) { - log::error!(target: LOG_TARGET, "cannot publish: too many pending deal proposals, wait for them to be expired/activated, deal idx: {}, err: {:?}", idx, e); - return None; - } - PendingProposals::::set(pending); - // PRE-COND: always succeeds, as there cannot be more deals than T::MaxDeals and this the size of the set - message_proposals.try_insert(hash).ok()?; - // PRE-COND: always succeeds as there cannot be more clients than T::MaxDeals - total_client_lockup.try_insert(deal.proposal.client.clone(), client_lockup) - .ok()?; - total_provider_lockup = provider_lockup; - Some(deal.proposal) - }).collect::>(); - ensure!(valid_deals.len() > 0, Error::::AllProposalsInvalid); + let hash = Self::hash_proposal(&deal.proposal); + let duplicate_in_state = PendingProposals::::get().contains(&hash); + let duplicate_in_message = message_proposals.contains(&hash); + if duplicate_in_state || duplicate_in_message { + log::error!(target: LOG_TARGET, "invalid deal: cannot publish duplicate deal idx: {}", idx); + return Err(Error::::DuplicateDeal.into()); + } + let mut pending = PendingProposals::::get(); + if let Err(e) = pending.try_insert(hash) { + log::error!(target: LOG_TARGET, "cannot publish: too many pending deal proposals, wait for them to be expired/activated, deal idx: {}, err: {:?}", idx, e); + return Err(Error::::TooManyPendingDeals.into()); + } + PendingProposals::::set(pending); + // PRE-COND: always succeeds, as there cannot be more deals than T::MaxDeals and this the size of the set + message_proposals.try_insert(hash).map_err(|_| { + DispatchError::Other("Unable to insert hash. More deals than T::MaxDeals") + })?; + // PRE-COND: always succeeds as there cannot be more clients than T::MaxDeals + total_client_lockup + .try_insert(deal.proposal.client.clone(), client_lockup) + .map_err(|_| { + DispatchError::Other( + "Unable to update client lockup. More clients than T::MaxDeals", + ) + })?; + total_provider_lockup = provider_lockup; + + valid_deals.push(deal.proposal) + } Ok((valid_deals, total_provider_lockup)) } @@ -1409,14 +1261,17 @@ pub mod pallet { activated_deals .try_push(ActiveDeal { client: proposal.client.clone(), - piece_cid: proposal.cid().map_err(|e| { - log::error!( - "there is invalid cid saved on-chain for deal: {}, {:?}", - deal_id, - e - ); - Error::::DealPreconditionFailed - })?, + piece_cid: proposal + .piece_commitment() + .map_err(|e| { + log::error!( + "there is invalid cid saved on-chain for deal: {}, {:?}", + deal_id, + e + ); + Error::::DealPreconditionFailed + })? + .cid(), piece_size: proposal.piece_size, }) .map_err(|_| { @@ -1486,13 +1341,13 @@ pub mod pallet { for deal_id in deal_ids { // Fetch the corresponding deal proposal, it's ok if it has already been deleted let Some(mut deal_proposal) = Proposals::::get(deal_id) else { - return Err(SectorTerminateError::DealNotFound)?; + return Err(Error::::DealNotFound.into()); }; // This should never happen, because we are getting deals // the storage provider with which we called the extrinsic. if *storage_provider != deal_proposal.provider { - return Err(SectorTerminateError::InvalidCaller)?; + return Err(Error::::InvalidCaller.into()); } if deal_proposal.end_block <= current_block { @@ -1504,7 +1359,7 @@ pub mod pallet { // If a sector is being terminated, it means that at some point, // the deals contained within were active let DealState::Active(ref mut active_deal_state) = deal_proposal.state else { - return Err(SectorTerminateError::DealIsNotActive)?; + return Err(Error::::DealIsNotActive.into()); }; // https://github.com/filecoin-project/builtin-actors/blob/54236ae89880bf4aa89b0dba6d9060c3fd2aacee/actors/market/src/lib.rs#L840-L844 diff --git a/pallets/market/src/mock.rs b/pallets/market/src/mock.rs index 657d8ac7d..9928e722e 100644 --- a/pallets/market/src/mock.rs +++ b/pallets/market/src/mock.rs @@ -157,7 +157,7 @@ pub fn sign_proposal(client: &str, proposal: DealProposalOf) -> ClientDeal pub const ALICE: &'static str = "//Alice"; pub const BOB: &'static str = "//Bob"; pub const PROVIDER: &'static str = "//StorageProvider"; -pub const INITIAL_FUNDS: u64 = 100; +pub const INITIAL_FUNDS: u64 = 1000; /// Build genesis storage according to the mock runtime. pub fn new_test_ext() -> sp_io::TestExternalities { diff --git a/pallets/market/src/test.rs b/pallets/market/src/test.rs index ba0044ab5..a47f45e23 100644 --- a/pallets/market/src/test.rs +++ b/pallets/market/src/test.rs @@ -17,11 +17,11 @@ use sp_core::H256; use sp_runtime::AccountId32; use crate::{ + error::DealSettlementError, mock::*, pallet::{lock_funds, slash_and_burn, unlock_funds}, - ActiveDealState, BalanceEntry, BalanceTable, Config, DealSettlementError, DealState, - DealsForBlock, Error, Event, PendingProposals, Proposals, PublishedDeal, SectorDeals, - SectorTerminateError, SettledDealData, + ActiveDealState, BalanceEntry, BalanceTable, Config, DealState, DealsForBlock, Error, Event, + PendingProposals, Proposals, PublishedDeal, SectorDeals, SettledDealData, }; #[test] fn initial_state() { @@ -262,7 +262,7 @@ fn publish_storage_deals_fails_invalid_signature() { RuntimeOrigin::signed(account::(PROVIDER)), bounded_vec![deal] ), - Error::::AllProposalsInvalid + Error::::WrongClientSignatureOnProposal ); }); } @@ -281,7 +281,7 @@ fn publish_storage_deals_fails_end_before_start() { RuntimeOrigin::signed(account::(PROVIDER)), bounded_vec![proposal] ), - Error::::AllProposalsInvalid + Error::::DealEndBeforeStart ); }); } @@ -304,7 +304,7 @@ fn publish_storage_deals_fails_must_be_unpublished() { RuntimeOrigin::signed(account::(PROVIDER)), bounded_vec![proposal] ), - Error::::AllProposalsInvalid + Error::::DealNotPublished ); }); } @@ -323,7 +323,7 @@ fn publish_storage_deals_fails_min_duration_out_of_bounds() { RuntimeOrigin::signed(account::(PROVIDER)), bounded_vec![proposal] ), - Error::::AllProposalsInvalid + Error::::DealDurationOutOfBounds ); }); } @@ -342,7 +342,7 @@ fn publish_storage_deals_fails_max_duration_out_of_bounds() { RuntimeOrigin::signed(account::(PROVIDER)), bounded_vec![proposal] ), - Error::::AllProposalsInvalid + Error::::DealDurationOutOfBounds ); }); } @@ -363,13 +363,13 @@ fn publish_storage_deals_fails_start_time_expired() { RuntimeOrigin::signed(account::(PROVIDER)), bounded_vec![proposal] ), - Error::::AllProposalsInvalid + Error::::DealStartExpired ); }); } /// Add enough balance to the provider so that the first proposal can be accepted and published. -/// Second proposal will be rejected, but first still published +/// All proposals will be rejected #[test] fn publish_storage_deals_fails_different_providers() { new_test_ext().execute_with(|| { @@ -378,32 +378,26 @@ fn publish_storage_deals_fails_different_providers() { let _ = Market::add_balance(RuntimeOrigin::signed(account::(ALICE)), 60); System::reset_events(); - assert_ok!(Market::publish_storage_deals( - RuntimeOrigin::signed(account::(PROVIDER)), - bounded_vec![ - DealProposalBuilder::::default().signed(ALICE), - // Proposal where second deal's provider is not a caller - DealProposalBuilder::::default() - .client(BOB) - .provider(BOB) - .signed(BOB), - ] - )); - assert_eq!( - events(), - [RuntimeEvent::Market(Event::::DealsPublished { - provider: account::(PROVIDER), - deals: bounded_vec!(PublishedDeal { - deal_id: 0, - client: account::(ALICE), - }) - })] + assert_noop!( + Market::publish_storage_deals( + RuntimeOrigin::signed(account::(PROVIDER)), + bounded_vec![ + DealProposalBuilder::::default().signed(ALICE), + // Proposal where second deal's provider is not a caller + DealProposalBuilder::::default() + .client(BOB) + .provider(BOB) + .signed(BOB), + ] + ), + Error::::ProposalsPublishedByIncorrectStorageProvider ); + assert_eq!(events(), []); }); } /// Add enough balance to the provider so that the first proposal can be accepted and published. -/// Second proposal will be rejected, but first still published +/// All proposals will be rejected #[test] fn publish_storage_deals_fails_client_not_enough_funds_for_second_deal() { new_test_ext().execute_with(|| { @@ -412,31 +406,25 @@ fn publish_storage_deals_fails_client_not_enough_funds_for_second_deal() { let _ = Market::add_balance(RuntimeOrigin::signed(account::(ALICE)), 60); System::reset_events(); - assert_ok!(Market::publish_storage_deals( - RuntimeOrigin::signed(account::(PROVIDER)), - bounded_vec![ - DealProposalBuilder::::default().signed(ALICE), - DealProposalBuilder::::default() - .piece_size(10) - .signed(ALICE), - ] - )); - assert_eq!( - events(), - [RuntimeEvent::Market(Event::::DealsPublished { - provider: account::(PROVIDER), - deals: bounded_vec!(PublishedDeal { - deal_id: 0, - client: account::(ALICE), - }) - })] + assert_noop!( + Market::publish_storage_deals( + RuntimeOrigin::signed(account::(PROVIDER)), + bounded_vec![ + DealProposalBuilder::::default().signed(ALICE), + DealProposalBuilder::::default() + .piece_size(10) + .signed(ALICE), + ] + ), + Error::::InsufficientFreeFunds ); + assert_eq!(events(), []); }); } /// Add enough balance to the provider so that the first proposal can be accepted and published. /// Collateral is 25 for the default deal, so provider should have at least 50. -/// Second proposal will be rejected, but first still published +/// Both proposals will be rejected #[test] fn publish_storage_deals_fails_provider_not_enough_funds_for_second_deal() { new_test_ext().execute_with(|| { @@ -446,25 +434,19 @@ fn publish_storage_deals_fails_provider_not_enough_funds_for_second_deal() { let _ = Market::add_balance(RuntimeOrigin::signed(account::(BOB)), 90); System::reset_events(); - assert_ok!(Market::publish_storage_deals( - RuntimeOrigin::signed(account::(PROVIDER)), - bounded_vec![ - DealProposalBuilder::::default().signed(ALICE), - DealProposalBuilder::::default() - .client(BOB) - .signed(BOB), - ] - )); - assert_eq!( - events(), - [RuntimeEvent::Market(Event::::DealsPublished { - provider: account::(PROVIDER), - deals: bounded_vec!(PublishedDeal { - deal_id: 0, - client: account::(ALICE), - }) - })] + assert_noop!( + Market::publish_storage_deals( + RuntimeOrigin::signed(account::(PROVIDER)), + bounded_vec![ + DealProposalBuilder::::default().signed(ALICE), + DealProposalBuilder::::default() + .client(BOB) + .signed(BOB), + ] + ), + Error::::InsufficientFreeFunds ); + assert_eq!(events(), []); }); } @@ -476,27 +458,21 @@ fn publish_storage_deals_fails_duplicate_deal_in_message() { let _ = Market::add_balance(RuntimeOrigin::signed(account::(ALICE)), 90); System::reset_events(); - assert_ok!(Market::publish_storage_deals( - RuntimeOrigin::signed(account::(PROVIDER)), - bounded_vec![ - DealProposalBuilder::::default() - .storage_price_per_block(1) - .signed(ALICE), - DealProposalBuilder::::default() - .storage_price_per_block(1) - .signed(ALICE), - ] - )); - assert_eq!( - events(), - [RuntimeEvent::Market(Event::::DealsPublished { - provider: account::(PROVIDER), - deals: bounded_vec!(PublishedDeal { - deal_id: 0, - client: account::(ALICE), - }) - })] + assert_noop!( + Market::publish_storage_deals( + RuntimeOrigin::signed(account::(PROVIDER)), + bounded_vec![ + DealProposalBuilder::::default() + .storage_price_per_block(1) + .signed(ALICE), + DealProposalBuilder::::default() + .storage_price_per_block(1) + .signed(ALICE), + ] + ), + Error::::DuplicateDeal ); + assert_eq!(events(), []); }); } @@ -531,7 +507,7 @@ fn publish_storage_deals_fails_duplicate_deal_in_state() { .storage_price_per_block(1) .signed(ALICE),] ), - Error::::AllProposalsInvalid + Error::::DuplicateDeal ); }); } @@ -543,11 +519,12 @@ fn publish_storage_deals() { let alice_proposal = DealProposalBuilder::::default().signed(ALICE); let alice_start_block = 100; let alice_deal_id = 0; + let alice_second_deal_id = 1; // We're not expecting for it to go through, but the call should not fail. let alice_second_proposal = DealProposalBuilder::::default() .piece_size(37) .signed(ALICE); - let bob_deal_id = 1; + let bob_deal_id = 2; let bob_start_block = 130; let bob_proposal = DealProposalBuilder::::default() .client(BOB) @@ -560,7 +537,7 @@ fn publish_storage_deals() { let alice_hash = Market::hash_proposal(&alice_proposal.proposal); let bob_hash = Market::hash_proposal(&bob_proposal.proposal); - let _ = Market::add_balance(RuntimeOrigin::signed(account::(ALICE)), 60); + let _ = Market::add_balance(RuntimeOrigin::signed(account::(ALICE)), 100); let _ = Market::add_balance(RuntimeOrigin::signed(account::(BOB)), 70); let _ = Market::add_balance(RuntimeOrigin::signed(account::(PROVIDER)), 75); System::reset_events(); @@ -572,8 +549,8 @@ fn publish_storage_deals() { assert_eq!( BalanceTable::::get(account::(ALICE)), BalanceEntry:: { - free: 10, - locked: 50 + free: 0, + locked: 100 } ); assert_eq!( @@ -586,8 +563,8 @@ fn publish_storage_deals() { assert_eq!( BalanceTable::::get(account::(PROVIDER)), BalanceEntry:: { - free: 35, - locked: 40 + free: 10, + locked: 65 } ); @@ -600,6 +577,10 @@ fn publish_storage_deals() { deal_id: alice_deal_id, client: account::(ALICE), }, + PublishedDeal { + deal_id: alice_second_deal_id, + client: account::(ALICE), + }, PublishedDeal { deal_id: bob_deal_id, client: account::(BOB), @@ -663,7 +644,7 @@ fn verify_deals_for_activation_fails_with_different_provider() { assert_noop!( Market::verify_deals_for_activation(&account::(PROVIDER), deals), - Error::::DealActivationError + Error::::InvalidProvider ); }); } @@ -687,7 +668,7 @@ fn verify_deals_for_activation_fails_with_invalid_deal_state() { assert_noop!( Market::verify_deals_for_activation(&account::(PROVIDER), deals), - Error::::DealActivationError + Error::::InvalidDealState ); }); } @@ -701,7 +682,7 @@ fn verify_deals_for_activation_fails_deal_not_in_pending() { assert_noop!( Market::verify_deals_for_activation(&account::(PROVIDER), deals), - Error::::DealActivationError + Error::::DealNotPending ); }); } @@ -724,7 +705,7 @@ fn verify_deals_for_activation_fails_sector_activation_on_deal_from_the_past() { assert_noop!( Market::verify_deals_for_activation(&account::(PROVIDER), deals), - Error::::DealActivationError + Error::::StartBlockElapsed ); }); } @@ -744,7 +725,7 @@ fn verify_deals_for_activation_fails_sector_expires_before_deal_ends() { assert_noop!( Market::verify_deals_for_activation(&account::(PROVIDER), deals), - Error::::DealActivationError + Error::::SectorExpiresBeforeDeal ); }); } @@ -1393,16 +1374,15 @@ fn test_lock_funds() { new_test_ext().execute_with(|| { assert_eq!( ::Currency::total_balance(&account::(PROVIDER)), - 100 + 1000 ); - // We can't get all 100, otherwise the account would be reaped assert_ok!(Market::add_balance( RuntimeOrigin::signed(account::(PROVIDER)), 90 )); assert_eq!( ::Currency::total_balance(&account::(PROVIDER)), - 10 + 910 ); assert_ok!(lock_funds::(&account::(PROVIDER), 25)); assert_eq!( @@ -1443,7 +1423,7 @@ fn test_unlock_funds() { new_test_ext().execute_with(|| { assert_eq!( ::Currency::total_balance(&account::(PROVIDER)), - 100 + 1000 ); // We can't get all 100, otherwise the account would be reaped assert_ok!(Market::add_balance( @@ -1452,7 +1432,7 @@ fn test_unlock_funds() { )); assert_eq!( ::Currency::total_balance(&account::(PROVIDER)), - 10 + 910 ); assert_ok!(lock_funds::(&account::(PROVIDER), 90)); assert_eq!( @@ -1501,7 +1481,7 @@ fn slash_and_burn_acc() { new_test_ext().execute_with(|| { assert_eq!( ::Currency::total_issuance(), - 300 + 3000 ); assert_ok!(Market::add_balance( RuntimeOrigin::signed(account::(PROVIDER)), @@ -1525,7 +1505,7 @@ fn slash_and_burn_acc() { ); assert_eq!( ::Currency::total_issuance(), - 290 + 2990 ); assert_eq!( @@ -1542,7 +1522,7 @@ fn slash_and_burn_acc() { ); assert_eq!( ::Currency::total_issuance(), - 290 + 2990 ); }); } @@ -1578,7 +1558,7 @@ fn on_sector_terminate_deal_not_found() { assert_err!( Market::on_sectors_terminate(&storage_provider, bounded_vec![sector_number]), - DispatchError::from(SectorTerminateError::DealNotFound) + Error::::DealNotFound ); assert_eq!(events(), []); @@ -1637,7 +1617,7 @@ fn on_sector_terminate_not_active() { assert_err!( Market::on_sectors_terminate(&storage_provider, bounded_vec![sector_number]), - DispatchError::from(SectorTerminateError::DealIsNotActive) + Error::::DealIsNotActive ); assert_eq!(events(), []); @@ -1718,7 +1698,7 @@ fn on_sector_terminate_active() { assert!(!Proposals::::contains_key(1)); assert_eq!( ::Currency::total_issuance(), - 285 + 2985 ); }); } diff --git a/primitives/commitment/Cargo.toml b/primitives/commitment/Cargo.toml index 711358210..675a57a6a 100644 --- a/primitives/commitment/Cargo.toml +++ b/primitives/commitment/Cargo.toml @@ -12,14 +12,15 @@ serde = ["dep:serde"] std = ["dep:filecoin-proofs"] [dependencies] -primitives-proofs.workspace = true -sealed.workspace = true - cid.workspace = true -sha2.workspace = true - +codec = { workspace = true, default-features = false, features = ["derive"] } filecoin-proofs = { optional = true, workspace = true } +primitives-proofs.workspace = true +scale-info = { workspace = true, default-features = false, features = ["derive"] } +sealed.workspace = true serde = { workspace = true, features = ["derive"], optional = true } +sha2.workspace = true +thiserror.workspace = true [dev-dependencies] rand = { workspace = true, features = ["std", "std_rng"] } diff --git a/primitives/commitment/src/lib.rs b/primitives/commitment/src/lib.rs index b1c3959a8..096c32fb6 100644 --- a/primitives/commitment/src/lib.rs +++ b/primitives/commitment/src/lib.rs @@ -7,7 +7,9 @@ mod zero; use core::{fmt::Display, marker::PhantomData}; use cid::{multihash::Multihash, Cid}; +use codec::{Decode, Encode}; use primitives_proofs::RegisteredSealProof; +use scale_info::TypeInfo; use sealed::sealed; use crate::piece::PaddedPieceSize; @@ -90,6 +92,23 @@ impl CommitmentKind for CommR { } } +// TODO: Implement TypeInfo for this type so we can use it in pallets. +#[derive(Clone, Eq, PartialEq, TypeInfo, Encode, Decode, thiserror::Error)] +pub enum CommitmentError { + #[error("bytes not a valid cid")] + InvalidCidBytes, + #[error("invalid multicodec for commitment")] + InvalidMultiCodec, + #[error("invalid multihash for commitment")] + InvalidMultiHash, +} + +impl core::fmt::Debug for CommitmentError { + fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { + core::fmt::Display::fmt(self, f) + } +} + #[cfg_attr(feature = "serde", derive(::serde::Deserialize, ::serde::Serialize))] #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct Commitment @@ -106,25 +125,25 @@ where { /// Creates a new `Commitment` from bytes of a valid CID. Returns an error /// if the bytes passed do not represent a valid commitment. - pub fn from_cid_bytes(bytes: &[u8]) -> Result { - let cid = Cid::try_from(bytes).map_err(|_| "bytes not a valid cid")?; + pub fn from_cid_bytes(bytes: &[u8]) -> Result { + let cid = Cid::try_from(bytes).map_err(|_| CommitmentError::InvalidCidBytes)?; Self::from_cid(&cid) } /// Creates a new `Commitment` from a CID. Returns an error if the CID /// passed does not represent a commitment kind. - pub fn from_cid(cid: &Cid) -> Result { + pub fn from_cid(cid: &Cid) -> Result { let mut raw = [0; 32]; raw.copy_from_slice(cid.hash().digest()); // Check multicodec of the cid if cid.codec() != Kind::multicodec() { - return Err("invalid multicodec for commitment"); + return Err(CommitmentError::InvalidMultiCodec); } // Check multihash of the cid if cid.hash().code() != Kind::multihash() { - return Err("invalid multihash for commitment"); + return Err(CommitmentError::InvalidMultiHash); } Ok(Self { @@ -161,7 +180,7 @@ impl TryFrom for Commitment where Kind: CommitmentKind, { - type Error = &'static str; + type Error = CommitmentError; fn try_from(value: Cid) -> Result { Self::from_cid(&value) diff --git a/primitives/commitment/src/piece.rs b/primitives/commitment/src/piece.rs index 36ee02ce3..7ef3b9595 100644 --- a/primitives/commitment/src/piece.rs +++ b/primitives/commitment/src/piece.rs @@ -1,5 +1,8 @@ use core::ops::{Add, AddAssign, Deref}; +use codec::{Decode, Encode}; +use scale_info::TypeInfo; + use crate::{CommP, Commitment, NODE_SIZE}; /// Piece info contains piece commitment and piece size. @@ -100,6 +103,22 @@ impl Into for UnpaddedPieceSize { } } +#[derive(Clone, Eq, PartialEq, TypeInfo, Encode, Decode, thiserror::Error)] +pub enum PaddedPieceSizeError { + #[error("minimum piece size is 128 bytes")] + SizeTooSmall, + #[error("padded piece size must be a power of 2")] + SizeNotPowerOfTwo, + #[error("padded_piece_size is not multiple of NODE_SIZE")] + NotAMultipleOfNodeSize, +} + +impl core::fmt::Debug for PaddedPieceSizeError { + fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { + core::fmt::Display::fmt(self, f) + } +} + /// Size of a piece in bytes with padding. The size is always a power of two /// number. #[cfg_attr(feature = "serde", derive(::serde::Deserialize, ::serde::Serialize))] @@ -112,17 +131,17 @@ impl PaddedPieceSize { /// Initialize new padded piece size. Error is returned if the size is /// invalid. - pub fn new(size: u64) -> Result { + pub fn new(size: u64) -> Result { if size < 128 { - return Err("minimum piece size is 128 bytes"); + return Err(PaddedPieceSizeError::SizeTooSmall); } if size.count_ones() != 1 { - return Err("padded piece size must be a power of 2"); + return Err(PaddedPieceSizeError::SizeNotPowerOfTwo); } if size % NODE_SIZE as u64 != 0 { - return Err("padded_piece_size is not multiple of NODE_SIZE"); + return Err(PaddedPieceSizeError::NotAMultipleOfNodeSize); } Ok(Self(size)) @@ -197,6 +216,7 @@ impl Into for PaddedPieceSize { filecoin_proofs::PaddedBytesAmount(self.0) } } + #[cfg(test)] mod tests { use super::*; @@ -212,7 +232,7 @@ mod tests { fn invalid_piece_checks() { assert_eq!( PaddedPieceSize::new(127), - Err("minimum piece size is 128 bytes") + Err(PaddedPieceSizeError::SizeTooSmall) ); assert_eq!( UnpaddedPieceSize::new(126), @@ -220,7 +240,7 @@ mod tests { ); assert_eq!( PaddedPieceSize::new(0b10000001), - Err("padded piece size must be a power of 2") + Err(PaddedPieceSizeError::SizeNotPowerOfTwo) ); assert_eq!( UnpaddedPieceSize::new(0b1110111000),