From b98150526c9f099ca7ac4c627a4869cbcde887bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Tue, 24 Oct 2017 12:13:00 +0200 Subject: [PATCH] Fix serialization of non-localized transactions (#6868) * Fix serialization of non-localized transactions. * Return proper SignedTransactions representation. --- ethcore/src/transaction.rs | 5 +++++ rpc/src/v1/helpers/dispatch.rs | 27 ++++++++++++++++++++------- rpc/src/v1/helpers/light_fetch.rs | 23 +++++++++++++++++------ rpc/src/v1/impls/signer.rs | 7 ++++--- rpc/src/v1/tests/mocked/eth.rs | 4 ++-- rpc/src/v1/tests/mocked/parity_set.rs | 2 +- rpc/src/v1/tests/mocked/signer.rs | 2 +- rpc/src/v1/tests/mocked/signing.rs | 8 +++++--- rpc/src/v1/types/transaction.rs | 11 +++++------ 9 files changed, 60 insertions(+), 29 deletions(-) diff --git a/ethcore/src/transaction.rs b/ethcore/src/transaction.rs index 02f77d5f3..3596f6c21 100644 --- a/ethcore/src/transaction.rs +++ b/ethcore/src/transaction.rs @@ -469,6 +469,11 @@ impl SignedTransaction { pub fn is_unsigned(&self) -> bool { self.transaction.is_unsigned() } + + /// Deconstructs this transaction back into `UnverifiedTransaction` + pub fn deconstruct(self) -> (UnverifiedTransaction, Address, Option) { + (self.transaction, self.sender, self.public) + } } /// Signed Transaction that is a part of canon blockchain. diff --git a/rpc/src/v1/helpers/dispatch.rs b/rpc/src/v1/helpers/dispatch.rs index 0da257035..d84bd6cad 100644 --- a/rpc/src/v1/helpers/dispatch.rs +++ b/rpc/src/v1/helpers/dispatch.rs @@ -71,6 +71,9 @@ pub trait Dispatcher: Send + Sync + Clone { fn sign(&self, accounts: Arc, filled: FilledTransactionRequest, password: SignWith) -> BoxFuture, Error>; + /// Converts a `SignedTransaction` into `RichRawTransaction` + fn enrich(&self, SignedTransaction) -> RpcRichRawTransaction; + /// "Dispatch" a local transaction. fn dispatch_transaction(&self, signed_transaction: PendingTransaction) -> Result; } @@ -163,6 +166,11 @@ impl Dispatcher for FullDispatcher RpcRichRawTransaction { + let block_number = self.client.best_block_header().number(); + RpcRichRawTransaction::from_signed(signed_transaction, block_number, self.client.eip86_transition()) + } + fn dispatch_transaction(&self, signed_transaction: PendingTransaction) -> Result { let hash = signed_transaction.transaction.hash(); @@ -261,11 +269,11 @@ impl LightDispatcher { transaction_queue: Arc>, ) -> Self { LightDispatcher { - sync: sync, - client: client, - on_demand: on_demand, - cache: cache, - transaction_queue: transaction_queue, + sync, + client, + on_demand, + cache, + transaction_queue, } } @@ -399,6 +407,11 @@ impl Dispatcher for LightDispatcher { .and_then(move |nonce| with_nonce(filled, nonce))) } + fn enrich(&self, signed_transaction: SignedTransaction) -> RpcRichRawTransaction { + let block_number = self.client.best_block_header().number(); + RpcRichRawTransaction::from_signed(signed_transaction, block_number, self.client.eip86_transition()) + } + fn dispatch_transaction(&self, signed_transaction: PendingTransaction) -> Result { let hash = signed_transaction.transaction.hash(); @@ -510,8 +523,8 @@ pub fn execute( }, ConfirmationPayload::SignTransaction(request) => { Box::new(dispatcher.sign(accounts, request, pass) - .map(|result| result - .map(RpcRichRawTransaction::from) + .map(move |result| result + .map(move |tx| dispatcher.enrich(tx)) .map(ConfirmationResponse::SignTransaction) )) }, diff --git a/rpc/src/v1/helpers/light_fetch.rs b/rpc/src/v1/helpers/light_fetch.rs index ac5902b51..2544f66b0 100644 --- a/rpc/src/v1/helpers/light_fetch.rs +++ b/rpc/src/v1/helpers/light_fetch.rs @@ -23,7 +23,7 @@ use ethcore::encoded; use ethcore::executed::{Executed, ExecutionError}; use ethcore::ids::BlockId; use ethcore::filter::Filter as EthcoreFilter; -use ethcore::transaction::{Action, Transaction as EthTransaction, SignedTransaction}; +use ethcore::transaction::{Action, Transaction as EthTransaction, SignedTransaction, LocalizedTransaction}; use ethcore::receipt::Receipt; use jsonrpc_core::{BoxFuture, Error}; @@ -65,13 +65,24 @@ pub struct LightFetch { /// Extract a transaction at given index. pub fn extract_transaction_at_index(block: encoded::Block, index: usize, eip86_transition: u64) -> Option { block.transactions().into_iter().nth(index) + // Verify if transaction signature is correct. .and_then(|tx| SignedTransaction::new(tx).ok()) - .map(|tx| Transaction::from_signed(tx, block.number(), eip86_transition)) - .map(|mut tx| { - tx.block_hash = Some(block.hash().into()); - tx.transaction_index = Some(index.into()); - tx + .map(|signed_tx| { + let (signed, sender, _) = signed_tx.deconstruct(); + let block_hash = block.hash(); + let block_number = block.number(); + let transaction_index = index; + let cached_sender = Some(sender); + + LocalizedTransaction { + signed, + block_number, + block_hash, + transaction_index, + cached_sender, + } }) + .map(|tx| Transaction::from_localized(tx, eip86_transition)) } diff --git a/rpc/src/v1/impls/signer.rs b/rpc/src/v1/impls/signer.rs index c864134fb..cecd1f2db 100644 --- a/rpc/src/v1/impls/signer.rs +++ b/rpc/src/v1/impls/signer.rs @@ -73,8 +73,8 @@ impl SignerClient { SignerClient { signer: signer.clone(), accounts: store.clone(), - dispatcher: dispatcher, - subscribers: subscribers, + dispatcher, + subscribers, } } @@ -205,7 +205,8 @@ impl Signer for SignerClient { }, ConfirmationPayload::SignTransaction(request) => { Self::verify_transaction(bytes, request, |pending_transaction| { - Ok(ConfirmationResponse::SignTransaction(pending_transaction.transaction.into())) + let rich = self.dispatcher.enrich(pending_transaction.transaction); + Ok(ConfirmationResponse::SignTransaction(rich)) }) }, ConfirmationPayload::EthSignMessage(address, data) => { diff --git a/rpc/src/v1/tests/mocked/eth.rs b/rpc/src/v1/tests/mocked/eth.rs index 337ef9513..9969ee286 100644 --- a/rpc/src/v1/tests/mocked/eth.rs +++ b/rpc/src/v1/tests/mocked/eth.rs @@ -547,7 +547,7 @@ fn rpc_eth_pending_transaction_by_hash() { tester.miner.pending_transactions.lock().insert(H256::zero(), tx); } - let response = r#"{"jsonrpc":"2.0","result":{"blockHash":null,"blockNumber":"0x0","chainId":null,"condition":null,"creates":null,"from":"0x0f65fe9276bc9a24ae7083ae28e2660ef72df99e","gas":"0x5208","gasPrice":"0x1","hash":"0x41df922fd0d4766fcc02e161f8295ec28522f329ae487f14d811e4b64c8d6e31","input":"0x","nonce":"0x0","publicKey":"0x7ae46da747962c2ee46825839c1ef9298e3bd2e70ca2938495c3693a485ec3eaa8f196327881090ff64cf4fbb0a48485d4f83098e189ed3b7a87d5941b59f789","r":"0x48b55bfa915ac795c431978d8a6a992b628d557da5ff759b307d495a36649353","raw":"0xf85f800182520894095e7baea6a6c7c4c2dfeb977efac326af552d870a801ba048b55bfa915ac795c431978d8a6a992b628d557da5ff759b307d495a36649353a0efffd310ac743f371de3b9f7f9cb56c0b28ad43601b4ab949f53faa07bd2c804","s":"0xefffd310ac743f371de3b9f7f9cb56c0b28ad43601b4ab949f53faa07bd2c804","standardV":"0x0","to":"0x095e7baea6a6c7c4c2dfeb977efac326af552d87","transactionIndex":null,"v":"0x1b","value":"0xa"},"id":1}"#; + let response = r#"{"jsonrpc":"2.0","result":{"blockHash":null,"blockNumber":null,"chainId":null,"condition":null,"creates":null,"from":"0x0f65fe9276bc9a24ae7083ae28e2660ef72df99e","gas":"0x5208","gasPrice":"0x1","hash":"0x41df922fd0d4766fcc02e161f8295ec28522f329ae487f14d811e4b64c8d6e31","input":"0x","nonce":"0x0","publicKey":"0x7ae46da747962c2ee46825839c1ef9298e3bd2e70ca2938495c3693a485ec3eaa8f196327881090ff64cf4fbb0a48485d4f83098e189ed3b7a87d5941b59f789","r":"0x48b55bfa915ac795c431978d8a6a992b628d557da5ff759b307d495a36649353","raw":"0xf85f800182520894095e7baea6a6c7c4c2dfeb977efac326af552d870a801ba048b55bfa915ac795c431978d8a6a992b628d557da5ff759b307d495a36649353a0efffd310ac743f371de3b9f7f9cb56c0b28ad43601b4ab949f53faa07bd2c804","s":"0xefffd310ac743f371de3b9f7f9cb56c0b28ad43601b4ab949f53faa07bd2c804","standardV":"0x0","to":"0x095e7baea6a6c7c4c2dfeb977efac326af552d87","transactionIndex":null,"v":"0x1b","value":"0xa"},"id":1}"#; let request = r#"{ "jsonrpc": "2.0", "method": "eth_getTransactionByHash", @@ -863,7 +863,7 @@ fn rpc_eth_sign_transaction() { let response = r#"{"jsonrpc":"2.0","result":{"#.to_owned() + r#""raw":"0x"# + &rlp.to_hex() + r#"","# + r#""tx":{"# + - r#""blockHash":null,"blockNumber":"0x0","# + + r#""blockHash":null,"blockNumber":null,"# + &format!("\"chainId\":{},", t.chain_id().map_or("null".to_owned(), |n| format!("{}", n))) + r#""condition":null,"creates":null,"# + &format!("\"from\":\"0x{:?}\",", &address) + diff --git a/rpc/src/v1/tests/mocked/parity_set.rs b/rpc/src/v1/tests/mocked/parity_set.rs index 1653a1908..ed27862ac 100644 --- a/rpc/src/v1/tests/mocked/parity_set.rs +++ b/rpc/src/v1/tests/mocked/parity_set.rs @@ -234,7 +234,7 @@ fn rpc_parity_remove_transaction() { let hash = signed.hash(); let request = r#"{"jsonrpc": "2.0", "method": "parity_removeTransaction", "params":[""#.to_owned() + &format!("0x{:?}", hash) + r#""], "id": 1}"#; - let response = r#"{"jsonrpc":"2.0","result":{"blockHash":null,"blockNumber":"0x0","chainId":null,"condition":null,"creates":null,"from":"0x0000000000000000000000000000000000000002","gas":"0x76c0","gasPrice":"0x9184e72a000","hash":"0xa2e0da8a8064e0b9f93e95a53c2db6d01280efb8ac72a708d25487e67dd0f8fc","input":"0x","nonce":"0x1","publicKey":null,"r":"0x1","raw":"0xe9018609184e72a0008276c0940000000000000000000000000000000000000005849184e72a80800101","s":"0x1","standardV":"0x4","to":"0x0000000000000000000000000000000000000005","transactionIndex":null,"v":"0x0","value":"0x9184e72a"},"id":1}"#; + let response = r#"{"jsonrpc":"2.0","result":{"blockHash":null,"blockNumber":null,"chainId":null,"condition":null,"creates":null,"from":"0x0000000000000000000000000000000000000002","gas":"0x76c0","gasPrice":"0x9184e72a000","hash":"0xa2e0da8a8064e0b9f93e95a53c2db6d01280efb8ac72a708d25487e67dd0f8fc","input":"0x","nonce":"0x1","publicKey":null,"r":"0x1","raw":"0xe9018609184e72a0008276c0940000000000000000000000000000000000000005849184e72a80800101","s":"0x1","standardV":"0x4","to":"0x0000000000000000000000000000000000000005","transactionIndex":null,"v":"0x0","value":"0x9184e72a"},"id":1}"#; miner.pending_transactions.lock().insert(hash, signed); assert_eq!(io.handle_request_sync(&request), Some(response.to_owned())); diff --git a/rpc/src/v1/tests/mocked/signer.rs b/rpc/src/v1/tests/mocked/signer.rs index 0211b5f24..3399cd741 100644 --- a/rpc/src/v1/tests/mocked/signer.rs +++ b/rpc/src/v1/tests/mocked/signer.rs @@ -456,7 +456,7 @@ fn should_confirm_sign_transaction_with_rlp() { let response = r#"{"jsonrpc":"2.0","result":{"#.to_owned() + r#""raw":"0x"# + &rlp.to_hex() + r#"","# + r#""tx":{"# + - r#""blockHash":null,"blockNumber":"0x0","# + + r#""blockHash":null,"blockNumber":null,"# + &format!("\"chainId\":{},", t.chain_id().map_or("null".to_owned(), |n| format!("{}", n))) + r#""condition":null,"creates":null,"# + &format!("\"from\":\"0x{:?}\",", &address) + diff --git a/rpc/src/v1/tests/mocked/signing.rs b/rpc/src/v1/tests/mocked/signing.rs index 41c8ec7a0..dbe5534ca 100644 --- a/rpc/src/v1/tests/mocked/signing.rs +++ b/rpc/src/v1/tests/mocked/signing.rs @@ -26,7 +26,7 @@ use v1::impls::SigningQueueClient; use v1::metadata::Metadata; use v1::traits::{EthSigning, ParitySigning, Parity}; use v1::helpers::{SignerService, SigningQueue, FullDispatcher}; -use v1::types::ConfirmationResponse; +use v1::types::{ConfirmationResponse, RichRawTransaction}; use v1::tests::helpers::TestMinerService; use v1::tests::mocked::parity; @@ -307,7 +307,7 @@ fn should_add_sign_transaction_to_the_queue() { let response = r#"{"jsonrpc":"2.0","result":{"#.to_owned() + r#""raw":"0x"# + &rlp.to_hex() + r#"","# + r#""tx":{"# + - r#""blockHash":null,"blockNumber":"0x0","# + + r#""blockHash":null,"blockNumber":null,"# + &format!("\"chainId\":{},", t.chain_id().map_or("null".to_owned(), |n| format!("{}", n))) + r#""condition":null,"creates":null,"# + &format!("\"from\":\"0x{:?}\",", &address) + @@ -334,7 +334,9 @@ fn should_add_sign_transaction_to_the_queue() { ::std::thread::spawn(move || loop { if signer.requests().len() == 1 { // respond - signer.request_confirmed(1.into(), Ok(ConfirmationResponse::SignTransaction(t.into()))); + signer.request_confirmed(1.into(), Ok(ConfirmationResponse::SignTransaction( + RichRawTransaction::from_signed(t.into(), 0x0, u64::max_value()) + ))); break } ::std::thread::sleep(Duration::from_millis(100)) diff --git a/rpc/src/v1/types/transaction.rs b/rpc/src/v1/types/transaction.rs index 570c21120..dbd326488 100644 --- a/rpc/src/v1/types/transaction.rs +++ b/rpc/src/v1/types/transaction.rs @@ -158,11 +158,10 @@ pub struct RichRawTransaction { pub transaction: Transaction } - -impl From for RichRawTransaction { - fn from(t: SignedTransaction) -> Self { - // TODO: change transition to 0 when EIP-86 is commonly used. - let tx: Transaction = Transaction::from_signed(t, 0, u64::max_value()); +impl RichRawTransaction { + /// Creates new `RichRawTransaction` from `SignedTransaction`. + pub fn from_signed(tx: SignedTransaction, block_number: u64, eip86_transition: u64) -> Self { + let tx = Transaction::from_signed(tx, block_number, eip86_transition); RichRawTransaction { raw: tx.raw.clone(), transaction: tx, @@ -213,7 +212,7 @@ impl Transaction { hash: t.hash().into(), nonce: t.nonce.into(), block_hash: None, - block_number: Some(block_number.into()), + block_number: None, transaction_index: None, from: t.sender().into(), to: match t.action {