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

Filling-in optional fields of TransactionRequest... #1305

Merged
merged 20 commits into from
Jun 18, 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
2 changes: 1 addition & 1 deletion parity/rpc_apis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ pub fn setup_rpc<T: Extendable>(server: T, deps: Arc<Dependencies>, apis: ApiSet
server.add_delegate(EthFilterClient::new(&deps.client, &deps.miner).to_delegate());

if deps.signer_port.is_some() {
server.add_delegate(EthSigningQueueClient::new(&deps.signer_queue).to_delegate());
server.add_delegate(EthSigningQueueClient::new(&deps.signer_queue, &deps.miner).to_delegate());
} else {
server.add_delegate(EthSigningUnsafeClient::new(&deps.client, &deps.secret_store, &deps.miner).to_delegate());
}
Expand Down
27 changes: 22 additions & 5 deletions rpc/src/v1/impls/eth_signing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,25 +24,40 @@ use util::numbers::*;
use util::keys::store::AccountProvider;
use v1::helpers::{SigningQueue, ConfirmationsQueue};
use v1::traits::EthSigning;
use v1::types::TransactionRequest;
use v1::types::{TransactionRequest, Bytes};
use v1::impls::{sign_and_dispatch, error_codes};


/// Implementation of functions that require signing when no trusted signer is used.
pub struct EthSigningQueueClient {
pub struct EthSigningQueueClient<M: MinerService> {
queue: Weak<ConfirmationsQueue>,
miner: Weak<M>,
}

impl EthSigningQueueClient {
impl<M: MinerService> EthSigningQueueClient<M> {
/// Creates a new signing queue client given shared signing queue.
pub fn new(queue: &Arc<ConfirmationsQueue>) -> Self {
pub fn new(queue: &Arc<ConfirmationsQueue>, miner: &Arc<M>) -> Self {
EthSigningQueueClient {
queue: Arc::downgrade(queue),
miner: Arc::downgrade(miner),
}
}

fn fill_optional_fields(&self, miner: Arc<M>, mut request: TransactionRequest) -> TransactionRequest {
Copy link
Contributor

@rphmeier rphmeier Jun 16, 2016

Choose a reason for hiding this comment

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

could be worthwhile at some point to make a type-level distinction between requests with fields potentially missing and fully fleshed-out requests, making it impossible to even attempt to use one which isn't complete.

Copy link
Collaborator Author

@tomusdrw tomusdrw Jun 17, 2016

Choose a reason for hiding this comment

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

Yup. In fact it would be best to fill those options in UI, but some of the stuff is not exposed yet. I will create an issue to refactor this part in the (near/post-release) future.

if let None = request.gas {
request.gas = Some(miner.sensible_gas_limit());
Copy link
Contributor

@gavofyork gavofyork Jun 18, 2016

Choose a reason for hiding this comment

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

no - miner.sensible_gas_price is now a fallback and should no longer be used directly. see the implementation of fn default_gas_price() in eth.rs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The logic here is the same as in impls/mod.rs#sign_and_dispatch, using signer does not change this behaviour.
Replacing this can go in a separate PR.

}
if let None = request.gas_price {
request.gas_price = Some(miner.sensible_gas_price());
Copy link
Contributor

Choose a reason for hiding this comment

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

trace! to help with the reasons that request state mutated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's added in ../impls/mod.rs when sending transaction anyhow, it's not really muted it's just filled in (cause dapp developer left this to us to fill reasonable default)

Copy link
Contributor

Choose a reason for hiding this comment

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

well note that value comes from some algorithm and was not set by the user can be helpful, but it's up to you

}
if let None = request.data {
request.data = Some(Bytes::new(Vec::new()));
}
request
}
}

impl EthSigning for EthSigningQueueClient {
impl<M: MinerService + 'static> EthSigning for EthSigningQueueClient<M> {

fn sign(&self, _params: Params) -> Result<Value, Error> {
warn!("Invoking eth_sign is not yet supported with signer enabled.");
Expand All @@ -54,6 +69,8 @@ impl EthSigning for EthSigningQueueClient {
from_params::<(TransactionRequest, )>(params)
.and_then(|(request, )| {
let queue = take_weak!(self.queue);
let miner = take_weak!(self.miner);
let request = self.fill_optional_fields(miner, request);
let id = queue.add_request(request);
let result = id.wait_with_timeout();
result.unwrap_or_else(|| to_value(&H256::new()))
Expand Down
6 changes: 5 additions & 1 deletion rpc/src/v1/tests/mocked/eth_signing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,25 @@ use jsonrpc_core::IoHandler;
use v1::impls::EthSigningQueueClient;
use v1::traits::EthSigning;
use v1::helpers::{ConfirmationsQueue, SigningQueue};
use v1::tests::helpers::TestMinerService;
use util::keys::TestAccount;

struct EthSigningTester {
pub queue: Arc<ConfirmationsQueue>,
pub miner: Arc<TestMinerService>,
pub io: IoHandler,
}

impl Default for EthSigningTester {
fn default() -> Self {
let queue = Arc::new(ConfirmationsQueue::default());
let miner = Arc::new(TestMinerService::default());
let io = IoHandler::new();
io.add_delegate(EthSigningQueueClient::new(&queue).to_delegate());
io.add_delegate(EthSigningQueueClient::new(&queue, &miner).to_delegate());

EthSigningTester {
queue: queue,
miner: miner,
io: io,
}
}
Expand Down
4 changes: 2 additions & 2 deletions sync/src/chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -458,12 +458,12 @@ impl ChainSync {
// Disable the peer for this syncing round if it gives invalid chain
if !valid_response {
trace!(target: "sync", "{} Deactivated for invalid headers response", peer_id);
self.deactivate_peer(io, peer_id);
self.deactivate_peer(io, peer_id);
}
if headers.is_empty() {
// Peer does not have any new subchain heads, deactivate it nd try with another
trace!(target: "sync", "{} Deactivated for no data", peer_id);
self.deactivate_peer(io, peer_id);
self.deactivate_peer(io, peer_id);
}
match self.state {
SyncState::ChainHead => {
Expand Down