Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

Client API refactoring - limiting errors to crate-level error types #1525

Merged
merged 9 commits into from
Jul 5, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions ethcore/src/client/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use std::sync::atomic::{AtomicUsize, Ordering as AtomicOrdering};
use util::*;
use util::panics::*;
use views::BlockView;
use error::{Error, ImportError, ExecutionError, BlockError, ImportResult};
use error::{ImportError, ExecutionError, BlockError, ImportResult};
use header::{BlockNumber};
use state::State;
use spec::Spec;
Expand All @@ -38,7 +38,9 @@ use filter::Filter;
use log_entry::LocalizedLogEntry;
use block_queue::{BlockQueue, BlockQueueInfo};
use blockchain::{BlockChain, BlockProvider, TreeRoute, ImportRoute};
use client::{BlockID, TransactionID, UncleID, TraceId, ClientConfig, DatabaseCompactionProfile, BlockChainClient, MiningBlockChainClient, TraceFilter, CallAnalytics};
use client::{BlockID, TransactionID, UncleID, TraceId, ClientConfig, DatabaseCompactionProfile,
BlockChainClient, MiningBlockChainClient, TraceFilter, CallAnalytics, TransactionImportError,
BlockImportError, TransactionImportResult};
use client::Error as ClientError;
use env_info::EnvInfo;
use executive::{Executive, Executed, TransactOptions, contract_address};
Expand All @@ -49,7 +51,7 @@ use trace;
pub use types::blockchain_info::BlockChainInfo;
pub use types::block_status::BlockStatus;
use evm::Factory as EvmFactory;
use miner::{Miner, MinerService, TransactionImportResult, AccountDetails};
use miner::{Miner, MinerService, AccountDetails};

const MAX_TX_QUEUE_SIZE: usize = 4096;

Expand Down Expand Up @@ -654,17 +656,17 @@ impl BlockChainClient for Client {
self.chain.block_receipts(hash).map(|receipts| rlp::encode(&receipts).to_vec())
}

fn import_block(&self, bytes: Bytes) -> ImportResult {
fn import_block(&self, bytes: Bytes) -> Result<H256, BlockImportError> {
{
let header = BlockView::new(&bytes).header_view();
if self.chain.is_known(&header.sha3()) {
return Err(ImportError::AlreadyInChain.into());
return Err(BlockImportError::Import(ImportError::AlreadyInChain));
}
if self.block_status(BlockID::Hash(header.parent_hash())) == BlockStatus::Unknown {
return Err(BlockError::UnknownParent(header.parent_hash()).into());
return Err(BlockImportError::Block(BlockError::UnknownParent(header.parent_hash())));
}
}
self.block_queue.import_block(bytes)
Ok(try!(self.block_queue.import_block(bytes)))
}

fn queue_info(&self) -> BlockQueueInfo {
Expand Down Expand Up @@ -778,12 +780,16 @@ impl BlockChainClient for Client {
self.build_last_hashes(self.chain.best_block_hash())
}

fn import_transactions(&self, transactions: Vec<SignedTransaction>) -> Vec<Result<TransactionImportResult, Error>> {
fn import_transactions(&self, transactions: Vec<SignedTransaction>) -> Vec<Result<TransactionImportResult, TransactionImportError>> {
let fetch_account = |a: &Address| AccountDetails {
nonce: self.latest_nonce(a),
balance: self.latest_balance(a),
};
self.miner.import_transactions(self, transactions, fetch_account)

self.miner.import_transactions(self, transactions, &fetch_account)
.into_iter()
.map(|res| res.map_err(|e| e.into()))
.collect()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.miner.import_transactions should return Result<_, TransactionImportError> so this code is redundant

}

fn queue_transactions(&self, transactions: Vec<Bytes>) {
Expand Down
8 changes: 4 additions & 4 deletions ethcore/src/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ use error::{ImportResult, ExecutionError};
use receipt::LocalizedReceipt;
use trace::LocalizedTrace;
use evm::Factory as EvmFactory;
use miner::{TransactionImportResult};
use error::Error as EthError;
pub use block_import_error::BlockImportError;
pub use transaction_import::{TransactionImportResult, TransactionImportError};

/// Options concerning what analytics we run on the call.
#[derive(Eq, PartialEq, Default, Clone, Copy, Debug)]
Expand Down Expand Up @@ -145,7 +145,7 @@ pub trait BlockChainClient : Sync + Send {
fn block_receipts(&self, hash: &H256) -> Option<Bytes>;

/// Import a block into the blockchain.
fn import_block(&self, bytes: Bytes) -> ImportResult;
fn import_block(&self, bytes: Bytes) -> Result<H256, BlockImportError>;

/// Get block queue information.
fn queue_info(&self) -> BlockQueueInfo;
Expand Down Expand Up @@ -188,7 +188,7 @@ pub trait BlockChainClient : Sync + Send {
fn last_hashes(&self) -> LastHashes;

/// import transactions from network/other 3rd party
fn import_transactions(&self, transactions: Vec<SignedTransaction>) -> Vec<Result<TransactionImportResult, EthError>>;
fn import_transactions(&self, transactions: Vec<SignedTransaction>) -> Vec<Result<TransactionImportResult, TransactionImportError>>;

/// Queue transactions for importing.
fn queue_transactions(&self, transactions: Vec<Bytes>);
Expand Down
12 changes: 8 additions & 4 deletions ethcore/src/client/test_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ use std::sync::atomic::{AtomicUsize, Ordering as AtomicOrder};
use util::*;
use transaction::{Transaction, LocalizedTransaction, SignedTransaction, Action};
use blockchain::TreeRoute;
use client::{BlockChainClient, MiningBlockChainClient, BlockChainInfo, BlockStatus, BlockID, TransactionID, UncleID, TraceId, TraceFilter, LastHashes, CallAnalytics};
use client::{BlockChainClient, MiningBlockChainClient, BlockChainInfo, BlockStatus, BlockID,
TransactionID, UncleID, TraceId, TraceFilter, LastHashes, CallAnalytics,
TransactionImportError, BlockImportError};
use header::{Header as BlockHeader, BlockNumber};
use filter::Filter;
use log_entry::LocalizedLogEntry;
Expand All @@ -38,7 +40,6 @@ use error::{ExecutionError};
use trace::LocalizedTrace;

use miner::{TransactionImportResult, AccountDetails};
use error::Error as EthError;

/// Test client.
pub struct TestBlockChainClient {
Expand Down Expand Up @@ -402,7 +403,7 @@ impl BlockChainClient for TestBlockChainClient {
None
}

fn import_block(&self, b: Bytes) -> ImportResult {
fn import_block(&self, b: Bytes) -> Result<H256, BlockImportError> {
let header = Rlp::new(&b).val_at::<BlockHeader>(0);
let h = header.hash();
let number: usize = header.number as usize;
Expand Down Expand Up @@ -487,7 +488,7 @@ impl BlockChainClient for TestBlockChainClient {
unimplemented!();
}

fn import_transactions(&self, transactions: Vec<SignedTransaction>) -> Vec<Result<TransactionImportResult, EthError>> {
fn import_transactions(&self, transactions: Vec<SignedTransaction>) -> Vec<Result<TransactionImportResult, TransactionImportError>> {
let nonces = self.nonces.read().unwrap();
let balances = self.balances.read().unwrap();
let fetch_account = |a: &Address| AccountDetails {
Expand All @@ -496,6 +497,9 @@ impl BlockChainClient for TestBlockChainClient {
};

self.miner.import_transactions(self, transactions, &fetch_account)
.into_iter()
.map(|res| res.map_err(|e| e.into()))
.collect()
}

fn queue_transactions(&self, transactions: Vec<Bytes>) {
Expand Down
20 changes: 18 additions & 2 deletions ethcore/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,11 @@ use util::*;
use header::BlockNumber;
use basic_types::LogBloom;
use client::Error as ClientError;

pub use types::executed::ExecutionError;
use ipc::binary::{BinaryConvertError, BinaryConvertable};
use types::block_import_error::BlockImportError;

#[derive(Debug, PartialEq)]
#[derive(Debug, PartialEq, Clone)]
/// Errors concerning transaction processing.
pub enum TransactionError {
/// Transaction is already imported to the queue
Expand Down Expand Up @@ -312,6 +313,21 @@ impl From<TrieError> for Error {
}
}

impl From<BlockImportError> for Error {
fn from(err: BlockImportError) -> Error {
match err {
BlockImportError::Block(e) => Error::Block(e),
BlockImportError::Import(e) => Error::Import(e),
BlockImportError::Other(s) => Error::Util(UtilError::SimpleString(s)),
}
}
}

binary_fixed_size!(BlockError);
binary_fixed_size!(ImportError);
binary_fixed_size!(TransactionError);


// TODO: uncomment below once https://github.com/rust-lang/rust/issues/27336 sorted.
/*#![feature(concat_idents)]
macro_rules! assimilate {
Expand Down
4 changes: 3 additions & 1 deletion ethcore/src/miner/miner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,10 @@ use transaction::SignedTransaction;
use receipt::{Receipt};
use spec::Spec;
use engine::Engine;
use miner::{MinerService, MinerStatus, TransactionQueue, AccountDetails, TransactionImportResult, TransactionOrigin};
use miner::{MinerService, MinerStatus, TransactionQueue, AccountDetails, TransactionOrigin};
use miner::work_notify::WorkPoster;
use client::TransactionImportResult;


/// Different possible definitions for pending transaction set.
#[derive(Debug)]
Expand Down
3 changes: 2 additions & 1 deletion ethcore/src/miner/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,10 @@ mod external;
mod transaction_queue;
mod work_notify;

pub use self::transaction_queue::{TransactionQueue, AccountDetails, TransactionImportResult, TransactionOrigin};
pub use self::transaction_queue::{TransactionQueue, AccountDetails, TransactionOrigin};
pub use self::miner::{Miner, MinerOptions, PendingSet};
pub use self::external::{ExternalMiner, ExternalMinerService};
pub use client::TransactionImportResult;

use std::collections::BTreeMap;
use util::{H256, U256, Address, Bytes};
Expand Down
11 changes: 2 additions & 9 deletions ethcore/src/miner/transaction_queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ use util::hash::{Address, H256};
use util::table::*;
use transaction::*;
use error::{Error, TransactionError};
use client::TransactionImportResult;

/// Transaction origin
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
Expand Down Expand Up @@ -309,15 +310,6 @@ pub struct TransactionQueueStatus {
pub future: usize,
}

#[derive(Debug, PartialEq)]
/// Represents the result of importing transaction.
pub enum TransactionImportResult {
/// Transaction was imported to current queue.
Current,
/// Transaction was imported to future queue.
Future
}

/// Details of account
pub struct AccountDetails {
/// Most recent account nonce
Expand Down Expand Up @@ -812,6 +804,7 @@ mod test {
use error::{Error, TransactionError};
use super::*;
use super::{TransactionSet, TransactionOrder, VerifiedTransaction};
use client::TransactionImportResult;

fn unwrap_tx_err(err: Result<TransactionImportResult, Error>) -> TransactionError {
match err.unwrap_err() {
Expand Down
44 changes: 44 additions & 0 deletions ethcore/src/types/block_import_error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Copyright 2015, 2016 Ethcore (UK) Ltd.
// This file is part of Parity.

// Parity is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.

// Parity is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License
// along with Parity. If not, see <http://www.gnu.org/licenses/>.

//! Block import error related types

use std::mem;
use ipc::binary::BinaryConvertError;
use std::collections::VecDeque;
use error::{ImportError, BlockError, Error};
use std::convert::From;

/// Error dedicated to import block function
#[derive(Binary, Debug)]
pub enum BlockImportError {
/// Import error
Import(ImportError),
/// Block error
Block(BlockError),
/// Other error
Other(String),
}

impl From<Error> for BlockImportError {
fn from(e: Error) -> Self {
match e {
Error::Block(block_error) => BlockImportError::Block(block_error),
Error::Import(import_error) => BlockImportError::Import(import_error),
_ => BlockImportError::Other(format!("other block import error: {:?}", e)),
}
}
}
2 changes: 2 additions & 0 deletions ethcore/src/types/mod.rs.in
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,5 @@ pub mod executed;
pub mod block_status;
pub mod account_diff;
pub mod state_diff;
pub mod transaction_import;
pub mod block_import_error;
52 changes: 52 additions & 0 deletions ethcore/src/types/transaction_import.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// Copyright 2015, 2016 Ethcore (UK) Ltd.
// This file is part of Parity.

// Parity is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.

// Parity is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License
// along with Parity. If not, see <http://www.gnu.org/licenses/>.

//! Transaction import result related types

use ipc::binary::{BinaryConvertError, BinaryConvertable};
use std::collections::VecDeque;
use error::{TransactionError, Error};
use std::mem;
use util::Populatable;

#[derive(Debug, Clone, PartialEq)]
/// Represents the result of importing transaction.
pub enum TransactionImportResult {
/// Transaction was imported to current queue.
Current,
/// Transaction was imported to future queue.
Future
}

binary_fixed_size!(TransactionImportResult);

/// Api-level error for transaction import
#[derive(Debug, Clone, Binary)]
pub enum TransactionImportError {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this type necessary? Can't we just use TransactionError?

/// Transaction error
Transaction(TransactionError),
/// Other error
Other(String),
}

impl From<Error> for TransactionImportError {
fn from(e: Error) -> Self {
match e {
Error::Transaction(transaction_error) => TransactionImportError::Transaction(transaction_error),
_ => TransactionImportError::Other(format!("other block import error: {:?}", e)),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo, we should never do this type of conversion, cause it makes TransactionImportError more generic then Error

}
}
}
6 changes: 3 additions & 3 deletions parity/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,8 @@ use rustc_serialize::hex::FromHex;
use ctrlc::CtrlC;
use util::{H256, ToPretty, NetworkConfiguration, PayloadInfo, Bytes, UtilError, paint, Colour, version};
use util::panics::{MayPanic, ForwardPanic, PanicHandler};
use ethcore::client::{BlockID, BlockChainClient, ClientConfig, get_db_path};
use ethcore::error::{Error, ImportError};
use ethcore::client::{BlockID, BlockChainClient, ClientConfig, get_db_path, BlockImportError};
use ethcore::error::{ImportError};
use ethcore::service::ClientService;
use ethcore::spec::Spec;
use ethsync::EthSync;
Expand Down Expand Up @@ -465,7 +465,7 @@ fn execute_import(conf: Configuration) {
while client.queue_info().is_full() { sleep(Duration::from_secs(1)); }
match client.import_block(bytes) {
Ok(_) => {}
Err(Error::Import(ImportError::AlreadyInChain)) => { trace!("Skipping block already in chain."); }
Err(BlockImportError::Import(ImportError::AlreadyInChain)) => { trace!("Skipping block already in chain."); }
Err(e) => die!("Cannot import block: {:?}", e)
}
informant.tick(client.deref(), None);
Expand Down
Loading