From f36dc053b5594f0367c8bfc7b103795d77559bc0 Mon Sep 17 00:00:00 2001 From: Pierre Krieger Date: Tue, 21 Jul 2020 13:59:49 +0200 Subject: [PATCH 1/7] Add sync_legacy_requests_received metric (#6698) --- client/network/src/protocol.rs | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 626cb04389236..d606a1be989c4 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -142,6 +142,7 @@ struct Metrics { finality_proofs: GaugeVec, justifications: GaugeVec, propagated_transactions: Counter, + legacy_requests_received: Counter, } impl Metrics { @@ -187,6 +188,10 @@ impl Metrics { "sync_propagated_transactions", "Number of transactions propagated to at least one peer", )?, r)?, + legacy_requests_received: register(Counter::new( + "sync_legacy_requests_received", + "Number of block/finality/light-client requests received on the legacy substream", + )?, r)?, }) } } @@ -715,6 +720,10 @@ impl Protocol { } fn on_block_request(&mut self, peer: PeerId, request: message::BlockRequest) { + if let Some(metrics) = &self.metrics { + metrics.legacy_requests_received.inc(); + } + trace!(target: "sync", "BlockRequest {} from {}: from {:?} to {:?} max {:?} for {:?}", request.id, peer, @@ -1399,6 +1408,11 @@ impl Protocol { request.method, request.block ); + + if let Some(metrics) = &self.metrics { + metrics.legacy_requests_received.inc(); + } + let proof = match self.context_data.chain.execution_proof( &BlockId::Hash(request.block), &request.method, @@ -1519,6 +1533,10 @@ impl Protocol { who: PeerId, request: message::RemoteReadRequest, ) { + if let Some(metrics) = &self.metrics { + metrics.legacy_requests_received.inc(); + } + if request.keys.is_empty() { debug!(target: "sync", "Invalid remote read request sent by {}", who); self.behaviour.disconnect_peer(&who); @@ -1568,6 +1586,10 @@ impl Protocol { who: PeerId, request: message::RemoteReadChildRequest, ) { + if let Some(metrics) = &self.metrics { + metrics.legacy_requests_received.inc(); + } + if request.keys.is_empty() { debug!(target: "sync", "Invalid remote child read request sent by {}", who); self.behaviour.disconnect_peer(&who); @@ -1624,6 +1646,10 @@ impl Protocol { who: PeerId, request: message::RemoteHeaderRequest>, ) { + if let Some(metrics) = &self.metrics { + metrics.legacy_requests_received.inc(); + } + trace!(target: "sync", "Remote header proof request {} from {} ({})", request.id, who, request.block); let (header, proof) = match self.context_data.chain.header_proof(&BlockId::Number(request.block)) { @@ -1654,6 +1680,10 @@ impl Protocol { who: PeerId, request: message::RemoteChangesRequest, ) { + if let Some(metrics) = &self.metrics { + metrics.legacy_requests_received.inc(); + } + trace!(target: "sync", "Remote changes proof request {} from {} for key {} ({}..{})", request.id, who, @@ -1717,6 +1747,10 @@ impl Protocol { who: PeerId, request: message::FinalityProofRequest, ) { + if let Some(metrics) = &self.metrics { + metrics.legacy_requests_received.inc(); + } + trace!(target: "sync", "Finality proof request from {} for {}", who, request.block); let finality_proof = self.finality_proof_provider.as_ref() .ok_or_else(|| String::from("Finality provider is not configured")) From 833fe6259115625f61347c8413bab29fded31210 Mon Sep 17 00:00:00 2001 From: pscott <30843220+pscott@users.noreply.github.com> Date: Tue, 21 Jul 2020 14:46:49 +0200 Subject: [PATCH 2/7] Improve overall performance (#6699) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Improve overall performance * Clean up code Co-authored-by: Bastian Köcher * Remove needless :: Co-authored-by: Bastian Köcher * Remove needless :: Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: Bastian Köcher Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- bin/node-template/node/src/service.rs | 12 +++++----- bin/node/bench/src/tempdb.rs | 1 - bin/node/cli/src/service.rs | 6 ++--- bin/node/testing/src/bench.rs | 1 - bin/utils/chain-spec-builder/src/main.rs | 2 +- client/api/src/in_mem.rs | 6 ++--- client/cli/src/config.rs | 16 +++++++------- client/cli/src/params/import_params.rs | 2 +- client/cli/src/params/keystore_params.rs | 2 +- client/consensus/aura/src/lib.rs | 4 ++-- client/db/src/lib.rs | 2 +- client/db/src/light.rs | 2 +- client/db/src/utils.rs | 4 ++-- client/executor/runtime-test/src/lib.rs | 2 +- client/executor/src/native_executor.rs | 2 +- client/executor/wasmi/src/lib.rs | 1 - .../finality-grandpa/src/communication/mod.rs | 4 ++-- client/keystore/src/lib.rs | 2 +- client/network-gossip/src/state_machine.rs | 2 +- client/network/src/block_requests.rs | 4 ++-- client/network/src/error.rs | 2 +- client/network/src/finality_requests.rs | 2 +- .../src/protocol/generic_proto/behaviour.rs | 2 +- .../src/protocol/sync/extra_requests.rs | 2 +- client/network/test/src/lib.rs | 2 +- client/rpc/src/state/state_light.rs | 2 +- client/service/src/builder.rs | 22 +++++++++---------- client/service/src/client/block_rules.rs | 4 ++-- client/service/test/src/lib.rs | 4 ++-- .../transaction-pool/graph/src/base_pool.rs | 2 +- client/transaction-pool/graph/src/ready.rs | 2 +- client/transaction-pool/src/api.rs | 2 +- frame/balances/src/lib.rs | 4 ++-- frame/contracts/src/exec.rs | 2 +- frame/contracts/src/lib.rs | 2 +- frame/contracts/src/rent.rs | 4 ++-- frame/elections-phragmen/src/lib.rs | 4 ++-- frame/multisig/src/lib.rs | 4 ++-- frame/offences/benchmarking/src/lib.rs | 2 +- frame/scored-pool/src/lib.rs | 2 +- frame/staking/fuzzer/src/submit_solution.rs | 2 +- frame/staking/src/benchmarking.rs | 4 ++-- frame/staking/src/lib.rs | 2 +- frame/staking/src/slashing.rs | 2 +- frame/staking/src/testing_utils.rs | 2 +- frame/support/procedural/src/storage/mod.rs | 4 ++-- frame/support/procedural/src/storage/parse.rs | 6 ++--- .../procedural/tools/derive/src/lib.rs | 4 ++-- frame/system/src/offchain.rs | 2 +- .../api/proc-macro/src/decl_runtime_apis.rs | 2 +- .../api/proc-macro/src/impl_runtime_apis.rs | 2 +- primitives/arithmetic/fuzzer/src/biguint.rs | 12 +++++----- primitives/arithmetic/src/fixed_point.rs | 14 ++++++------ .../common/src/import_queue/basic_queue.rs | 2 +- primitives/core/src/crypto.rs | 2 +- primitives/core/src/offchain/testing.rs | 2 +- primitives/core/src/testing.rs | 10 ++++----- .../fuzzer/src/balance_solution.rs | 2 +- primitives/npos-elections/src/lib.rs | 6 ++--- primitives/npos-elections/src/reduce.rs | 4 ++-- .../src/changes_trie/changes_iterator.rs | 6 ++--- primitives/state-machine/src/ext.rs | 8 +++---- .../state-machine/src/in_memory_backend.rs | 2 +- .../state-machine/src/proving_backend.rs | 2 +- primitives/state-machine/src/trie_backend.rs | 2 +- .../state-machine/src/trie_backend_essence.rs | 6 ++--- primitives/wasm-interface/src/lib.rs | 4 ++-- test-utils/runtime/client/src/lib.rs | 2 +- test-utils/runtime/client/src/trait_tests.rs | 8 +++---- test-utils/runtime/src/genesismap.rs | 2 +- utils/frame/rpc/system/src/lib.rs | 2 +- utils/wasm-builder/src/lib.rs | 4 ++-- utils/wasm-builder/src/wasm_project.rs | 2 +- 73 files changed, 141 insertions(+), 144 deletions(-) diff --git a/bin/node-template/node/src/service.rs b/bin/node-template/node/src/service.rs index 2d1cc878b41cb..04eb2add2751a 100644 --- a/bin/node-template/node/src/service.rs +++ b/bin/node-template/node/src/service.rs @@ -156,7 +156,7 @@ pub fn new_full(config: Configuration) -> Result { // if the node isn't actively participating in consensus then it doesn't // need a keystore, regardless of which protocol we use below. let keystore = if role.is_authority() { - Some(keystore.clone() as sp_core::traits::BareCryptoStorePtr) + Some(keystore as sp_core::traits::BareCryptoStorePtr) } else { None }; @@ -182,11 +182,11 @@ pub fn new_full(config: Configuration) -> Result { let grandpa_config = sc_finality_grandpa::GrandpaParams { config: grandpa_config, link: grandpa_link, - network: network.clone(), - inherent_data_providers: inherent_data_providers.clone(), + network, + inherent_data_providers, telemetry_on_connect: Some(telemetry_on_connect_sinks.on_connect_stream()), voting_rule: sc_finality_grandpa::VotingRulesBuilder::default().build(), - prometheus_registry: prometheus_registry.clone(), + prometheus_registry, shared_voter_state: SharedVoterState::empty(), }; @@ -200,7 +200,7 @@ pub fn new_full(config: Configuration) -> Result { sc_finality_grandpa::setup_disabled_grandpa( client, &inherent_data_providers, - network.clone(), + network, )?; } @@ -221,7 +221,7 @@ pub fn new_light(config: Configuration) -> Result { let pool_api = sc_transaction_pool::LightChainApi::new( builder.client().clone(), - fetcher.clone(), + fetcher, ); let pool = Arc::new(sc_transaction_pool::BasicPool::new_light( builder.config().transaction_pool.clone(), diff --git a/bin/node/bench/src/tempdb.rs b/bin/node/bench/src/tempdb.rs index 770bafec6f38d..4020fd1029368 100644 --- a/bin/node/bench/src/tempdb.rs +++ b/bin/node/bench/src/tempdb.rs @@ -124,7 +124,6 @@ impl Clone for TempDatabase { .map(|f_result| f_result.expect("failed to read file in seed db") .path() - .clone() ).collect(); fs_extra::copy_items( &self_db_files, diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index 5074bda6651ef..e817bb2a8c72a 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -272,7 +272,7 @@ pub fn new_full_base( // if the node isn't actively participating in consensus then it doesn't // need a keystore, regardless of which protocol we use below. let keystore = if role.is_authority() { - Some(keystore.clone() as BareCryptoStorePtr) + Some(keystore as BareCryptoStorePtr) } else { None }; @@ -302,7 +302,7 @@ pub fn new_full_base( inherent_data_providers: inherent_data_providers.clone(), telemetry_on_connect: Some(telemetry_on_connect_sinks.on_connect_stream()), voting_rule: grandpa::VotingRulesBuilder::default().build(), - prometheus_registry: prometheus_registry.clone(), + prometheus_registry, shared_voter_state, }; @@ -403,7 +403,7 @@ pub fn new_light_base(config: Configuration) -> Result<( babe_block_import, None, Some(Box::new(finality_proof_import)), - client.clone(), + client, select_chain, inherent_data_providers.clone(), spawn_task_handle, diff --git a/bin/node/testing/src/bench.rs b/bin/node/testing/src/bench.rs index 6f351a70019ee..90e1a16eb1226 100644 --- a/bin/node/testing/src/bench.rs +++ b/bin/node/testing/src/bench.rs @@ -118,7 +118,6 @@ impl Clone for BenchDb { .map(|f_result| f_result.expect("failed to read file in seed db") .path() - .clone() ).collect(); fs_extra::copy_items( &seed_db_files, diff --git a/bin/utils/chain-spec-builder/src/main.rs b/bin/utils/chain-spec-builder/src/main.rs index 4fbcc1e850723..2bfbb0952775d 100644 --- a/bin/utils/chain-spec-builder/src/main.rs +++ b/bin/utils/chain-spec-builder/src/main.rs @@ -131,7 +131,7 @@ fn generate_chain_spec( Default::default(), ); - chain_spec.as_json(false).map_err(|err| err.to_string()) + chain_spec.as_json(false).map_err(|err| err) } fn generate_authority_keys_and_store( diff --git a/client/api/src/in_mem.rs b/client/api/src/in_mem.rs index 9bfdcdd4d5aea..7d27326678f58 100644 --- a/client/api/src/in_mem.rs +++ b/client/api/src/in_mem.rs @@ -124,7 +124,7 @@ impl Clone for Blockchain { fn clone(&self) -> Self { let storage = Arc::new(RwLock::new(self.storage.read().clone())); Blockchain { - storage: storage.clone(), + storage, } } } @@ -155,7 +155,7 @@ impl Blockchain { aux: HashMap::new(), })); Blockchain { - storage: storage.clone(), + storage, } } @@ -346,7 +346,7 @@ impl HeaderMetadata for Blockchain { fn header_metadata(&self, hash: Block::Hash) -> Result, Self::Error> { self.header(BlockId::hash(hash))?.map(|header| CachedHeaderMetadata::from(&header)) - .ok_or(sp_blockchain::Error::UnknownBlock(format!("header not found: {}", hash))) + .ok_or_else(|| sp_blockchain::Error::UnknownBlock(format!("header not found: {}", hash))) } fn insert_header_metadata(&self, _hash: Block::Hash, _metadata: CachedHeaderMetadata) { diff --git a/client/cli/src/config.rs b/client/cli/src/config.rs index fa3f09116c314..efda45a0eca9a 100644 --- a/client/cli/src/config.rs +++ b/client/cli/src/config.rs @@ -158,7 +158,7 @@ pub trait CliConfiguration: Sized { fn database_cache_size(&self) -> Result> { Ok(self.database_params() .map(|x| x.database_cache_size()) - .unwrap_or(Default::default())) + .unwrap_or_default()) } /// Get the database backend variant. @@ -195,7 +195,7 @@ pub trait CliConfiguration: Sized { fn state_cache_size(&self) -> Result { Ok(self.import_params() .map(|x| x.state_cache_size()) - .unwrap_or(Default::default())) + .unwrap_or_default()) } /// Get the state cache child ratio (if any). @@ -212,7 +212,7 @@ pub trait CliConfiguration: Sized { fn pruning(&self, unsafe_pruning: bool, role: &Role) -> Result { self.pruning_params() .map(|x| x.pruning(unsafe_pruning, role)) - .unwrap_or(Ok(Default::default())) + .unwrap_or_else(|| Ok(Default::default())) } /// Get the chain ID (string). @@ -236,7 +236,7 @@ pub trait CliConfiguration: Sized { fn wasm_method(&self) -> Result { Ok(self.import_params() .map(|x| x.wasm_method()) - .unwrap_or(Default::default())) + .unwrap_or_default()) } /// Get the execution strategies. @@ -251,7 +251,7 @@ pub trait CliConfiguration: Sized { Ok(self .import_params() .map(|x| x.execution_strategies(is_dev, is_validator)) - .unwrap_or(Default::default())) + .unwrap_or_default()) } /// Get the RPC HTTP address (`None` if disabled). @@ -365,7 +365,7 @@ pub trait CliConfiguration: Sized { fn tracing_targets(&self) -> Result> { Ok(self.import_params() .map(|x| x.tracing_targets()) - .unwrap_or(Default::default())) + .unwrap_or_else(|| Default::default())) } /// Get the TracingReceiver value from the current object @@ -375,7 +375,7 @@ pub trait CliConfiguration: Sized { fn tracing_receiver(&self) -> Result { Ok(self.import_params() .map(|x| x.tracing_receiver()) - .unwrap_or(Default::default())) + .unwrap_or_default()) } /// Get the node key from the current object @@ -385,7 +385,7 @@ pub trait CliConfiguration: Sized { fn node_key(&self, net_config_dir: &PathBuf) -> Result { self.node_key_params() .map(|x| x.node_key(net_config_dir)) - .unwrap_or(Ok(Default::default())) + .unwrap_or_else(|| Ok(Default::default())) } /// Get maximum runtime instances diff --git a/client/cli/src/params/import_params.rs b/client/cli/src/params/import_params.rs index c2fb34f90e6fc..e60779429b179 100644 --- a/client/cli/src/params/import_params.rs +++ b/client/cli/src/params/import_params.rs @@ -113,7 +113,7 @@ impl ImportParams { default }; - exec.execution.unwrap_or(strat.unwrap_or(default)).into() + exec.execution.unwrap_or_else(|| strat.unwrap_or(default)).into() }; let default_execution_import_block = if is_validator { diff --git a/client/cli/src/params/keystore_params.rs b/client/cli/src/params/keystore_params.rs index 8b20dd247aec6..a6eb438cc0780 100644 --- a/client/cli/src/params/keystore_params.rs +++ b/client/cli/src/params/keystore_params.rs @@ -94,7 +94,7 @@ impl KeystoreParams { let path = self .keystore_path .clone() - .unwrap_or(base_path.join(DEFAULT_KEYSTORE_CONFIG_PATH)); + .unwrap_or_else(|| base_path.join(DEFAULT_KEYSTORE_CONFIG_PATH)); Ok(KeystoreConfig::Path { path, password }) } diff --git a/client/consensus/aura/src/lib.rs b/client/consensus/aura/src/lib.rs index 19bc3bae6c30b..8763239771abb 100644 --- a/client/consensus/aura/src/lib.rs +++ b/client/consensus/aura/src/lib.rs @@ -165,7 +165,7 @@ pub fn start_aura( CAW: CanAuthorWith + Send, { let worker = AuraWorker { - client: client.clone(), + client, block_import: Arc::new(Mutex::new(block_import)), env, keystore, @@ -839,7 +839,7 @@ pub fn import_queue( initialize_authorities_cache(&*client)?; let verifier = AuraVerifier { - client: client.clone(), + client, inherent_data_providers, phantom: PhantomData, }; diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 7cfde1e1d9d0d..086db73728f1b 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -512,7 +512,7 @@ impl HeaderMetadata for BlockchainDb { header_metadata.clone(), ); header_metadata - }).ok_or(ClientError::UnknownBlock(format!("header not found in db: {}", hash))) + }).ok_or_else(|| ClientError::UnknownBlock(format!("header not found in db: {}", hash))) }, Ok) } diff --git a/client/db/src/light.rs b/client/db/src/light.rs index 3dc6453cd90da..139ecf3b22c72 100644 --- a/client/db/src/light.rs +++ b/client/db/src/light.rs @@ -200,7 +200,7 @@ impl HeaderMetadata for LightStorage { header_metadata.clone(), ); header_metadata - }).ok_or(ClientError::UnknownBlock(format!("header not found in db: {}", hash))) + }).ok_or_else(|| ClientError::UnknownBlock(format!("header not found in db: {}", hash))) }, Ok) } diff --git a/client/db/src/utils.rs b/client/db/src/utils.rs index c25b978be0fc1..168ab9bbb71f6 100644 --- a/client/db/src/utils.rs +++ b/client/db/src/utils.rs @@ -181,8 +181,8 @@ pub fn insert_hash_to_key_mapping, H: AsRef<[u8]> + Clone>( ) -> sp_blockchain::Result<()> { transaction.set_from_vec( key_lookup_col, - hash.clone().as_ref(), - number_and_hash_to_lookup_key(number, hash)?, + hash.as_ref(), + number_and_hash_to_lookup_key(number, hash.clone())?, ); Ok(()) } diff --git a/client/executor/runtime-test/src/lib.rs b/client/executor/runtime-test/src/lib.rs index 4962c558eaa2d..41c9c6d9cbcd7 100644 --- a/client/executor/runtime-test/src/lib.rs +++ b/client/executor/runtime-test/src/lib.rs @@ -353,7 +353,7 @@ fn execute_sandboxed( Memory::new() can't return a Error qed" ), }; - env_builder.add_memory("env", "memory", memory.clone()); + env_builder.add_memory("env", "memory", memory); env_builder }; diff --git a/client/executor/src/native_executor.rs b/client/executor/src/native_executor.rs index b1eb504d5a2b3..0aeec98067f40 100644 --- a/client/executor/src/native_executor.rs +++ b/client/executor/src/native_executor.rs @@ -336,7 +336,7 @@ impl CodeExecutor for NativeExecutor { let res = with_externalities_safe(&mut **ext, move || (call)()) .and_then(|r| r .map(NativeOrEncoded::Native) - .map_err(|s| Error::ApiError(s.to_string())) + .map_err(|s| Error::ApiError(s)) ); Ok(res) diff --git a/client/executor/wasmi/src/lib.rs b/client/executor/wasmi/src/lib.rs index e4b4aca40967d..1632aa3c18ad5 100644 --- a/client/executor/wasmi/src/lib.rs +++ b/client/executor/wasmi/src/lib.rs @@ -234,7 +234,6 @@ impl<'a> Sandbox for FunctionExecutor<'a> { table.get(dispatch_thunk_id) .map_err(|_| "dispatch_thunk_idx is out of the table bounds")? .ok_or_else(|| "dispatch_thunk_idx points on an empty table entry")? - .clone() }; let guest_env = match sandbox::GuestEnvironment::decode(&self.sandbox_store, raw_env_def) { diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index b7bbad9f8e7c4..a8bfb84416b8f 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -701,8 +701,8 @@ impl Sink> for OutgoingMessages keystore.local_id().clone(), self.round, self.set_id, - ).ok_or( - Error::Signing(format!( + ).ok_or_else( + || Error::Signing(format!( "Failed to sign GRANDPA vote for round {} targetting {:?}", self.round, target_hash )) )?; diff --git a/client/keystore/src/lib.rs b/client/keystore/src/lib.rs index 7fec32bae246a..f337f64d1c5f9 100644 --- a/client/keystore/src/lib.rs +++ b/client/keystore/src/lib.rs @@ -310,7 +310,7 @@ impl BareCryptoStore for Store { .fold(Vec::new(), |mut v, k| { v.push(CryptoTypePublicPair(sr25519::CRYPTO_ID, k.clone())); v.push(CryptoTypePublicPair(ed25519::CRYPTO_ID, k.clone())); - v.push(CryptoTypePublicPair(ecdsa::CRYPTO_ID, k.clone())); + v.push(CryptoTypePublicPair(ecdsa::CRYPTO_ID, k)); v })) } diff --git a/client/network-gossip/src/state_machine.rs b/client/network-gossip/src/state_machine.rs index da07bde3e7d1a..80a0f9e70bcbf 100644 --- a/client/network-gossip/src/state_machine.rs +++ b/client/network-gossip/src/state_machine.rs @@ -180,7 +180,7 @@ impl ConsensusGossip { let validator = self.validator.clone(); let mut context = NetworkContext { gossip: self, network }; - validator.new_peer(&mut context, &who, role.clone()); + validator.new_peer(&mut context, &who, role); } fn register_message_hashed( diff --git a/client/network/src/block_requests.rs b/client/network/src/block_requests.rs index 8f5116657a5c2..1aa557d6cdcbc 100644 --- a/client/network/src/block_requests.rs +++ b/client/network/src/block_requests.rs @@ -409,7 +409,7 @@ where }, body: if get_body { self.chain.block_body(&BlockId::Hash(hash))? - .unwrap_or(Vec::new()) + .unwrap_or_default() .iter_mut() .map(|extrinsic| extrinsic.encode()) .collect() @@ -418,7 +418,7 @@ where }, receipt: Vec::new(), message_queue: Vec::new(), - justification: justification.unwrap_or(Vec::new()), + justification: justification.unwrap_or_default(), is_empty_justification, }; diff --git a/client/network/src/error.rs b/client/network/src/error.rs index b87e495983eaf..d5a4024ef53d7 100644 --- a/client/network/src/error.rs +++ b/client/network/src/error.rs @@ -32,7 +32,7 @@ pub enum Error { /// Io error Io(std::io::Error), /// Client error - Client(sp_blockchain::Error), + Client(Box), /// The same bootnode (based on address) is registered with two different peer ids. #[display( fmt = "The same bootnode (`{}`) is registered with two different peer ids: `{}` and `{}`", diff --git a/client/network/src/finality_requests.rs b/client/network/src/finality_requests.rs index 9bb3cfec744e6..de737cdd20a4e 100644 --- a/client/network/src/finality_requests.rs +++ b/client/network/src/finality_requests.rs @@ -206,7 +206,7 @@ where let finality_proof = if let Some(provider) = &self.finality_proof_provider { provider .prove_finality(block_hash, &request.request)? - .unwrap_or(Vec::new()) + .unwrap_or_default() } else { log::error!("Answering a finality proof request while finality provider is empty"); return Err(From::from("Empty finality proof provider".to_string())) diff --git a/client/network/src/protocol/generic_proto/behaviour.rs b/client/network/src/protocol/generic_proto/behaviour.rs index 0e56b03b7ad29..215eb7393385b 100644 --- a/client/network/src/protocol/generic_proto/behaviour.rs +++ b/client/network/src/protocol/generic_proto/behaviour.rs @@ -806,7 +806,7 @@ impl GenericProto { debug!(target: "sub-libp2p", "PSM => Accept({:?}, {:?}): Obsolete incoming, sending back dropped", index, incoming.peer_id); debug!(target: "sub-libp2p", "PSM <= Dropped({:?})", incoming.peer_id); - self.peerset.dropped(incoming.peer_id.clone()); + self.peerset.dropped(incoming.peer_id); return } diff --git a/client/network/src/protocol/sync/extra_requests.rs b/client/network/src/protocol/sync/extra_requests.rs index 6d688c130fafd..d025b86b2536f 100644 --- a/client/network/src/protocol/sync/extra_requests.rs +++ b/client/network/src/protocol/sync/extra_requests.rs @@ -141,7 +141,7 @@ impl ExtraRequests { request, ); } - self.failed_requests.entry(request).or_insert(Vec::new()).push((who, Instant::now())); + self.failed_requests.entry(request).or_default().push((who, Instant::now())); self.pending_requests.push_front(request); } else { trace!(target: "sync", "No active {} request to {:?}", diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index d0f1d4752bb5e..30508711a6a4f 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -678,7 +678,7 @@ pub trait TestNetFactory: Sized { protocol_id: ProtocolId::from(&b"test-protocol-name"[..]), import_queue, block_announce_validator: config.block_announce_validator - .unwrap_or(Box::new(DefaultBlockAnnounceValidator)), + .unwrap_or_else(|| Box::new(DefaultBlockAnnounceValidator)), metrics_registry: None, }).unwrap(); diff --git a/client/rpc/src/state/state_light.rs b/client/rpc/src/state/state_light.rs index ec275a2d78b79..c7e218541aa5f 100644 --- a/client/rpc/src/state/state_light.rs +++ b/client/rpc/src/state/state_light.rs @@ -539,7 +539,7 @@ fn resolve_header>( maybe_header.then(move |result| ready(result.and_then(|maybe_header| - maybe_header.ok_or(ClientError::UnknownBlock(format!("{}", block))) + maybe_header.ok_or_else(|| ClientError::UnknownBlock(format!("{}", block))) ).map_err(client_err)), ) } diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index 6f46b8bbb74da..fe8fdcef13cfa 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -438,7 +438,7 @@ impl ServiceBuilder<(), (), (), (), (), (), (), (), (), (), ()> { backend, task_manager, keystore, - fetcher: Some(fetcher.clone()), + fetcher: Some(fetcher), select_chain: None, import_queue: (), finality_proof_request_builder: None, @@ -1286,7 +1286,7 @@ fn gen_handler( client.clone(), subscriptions.clone(), remote_backend.clone(), - on_demand.clone() + on_demand, ); (chain, state, child_state) @@ -1298,15 +1298,15 @@ fn gen_handler( }; let author = sc_rpc::author::Author::new( - client.clone(), - transaction_pool.clone(), + client, + transaction_pool, subscriptions, - keystore.clone(), + keystore, deny_unsafe, ); - let system = system::System::new(system_info, system_rpc_tx.clone(), deny_unsafe); + let system = system::System::new(system_info, system_rpc_tx, deny_unsafe); - let maybe_offchain_rpc = offchain_storage.clone() + let maybe_offchain_rpc = offchain_storage .map(|storage| { let offchain = sc_rpc::offchain::Offchain::new(storage, deny_unsafe); // FIXME: Use plain Option (don't collect into HashMap) when we upgrade to jsonrpc 14.1 @@ -1357,7 +1357,7 @@ fn build_network( { let transaction_pool_adapter = Arc::new(TransactionPoolAdapter { imports_external_transactions: !matches!(config.role, Role::Light), - pool: transaction_pool.clone(), + pool: transaction_pool, client: client.clone(), }); @@ -1391,8 +1391,8 @@ fn build_network( chain: client.clone(), finality_proof_provider, finality_proof_request_builder, - on_demand: on_demand.clone(), - transaction_pool: transaction_pool_adapter.clone() as _, + on_demand: on_demand, + transaction_pool: transaction_pool_adapter as _, import_queue: Box::new(import_queue), protocol_id, block_announce_validator, @@ -1407,7 +1407,7 @@ fn build_network( let future = build_network_future( config.role.clone(), network_mut, - client.clone(), + client, network_status_sinks.clone(), system_rpc_rx, has_bootnodes, diff --git a/client/service/src/client/block_rules.rs b/client/service/src/client/block_rules.rs index 247d09197b604..e862379a56414 100644 --- a/client/service/src/client/block_rules.rs +++ b/client/service/src/client/block_rules.rs @@ -52,8 +52,8 @@ impl BlockRules { bad_blocks: BadBlocks, ) -> Self { Self { - bad: bad_blocks.unwrap_or(HashSet::new()), - forks: fork_blocks.unwrap_or(vec![]).into_iter().collect(), + bad: bad_blocks.unwrap_or_else(|| HashSet::new()), + forks: fork_blocks.unwrap_or_else(|| vec![]).into_iter().collect(), } } diff --git a/client/service/test/src/lib.rs b/client/service/test/src/lib.rs index ac95dd11e8b27..b0dd2c0e257df 100644 --- a/client/service/test/src/lib.rs +++ b/client/service/test/src/lib.rs @@ -518,7 +518,7 @@ pub fn sync( let temp = tempdir_with_prefix("substrate-sync-test"); let mut network = TestNet::new( &temp, - spec.clone(), + spec, (0..NUM_FULL_NODES).map(|_| { |cfg| full_builder(cfg) }), (0..NUM_LIGHT_NODES).map(|_| { |cfg| light_builder(cfg) }), // Note: this iterator is empty but we can't just use `iter::empty()`, otherwise @@ -592,7 +592,7 @@ pub fn consensus( let temp = tempdir_with_prefix("substrate-consensus-test"); let mut network = TestNet::new( &temp, - spec.clone(), + spec, (0..NUM_FULL_NODES / 2).map(|_| { |cfg| full_builder(cfg).map(|s| (s, ())) }), (0..NUM_LIGHT_NODES / 2).map(|_| { |cfg| light_builder(cfg) }), authorities.into_iter().map(|key| (key, { |cfg| full_builder(cfg).map(|s| (s, ())) })), diff --git a/client/transaction-pool/graph/src/base_pool.rs b/client/transaction-pool/graph/src/base_pool.rs index 25da341e6794e..81d8e802c2c9e 100644 --- a/client/transaction-pool/graph/src/base_pool.rs +++ b/client/transaction-pool/graph/src/base_pool.rs @@ -278,7 +278,7 @@ impl BasePool, ) -> error::Result> { if self.is_imported(&tx.hash) { - return Err(error::Error::AlreadyImported(Box::new(tx.hash.clone()))) + return Err(error::Error::AlreadyImported(Box::new(tx.hash))) } let tx = WaitingTransaction::new( diff --git a/client/transaction-pool/graph/src/ready.rs b/client/transaction-pool/graph/src/ready.rs index b98512b05d5cf..cbdb25078931e 100644 --- a/client/transaction-pool/graph/src/ready.rs +++ b/client/transaction-pool/graph/src/ready.rs @@ -538,7 +538,7 @@ impl Iterator for BestIterator { } } - return Some(best.transaction.clone()) + return Some(best.transaction) } } } diff --git a/client/transaction-pool/src/api.rs b/client/transaction-pool/src/api.rs index a14d5b0db18ea..c6671fd5bd7f0 100644 --- a/client/transaction-pool/src/api.rs +++ b/client/transaction-pool/src/api.rs @@ -305,7 +305,7 @@ impl sc_transaction_graph::ChainApi for fn block_body(&self, id: &BlockId) -> Self::BodyFuture { let header = self.client.header(*id) - .and_then(|h| h.ok_or(sp_blockchain::Error::UnknownBlock(format!("{}", id)))); + .and_then(|h| h.ok_or_else(|| sp_blockchain::Error::UnknownBlock(format!("{}", id)))); let header = match header { Ok(header) => header, Err(err) => { diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 3056cd19759db..0bd57e3828c24 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -1092,7 +1092,7 @@ impl, I: Instance> Currency for Module where // defensive only: overflow should never happen, however in case it does, then this // operation is a no-op. - account.free = account.free.checked_add(&value).ok_or(Self::PositiveImbalance::zero())?; + account.free = account.free.checked_add(&value).ok_or_else(|| Self::PositiveImbalance::zero())?; Ok(PositiveImbalance::new(value)) }).unwrap_or_else(|x| x) @@ -1153,7 +1153,7 @@ impl, I: Instance> Currency for Module where }; account.free = value; Ok(imbalance) - }).unwrap_or(SignedImbalance::Positive(Self::PositiveImbalance::zero())) + }).unwrap_or_else(|_| SignedImbalance::Positive(Self::PositiveImbalance::zero())) } } diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 67e2a4375e44c..f6327f7f2d9b0 100644 --- a/frame/contracts/src/exec.rs +++ b/frame/contracts/src/exec.rs @@ -801,7 +801,7 @@ where fn rent_allowance(&self) -> BalanceOf { storage::rent_allowance::(&self.ctx.self_account) - .unwrap_or(>::max_value()) // Must never be triggered actually + .unwrap_or_else(|_| >::max_value()) // Must never be triggered actually } fn block_number(&self) -> T::BlockNumber { self.block_number } diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 6194e3a694059..4b3a48119f280 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -648,7 +648,7 @@ impl Module { let cfg = Config::preload(); let vm = WasmVm::new(&cfg.schedule); let loader = WasmLoader::new(&cfg.schedule); - let mut ctx = ExecutionContext::top_level(origin.clone(), &cfg, &vm, &loader); + let mut ctx = ExecutionContext::top_level(origin, &cfg, &vm, &loader); func(&mut ctx, gas_meter) } } diff --git a/frame/contracts/src/rent.rs b/frame/contracts/src/rent.rs index a3f582810afe1..908faca9a6c0c 100644 --- a/frame/contracts/src/rent.rs +++ b/frame/contracts/src/rent.rs @@ -104,7 +104,7 @@ fn compute_fee_per_block( effective_storage_size .checked_mul(&T::RentByteFee::get()) - .unwrap_or(>::max_value()) + .unwrap_or_else(|| >::max_value()) } /// Returns amount of funds available to consume by rent mechanism. @@ -179,7 +179,7 @@ fn consider_case( let dues = fee_per_block .checked_mul(&blocks_passed.saturated_into::().into()) - .unwrap_or(>::max_value()); + .unwrap_or_else(|| >::max_value()); let insufficient_rent = rent_budget < dues; // If the rent payment cannot be withdrawn due to locks on the account balance, then evict the diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index e3ecb6ea229b5..50c5de9bc0de4 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -607,7 +607,7 @@ decl_module! { // returns NoMember error in case of error. let _ = Self::remove_and_replace_member(&who)?; T::Currency::unreserve(&who, T::CandidacyBond::get()); - Self::deposit_event(RawEvent::MemberRenounced(who.clone())); + Self::deposit_event(RawEvent::MemberRenounced(who)); }, Renouncing::RunnerUp => { let mut runners_up_with_stake = Self::runners_up(); @@ -1002,7 +1002,7 @@ impl Module { ); T::ChangeMembers::change_members_sorted( &incoming, - &outgoing.clone(), + &outgoing, &new_members_ids, ); T::ChangeMembers::set_prime(prime); diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index cbe6f2054ca3c..f8f6e8ed63bc9 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -295,12 +295,12 @@ decl_module! { ensure!(!other_signatories.is_empty(), Error::::TooFewSignatories); let other_signatories_len = other_signatories.len(); ensure!(other_signatories_len < max_sigs, Error::::TooManySignatories); - let signatories = Self::ensure_sorted_and_insert(other_signatories, who.clone())?; + let signatories = Self::ensure_sorted_and_insert(other_signatories, who)?; let id = Self::multi_account_id(&signatories, 1); let call_len = call.using_encoded(|c| c.len()); - let result = call.dispatch(RawOrigin::Signed(id.clone()).into()); + let result = call.dispatch(RawOrigin::Signed(id).into()); result.map(|post_dispatch_info| post_dispatch_info.actual_weight .map(|actual_weight| weight_of::as_multi_threshold_1::( diff --git a/frame/offences/benchmarking/src/lib.rs b/frame/offences/benchmarking/src/lib.rs index b47c14296a08e..1aa9fed85b127 100644 --- a/frame/offences/benchmarking/src/lib.rs +++ b/frame/offences/benchmarking/src/lib.rs @@ -257,7 +257,7 @@ benchmarks! { .flat_map(|reporter| vec![ frame_system::Event::::NewAccount(reporter.clone()).into(), ::Event::from( - pallet_balances::Event::::Endowed(reporter.clone(), (reward_amount / r).into()) + pallet_balances::Event::::Endowed(reporter, (reward_amount / r).into()) ).into() ]); diff --git a/frame/scored-pool/src/lib.rs b/frame/scored-pool/src/lib.rs index 35c36b03195e8..90d4aca4e42a4 100644 --- a/frame/scored-pool/src/lib.rs +++ b/frame/scored-pool/src/lib.rs @@ -355,7 +355,7 @@ decl_module! { // if there is already an element with `score`, we insert // right before that. if not, the search returns a location // where we can insert while maintaining order. - let item = (who.clone(), Some(score.clone())); + let item = (who, Some(score.clone())); let location = pool .binary_search_by_key( &Reverse(score), diff --git a/frame/staking/fuzzer/src/submit_solution.rs b/frame/staking/fuzzer/src/submit_solution.rs index 7293cf23890a5..6812a739c4929 100644 --- a/frame/staking/fuzzer/src/submit_solution.rs +++ b/frame/staking/fuzzer/src/submit_solution.rs @@ -162,7 +162,7 @@ fn main() { match mode { Mode::WeakerSubmission => { assert_eq!( - call.dispatch_bypass_filter(origin.clone().into()).unwrap_err().error, + call.dispatch_bypass_filter(origin.into()).unwrap_err().error, DispatchError::Module { index: 0, error: 16, diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index b2035c22b675c..d92cd8717914c 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -61,7 +61,7 @@ pub fn create_validator_with_nominators( let validator_prefs = ValidatorPrefs { commission: Perbill::from_percent(50), }; - Staking::::validate(RawOrigin::Signed(v_controller.clone()).into(), validator_prefs)?; + Staking::::validate(RawOrigin::Signed(v_controller).into(), validator_prefs)?; let stash_lookup: ::Source = T::Lookup::unlookup(v_stash.clone()); points_total += 10; @@ -375,7 +375,7 @@ benchmarks! { for _ in 0 .. l { staking_ledger.unlocking.push(unlock_chunk.clone()) } - Ledger::::insert(controller.clone(), staking_ledger.clone()); + Ledger::::insert(controller, staking_ledger); let slash_amount = T::Currency::minimum_balance() * 10.into(); let balance_before = T::Currency::free_balance(&stash); }: { diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index be07c7e18a4f4..dd4ad5fc7e5ab 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -1626,7 +1626,7 @@ decl_module! { let era = Self::current_era().unwrap_or(0) + T::BondingDuration::get(); ledger.unlocking.push(UnlockChunk { value, era }); Self::update_ledger(&controller, &ledger); - Self::deposit_event(RawEvent::Unbonded(ledger.stash.clone(), value)); + Self::deposit_event(RawEvent::Unbonded(ledger.stash, value)); } } diff --git a/frame/staking/src/slashing.rs b/frame/staking/src/slashing.rs index 4e43b754b8e37..af9a92f16a463 100644 --- a/frame/staking/src/slashing.rs +++ b/frame/staking/src/slashing.rs @@ -370,7 +370,7 @@ fn slash_nominators( let mut era_slash = as Store>::NominatorSlashInEra::get( &slash_era, stash, - ).unwrap_or(Zero::zero()); + ).unwrap_or_else(|| Zero::zero()); era_slash += own_slash_difference; diff --git a/frame/staking/src/testing_utils.rs b/frame/staking/src/testing_utils.rs index 27a2575eb0df3..02acd135e63e4 100644 --- a/frame/staking/src/testing_utils.rs +++ b/frame/staking/src/testing_utils.rs @@ -158,7 +158,7 @@ pub fn get_weak_solution( // self stake >::iter().for_each(|(who, _p)| { - *backing_stake_of.entry(who.clone()).or_insert(Zero::zero()) += + *backing_stake_of.entry(who.clone()).or_insert_with(|| Zero::zero()) += >::slashable_balance_of(&who) }); diff --git a/frame/support/procedural/src/storage/mod.rs b/frame/support/procedural/src/storage/mod.rs index 766141f5aaf8f..b42639c30c5b9 100644 --- a/frame/support/procedural/src/storage/mod.rs +++ b/frame/support/procedural/src/storage/mod.rs @@ -249,7 +249,7 @@ impl StorageLineDefExt { StorageLineTypeDef::DoubleMap(map) => map.value.clone(), }; let is_option = ext::extract_type_option(&query_type).is_some(); - let value_type = ext::extract_type_option(&query_type).unwrap_or(query_type.clone()); + let value_type = ext::extract_type_option(&query_type).unwrap_or_else(|| query_type.clone()); let module_runtime_generic = &def.module_runtime_generic; let module_runtime_trait = &def.module_runtime_trait; @@ -328,7 +328,7 @@ impl StorageLineDefExt { pub enum StorageLineTypeDef { Map(MapDef), - DoubleMap(DoubleMapDef), + DoubleMap(Box), Simple(syn::Type), } diff --git a/frame/support/procedural/src/storage/parse.rs b/frame/support/procedural/src/storage/parse.rs index 5a3bb3f40cd4b..b1ef2916ad886 100644 --- a/frame/support/procedural/src/storage/parse.rs +++ b/frame/support/procedural/src/storage/parse.rs @@ -198,7 +198,7 @@ impl_parse_for_opt!(DeclStorageBuild => keyword::build); #[derive(ToTokens, Debug)] enum DeclStorageType { Map(DeclStorageMap), - DoubleMap(DeclStorageDoubleMap), + DoubleMap(Box), Simple(syn::Type), } @@ -478,13 +478,13 @@ fn parse_storage_line_defs( } ), DeclStorageType::DoubleMap(map) => super::StorageLineTypeDef::DoubleMap( - super::DoubleMapDef { + Box::new(super::DoubleMapDef { hasher1: map.hasher1.inner.ok_or_else(no_hasher_error)?.into(), hasher2: map.hasher2.inner.ok_or_else(no_hasher_error)?.into(), key1: map.key1, key2: map.key2, value: map.value, - } + }) ), DeclStorageType::Simple(expr) => super::StorageLineTypeDef::Simple(expr), }; diff --git a/frame/support/procedural/tools/derive/src/lib.rs b/frame/support/procedural/tools/derive/src/lib.rs index ec5af13b6753f..6e5d6c896cbf8 100644 --- a/frame/support/procedural/tools/derive/src/lib.rs +++ b/frame/support/procedural/tools/derive/src/lib.rs @@ -30,7 +30,7 @@ pub(crate) fn fields_idents( fields: impl Iterator, ) -> impl Iterator { fields.enumerate().map(|(ix, field)| { - field.ident.clone().map(|i| quote!{#i}).unwrap_or_else(|| { + field.ident.map(|i| quote!{#i}).unwrap_or_else(|| { let f_ix: syn::Ident = syn::Ident::new(&format!("f_{}", ix), Span::call_site()); quote!( #f_ix ) }) @@ -41,7 +41,7 @@ pub(crate) fn fields_access( fields: impl Iterator, ) -> impl Iterator { fields.enumerate().map(|(ix, field)| { - field.ident.clone().map(|i| quote!( #i )).unwrap_or_else(|| { + field.ident.map(|i| quote!( #i )).unwrap_or_else(|| { let f_ix: syn::Index = syn::Index { index: ix as u32, span: Span::call_site(), diff --git a/frame/system/src/offchain.rs b/frame/system/src/offchain.rs index 1290ca6378eb8..6e6284b57fdc3 100644 --- a/frame/system/src/offchain.rs +++ b/frame/system/src/offchain.rs @@ -185,7 +185,7 @@ impl, X> Signer let generic_public = C::GenericPublic::from(key); let public = generic_public.into(); let account_id = public.clone().into_account(); - Account::new(index, account_id, public.clone()) + Account::new(index, account_id, public) }) } } diff --git a/primitives/api/proc-macro/src/decl_runtime_apis.rs b/primitives/api/proc-macro/src/decl_runtime_apis.rs index 93ec09d0e615b..8d9eeebef678a 100644 --- a/primitives/api/proc-macro/src/decl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/decl_runtime_apis.rs @@ -252,7 +252,7 @@ fn generate_native_call_generators(decl: &ItemTrait) -> Result { } FnArg::Typed(arg) }, - r => r.clone(), + r => r, }); let (impl_generics, ty_generics, where_clause) = decl.generics.split_for_impl(); diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index 97b159e6f0791..85f5a1797b1e3 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -417,7 +417,7 @@ fn extend_with_runtime_decl_path(mut trait_: Path) -> Path { }; let pos = trait_.segments.len() - 1; - trait_.segments.insert(pos, runtime.clone().into()); + trait_.segments.insert(pos, runtime.into()); trait_ } diff --git a/primitives/arithmetic/fuzzer/src/biguint.rs b/primitives/arithmetic/fuzzer/src/biguint.rs index 0966c128954a7..9763245f4c7e0 100644 --- a/primitives/arithmetic/fuzzer/src/biguint.rs +++ b/primitives/arithmetic/fuzzer/src/biguint.rs @@ -48,8 +48,8 @@ fn main() { digits_u.reverse(); digits_v.reverse(); - let num_u = num_bigint::BigUint::new(digits_u.clone()); - let num_v = num_bigint::BigUint::new(digits_v.clone()); + let num_u = num_bigint::BigUint::new(digits_u); + let num_v = num_bigint::BigUint::new(digits_v); if check_digit_lengths(&u, &v, 4) { assert_eq!(u.cmp(&v), ue.cmp(&ve)); @@ -146,14 +146,14 @@ fn main() { // Division if v.len() == 1 && v.get(0) != 0 { - let w = u.clone().div_unit(v.get(0)); - let num_w = num_u.clone() / &num_v; + let w = u.div_unit(v.get(0)); + let num_w = num_u / &num_v; assert_biguints_eq(&w, &num_w); } else if u.len() > v.len() && v.len() > 0 { let num_remainder = num_u.clone() % num_v.clone(); - let (w, remainder) = u.clone().div(&v, return_remainder).unwrap(); - let num_w = num_u.clone() / &num_v; + let (w, remainder) = u.div(&v, return_remainder).unwrap(); + let num_w = num_u / &num_v; assert_biguints_eq(&w, &num_w); diff --git a/primitives/arithmetic/src/fixed_point.rs b/primitives/arithmetic/src/fixed_point.rs index 8653ee2c8f7c8..59c237efb6260 100644 --- a/primitives/arithmetic/src/fixed_point.rs +++ b/primitives/arithmetic/src/fixed_point.rs @@ -84,7 +84,7 @@ pub trait FixedPointNumber: fn saturating_from_integer(int: N) -> Self { let mut n: I129 = int.into(); n.value = n.value.saturating_mul(Self::DIV.saturated_into()); - Self::from_inner(from_i129(n).unwrap_or(to_bound(int, 0))) + Self::from_inner(from_i129(n).unwrap_or_else(|| to_bound(int, 0))) } /// Creates `self` from an integer number `int`. @@ -101,7 +101,7 @@ pub trait FixedPointNumber: if d == D::zero() { panic!("attempt to divide by zero") } - Self::checked_from_rational(n, d).unwrap_or(to_bound(n, d)) + Self::checked_from_rational(n, d).unwrap_or_else(|| to_bound(n, d)) } /// Creates `self` from a rational number. Equal to `n / d`. @@ -137,7 +137,7 @@ pub trait FixedPointNumber: /// /// Returns `N::min` or `N::max` if the result does not fit in `N`. fn saturating_mul_int(self, n: N) -> N { - self.checked_mul_int(n).unwrap_or(to_bound(self.into_inner(), n)) + self.checked_mul_int(n).unwrap_or_else(|| to_bound(self.into_inner(), n)) } /// Checked division for integer type `N`. Equal to `self / d`. @@ -160,7 +160,7 @@ pub trait FixedPointNumber: if d == N::zero() { panic!("attempt to divide by zero") } - self.checked_div_int(d).unwrap_or(to_bound(self.into_inner(), d)) + self.checked_div_int(d).unwrap_or_else(|| to_bound(self.into_inner(), d)) } /// Saturating multiplication for integer type `N`, adding the result back. @@ -183,7 +183,7 @@ pub trait FixedPointNumber: if inner >= Self::Inner::zero() { self } else { - Self::from_inner(inner.checked_neg().unwrap_or(Self::Inner::max_value())) + Self::from_inner(inner.checked_neg().unwrap_or_else(|| Self::Inner::max_value())) } } @@ -301,7 +301,7 @@ impl From for I129 { if n < N::zero() { let value: u128 = n.checked_neg() .map(|n| n.unique_saturated_into()) - .unwrap_or(N::max_value().unique_saturated_into().saturating_add(1)); + .unwrap_or_else(|| N::max_value().unique_saturated_into().saturating_add(1)); I129 { value, negative: true } } else { I129 { value: n.unique_saturated_into(), negative: false } @@ -399,7 +399,7 @@ macro_rules! implement_fixed { } fn saturating_mul(self, rhs: Self) -> Self { - self.checked_mul(&rhs).unwrap_or(to_bound(self.0, rhs.0)) + self.checked_mul(&rhs).unwrap_or_else(|| to_bound(self.0, rhs.0)) } fn saturating_pow(self, exp: usize) -> Self { diff --git a/primitives/consensus/common/src/import_queue/basic_queue.rs b/primitives/consensus/common/src/import_queue/basic_queue.rs index 8eb194841f196..dddc332f43e97 100644 --- a/primitives/consensus/common/src/import_queue/basic_queue.rs +++ b/primitives/consensus/common/src/import_queue/basic_queue.rs @@ -108,7 +108,7 @@ impl ImportQueue for BasicQueue ) { let _ = self.sender .unbounded_send( - ToWorkerMsg::ImportJustification(who.clone(), hash, number, justification) + ToWorkerMsg::ImportJustification(who, hash, number, justification) ); } diff --git a/primitives/core/src/crypto.rs b/primitives/core/src/crypto.rs index b5bb0b935b543..6250c67e3bae4 100644 --- a/primitives/core/src/crypto.rs +++ b/primitives/core/src/crypto.rs @@ -180,7 +180,7 @@ impl DeriveJunction { impl> From for DeriveJunction { fn from(j: T) -> DeriveJunction { let j = j.as_ref(); - let (code, hard) = if j.starts_with("/") { + let (code, hard) = if j.starts_with('/') { (&j[1..], true) } else { (j, false) diff --git a/primitives/core/src/offchain/testing.rs b/primitives/core/src/offchain/testing.rs index 9145477722d78..c939c5cfccc14 100644 --- a/primitives/core/src/offchain/testing.rs +++ b/primitives/core/src/offchain/testing.rs @@ -359,7 +359,7 @@ impl offchain::Externalities for TestOffchainExt { if let Some(req) = state.requests.get_mut(&request_id) { let response = req.response .as_mut() - .expect(&format!("No response provided for request: {:?}", request_id)); + .unwrap_or_else(|| panic!("No response provided for request: {:?}", request_id)); if req.read >= response.len() { // Remove the pending request as per spec. diff --git a/primitives/core/src/testing.rs b/primitives/core/src/testing.rs index 1d88e1fad5513..e512d3a39e212 100644 --- a/primitives/core/src/testing.rs +++ b/primitives/core/src/testing.rs @@ -90,7 +90,7 @@ impl crate::traits::BareCryptoStore for KeyStore { v })) }) - .unwrap_or(Ok(vec![])) + .unwrap_or_else(|| Ok(vec![])) } fn sr25519_public_keys(&self, id: KeyTypeId) -> Vec { @@ -222,19 +222,19 @@ impl crate::traits::BareCryptoStore for KeyStore { ed25519::CRYPTO_ID => { let key_pair: ed25519::Pair = self .ed25519_key_pair(id, &ed25519::Public::from_slice(key.1.as_slice())) - .ok_or(Error::PairNotFound("ed25519".to_owned()))?; + .ok_or_else(|| Error::PairNotFound("ed25519".to_owned()))?; return Ok(key_pair.sign(msg).encode()); } sr25519::CRYPTO_ID => { let key_pair: sr25519::Pair = self .sr25519_key_pair(id, &sr25519::Public::from_slice(key.1.as_slice())) - .ok_or(Error::PairNotFound("sr25519".to_owned()))?; + .ok_or_else(|| Error::PairNotFound("sr25519".to_owned()))?; return Ok(key_pair.sign(msg).encode()); } ecdsa::CRYPTO_ID => { let key_pair: ecdsa::Pair = self .ecdsa_key_pair(id, &ecdsa::Public::from_slice(key.1.as_slice())) - .ok_or(Error::PairNotFound("ecdsa".to_owned()))?; + .ok_or_else(|| Error::PairNotFound("ecdsa".to_owned()))?; return Ok(key_pair.sign(msg).encode()); } _ => Err(Error::KeyNotSupported(id)) @@ -249,7 +249,7 @@ impl crate::traits::BareCryptoStore for KeyStore { ) -> Result { let transcript = make_transcript(transcript_data); let pair = self.sr25519_key_pair(key_type, public) - .ok_or(Error::PairNotFound("Not found".to_owned()))?; + .ok_or_else(|| Error::PairNotFound("Not found".to_owned()))?; let (inout, proof, _) = pair.as_ref().vrf_sign(transcript); Ok(VRFSignature { diff --git a/primitives/npos-elections/fuzzer/src/balance_solution.rs b/primitives/npos-elections/fuzzer/src/balance_solution.rs index e1bd3bd0a07dd..13f9b29706aed 100644 --- a/primitives/npos-elections/fuzzer/src/balance_solution.rs +++ b/primitives/npos-elections/fuzzer/src/balance_solution.rs @@ -114,7 +114,7 @@ fn main() { *stake_of_tree.get(who).unwrap() }; - let mut staked = assignment_ratio_to_staked(assignments.clone(), &stake_of); + let mut staked = assignment_ratio_to_staked(assignments, &stake_of); let winners = to_without_backing(winners); let mut support = build_support_map(winners.as_ref(), staked.as_ref()).0; diff --git a/primitives/npos-elections/src/lib.rs b/primitives/npos-elections/src/lib.rs index 592ed3b717350..b3eb3ed6cc71c 100644 --- a/primitives/npos-elections/src/lib.rs +++ b/primitives/npos-elections/src/lib.rs @@ -416,7 +416,7 @@ pub fn seq_phragmen( n.load.n(), n.budget, c.approval_stake, - ).unwrap_or(Bounded::max_value()); + ).unwrap_or_else(|_| Bounded::max_value()); let temp_d = n.load.d(); let temp = Rational128::from(temp_n, temp_d); c.score = c.score.lazy_saturating_add(temp); @@ -470,14 +470,14 @@ pub fn seq_phragmen( n.load.n(), ) // If result cannot fit in u128. Not much we can do about it. - .unwrap_or(Bounded::max_value()); + .unwrap_or_else(|_| Bounded::max_value()); TryFrom::try_from(parts) // If the result cannot fit into R::Inner. Defensive only. This can // never happen. `desired_scale * e / n`, where `e / n < 1` always // yields a value smaller than `desired_scale`, which will fit into // R::Inner. - .unwrap_or(Bounded::max_value()) + .unwrap_or_else(|_| Bounded::max_value()) } else { // defensive only. Both edge and voter loads are built from // scores, hence MUST have the same denominator. diff --git a/primitives/npos-elections/src/reduce.rs b/primitives/npos-elections/src/reduce.rs index d0b4afe73dff9..6d458a5fffb38 100644 --- a/primitives/npos-elections/src/reduce.rs +++ b/primitives/npos-elections/src/reduce.rs @@ -362,11 +362,11 @@ fn reduce_all(assignments: &mut Vec>) -> u32 // create both. let voter_node = tree .entry(voter_id.clone()) - .or_insert(Node::new(voter_id).into_ref()) + .or_insert_with(|| Node::new(voter_id).into_ref()) .clone(); let target_node = tree .entry(target_id.clone()) - .or_insert(Node::new(target_id).into_ref()) + .or_insert_with(|| Node::new(target_id).into_ref()) .clone(); // If one exists but the other one doesn't, or if both does not, then set the existing diff --git a/primitives/state-machine/src/changes_trie/changes_iterator.rs b/primitives/state-machine/src/changes_trie/changes_iterator.rs index f27493ee4b4b6..f9398b3ce5dd4 100644 --- a/primitives/state-machine/src/changes_trie/changes_iterator.rs +++ b/primitives/state-machine/src/changes_trie/changes_iterator.rs @@ -46,7 +46,7 @@ pub fn key_changes<'a, H: Hasher, Number: BlockNumber>( key: &'a [u8], ) -> Result, String> { // we can't query any roots before root - let max = ::std::cmp::min(max.clone(), end.number.clone()); + let max = std::cmp::min(max, end.number.clone()); Ok(DrilldownIterator { essence: DrilldownIteratorEssence { @@ -85,7 +85,7 @@ pub fn key_changes_proof<'a, H: Hasher, Number: BlockNumber>( key: &[u8], ) -> Result>, String> where H::Out: Codec { // we can't query any roots before root - let max = ::std::cmp::min(max.clone(), end.number.clone()); + let max = std::cmp::min(max, end.number.clone()); let mut iter = ProvingDrilldownIterator { essence: DrilldownIteratorEssence { @@ -156,7 +156,7 @@ pub fn key_changes_proof_check_with_db<'a, H: Hasher, Number: BlockNumber>( key: &[u8] ) -> Result, String> where H::Out: Encode { // we can't query any roots before root - let max = ::std::cmp::min(max.clone(), end.number.clone()); + let max = std::cmp::min(max, end.number.clone()); DrilldownIterator { essence: DrilldownIteratorEssence { diff --git a/primitives/state-machine/src/ext.rs b/primitives/state-machine/src/ext.rs index cd4f83661b9e3..d7d4bc145eb06 100644 --- a/primitives/state-machine/src/ext.rs +++ b/primitives/state-machine/src/ext.rs @@ -471,8 +471,8 @@ where let root = self .storage(prefixed_storage_key.as_slice()) .and_then(|k| Decode::decode(&mut &k[..]).ok()) - .unwrap_or( - empty_child_trie_root::>() + .unwrap_or_else( + || empty_child_trie_root::>() ); trace!(target: "state", "{:04x}: ChildRoot({})(cached) {}", self.id, @@ -512,8 +512,8 @@ where let root = self .storage(prefixed_storage_key.as_slice()) .and_then(|k| Decode::decode(&mut &k[..]).ok()) - .unwrap_or( - empty_child_trie_root::>() + .unwrap_or_else( + || empty_child_trie_root::>() ); trace!(target: "state", "{:04x}: ChildRoot({})(no_change) {}", self.id, diff --git a/primitives/state-machine/src/in_memory_backend.rs b/primitives/state-machine/src/in_memory_backend.rs index 8c0ae1ec8bf41..f211f60202730 100644 --- a/primitives/state-machine/src/in_memory_backend.rs +++ b/primitives/state-machine/src/in_memory_backend.rs @@ -109,7 +109,7 @@ where Some(map) => insert_into_memory_db::( root, self.backend_storage_mut(), - map.clone().into_iter().chain(new_child_roots.into_iter()), + map.into_iter().chain(new_child_roots.into_iter()), ), None => insert_into_memory_db::( root, diff --git a/primitives/state-machine/src/proving_backend.rs b/primitives/state-machine/src/proving_backend.rs index 1f25005bc37b1..0888c561cae30 100644 --- a/primitives/state-machine/src/proving_backend.rs +++ b/primitives/state-machine/src/proving_backend.rs @@ -71,7 +71,7 @@ impl<'a, S, H> ProvingBackendRecorder<'a, S, H> let storage_key = child_info.storage_key(); let root = self.storage(storage_key)? .and_then(|r| Decode::decode(&mut &r[..]).ok()) - .unwrap_or(empty_child_trie_root::>()); + .unwrap_or_else(|| empty_child_trie_root::>()); let mut read_overlay = S::Overlay::default(); let eph = Ephemeral::new( diff --git a/primitives/state-machine/src/trie_backend.rs b/primitives/state-machine/src/trie_backend.rs index 2d4ab782cba70..e0a86bbd193a1 100644 --- a/primitives/state-machine/src/trie_backend.rs +++ b/primitives/state-machine/src/trie_backend.rs @@ -202,7 +202,7 @@ impl, H: Hasher> Backend for TrieBackend where let prefixed_storage_key = child_info.prefixed_storage_key(); let mut root = match self.storage(prefixed_storage_key.as_slice()) { Ok(value) => - value.and_then(|r| Decode::decode(&mut &r[..]).ok()).unwrap_or(default_root.clone()), + value.and_then(|r| Decode::decode(&mut &r[..]).ok()).unwrap_or_else(|| default_root.clone()), Err(e) => { warn!(target: "trie", "Failed to read child storage root: {}", e); default_root.clone() diff --git a/primitives/state-machine/src/trie_backend_essence.rs b/primitives/state-machine/src/trie_backend_essence.rs index c0ec15c1371ee..72864e312b6ab 100644 --- a/primitives/state-machine/src/trie_backend_essence.rs +++ b/primitives/state-machine/src/trie_backend_essence.rs @@ -171,7 +171,7 @@ impl, H: Hasher> TrieBackendEssence where H::Out: key: &[u8], ) -> Result, String> { let root = self.child_root(child_info)? - .unwrap_or(empty_child_trie_root::>().encode()); + .unwrap_or_else(|| empty_child_trie_root::>().encode()); let map_e = |e| format!("Trie lookup error: {}", e); @@ -186,7 +186,7 @@ impl, H: Hasher> TrieBackendEssence where H::Out: f: F, ) { let root = match self.child_root(child_info) { - Ok(v) => v.unwrap_or(empty_child_trie_root::>().encode()), + Ok(v) => v.unwrap_or_else(|| empty_child_trie_root::>().encode()), Err(e) => { debug!(target: "trie", "Error while iterating child storage: {}", e); return; @@ -211,7 +211,7 @@ impl, H: Hasher> TrieBackendEssence where H::Out: mut f: F, ) { let root_vec = match self.child_root(child_info) { - Ok(v) => v.unwrap_or(empty_child_trie_root::>().encode()), + Ok(v) => v.unwrap_or_else(|| empty_child_trie_root::>().encode()), Err(e) => { debug!(target: "trie", "Error while iterating child storage: {}", e); return; diff --git a/primitives/wasm-interface/src/lib.rs b/primitives/wasm-interface/src/lib.rs index d3ca4ecb5e937..c432a966056c5 100644 --- a/primitives/wasm-interface/src/lib.rs +++ b/primitives/wasm-interface/src/lib.rs @@ -20,6 +20,7 @@ #![cfg_attr(not(feature = "std"), no_std)] use sp_std::{ + vec, borrow::Cow, marker::PhantomData, mem, iter::Iterator, result, vec::Vec, }; @@ -275,8 +276,7 @@ impl PartialEq for dyn Function { pub trait FunctionContext { /// Read memory from `address` into a vector. fn read_memory(&self, address: Pointer, size: WordSize) -> Result> { - let mut vec = Vec::with_capacity(size as usize); - vec.resize(size as usize, 0); + let mut vec = vec![0; size as usize]; self.read_memory_into(address, &mut vec)?; Ok(vec) } diff --git a/test-utils/runtime/client/src/lib.rs b/test-utils/runtime/client/src/lib.rs index 4e9034fb4d493..97cf13ed2ae56 100644 --- a/test-utils/runtime/client/src/lib.rs +++ b/test-utils/runtime/client/src/lib.rs @@ -348,7 +348,7 @@ pub fn new_light() -> ( let storage = sc_client_db::light::LightStorage::new_test(); let blockchain = Arc::new(sc_light::Blockchain::new(storage)); - let backend = Arc::new(LightBackend::new(blockchain.clone())); + let backend = Arc::new(LightBackend::new(blockchain)); let executor = new_native_executor(); let local_call_executor = client::LocalCallExecutor::new(backend.clone(), executor, sp_core::tasks::executor(), Default::default()); let call_executor = LightExecutor::new( diff --git a/test-utils/runtime/client/src/trait_tests.rs b/test-utils/runtime/client/src/trait_tests.rs index 537ff1197e7ef..b240a42a78555 100644 --- a/test-utils/runtime/client/src/trait_tests.rs +++ b/test-utils/runtime/client/src/trait_tests.rs @@ -284,7 +284,7 @@ pub fn test_children_for_backend(backend: Arc) where Default::default(), false, ).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, b4.clone()).unwrap(); + client.import(BlockOrigin::Own, b4).unwrap(); // // B2 -> C3 let mut builder = client.new_block_at( @@ -413,7 +413,7 @@ pub fn test_blockchain_query_by_number_gets_canonical(backend: Arc C3 let mut builder = client.new_block_at( @@ -429,7 +429,7 @@ pub fn test_blockchain_query_by_number_gets_canonical(backend: Arc D2 let mut builder = client.new_block_at( @@ -445,7 +445,7 @@ pub fn test_blockchain_query_by_number_gets_canonical(backend: Arc Self { GenesisConfig { changes_trie_config, - authorities: authorities.clone(), + authorities: authorities, balances: endowed_accounts.into_iter().map(|a| (a, balance)).collect(), heap_pages_override, extra_storage, diff --git a/utils/frame/rpc/system/src/lib.rs b/utils/frame/rpc/system/src/lib.rs index dc87f622fdc08..320423623668a 100644 --- a/utils/frame/rpc/system/src/lib.rs +++ b/utils/frame/rpc/system/src/lib.rs @@ -263,7 +263,7 @@ fn adjust_nonce( // `provides` tag. And increment the nonce if we find a transaction // that matches the current one. let mut current_nonce = nonce.clone(); - let mut current_tag = (account.clone(), nonce.clone()).encode(); + let mut current_tag = (account.clone(), nonce).encode(); for tx in pool.ready() { log::debug!( target: "rpc", diff --git a/utils/wasm-builder/src/lib.rs b/utils/wasm-builder/src/lib.rs index 95b75c5867f60..c68921d05a65b 100644 --- a/utils/wasm-builder/src/lib.rs +++ b/utils/wasm-builder/src/lib.rs @@ -189,7 +189,7 @@ fn check_skip_build() -> bool { /// Write to the given `file` if the `content` is different. fn write_file_if_changed(file: PathBuf, content: String) { if fs::read_to_string(&file).ok().as_ref() != Some(&content) { - fs::write(&file, content).expect(&format!("Writing `{}` can not fail!", file.display())); + fs::write(&file, content).unwrap_or_else(|_| panic!("Writing `{}` can not fail!", file.display())); } } @@ -200,7 +200,7 @@ fn copy_file_if_changed(src: PathBuf, dst: PathBuf) { if src_file != dst_file { fs::copy(&src, &dst) - .expect(&format!("Copying `{}` to `{}` can not fail; qed", src.display(), dst.display())); + .unwrap_or_else(|_| panic!("Copying `{}` to `{}` can not fail; qed", src.display(), dst.display())); } } diff --git a/utils/wasm-builder/src/wasm_project.rs b/utils/wasm-builder/src/wasm_project.rs index 7df3524e8aaf8..6f8f47881b03f 100644 --- a/utils/wasm-builder/src/wasm_project.rs +++ b/utils/wasm-builder/src/wasm_project.rs @@ -205,7 +205,7 @@ fn find_and_clear_workspace_members(wasm_workspace: &Path) -> Vec { .map(|d| d.into_path()) .filter(|p| p.is_dir()) .filter_map(|p| p.file_name().map(|f| f.to_owned()).and_then(|s| s.into_string().ok())) - .filter(|f| !f.starts_with(".") && f != "target") + .filter(|f| !f.starts_with('.') && f != "target") .collect::>(); let mut i = 0; From 056879f376c46154847927928511c6fd127bef28 Mon Sep 17 00:00:00 2001 From: Dan Forbes Date: Tue, 21 Jul 2020 06:14:25 -0700 Subject: [PATCH 3/7] Remove dead link to out-of-date style guide (#6682) * Remove dead link to out-of-date style guide * Replace dead link with self-hosted doc * Use relative link to style guide Co-authored-by: Benjamin Kampmann * Format style guide Co-authored-by: Benjamin Kampmann * Formatting Co-authored-by: Benjamin Kampmann --- docs/CONTRIBUTING.adoc | 2 +- docs/STYLE_GUIDE.md | 146 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 147 insertions(+), 1 deletion(-) create mode 100644 docs/STYLE_GUIDE.md diff --git a/docs/CONTRIBUTING.adoc b/docs/CONTRIBUTING.adoc index 1d82a43921d2f..491e24aeaec85 100644 --- a/docs/CONTRIBUTING.adoc +++ b/docs/CONTRIBUTING.adoc @@ -14,7 +14,7 @@ There are a few basic ground-rules for contributors (including the maintainer(s) . **Non-master branches**, prefixed with a short name moniker (e.g. `gav-my-feature`) must be used for ongoing work. . **All modifications** must be made in a **pull-request** to solicit feedback from other contributors. . A pull-request *must not be merged until CI* has finished successfully. -. Contributors should adhere to the https://wiki.parity.io/Substrate-Style-Guide[house coding style]. +. Contributors should adhere to the ./STYLE_GUIDE.md[house coding style]. == Merge Process diff --git a/docs/STYLE_GUIDE.md b/docs/STYLE_GUIDE.md new file mode 100644 index 0000000000000..e6f217f2b4859 --- /dev/null +++ b/docs/STYLE_GUIDE.md @@ -0,0 +1,146 @@ +--- +title: Style Guide for Rust in Substrate +--- + +# Formatting + +- Indent using tabs. +- Lines should be longer than 100 characters long only in exceptional circumstances and certainly + no longer than 120. For this purpose, tabs are considered 4 characters wide. +- Indent levels should be greater than 5 only in exceptional circumstances and certainly no + greater than 8. If they are greater than 5, then consider using `let` or auxiliary functions in + order to strip out complex inline expressions. +- Never have spaces on a line prior to a non-whitespace character +- Follow-on lines are only ever a single indent from the original line. + +```rust +fn calculation(some_long_variable_a: i8, some_long_variable_b: i8) -> bool { + let x = some_long_variable_a * some_long_variable_b + - some_long_variable_b / some_long_variable_a + + sqrt(some_long_variable_a) - sqrt(some_long_variable_b); + x > 10 +} +``` + +- Indent level should follow open parens/brackets, but should be collapsed to the smallest number + of levels actually used: + +```rust +fn calculate( + some_long_variable_a: f32, + some_long_variable_b: f32, + some_long_variable_c: f32, +) -> f32 { + (-some_long_variable_b + sqrt( + // two parens open, but since we open & close them both on the + // same line, only one indent level is used + some_long_variable_b * some_long_variable_b + - 4 * some_long_variable_a * some_long_variable_c + // both closed here at beginning of line, so back to the original indent + // level + )) / (2 * some_long_variable_a) +} +``` + +- `where` is indented, and its items are indented one further. +- Argument lists or function invocations that are too long to fit on one line are indented + similarly to code blocks, and once one param is indented in such a way, all others should be, + too. Run-on parameter lists are also acceptable for single-line run-ons of basic function calls. + +```rust +// OK +fn foo( + really_long_parameter_name_1: SomeLongTypeName, + really_long_parameter_name_2: SomeLongTypeName, + shrt_nm_1: u8, + shrt_nm_2: u8, +) { + ... +} + +// NOT OK +fn foo(really_long_parameter_name_1: SomeLongTypeName, really_long_parameter_name_2: SomeLongTypeName, + shrt_nm_1: u8, shrt_nm_2: u8) { + ... +} +``` + +```rust +{ + // Complex line (not just a function call, also a let statement). Full + // structure. + let (a, b) = bar( + really_long_parameter_name_1, + really_long_parameter_name_2, + shrt_nm_1, + shrt_nm_2, + ); + + // Long, simple function call. + waz( + really_long_parameter_name_1, + really_long_parameter_name_2, + shrt_nm_1, + shrt_nm_2, + ); + + // Short function call. Inline. + baz(a, b); +} +``` + +- Always end last item of a multi-line comma-delimited set with `,` when legal: + +```rust +struct Point { + x: T, + y: T, // <-- Multiline comma-delimited lists end with a trailing , +} + +// Single line comma-delimited items do not have a trailing `,` +enum Meal { Breakfast, Lunch, Dinner }; +``` + +- Avoid trailing `;`s where unneeded. + +```rust +if condition { + return 1 // <-- no ; here +} +``` + +- `match` arms may be either blocks or have a trailing `,` but not both. +- Blocks should not be used unnecessarily. + +```rust +match meal { + Meal::Breakfast => "eggs", + Meal::Lunch => { check_diet(); recipe() }, +// Meal::Dinner => { return Err("Fasting") } // WRONG + Meal::Dinner => return Err("Fasting"), +} +``` + +# Style + +- Panickers require explicit proofs they don't trigger. Calling `unwrap` is discouraged. The + exception to this rule is test code. Avoiding panickers by restructuring code is preferred if + feasible. + +```rust +let mut target_path = + self.path().expect( + "self is instance of DiskDirectory;\ + DiskDirectory always returns path;\ + qed" + ); +``` + +- Unsafe code requires explicit proofs just as panickers do. When introducing unsafe code, + consider tradeoffs between efficiency on one hand and reliability, maintenance costs, and + security on the other. Here is a list of questions that may help evaluating the tradeoff while + preparing or reviewing a PR: + - how much more performant or compact the resulting code will be using unsafe code, + - how likely is it that invariants could be violated, + - are issues stemming from the use of unsafe code caught by existing tests/tooling, + - what are the consequences if the problems slip into production. From aa36bf284178daaea56399fedf5fcae8b9e282bc Mon Sep 17 00:00:00 2001 From: Denis Pisarev Date: Tue, 21 Jul 2020 16:55:54 +0200 Subject: [PATCH 4/7] "cargo test" jobs optimization (#6606) * change (ci): 3 jobs in 1 decreases concurrency and is more effectiv; w/o release it's ~20% faster, but needs testing on prod; wasmtest tests are already running within cargo test --workspace * fix (test): these ones were failing on nightly * save: cargo profiles [skip ci] * change (ci): one test to run them all * change (ci): rebase * Revert "change (ci): rebase" This reverts commit 8a6b7ea043a460bf71526ccaa4c7a68899a3b2bc. * fix (config): fix manifest * change (ci): bench release --- .gitlab-ci.yml | 61 ++------------------- frame/support/test/tests/decl_module_ui.rs | 2 +- frame/support/test/tests/decl_storage_ui.rs | 1 + 3 files changed, 7 insertions(+), 57 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b8e66d9eb75c6..1c9c2e513944e 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -221,13 +221,15 @@ test-linux-stable: &test-linux <<: *default-vars # Enable debug assertions since we are running optimized builds for testing # but still want to have debug assertions. - RUSTFLAGS: "-Cdebug-assertions=y -Dwarnings" - RUST_BACKTRACE: 1 + RUSTFLAGS: "-Cdebug-assertions=y -Dwarnings" + RUST_BACKTRACE: 1 + WASM_BUILD_NO_COLOR: 1 except: variables: - $DEPLOY_TAG script: - - WASM_BUILD_NO_COLOR=1 time cargo test --all --release --verbose --locked + # this job runs all tests in former runtime-benchmarks, frame-staking and wasmtime tests + - time cargo test --workspace --locked --release --verbose --features runtime-benchmarks --manifest-path bin/node/cli/Cargo.toml - sccache -s unleash-check: @@ -240,24 +242,6 @@ unleash-check: - cargo install cargo-unleash ${CARGO_UNLEASH_INSTALL_PARAMS} - cargo unleash check ${CARGO_UNLEASH_PKG_DEF} -test-frame-staking: - # into one job - stage: test - <<: *docker-env - variables: - <<: *default-vars - # Enable debug assertions since we are running optimized builds for testing - # but still want to have debug assertions. - RUSTFLAGS: -Cdebug-assertions=y - RUST_BACKTRACE: 1 - except: - variables: - - $DEPLOY_TAG - script: - - cd frame/staking/ - - WASM_BUILD_NO_COLOR=1 time cargo test --release --verbose --no-default-features --features "std" - - sccache -s - test-frame-examples-compile-to-wasm: # into one job stage: test @@ -278,41 +262,6 @@ test-frame-examples-compile-to-wasm: - cargo +nightly build --target=wasm32-unknown-unknown --no-default-features - sccache -s -test-wasmtime: - stage: test - <<: *docker-env - variables: - <<: *default-vars - # Enable debug assertions since we are running optimized builds for testing - # but still want to have debug assertions. - RUSTFLAGS: -Cdebug-assertions=y - RUST_BACKTRACE: 1 - except: - variables: - - $DEPLOY_TAG - script: - - cd client/executor - - WASM_BUILD_NO_COLOR=1 time cargo test --release --verbose --features wasmtime - - sccache -s - -test-runtime-benchmarks: - # into one job - stage: test - <<: *docker-env - variables: - <<: *default-vars - # Enable debug assertions since we are running optimized builds for testing - # but still want to have debug assertions. - RUSTFLAGS: -Cdebug-assertions=y - RUST_BACKTRACE: 1 - except: - variables: - - $DEPLOY_TAG - script: - - cd bin/node/cli - - WASM_BUILD_NO_COLOR=1 time cargo test --workspace --release --verbose --features runtime-benchmarks - - sccache -s - test-linux-stable-int: <<: *test-linux except: diff --git a/frame/support/test/tests/decl_module_ui.rs b/frame/support/test/tests/decl_module_ui.rs index 90d105e7cfae3..7df64bc52f412 100644 --- a/frame/support/test/tests/decl_module_ui.rs +++ b/frame/support/test/tests/decl_module_ui.rs @@ -15,7 +15,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -//#[rustversion::attr(not(stable), ignore)] +#[rustversion::attr(not(stable), ignore)] #[test] fn decl_module_ui() { // As trybuild is using `cargo check`, we don't need the real WASM binaries. diff --git a/frame/support/test/tests/decl_storage_ui.rs b/frame/support/test/tests/decl_storage_ui.rs index d771b6e0eef83..56529d62c28ff 100644 --- a/frame/support/test/tests/decl_storage_ui.rs +++ b/frame/support/test/tests/decl_storage_ui.rs @@ -15,6 +15,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +#[rustversion::attr(not(stable), ignore)] #[test] fn decl_storage_ui() { // As trybuild is using `cargo check`, we don't need the real WASM binaries. From 4b63b456e0a5b939f9a8a59963fa379519491095 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Tue, 21 Jul 2020 17:33:33 +0200 Subject: [PATCH 5/7] Properly filter out duplicate voters in elections. (#6693) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Prevent duplicate voter * Update primitives/npos-elections/src/lib.rs Co-authored-by: Bastian Köcher Co-authored-by: Bastian Köcher --- frame/staking/src/tests.rs | 95 ++++++++++++++++++++++++++ primitives/npos-elections/src/lib.rs | 4 ++ primitives/npos-elections/src/tests.rs | 60 ++++++++++++++++ 3 files changed, 159 insertions(+) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index a3cfed9e2f260..a957b6ef33a79 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -1715,6 +1715,101 @@ fn bond_with_little_staked_value_bounded() { }); } +#[test] +fn bond_with_duplicate_vote_should_be_ignored_by_npos_election() { + ExtBuilder::default() + .validator_count(2) + .nominate(false) + .minimum_validator_count(1) + .build() + .execute_with(|| { + // disable the nominator + assert_ok!(Staking::chill(Origin::signed(100))); + // make stakes equal. + assert_ok!(Staking::bond_extra(Origin::signed(31), 999)); + + assert_eq!( + >::iter() + .map(|(v, _)| (v, Staking::ledger(v - 1).unwrap().total)) + .collect::>(), + vec![(31, 1000), (21, 1000), (11, 1000)], + ); + assert_eq!(>::iter().map(|(n, _)| n).collect::>(), vec![]); + + // give the man some money + let initial_balance = 1000; + for i in [1, 2, 3, 4,].iter() { + let _ = Balances::make_free_balance_be(i, initial_balance); + } + + assert_ok!(Staking::bond(Origin::signed(1), 2, 1000, RewardDestination::Controller)); + assert_ok!(Staking::nominate(Origin::signed(2), vec![11, 11, 11, 21, 31,])); + + assert_ok!(Staking::bond(Origin::signed(3), 4, 1000, RewardDestination::Controller)); + assert_ok!(Staking::nominate(Origin::signed(4), vec![21, 31])); + + // winners should be 21 and 31. Otherwise this election is taking duplicates into account. + + let sp_npos_elections::ElectionResult { + winners, + assignments, + } = Staking::do_phragmen::().unwrap(); + let winners = sp_npos_elections::to_without_backing(winners); + + assert_eq!(winners, vec![31, 21]); + // only distribution to 21 and 31. + assert_eq!(assignments.iter().find(|a| a.who == 1).unwrap().distribution.len(), 2); + }); +} + +#[test] +fn bond_with_duplicate_vote_should_be_ignored_by_npos_election_elected() { + // same as above but ensures that even when the duple is being elected, everything is sane. + ExtBuilder::default() + .validator_count(2) + .nominate(false) + .minimum_validator_count(1) + .build() + .execute_with(|| { + // disable the nominator + assert_ok!(Staking::chill(Origin::signed(100))); + // make stakes equal. + assert_ok!(Staking::bond_extra(Origin::signed(31), 99)); + + assert_eq!( + >::iter() + .map(|(v, _)| (v, Staking::ledger(v - 1).unwrap().total)) + .collect::>(), + vec![(31, 100), (21, 1000), (11, 1000)], + ); + assert_eq!(>::iter().map(|(n, _)| n).collect::>(), vec![]); + + // give the man some money + let initial_balance = 1000; + for i in [1, 2, 3, 4,].iter() { + let _ = Balances::make_free_balance_be(i, initial_balance); + } + + assert_ok!(Staking::bond(Origin::signed(1), 2, 1000, RewardDestination::Controller)); + assert_ok!(Staking::nominate(Origin::signed(2), vec![11, 11, 11, 21, 31,])); + + assert_ok!(Staking::bond(Origin::signed(3), 4, 1000, RewardDestination::Controller)); + assert_ok!(Staking::nominate(Origin::signed(4), vec![21, 31])); + + // winners should be 21 and 31. Otherwise this election is taking duplicates into account. + + let sp_npos_elections::ElectionResult { + winners, + assignments, + } = Staking::do_phragmen::().unwrap(); + + let winners = sp_npos_elections::to_without_backing(winners); + assert_eq!(winners, vec![21, 11]); + // only distribution to 21 and 31. + assert_eq!(assignments.iter().find(|a| a.who == 1).unwrap().distribution.len(), 2); + }); +} + #[test] fn new_era_elects_correct_number_of_validators() { ExtBuilder::default() diff --git a/primitives/npos-elections/src/lib.rs b/primitives/npos-elections/src/lib.rs index b3eb3ed6cc71c..9ac058f8c3ef6 100644 --- a/primitives/npos-elections/src/lib.rs +++ b/primitives/npos-elections/src/lib.rs @@ -371,6 +371,10 @@ pub fn seq_phragmen( voters.extend(initial_voters.into_iter().map(|(who, voter_stake, votes)| { let mut edges: Vec> = Vec::with_capacity(votes.len()); for v in votes { + if edges.iter().any(|e| e.who == v) { + // duplicate edge. + continue; + } if let Some(idx) = c_idx_cache.get(&v) { // This candidate is valid + already cached. candidates[*idx].approval_stake = candidates[*idx].approval_stake diff --git a/primitives/npos-elections/src/tests.rs b/primitives/npos-elections/src/tests.rs index 80c742117d979..c630f0ae35984 100644 --- a/primitives/npos-elections/src/tests.rs +++ b/primitives/npos-elections/src/tests.rs @@ -588,6 +588,66 @@ fn self_votes_should_be_kept() { ); } +#[test] +fn duplicate_target_is_ignored() { + let candidates = vec![1, 2, 3]; + let voters = vec![ + (10, 100, vec![1, 1, 2, 3]), + (20, 100, vec![2, 3]), + (30, 50, vec![1, 1, 2]), + ]; + + let ElectionResult { winners, assignments } = seq_phragmen::<_, Perbill>( + 2, + 2, + candidates, + voters, + ).unwrap(); + let winners = to_without_backing(winners); + + assert_eq!(winners, vec![(2), (3)]); + assert_eq!( + assignments + .into_iter() + .map(|x| (x.who, x.distribution.into_iter().map(|(w, _)| w).collect::>())) + .collect::>(), + vec![ + (10, vec![2, 3]), + (20, vec![2, 3]), + (30, vec![2]), + ], + ); +} + +#[test] +fn duplicate_target_is_ignored_when_winner() { + let candidates = vec![1, 2, 3]; + let voters = vec![ + (10, 100, vec![1, 1, 2, 3]), + (20, 100, vec![1, 2]), + ]; + + let ElectionResult { winners, assignments } = seq_phragmen::<_, Perbill>( + 2, + 2, + candidates, + voters, + ).unwrap(); + let winners = to_without_backing(winners); + + assert_eq!(winners, vec![1, 2]); + assert_eq!( + assignments + .into_iter() + .map(|x| (x.who, x.distribution.into_iter().map(|(w, _)| w).collect::>())) + .collect::>(), + vec![ + (10, vec![1, 2]), + (20, vec![1, 2]), + ], + ); +} + mod assignment_convert_normalize { use super::*; #[test] From 6af309a30c60631899948a5ec893fe174d9689ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 21 Jul 2020 17:39:56 +0200 Subject: [PATCH 6/7] pallet-swap-action: Change `BalanceSwapAction` signature (#6580) Instead of requiring `T: Trait` in `BalanceSwapAction`, we directly depend on `AccountId`. This fixes a compilation error on wasm, where `Runtime` does not implement `Debug`, but `BalanceSwapAction` required it. --- frame/atomic-swap/src/lib.rs | 40 +++++++++++++++------------------- frame/atomic-swap/src/tests.rs | 2 +- 2 files changed, 18 insertions(+), 24 deletions(-) diff --git a/frame/atomic-swap/src/lib.rs b/frame/atomic-swap/src/lib.rs index 7e8354f8b65d6..65794792d0aa4 100644 --- a/frame/atomic-swap/src/lib.rs +++ b/frame/atomic-swap/src/lib.rs @@ -74,61 +74,55 @@ pub type HashedProof = [u8; 32]; /// succeeds with best efforts. /// - **Claim**: claim any resources reserved in the first phrase. /// - **Cancel**: cancel any resources reserved in the first phrase. -pub trait SwapAction { +pub trait SwapAction { /// Reserve the resources needed for the swap, from the given `source`. The reservation is /// allowed to fail. If that is the case, the the full swap creation operation is cancelled. - fn reserve(&self, source: &T::AccountId) -> DispatchResult; + fn reserve(&self, source: &AccountId) -> DispatchResult; /// Claim the reserved resources, with `source` and `target`. Returns whether the claim /// succeeds. - fn claim(&self, source: &T::AccountId, target: &T::AccountId) -> bool; + fn claim(&self, source: &AccountId, target: &AccountId) -> bool; /// Weight for executing the operation. fn weight(&self) -> Weight; /// Cancel the resources reserved in `source`. - fn cancel(&self, source: &T::AccountId); + fn cancel(&self, source: &AccountId); } /// A swap action that only allows transferring balances. #[derive(Clone, RuntimeDebug, Eq, PartialEq, Encode, Decode)] -pub struct BalanceSwapAction> { - value: ::AccountId>>::Balance, +pub struct BalanceSwapAction> { + value: >::Balance, _marker: PhantomData, } -impl BalanceSwapAction where - C: ReservableCurrency, -{ +impl BalanceSwapAction where C: ReservableCurrency { /// Create a new swap action value of balance. - pub fn new(value: ::AccountId>>::Balance) -> Self { + pub fn new(value: >::Balance) -> Self { Self { value, _marker: PhantomData } } } -impl Deref for BalanceSwapAction where - C: ReservableCurrency, -{ - type Target = ::AccountId>>::Balance; +impl Deref for BalanceSwapAction where C: ReservableCurrency { + type Target = >::Balance; fn deref(&self) -> &Self::Target { &self.value } } -impl DerefMut for BalanceSwapAction where - C: ReservableCurrency, -{ +impl DerefMut for BalanceSwapAction where C: ReservableCurrency { fn deref_mut(&mut self) -> &mut Self::Target { &mut self.value } } -impl SwapAction for BalanceSwapAction where - C: ReservableCurrency, +impl SwapAction for BalanceSwapAction + where C: ReservableCurrency { - fn reserve(&self, source: &T::AccountId) -> DispatchResult { + fn reserve(&self, source: &AccountId) -> DispatchResult { C::reserve(&source, self.value) } - fn claim(&self, source: &T::AccountId, target: &T::AccountId) -> bool { + fn claim(&self, source: &AccountId, target: &AccountId) -> bool { C::repatriate_reserved(source, target, self.value, BalanceStatus::Free).is_ok() } @@ -136,7 +130,7 @@ impl SwapAction for BalanceSwapAction where T::DbWeight::get().reads_writes(1, 1) } - fn cancel(&self, source: &T::AccountId) { + fn cancel(&self, source: &AccountId) { C::unreserve(source, self.value); } } @@ -146,7 +140,7 @@ pub trait Trait: frame_system::Trait { /// The overarching event type. type Event: From> + Into<::Event>; /// Swap action. - type SwapAction: SwapAction + Parameter; + type SwapAction: SwapAction + Parameter; /// Limit of proof size. /// /// Atomic swap is only atomic if once the proof is revealed, both parties can submit the proofs diff --git a/frame/atomic-swap/src/tests.rs b/frame/atomic-swap/src/tests.rs index 82cd30d5d32dd..6690a24d364d6 100644 --- a/frame/atomic-swap/src/tests.rs +++ b/frame/atomic-swap/src/tests.rs @@ -68,7 +68,7 @@ parameter_types! { } impl Trait for Test { type Event = (); - type SwapAction = BalanceSwapAction; + type SwapAction = BalanceSwapAction; type ProofLimit = ProofLimit; } type System = frame_system::Module; From a0724bacb44d76f3b397a2e6ede8fb25a28a7540 Mon Sep 17 00:00:00 2001 From: Xiliang Chen Date: Wed, 22 Jul 2020 03:47:08 +1200 Subject: [PATCH 7/7] make impl_outer_origin default to use frame_system (#6695) --- frame/im-online/src/mock.rs | 2 +- frame/support/src/metadata.rs | 2 +- frame/support/src/origin.rs | 14 +++++++------- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/frame/im-online/src/mock.rs b/frame/im-online/src/mock.rs index 968aad1f95bab..29fe6acb3337c 100644 --- a/frame/im-online/src/mock.rs +++ b/frame/im-online/src/mock.rs @@ -28,7 +28,7 @@ use sp_runtime::testing::{Header, UintAuthorityId, TestXt}; use sp_runtime::traits::{IdentityLookup, BlakeTwo256, ConvertInto}; use sp_core::H256; use frame_support::{impl_outer_origin, impl_outer_dispatch, parameter_types, weights::Weight}; -use frame_system as system; + impl_outer_origin!{ pub enum Origin for Runtime {} } diff --git a/frame/support/src/metadata.rs b/frame/support/src/metadata.rs index dca365ff8c9d1..aa7d71b52e0b6 100644 --- a/frame/support/src/metadata.rs +++ b/frame/support/src/metadata.rs @@ -413,7 +413,7 @@ mod tests { } impl_outer_origin! { - pub enum Origin for TestRuntime {} + pub enum Origin for TestRuntime where system = system {} } impl_outer_dispatch! { diff --git a/frame/support/src/origin.rs b/frame/support/src/origin.rs index ba9af6c98243a..df75f8dc65645 100644 --- a/frame/support/src/origin.rs +++ b/frame/support/src/origin.rs @@ -33,7 +33,7 @@ macro_rules! impl_outer_origin { ) => { $crate::impl_outer_origin! { $(#[$attr])* - pub enum $name for $runtime where system = system { + pub enum $name for $runtime where system = frame_system { $( $rest_without_system )* } } @@ -350,7 +350,7 @@ macro_rules! impl_outer_origin { mod tests { use codec::{Encode, Decode}; use crate::traits::{Filter, OriginTrait}; - mod system { + mod frame_system { use super::*; pub trait Trait { @@ -404,7 +404,7 @@ mod tests { } } - impl system::Trait for TestRuntime { + impl frame_system::Trait for TestRuntime { type AccountId = u32; type Call = u32; type BaseCallFilter = BaseCallFilter; @@ -425,21 +425,21 @@ mod tests { ); impl_outer_origin!( - pub enum OriginWithSystem for TestRuntime where system = system { + pub enum OriginWithSystem for TestRuntime where system = frame_system { origin_without_generic, origin_with_generic } ); impl_outer_origin!( - pub enum OriginWithSystem2 for TestRuntime where system = system { + pub enum OriginWithSystem2 for TestRuntime where system = frame_system { origin_with_generic, origin_without_generic, } ); impl_outer_origin!( - pub enum OriginEmpty for TestRuntime where system = system {} + pub enum OriginEmpty for TestRuntime where system = frame_system {} ); #[test] @@ -464,7 +464,7 @@ mod tests { assert_eq!(origin.filter_call(&1), false); origin.set_caller_from(OriginWithSystem::root()); - assert!(matches!(origin.caller, OriginWithSystemCaller::system(system::RawOrigin::Root))); + assert!(matches!(origin.caller, OriginWithSystemCaller::system(frame_system::RawOrigin::Root))); assert_eq!(origin.filter_call(&0), false); assert_eq!(origin.filter_call(&1), false);