From bcf0e23a4bfd6c19dd7af380096c0083ded03a50 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Thu, 9 Feb 2017 13:24:03 +0100 Subject: [PATCH 1/4] take_weakf --- rpc/src/v1/mod.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/rpc/src/v1/mod.rs b/rpc/src/v1/mod.rs index f278718ae..44f942cb5 100644 --- a/rpc/src/v1/mod.rs +++ b/rpc/src/v1/mod.rs @@ -22,7 +22,16 @@ macro_rules! take_weak { ($weak: expr) => { match $weak.upgrade() { Some(arc) => arc, - None => return Err(Error::internal_error()) + None => return Err(Error::internal_error()), + } + } +} + +macro_rules! take_weakf { + ($weak: expr) => { + match $weak.upgrade() { + Some(arc) => arc, + None => return ::futures::future::err(Error::internal_error()), } } } From bce6bf92d97f2694bcc70b5ed8f8b9edad9be7b8 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Thu, 9 Feb 2017 15:01:15 +0100 Subject: [PATCH 2/4] simplify code --- rpc/src/v1/impls/personal.rs | 33 +++++++++----------- rpc/src/v1/impls/signing.rs | 49 +++++++++++++----------------- rpc/src/v1/impls/signing_unsafe.rs | 24 +++++---------- rpc/src/v1/mod.rs | 4 ++- 4 files changed, 46 insertions(+), 64 deletions(-) diff --git a/rpc/src/v1/impls/personal.rs b/rpc/src/v1/impls/personal.rs index 774e63d13..03ce5ffeb 100644 --- a/rpc/src/v1/impls/personal.rs +++ b/rpc/src/v1/impls/personal.rs @@ -95,35 +95,30 @@ impl Personal for PersonalClient { } fn send_transaction(&self, meta: Metadata, request: TransactionRequest, password: String) -> BoxFuture { - let setup = || { - let dispatcher = self.dispatcher.clone(); - let accounts = take_weak!(self.accounts); + let dispatcher = self.dispatcher.clone(); + let accounts = take_weakf!(self.accounts); - Ok((accounts, dispatcher)) + let default = match request.from.as_ref() { + Some(account) => Ok(account.clone().into()), + None => accounts + .default_address(meta.dapp_id.unwrap_or_default().into()) + .map_err(|e| errors::account("Cannot find default account.", e)), }; - future::done(setup()) - .and_then(move |(accounts, dispatcher)| { - let default = match request.from.as_ref() { - Some(account) => Ok(account.clone().into()), - None => accounts - .default_address(meta.dapp_id.unwrap_or_default().into()) - .map_err(|e| errors::account("Cannot find default account.", e)), - }; + let default = match default { + Ok(default) => default, + Err(e) => return future::err(e).boxed(), + }; - let dis = dispatcher.clone(); - future::done(default) - .and_then(move |default| dis.fill_optional_fields(request.into(), default)) - .map(move |tx| (tx, accounts, dispatcher)) - }) - .and_then(move |(filled, accounts, dispatcher)| { + dispatcher.fill_optional_fields(request.into(), default) + .and_then(move |filled| { let condition = filled.condition.clone().map(Into::into); dispatcher.sign(&accounts, filled, SignWith::Password(password)) .map(|tx| tx.into_value()) .map(move |tx| PendingTransaction::new(tx, condition)) .map(move |tx| (tx, dispatcher)) }) - .and_then(move |(pending_tx, dispatcher)| { + .and_then(|(pending_tx, dispatcher)| { let network_id = pending_tx.network_id(); trace!(target: "miner", "send_transaction: dispatching tx: {} for network ID {:?}", ::rlp::encode(&*pending_tx).to_vec().pretty(), network_id); diff --git a/rpc/src/v1/impls/signing.rs b/rpc/src/v1/impls/signing.rs index 18833723b..e646005cd 100644 --- a/rpc/src/v1/impls/signing.rs +++ b/rpc/src/v1/impls/signing.rs @@ -82,36 +82,29 @@ impl SigningQueueClient { } fn dispatch(&self, payload: RpcConfirmationPayload, default_account: DefaultAccount) -> BoxFuture { - let setup = move || { - let accounts = take_weak!(self.accounts); - let default_account = default_account; - let default_account = match default_account { - DefaultAccount::Provided(acc) => acc, - DefaultAccount::ForDapp(dapp) => accounts.default_address(dapp).ok().unwrap_or_default(), - }; - - Ok((self.dispatcher.clone(), accounts, default_account)) + let accounts = take_weakf!(self.accounts); + let default_account = match default_account { + DefaultAccount::Provided(acc) => acc, + DefaultAccount::ForDapp(dapp) => accounts.default_address(dapp).ok().unwrap_or_default(), }; - let weak_signer = self.signer.clone(); - future::done(setup()) - .and_then(move |(dispatcher, accounts, default_account)| { - dispatch::from_rpc(payload, default_account, &dispatcher) - .and_then(move |payload| { - let sender = payload.sender(); - if accounts.is_unlocked(sender) { - dispatch::execute(dispatcher, &accounts, payload, dispatch::SignWith::Nothing) - .map(|v| v.into_value()) - .map(DispatchResult::Value) - .boxed() - } else { - future::lazy(move || - take_weak!(weak_signer).add_request(payload) - .map(DispatchResult::Promise) - .map_err(|_| errors::request_rejected_limit()) - ).boxed() - } - }) + let dispatcher = self.dispatcher.clone(); + let signer = take_weakf!(self.signer); + dispatch::from_rpc(payload, default_account, &dispatcher) + .and_then(move |payload| { + let sender = payload.sender(); + if accounts.is_unlocked(sender) { + dispatch::execute(dispatcher, &accounts, payload, dispatch::SignWith::Nothing) + .map(|v| v.into_value()) + .map(DispatchResult::Value) + .boxed() + } else { + future::done( + signer.add_request(payload) + .map(DispatchResult::Promise) + .map_err(|_| errors::request_rejected_limit()) + ).boxed() + } }) .boxed() } diff --git a/rpc/src/v1/impls/signing_unsafe.rs b/rpc/src/v1/impls/signing_unsafe.rs index 6ae1cce30..333b823f9 100644 --- a/rpc/src/v1/impls/signing_unsafe.rs +++ b/rpc/src/v1/impls/signing_unsafe.rs @@ -52,26 +52,18 @@ impl SigningUnsafeClient { } fn handle(&self, payload: RpcConfirmationPayload, account: DefaultAccount) -> BoxFuture { - let setup = move || { - let accounts = take_weak!(self.accounts); - let default_account = account; - let default_account = match default_account { - DefaultAccount::Provided(acc) => acc, - DefaultAccount::ForDapp(dapp) => accounts.default_address(dapp).ok().unwrap_or_default(), - }; - - Ok((accounts, default_account)) + let accounts = take_weakf!(self.accounts); + let default = match account { + DefaultAccount::Provided(acc) => acc, + DefaultAccount::ForDapp(dapp) => accounts.default_address(dapp).ok().unwrap_or_default(), }; let dis = self.dispatcher.clone(); - future::done(setup()) - .and_then(move |(accounts, default)| { - dispatch::from_rpc(payload, default, &dis) - .and_then(move |payload| { - dispatch::execute(dis, &accounts, payload, dispatch::SignWith::Nothing) - }) - .map(|v| v.into_value()) + dispatch::from_rpc(payload, default, &dis) + .and_then(move |payload| { + dispatch::execute(dis, &accounts, payload, dispatch::SignWith::Nothing) }) + .map(|v| v.into_value()) .boxed() } } diff --git a/rpc/src/v1/mod.rs b/rpc/src/v1/mod.rs index 44f942cb5..b54f444c1 100644 --- a/rpc/src/v1/mod.rs +++ b/rpc/src/v1/mod.rs @@ -18,6 +18,7 @@ //! //! Compliant with ethereum rpc. +// Upgrade a weak pointer, returning an error on failure. macro_rules! take_weak { ($weak: expr) => { match $weak.upgrade() { @@ -27,11 +28,12 @@ macro_rules! take_weak { } } +// Upgrade a weak pointer, returning an error leaf-future on failure. macro_rules! take_weakf { ($weak: expr) => { match $weak.upgrade() { Some(arc) => arc, - None => return ::futures::future::err(Error::internal_error()), + None => return ::futures::future::err(Error::internal_error()).boxed(), } } } From c83d27420c2ce46ad0743a6b2788f0d656a8c871 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Thu, 9 Feb 2017 15:10:14 +0100 Subject: [PATCH 3/4] address grumbles --- rpc/src/v1/helpers/dispatch.rs | 2 +- rpc/src/v1/impls/signing.rs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/rpc/src/v1/helpers/dispatch.rs b/rpc/src/v1/helpers/dispatch.rs index 36d5ad864..47f5b4b9c 100644 --- a/rpc/src/v1/helpers/dispatch.rs +++ b/rpc/src/v1/helpers/dispatch.rs @@ -151,7 +151,7 @@ impl Dispatcher for FullDispatcher ParitySigning for SigningQueueClient { }); // and wait for that to resolve. - p.then(|result| futures::done(result.expect("Ready is never dropped nor canceled."))).boxed() + p.then(|result| futures::done(result.expect("Ready is never dropped nor canceled."))) }).boxed() } } @@ -195,7 +195,7 @@ impl EthSigning for SigningQueueClient { } }); - p.then(|result| futures::done(result.expect("Ready is never dropped nor canceled."))).boxed() + p.then(|result| futures::done(result.expect("Ready is never dropped nor canceled."))) }).boxed() } @@ -213,7 +213,7 @@ impl EthSigning for SigningQueueClient { } }); - p.then(|result| futures::done(result.expect("Ready is never dropped nor canceled."))).boxed() + p.then(|result| futures::done(result.expect("Ready is never dropped nor canceled."))) }).boxed() } @@ -231,7 +231,7 @@ impl EthSigning for SigningQueueClient { } }); - p.then(|result| futures::done(result.expect("Ready is never dropped nor canceled."))).boxed() + p.then(|result| futures::done(result.expect("Ready is never dropped nor canceled."))) }).boxed() } } From 0d09a473a70ef690744949c73d861c3c6b84dc86 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Thu, 9 Feb 2017 20:38:43 +0100 Subject: [PATCH 4/4] remove let inner pattern with take_weakf and try_bf --- rpc/src/v1/helpers/dispatch.rs | 79 +++++++++----------- rpc/src/v1/impls/eth.rs | 133 +++++++++++++-------------------- rpc/src/v1/mod.rs | 11 +++ 3 files changed, 101 insertions(+), 122 deletions(-) diff --git a/rpc/src/v1/helpers/dispatch.rs b/rpc/src/v1/helpers/dispatch.rs index 47f5b4b9c..0fbe318d4 100644 --- a/rpc/src/v1/helpers/dispatch.rs +++ b/rpc/src/v1/helpers/dispatch.rs @@ -92,57 +92,50 @@ impl Dispatcher for FullDispatcher BoxFuture { - let inner = move || { - let (client, miner) = (take_weak!(self.client), take_weak!(self.miner)); - let request = request; - Ok(FilledTransactionRequest { - from: request.from.unwrap_or(default_sender), - used_default_from: request.from.is_none(), - to: request.to, - nonce: request.nonce, - gas_price: request.gas_price.unwrap_or_else(|| default_gas_price(&*client, &*miner)), - gas: request.gas.unwrap_or_else(|| miner.sensible_gas_limit()), - value: request.value.unwrap_or_else(|| 0.into()), - data: request.data.unwrap_or_else(Vec::new), - condition: request.condition, - }) - }; - future::done(inner()).boxed() + let (client, miner) = (take_weakf!(self.client), take_weakf!(self.miner)); + let request = request; + future::ok(FilledTransactionRequest { + from: request.from.unwrap_or(default_sender), + used_default_from: request.from.is_none(), + to: request.to, + nonce: request.nonce, + gas_price: request.gas_price.unwrap_or_else(|| default_gas_price(&*client, &*miner)), + gas: request.gas.unwrap_or_else(|| miner.sensible_gas_limit()), + value: request.value.unwrap_or_else(|| 0.into()), + data: request.data.unwrap_or_else(Vec::new), + condition: request.condition, + }).boxed() } fn sign(&self, accounts: &AccountProvider, filled: FilledTransactionRequest, password: SignWith) -> BoxFuture, Error> { - let inner = move || { - let (client, miner) = (take_weak!(self.client), take_weak!(self.miner)); - let network_id = client.signing_network_id(); - let address = filled.from; - let signed_transaction = { - let t = Transaction { - nonce: filled.nonce - .or_else(|| miner - .last_nonce(&filled.from) - .map(|nonce| nonce + U256::one())) - .unwrap_or_else(|| client.latest_nonce(&filled.from)), + let (client, miner) = (take_weakf!(self.client), take_weakf!(self.miner)); + let network_id = client.signing_network_id(); + let address = filled.from; + future::ok({ + let t = Transaction { + nonce: filled.nonce + .or_else(|| miner + .last_nonce(&filled.from) + .map(|nonce| nonce + U256::one())) + .unwrap_or_else(|| client.latest_nonce(&filled.from)), - action: filled.to.map_or(Action::Create, Action::Call), - gas: filled.gas, - gas_price: filled.gas_price, - value: filled.value, - data: filled.data, - }; - - let hash = t.hash(network_id); - let signature = signature(accounts, address, hash, password)?; - signature.map(|sig| { - SignedTransaction::new(t.with_signature(sig, network_id)) - .expect("Transaction was signed by AccountsProvider; it never produces invalid signatures; qed") - }) + action: filled.to.map_or(Action::Create, Action::Call), + gas: filled.gas, + gas_price: filled.gas_price, + value: filled.value, + data: filled.data, }; - Ok(signed_transaction) - }; - future::done(inner()).boxed() + let hash = t.hash(network_id); + let signature = try_bf!(signature(accounts, address, hash, password)); + + signature.map(|sig| { + SignedTransaction::new(t.with_signature(sig, network_id)) + .expect("Transaction was signed by AccountsProvider; it never produces invalid signatures; qed") + }) + }).boxed() } fn dispatch_transaction(&self, signed_transaction: PendingTransaction) -> Result { diff --git a/rpc/src/v1/impls/eth.rs b/rpc/src/v1/impls/eth.rs index 3747c6192..7a954ccaf 100644 --- a/rpc/src/v1/impls/eth.rs +++ b/rpc/src/v1/impls/eth.rs @@ -359,133 +359,108 @@ impl Eth for EthClient where fn balance(&self, address: RpcH160, num: Trailing) -> BoxFuture { let address = address.into(); - let inner = || { - match num.0.clone() { - BlockNumber::Pending => Ok(take_weak!(self.miner).balance(&*take_weak!(self.client), &address).into()), - id => { - let client = take_weak!(self.client); + let res = match num.0.clone() { + BlockNumber::Pending => Ok(take_weakf!(self.miner).balance(&*take_weakf!(self.client), &address).into()), + id => { + let client = take_weakf!(self.client); - check_known(&*client, id.clone())?; - match client.balance(&address, id.into()) { - Some(balance) => Ok(balance.into()), - None => Err(errors::state_pruned()), - } + try_bf!(check_known(&*client, id.clone())); + match client.balance(&address, id.into()) { + Some(balance) => Ok(balance.into()), + None => Err(errors::state_pruned()), } } }; - future::done(inner()).boxed() + future::done(res).boxed() } fn storage_at(&self, address: RpcH160, pos: RpcU256, num: Trailing) -> BoxFuture { let address: Address = RpcH160::into(address); let position: U256 = RpcU256::into(pos); - let inner = || { - match num.0.clone() { - BlockNumber::Pending => Ok(take_weak!(self.miner).storage_at(&*take_weak!(self.client), &address, &H256::from(position)).into()), - id => { - let client = take_weak!(self.client); + let res = match num.0.clone() { + BlockNumber::Pending => Ok(take_weakf!(self.miner).storage_at(&*take_weakf!(self.client), &address, &H256::from(position)).into()), + id => { + let client = take_weakf!(self.client); - check_known(&*client, id.clone())?; - match client.storage_at(&address, &H256::from(position), id.into()) { - Some(s) => Ok(s.into()), - None => Err(errors::state_pruned()), - } + try_bf!(check_known(&*client, id.clone())); + match client.storage_at(&address, &H256::from(position), id.into()) { + Some(s) => Ok(s.into()), + None => Err(errors::state_pruned()), } } }; - future::done(inner()).boxed() + future::done(res).boxed() } fn transaction_count(&self, address: RpcH160, num: Trailing) -> BoxFuture { let address: Address = RpcH160::into(address); - let inner = move || { - match num.0.clone() { - BlockNumber::Pending => Ok(take_weak!(self.miner).nonce(&*take_weak!(self.client), &address).into()), - id => { - let client = take_weak!(self.client); + let res = match num.0.clone() { + BlockNumber::Pending => Ok(take_weakf!(self.miner).nonce(&*take_weakf!(self.client), &address).into()), + id => { + let client = take_weakf!(self.client); - check_known(&*client, id.clone())?; - match client.nonce(&address, id.into()) { - Some(nonce) => Ok(nonce.into()), - None => Err(errors::state_pruned()), - } + try_bf!(check_known(&*client, id.clone())); + match client.nonce(&address, id.into()) { + Some(nonce) => Ok(nonce.into()), + None => Err(errors::state_pruned()), } } }; - future::done(inner()).boxed() + future::done(res).boxed() } fn block_transaction_count_by_hash(&self, hash: RpcH256) -> BoxFuture, Error> { - let inner = || { - Ok(take_weak!(self.client).block(BlockId::Hash(hash.into())) - .map(|block| block.transactions_count().into())) - }; - - future::done(inner()).boxed() + future::ok(take_weakf!(self.client).block(BlockId::Hash(hash.into())) + .map(|block| block.transactions_count().into())).boxed() } fn block_transaction_count_by_number(&self, num: BlockNumber) -> BoxFuture, Error> { - let inner = || { - match num { - BlockNumber::Pending => Ok(Some( - take_weak!(self.miner).status().transactions_in_pending_block.into() - )), - _ => Ok( - take_weak!(self.client).block(num.into()) - .map(|block| block.transactions_count().into()) - ) - } - }; - - future::done(inner()).boxed() + future::ok(match num { + BlockNumber::Pending => Some( + take_weakf!(self.miner).status().transactions_in_pending_block.into() + ), + _ => + take_weakf!(self.client).block(num.into()) + .map(|block| block.transactions_count().into()) + }).boxed() } fn block_uncles_count_by_hash(&self, hash: RpcH256) -> BoxFuture, Error> { - let inner = || { - Ok(take_weak!(self.client).block(BlockId::Hash(hash.into())) + future::ok(take_weakf!(self.client).block(BlockId::Hash(hash.into())) .map(|block| block.uncles_count().into())) - }; - - future::done(inner()).boxed() + .boxed() } fn block_uncles_count_by_number(&self, num: BlockNumber) -> BoxFuture, Error> { - let inner = || { - match num { - BlockNumber::Pending => Ok(Some(0.into())), - _ => Ok( - take_weak!(self.client).block(num.into()) - .map(|block| block.uncles_count().into()) - ), - } - }; - - future::done(inner()).boxed() + future::ok(match num { + BlockNumber::Pending => Some(0.into()), + _ => take_weakf!(self.client).block(num.into()) + .map(|block| block.uncles_count().into() + ), + }).boxed() } fn code_at(&self, address: RpcH160, num: Trailing) -> BoxFuture { let address: Address = RpcH160::into(address); - let inner = || { - match num.0.clone() { - BlockNumber::Pending => Ok(take_weak!(self.miner).code(&*take_weak!(self.client), &address).map_or_else(Bytes::default, Bytes::new)), - id => { - let client = take_weak!(self.client); + let res = match num.0.clone() { + BlockNumber::Pending => Ok(take_weakf!(self.miner).code(&*take_weakf!(self.client), &address).map_or_else(Bytes::default, Bytes::new)), + id => { + let client = take_weakf!(self.client); - check_known(&*client, id.clone())?; - match client.code(&address, id.into()) { - Some(code) => Ok(code.map_or_else(Bytes::default, Bytes::new)), - None => Err(errors::state_pruned()), - } + try_bf!(check_known(&*client, id.clone())); + match client.code(&address, id.into()) { + Some(code) => Ok(code.map_or_else(Bytes::default, Bytes::new)), + None => Err(errors::state_pruned()), } } }; - future::done(inner()).boxed() + future::done(res).boxed() } fn block_by_hash(&self, hash: RpcH256, include_txs: bool) -> BoxFuture, Error> { diff --git a/rpc/src/v1/mod.rs b/rpc/src/v1/mod.rs index b54f444c1..c69acbea3 100644 --- a/rpc/src/v1/mod.rs +++ b/rpc/src/v1/mod.rs @@ -38,6 +38,17 @@ macro_rules! take_weakf { } } +// short for "try_boxfuture" +// unwrap a result, returning a BoxFuture<_, Err> on failure. +macro_rules! try_bf { + ($res: expr) => { + match $res { + Ok(val) => val, + Err(e) => return ::futures::future::err(e.into()).boxed(), + } + } +} + #[macro_use] mod helpers; mod impls;