From d14d590c2bcb3c282ef5615615d56e9e536bec66 Mon Sep 17 00:00:00 2001 From: debris Date: Wed, 6 Apr 2016 12:15:20 +0200 Subject: [PATCH] fixed #875 and added tests for eth_sendTransaction --- miner/src/lib.rs | 3 ++ miner/src/miner.rs | 4 ++ miner/src/transaction_queue.rs | 30 +++++++++++ rpc/src/v1/impls/eth.rs | 14 ++++-- rpc/src/v1/impls/personal.rs | 2 +- rpc/src/v1/tests/eth.rs | 53 ++++++++++++++++++-- rpc/src/v1/tests/helpers/account_provider.rs | 33 ++++++++---- rpc/src/v1/tests/helpers/miner_service.rs | 51 ++++++++++++++----- rpc/src/v1/tests/personal.rs | 28 ++++++++--- 9 files changed, 181 insertions(+), 37 deletions(-) diff --git a/miner/src/lib.rs b/miner/src/lib.rs index 0daac48e6..9f757fb67 100644 --- a/miner/src/lib.rs +++ b/miner/src/lib.rs @@ -108,6 +108,9 @@ pub trait MinerService : Send + Sync { /// Query pending transactions for hash fn transaction(&self, hash: &H256) -> Option; + /// Returns highest transaction nonce for given address. + fn last_nonce(&self, address: &Address) -> Option; + /// Suggested gas price fn sensible_gas_price(&self) -> U256 { x!(20000000000u64) } } diff --git a/miner/src/miner.rs b/miner/src/miner.rs index 70bf1711a..aee344925 100644 --- a/miner/src/miner.rs +++ b/miner/src/miner.rs @@ -227,6 +227,10 @@ impl MinerService for Miner { queue.find(hash) } + fn last_nonce(&self, address: &Address) -> Option { + self.transaction_queue.lock().unwrap().last_nonce(address) + } + fn update_sealing(&self, chain: &BlockChainClient) { if self.sealing_enabled.load(atomic::Ordering::Relaxed) { let current_no = chain.chain_info().best_block_number; diff --git a/miner/src/transaction_queue.rs b/miner/src/transaction_queue.rs index 659e1a663..8cd9157ee 100644 --- a/miner/src/transaction_queue.rs +++ b/miner/src/transaction_queue.rs @@ -523,6 +523,11 @@ impl TransactionQueue { self.last_nonces.clear(); } + /// Returns highest transaction nonce for given address. + pub fn last_nonce(&self, address: &Address) -> Option { + self.last_nonces.get(address).cloned() + } + /// Checks if there are any transactions in `future` that should actually be promoted to `current` /// (because nonce matches). fn move_matching_future_to_current(&mut self, address: Address, mut current_nonce: U256, first_nonce: U256) { @@ -1255,4 +1260,29 @@ mod test { assert_eq!(stats.future, 0); assert_eq!(stats.pending, 1); } + + #[test] + fn should_return_none_when_transaction_from_given_address_does_not_exist() { + // given + let mut txq = TransactionQueue::new(); + + // then + assert_eq!(txq.last_nonce(&Address::default()), None); + } + + #[test] + fn should_return_correct_nonce_when_transactions_from_given_address_exist() { + // given + let mut txq = TransactionQueue::new(); + let tx = new_tx(); + let from = tx.sender().unwrap(); + let nonce = tx.nonce; + let details = |a: &Address| AccountDetails { nonce: nonce, balance: !U256::zero() }; + + // when + txq.add(tx, &details).unwrap(); + + // then + assert_eq!(txq.last_nonce(&from), Some(nonce)); + } } diff --git a/rpc/src/v1/impls/eth.rs b/rpc/src/v1/impls/eth.rs index dd241c9ec..1fff89ba8 100644 --- a/rpc/src/v1/impls/eth.rs +++ b/rpc/src/v1/impls/eth.rs @@ -184,11 +184,15 @@ impl EthClient fn dispatch_transaction(&self, signed_transaction: SignedTransaction, raw_transaction: Vec) -> Result { let hash = signed_transaction.hash(); - + let import = { + let miner = take_weak!(self.miner); let client = take_weak!(self.client); take_weak!(self.miner).import_transactions(vec![signed_transaction], |a: &Address| AccountDetails { - nonce: client.nonce(a), + nonce: miner + .last_nonce(a) + .map(|nonce| nonce + U256::one()) + .unwrap_or_else(|| client.nonce(a)), balance: client.balance(a), }) }; @@ -484,7 +488,11 @@ impl Eth for EthClient let client = take_weak!(self.client); let miner = take_weak!(self.miner); EthTransaction { - nonce: request.nonce.unwrap_or_else(|| client.nonce(&request.from)), + nonce: request.nonce + .or_else(|| miner + .last_nonce(&request.from) + .map(|nonce| nonce + U256::one())) + .unwrap_or_else(|| client.nonce(&request.from)), action: request.to.map_or(Action::Create, Action::Call), gas: request.gas.unwrap_or_else(default_gas), gas_price: request.gas_price.unwrap_or_else(|| miner.sensible_gas_price()), diff --git a/rpc/src/v1/impls/personal.rs b/rpc/src/v1/impls/personal.rs index 2822059d6..5bb0d3eee 100644 --- a/rpc/src/v1/impls/personal.rs +++ b/rpc/src/v1/impls/personal.rs @@ -49,7 +49,7 @@ impl Personal for PersonalClient where A: AccountProvider + 'static { |(pass, )| { let store = take_weak!(self.accounts); match store.new_account(&pass) { - Ok(address) => Ok(Value::String(format!("0x{:?}", address))), + Ok(address) => to_value(&address), Err(_) => Err(Error::internal_error()) } } diff --git a/rpc/src/v1/tests/eth.rs b/rpc/src/v1/tests/eth.rs index 40748d990..bde351486 100644 --- a/rpc/src/v1/tests/eth.rs +++ b/rpc/src/v1/tests/eth.rs @@ -23,6 +23,7 @@ use util::numbers::{Uint, U256}; use ethcore::client::{TestBlockChainClient, EachBlockWith, Executed, TransactionId}; use ethcore::log_entry::{LocalizedLogEntry, LogEntry}; use ethcore::receipt::LocalizedReceipt; +use ethcore::transaction::{Transaction, Action}; use v1::{Eth, EthClient}; use v1::tests::helpers::{TestAccount, TestAccountProvider, TestSyncProvider, Config, TestMinerService, TestExternalMiner}; @@ -52,7 +53,7 @@ fn miner_service() -> Arc { struct EthTester { pub client: Arc, pub sync: Arc, - _accounts_provider: Arc, + pub accounts_provider: Arc, miner: Arc, hashrates: Arc>>, pub io: IoHandler, @@ -72,7 +73,7 @@ impl Default for EthTester { EthTester { client: client, sync: sync, - _accounts_provider: ap, + accounts_provider: ap, miner: miner, io: io, hashrates: hashrates, @@ -453,9 +454,53 @@ fn rpc_eth_estimate_gas_default_block() { } #[test] -#[ignore] fn rpc_eth_send_transaction() { - unimplemented!() + let account = TestAccount::new("123"); + let address = account.address(); + let secret = account.secret.clone(); + + let tester = EthTester::default(); + tester.accounts_provider.accounts.write().unwrap().insert(address.clone(), account); + let request = r#"{ + "jsonrpc": "2.0", + "method": "eth_sendTransaction", + "params": [{ + "from": ""#.to_owned() + format!("0x{:?}", address).as_ref() + r#"", + "to": "0xd46e8dd67c5d32be8058bb8eb970870f07244567", + "gas": "0x76c0", + "gasPrice": "0x9184e72a000", + "value": "0x9184e72a" + }], + "id": 1 + }"#; + + let t = Transaction { + nonce: U256::zero(), + gas_price: U256::from(0x9184e72a000u64), + gas: U256::from(0x76c0), + action: Action::Call(Address::from_str("d46e8dd67c5d32be8058bb8eb970870f07244567").unwrap()), + value: U256::from(0x9184e72au64), + data: vec![] + }.sign(&secret); + + let response = r#"{"jsonrpc":"2.0","result":""#.to_owned() + format!("0x{:?}", t.hash()).as_ref() + r#"","id":1}"#; + + assert_eq!(tester.io.handle_request(request.as_ref()), Some(response)); + + tester.miner.last_nonces.write().unwrap().insert(address.clone(), U256::zero()); + + let t = Transaction { + nonce: U256::one(), + gas_price: U256::from(0x9184e72a000u64), + gas: U256::from(0x76c0), + action: Action::Call(Address::from_str("d46e8dd67c5d32be8058bb8eb970870f07244567").unwrap()), + value: U256::from(0x9184e72au64), + data: vec![] + }.sign(&secret); + + let response = r#"{"jsonrpc":"2.0","result":""#.to_owned() + format!("0x{:?}", t.hash()).as_ref() + r#"","id":1}"#; + + assert_eq!(tester.io.handle_request(request.as_ref()), Some(response)); } #[test] diff --git a/rpc/src/v1/tests/helpers/account_provider.rs b/rpc/src/v1/tests/helpers/account_provider.rs index 6ef6e2b59..cace69658 100644 --- a/rpc/src/v1/tests/helpers/account_provider.rs +++ b/rpc/src/v1/tests/helpers/account_provider.rs @@ -20,7 +20,7 @@ use std::sync::RwLock; use std::collections::HashMap; use std::io; use util::hash::{Address, H256, FixedHash}; -use util::crypto::{Secret, Signature}; +use util::crypto::{Secret, Signature, KeyPair}; use util::keys::store::{AccountProvider, SigningError, EncryptedHashMapError}; /// Account mock. @@ -30,23 +30,31 @@ pub struct TestAccount { pub unlocked: bool, /// Account's password. pub password: String, + /// Account's secret. + pub secret: Secret, } impl TestAccount { /// Creates new test account. pub fn new(password: &str) -> Self { + let pair = KeyPair::create().unwrap(); TestAccount { unlocked: false, password: password.to_owned(), + secret: pair.secret().clone() } } + + /// Returns account address. + pub fn address(&self) -> Address { + KeyPair::from_secret(self.secret.clone()).unwrap().address() + } } /// Test account provider. pub struct TestAccountProvider { - accounts: RwLock>, - /// Added accounts passwords. - pub adds: RwLock>, + /// Test provider accounts. + pub accounts: RwLock>, } impl TestAccountProvider { @@ -54,7 +62,6 @@ impl TestAccountProvider { pub fn new(accounts: HashMap) -> Self { TestAccountProvider { accounts: RwLock::new(accounts), - adds: RwLock::new(vec![]), } } } @@ -76,14 +83,20 @@ impl AccountProvider for TestAccountProvider { } fn new_account(&self, pass: &str) -> Result { - let mut adds = self.adds.write().unwrap(); - let address = Address::from(adds.len() as u64 + 2); - adds.push(pass.to_owned()); + let account = TestAccount::new(pass); + let address = KeyPair::from_secret(account.secret.clone()).unwrap().address(); + self.accounts.write().unwrap().insert(address.clone(), account); Ok(address) } - fn account_secret(&self, _account: &Address) -> Result { - Ok(Secret::random()) + fn account_secret(&self, address: &Address) -> Result { + // todo: consider checking if account is unlock. some test may need alteration then. + self.accounts + .read() + .unwrap() + .get(address) + .ok_or(SigningError::NoAccount) + .map(|acc| acc.secret.clone()) } fn sign(&self, _account: &Address, _message: &H256) -> Result { diff --git a/rpc/src/v1/tests/helpers/miner_service.rs b/rpc/src/v1/tests/helpers/miner_service.rs index 517f2deb5..80a5e356d 100644 --- a/rpc/src/v1/tests/helpers/miner_service.rs +++ b/rpc/src/v1/tests/helpers/miner_service.rs @@ -16,7 +16,7 @@ //! Test implementation of miner service. -use util::{Address, H256, Bytes}; +use util::{Address, H256, Bytes, U256}; use util::standard::*; use ethcore::error::Error; use ethcore::client::BlockChainClient; @@ -27,19 +27,22 @@ use ethminer::{MinerService, MinerStatus, AccountDetails}; /// Test miner service. pub struct TestMinerService { /// Imported transactions. - pub imported_transactions: RwLock>, + pub imported_transactions: Mutex>, /// Latest closed block. pub latest_closed_block: Mutex>, /// Pre-existed pending transactions pub pending_transactions: Mutex>, + /// Last nonces. + pub last_nonces: RwLock>, } impl Default for TestMinerService { fn default() -> TestMinerService { TestMinerService { - imported_transactions: RwLock::new(Vec::new()), + imported_transactions: Mutex::new(Vec::new()), latest_closed_block: Mutex::new(None), pending_transactions: Mutex::new(HashMap::new()), + last_nonces: RwLock::new(HashMap::new()), } } } @@ -56,28 +59,52 @@ impl MinerService for TestMinerService { } /// Imports transactions to transaction queue. - fn import_transactions(&self, _transactions: Vec, _fetch_account: T) -> Vec> - where T: Fn(&Address) -> AccountDetails { unimplemented!(); } + fn import_transactions(&self, transactions: Vec, _fetch_account: T) -> Vec> + where T: Fn(&Address) -> AccountDetails { + // lets assume that all txs are valid + self.imported_transactions.lock().unwrap().extend_from_slice(&transactions); + + transactions + .iter() + .map(|_| Ok(())) + .collect() + } /// Returns hashes of transactions currently in pending - fn pending_transactions_hashes(&self) -> Vec { vec![] } + fn pending_transactions_hashes(&self) -> Vec { + vec![] + } /// Removes all transactions from the queue and restart mining operation. - fn clear_and_reset(&self, _chain: &BlockChainClient) { unimplemented!(); } + fn clear_and_reset(&self, _chain: &BlockChainClient) { + unimplemented!(); + } /// Called when blocks are imported to chain, updates transactions queue. - fn chain_new_blocks(&self, _chain: &BlockChainClient, _imported: &[H256], _invalid: &[H256], _enacted: &[H256], _retracted: &[H256]) { unimplemented!(); } + fn chain_new_blocks(&self, _chain: &BlockChainClient, _imported: &[H256], _invalid: &[H256], _enacted: &[H256], _retracted: &[H256]) { + unimplemented!(); + } /// New chain head event. Restart mining operation. - fn update_sealing(&self, _chain: &BlockChainClient) { unimplemented!(); } + fn update_sealing(&self, _chain: &BlockChainClient) { + unimplemented!(); + } - fn map_sealing_work(&self, _chain: &BlockChainClient, _f: F) -> Option where F: FnOnce(&ClosedBlock) -> T { unimplemented!(); } + fn map_sealing_work(&self, _chain: &BlockChainClient, _f: F) -> Option where F: FnOnce(&ClosedBlock) -> T { + unimplemented!(); + } fn transaction(&self, hash: &H256) -> Option { - self.pending_transactions.lock().unwrap().get(hash).and_then(|tx_ref| Some(tx_ref.clone())) + self.pending_transactions.lock().unwrap().get(hash).cloned() + } + + fn last_nonce(&self, address: &Address) -> Option { + self.last_nonces.read().unwrap().get(address).cloned() } /// Submit `seal` as a valid solution for the header of `pow_hash`. /// Will check the seal, but not actually insert the block into the chain. - fn submit_seal(&self, _chain: &BlockChainClient, _pow_hash: H256, _seal: Vec) -> Result<(), Error> { unimplemented!(); } + fn submit_seal(&self, _chain: &BlockChainClient, _pow_hash: H256, _seal: Vec) -> Result<(), Error> { + unimplemented!(); + } } diff --git a/rpc/src/v1/tests/personal.rs b/rpc/src/v1/tests/personal.rs index 261527c47..458669b42 100644 --- a/rpc/src/v1/tests/personal.rs +++ b/rpc/src/v1/tests/personal.rs @@ -22,8 +22,7 @@ use util::numbers::*; use std::collections::*; fn accounts_provider() -> Arc { - let mut accounts = HashMap::new(); - accounts.insert(Address::from(1), TestAccount::new("test")); + let accounts = HashMap::new(); let ap = TestAccountProvider::new(accounts); Arc::new(ap) } @@ -38,7 +37,11 @@ fn setup() -> (Arc, IoHandler) { #[test] fn accounts() { - let (_test_provider, io) = setup(); + let (test_provider, io) = setup(); + test_provider.accounts + .write() + .unwrap() + .insert(Address::from(1), TestAccount::new("test")); let request = r#"{"jsonrpc": "2.0", "method": "personal_listAccounts", "params": [], "id": 1}"#; let response = r#"{"jsonrpc":"2.0","result":["0x0000000000000000000000000000000000000001"],"id":1}"#; @@ -49,11 +52,22 @@ fn accounts() { #[test] fn new_account() { - let (_test_provider, io) = setup(); - + let (test_provider, io) = setup(); let request = r#"{"jsonrpc": "2.0", "method": "personal_newAccount", "params": ["pass"], "id": 1}"#; - let response = r#"{"jsonrpc":"2.0","result":"0x0000000000000000000000000000000000000002","id":1}"#; - assert_eq!(io.handle_request(request), Some(response.to_owned())); + let res = io.handle_request(request); + + let accounts = test_provider.accounts.read().unwrap(); + assert_eq!(accounts.len(), 1); + + let address = accounts + .keys() + .nth(0) + .cloned() + .unwrap(); + + let response = r#"{"jsonrpc":"2.0","result":""#.to_owned() + format!("0x{:?}", address).as_ref() + r#"","id":1}"#; + + assert_eq!(res, Some(response)); }