-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Filling-in optional fields of TransactionRequest... #1305
Changes from all commits
efa8f66
b89888e
818b87e
dcd64f7
6803375
7284df9
5538527
87e9ca7
7649037
77c3e10
9603597
80e56cb
5a0d367
1e9da1e
88b0358
10bbe8c
07641b8
323de58
8a02b1e
ba24e80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
if let None = request.gas { | ||
request.gas = Some(miner.sensible_gas_limit()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic here is the same as in |
||
} | ||
if let None = request.gas_price { | ||
request.gas_price = Some(miner.sensible_gas_price()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's added in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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."); | ||
|
@@ -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())) | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.