From b99033368b4954c12e62c1917d8f9cd77576efe8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Wed, 13 May 2020 18:40:52 +0200 Subject: [PATCH] Fix minor clippy lints in grandpa (#5988) * grandpa: fix clippy lints about identity conversions * grandpa: fix clippy lints about unwrap_or_default * grandpa: fix clippy lints about explicit return * grandpa: fix clippy lints about unnecessary intermediary * grandpa: fix clippy lints about to_string * grandpa: fix clippy lints about unused imports * grandpa: fix clippy lints about increments * grandpa: fix clippy lints about unnecessary matches * grandpa: fix clippy lints about struct arguments * Fix clippy::redundant_clone * Fix clippy::clone_on_copy * Fix clippy::or_fun_call * Fix clippy::identity_conversion --- .../client/finality-grandpa/rpc/src/error.rs | 2 +- .../client/finality-grandpa/src/authorities.rs | 4 ++-- .../client/finality-grandpa/src/aux_schema.rs | 4 ++-- .../src/communication/gossip.rs | 4 ++-- .../finality-grandpa/src/communication/mod.rs | 12 +++++------- .../src/communication/periodic.rs | 4 ++-- .../client/finality-grandpa/src/environment.rs | 10 +++++----- .../finality-grandpa/src/finality_proof.rs | 6 +++--- substrate/client/finality-grandpa/src/import.rs | 16 ++++++++-------- .../finality-grandpa/src/justification.rs | 14 +++++++------- substrate/client/finality-grandpa/src/lib.rs | 17 +++++++---------- .../client/finality-grandpa/src/light_import.rs | 8 ++++---- .../client/finality-grandpa/src/observer.rs | 4 ++-- .../finality-grandpa/src/until_imported.rs | 6 +++--- 14 files changed, 53 insertions(+), 58 deletions(-) diff --git a/substrate/client/finality-grandpa/rpc/src/error.rs b/substrate/client/finality-grandpa/rpc/src/error.rs index 2a5e6955e5..c4a3877f51 100644 --- a/substrate/client/finality-grandpa/rpc/src/error.rs +++ b/substrate/client/finality-grandpa/rpc/src/error.rs @@ -33,7 +33,7 @@ pub enum Error { impl From for jsonrpc_core::Error { fn from(error: Error) -> Self { jsonrpc_core::Error { - message: format!("{}", error).into(), + message: format!("{}", error), code: jsonrpc_core::ErrorCode::ServerError(NOT_READY_ERROR_CODE), data: None, } diff --git a/substrate/client/finality-grandpa/src/authorities.rs b/substrate/client/finality-grandpa/src/authorities.rs index 80c1f4ad3f..12cb1456d3 100644 --- a/substrate/client/finality-grandpa/src/authorities.rs +++ b/substrate/client/finality-grandpa/src/authorities.rs @@ -229,8 +229,8 @@ where (&number, &hash), pending.delay); self.pending_standard_changes.import( - hash.clone(), - number.clone(), + hash, + number, pending, is_descendent_of, )?; diff --git a/substrate/client/finality-grandpa/src/aux_schema.rs b/substrate/client/finality-grandpa/src/aux_schema.rs index e4e8a98042..4ed96d058a 100644 --- a/substrate/client/finality-grandpa/src/aux_schema.rs +++ b/substrate/client/finality-grandpa/src/aux_schema.rs @@ -328,7 +328,7 @@ pub(crate) fn load_persistent( } Some(other) => return Err(ClientError::Backend( format!("Unsupported GRANDPA DB version: {:?}", other) - ).into()), + )), } // genesis. @@ -336,7 +336,7 @@ pub(crate) fn load_persistent( from genesis on what appears to be first startup."); let genesis_authorities = genesis_authorities()?; - let genesis_set = AuthoritySet::genesis(genesis_authorities.clone()) + let genesis_set = AuthoritySet::genesis(genesis_authorities) .expect("genesis authorities is non-empty; all weights are non-zero; qed."); let state = make_genesis_round(); let base = state.prevote_ghost diff --git a/substrate/client/finality-grandpa/src/communication/gossip.rs b/substrate/client/finality-grandpa/src/communication/gossip.rs index 183ffc65e8..7fe17e974b 100644 --- a/substrate/client/finality-grandpa/src/communication/gossip.rs +++ b/substrate/client/finality-grandpa/src/communication/gossip.rs @@ -887,7 +887,7 @@ impl Inner { // any catch up requests until we import this one (either with a // success or failure). self.pending_catch_up = PendingCatchUp::Processing { - instant: instant.clone(), + instant: *instant, }; // always discard catch up messages, they're point-to-point @@ -1281,7 +1281,7 @@ impl GossipValidator { inner: parking_lot::RwLock::new(Inner::new(config)), set_state, report_sender: tx, - metrics: metrics, + metrics, }; (val, rx) diff --git a/substrate/client/finality-grandpa/src/communication/mod.rs b/substrate/client/finality-grandpa/src/communication/mod.rs index 16af54986a..d50968a468 100644 --- a/substrate/client/finality-grandpa/src/communication/mod.rs +++ b/substrate/client/finality-grandpa/src/communication/mod.rs @@ -236,16 +236,14 @@ impl> NetworkBridge { let (neighbor_packet_worker, neighbor_packet_sender) = periodic::NeighborPacketWorker::new(); - let bridge = NetworkBridge { + NetworkBridge { service, gossip_engine, validator, neighbor_sender: neighbor_packet_sender, neighbor_packet_worker: Arc::new(Mutex::new(neighbor_packet_worker)), gossip_validator_report_stream: Arc::new(Mutex::new(report_stream)), - }; - - bridge + } } /// Note the beginning of a new round to the `GossipValidator`. @@ -304,7 +302,7 @@ impl> NetworkBridge { match decoded { Err(ref e) => { debug!(target: "afg", "Skipping malformed message {:?}: {}", notification, e); - return future::ready(None); + future::ready(None) } Ok(GossipMessage::Vote(msg)) => { // check signature. @@ -343,7 +341,7 @@ impl> NetworkBridge { } _ => { debug!(target: "afg", "Skipping unknown message type"); - return future::ready(None); + future::ready(None) } } }); @@ -666,7 +664,7 @@ impl Sink> for OutgoingMessages // when locals exist, sign messages on import if let Some((ref pair, _)) = self.locals { - let target_hash = msg.target().0.clone(); + let target_hash = *(msg.target().0); let signed = sp_finality_grandpa::sign_message( msg, pair, diff --git a/substrate/client/finality-grandpa/src/communication/periodic.rs b/substrate/client/finality-grandpa/src/communication/periodic.rs index f894624bdf..dadd7deb57 100644 --- a/substrate/client/finality-grandpa/src/communication/periodic.rs +++ b/substrate/client/finality-grandpa/src/communication/periodic.rs @@ -86,7 +86,7 @@ impl Stream for NeighborPacketWorker { this.delay.reset(REBROADCAST_AFTER); this.last = Some((to.clone(), packet.clone())); - return Poll::Ready(Some((to, GossipMessage::::from(packet.clone())))); + return Poll::Ready(Some((to, GossipMessage::::from(packet)))); } // Don't return yet, maybe the timer fired. Poll::Pending => {}, @@ -108,6 +108,6 @@ impl Stream for NeighborPacketWorker { return Poll::Ready(Some((to.clone(), GossipMessage::::from(packet.clone())))); } - return Poll::Pending; + Poll::Pending } } diff --git a/substrate/client/finality-grandpa/src/environment.rs b/substrate/client/finality-grandpa/src/environment.rs index 1db1bcbb8d..34a2358159 100644 --- a/substrate/client/finality-grandpa/src/environment.rs +++ b/substrate/client/finality-grandpa/src/environment.rs @@ -108,7 +108,7 @@ impl Decode for CompletedRounds { fn decode(value: &mut I) -> Result { <(Vec>, SetId, Vec)>::decode(value) .map(|(rounds, set_id, voters)| CompletedRounds { - rounds: rounds.into(), + rounds, set_id, voters, }) @@ -248,14 +248,14 @@ impl VoterSetState { { if let VoterSetState::Live { completed_rounds, current_rounds } = self { if current_rounds.contains_key(&round) { - return Ok((completed_rounds, current_rounds)); + Ok((completed_rounds, current_rounds)) } else { let msg = "Voter acting on a live round we are not tracking."; - return Err(Error::Safety(msg.to_string())); + Err(Error::Safety(msg.to_string())) } } else { let msg = "Voter acting while in paused state."; - return Err(Error::Safety(msg.to_string())); + Err(Error::Safety(msg.to_string())) } } } @@ -622,7 +622,7 @@ where restricted_number >= base_header.number() && restricted_number < target_header.number() }) - .or(Some((target_header.hash(), *target_header.number()))) + .or_else(|| Some((target_header.hash(), *target_header.number()))) }, Ok(None) => { debug!(target: "afg", "Encountered error finding best chain containing {:?}: couldn't find target block", block); diff --git a/substrate/client/finality-grandpa/src/finality_proof.rs b/substrate/client/finality-grandpa/src/finality_proof.rs index bf3662aba3..867d2b27ab 100644 --- a/substrate/client/finality-grandpa/src/finality_proof.rs +++ b/substrate/client/finality-grandpa/src/finality_proof.rs @@ -183,7 +183,7 @@ impl sc_network::config::FinalityProofProvider for FinalityProo let request: FinalityProofRequest = Decode::decode(&mut &request[..]) .map_err(|e| { warn!(target: "afg", "Unable to decode finality proof request: {}", e.what()); - ClientError::Backend(format!("Invalid finality proof request")) + ClientError::Backend("Invalid finality proof request".to_string()) })?; match request { FinalityProofRequest::Original(request) => prove_finality::<_, _, GrandpaJustification>( @@ -397,7 +397,7 @@ pub(crate) fn prove_finality, J>( } // else search for the next justification - current_number = current_number + One::one(); + current_number += One::one(); } if finality_proof.is_empty() { @@ -513,7 +513,7 @@ fn check_finality_proof_fragment( new_authorities_proof, )?; - current_set_id = current_set_id + 1; + current_set_id += 1; } Ok(AuthoritiesOrEffects::Effects(FinalityEffects { diff --git a/substrate/client/finality-grandpa/src/import.rs b/substrate/client/finality-grandpa/src/import.rs index c1e32dfa6c..4960b23dc5 100644 --- a/substrate/client/finality-grandpa/src/import.rs +++ b/substrate/client/finality-grandpa/src/import.rs @@ -294,7 +294,7 @@ where } } - let number = block.header.number().clone(); + let number = *(block.header.number()); let maybe_change = self.check_new_change( &block.header, hash, @@ -326,7 +326,7 @@ where guard.as_mut().add_pending_change( change, &is_descendent_of, - ).map_err(|e| ConsensusError::from(ConsensusError::ClientImport(e.to_string())))?; + ).map_err(|e| ConsensusError::ClientImport(e.to_string()))?; } let applied_changes = { @@ -417,14 +417,14 @@ impl BlockImport new_cache: HashMap>, ) -> Result { let hash = block.post_hash(); - let number = block.header.number().clone(); + let number = *block.header.number(); // early exit if block already in chain, otherwise the check for // authority changes will error when trying to re-import a change block match self.inner.status(BlockId::Hash(hash)) { Ok(BlockStatus::InChain) => return Ok(ImportResult::AlreadyInChain), Ok(BlockStatus::Unknown) => {}, - Err(e) => return Err(ConsensusError::ClientImport(e.to_string()).into()), + Err(e) => return Err(ConsensusError::ClientImport(e.to_string())), } // on initial sync we will restrict logging under info to avoid spam. @@ -456,7 +456,7 @@ impl BlockImport e, ); pending_changes.revert(); - return Err(ConsensusError::ClientImport(e.to_string()).into()); + return Err(ConsensusError::ClientImport(e.to_string())); }, } }; @@ -466,7 +466,7 @@ impl BlockImport // Send the pause signal after import but BEFORE sending a `ChangeAuthorities` message. if do_pause { let _ = self.send_voter_commands.unbounded_send( - VoterCommand::Pause(format!("Forced change scheduled after inactivity")) + VoterCommand::Pause("Forced change scheduled after inactivity".to_string()) ); } @@ -633,7 +633,7 @@ where ); let justification = match justification { - Err(e) => return Err(ConsensusError::ClientImport(e.to_string()).into()), + Err(e) => return Err(ConsensusError::ClientImport(e.to_string())), Ok(justification) => justification, }; @@ -668,7 +668,7 @@ where Error::Client(error) => ConsensusError::ClientImport(error.to_string()), Error::Safety(error) => ConsensusError::ClientImport(error), Error::Timer(error) => ConsensusError::ClientImport(error.to_string()), - }.into()); + }); }, Ok(_) => { assert!(!enacts_change, "returns Ok when no authority set change should be enacted; qed;"); diff --git a/substrate/client/finality-grandpa/src/justification.rs b/substrate/client/finality-grandpa/src/justification.rs index cbaa2cb441..98bc859b5f 100644 --- a/substrate/client/finality-grandpa/src/justification.rs +++ b/substrate/client/finality-grandpa/src/justification.rs @@ -61,7 +61,7 @@ impl GrandpaJustification { }; for signed in commit.precommits.iter() { - let mut current_hash = signed.precommit.target_hash.clone(); + let mut current_hash = signed.precommit.target_hash; loop { if current_hash == commit.target_hash { break; } @@ -71,7 +71,7 @@ impl GrandpaJustification { return error(); } - let parent_hash = current_header.parent_hash().clone(); + let parent_hash = *current_header.parent_hash(); if votes_ancestries_hashes.insert(current_hash) { votes_ancestries.push(current_header); } @@ -131,16 +131,16 @@ impl GrandpaJustification { let mut buf = Vec::new(); let mut visited_hashes = HashSet::new(); for signed in self.commit.precommits.iter() { - if let Err(_) = sp_finality_grandpa::check_message_signature_with_buffer( + if sp_finality_grandpa::check_message_signature_with_buffer( &finality_grandpa::Message::Precommit(signed.precommit.clone()), &signed.id, &signed.signature, self.round, set_id, &mut buf, - ) { + ).is_err() { return Err(ClientError::BadJustification( - "invalid signature for precommit in grandpa justification".to_string()).into()); + "invalid signature for precommit in grandpa justification".to_string())); } if self.commit.target_hash == signed.precommit.target_hash { @@ -157,7 +157,7 @@ impl GrandpaJustification { }, _ => { return Err(ClientError::BadJustification( - "invalid precommit ancestry proof in grandpa justification".to_string()).into()); + "invalid precommit ancestry proof in grandpa justification".to_string())); }, } } @@ -169,7 +169,7 @@ impl GrandpaJustification { if visited_hashes != ancestry_hashes { return Err(ClientError::BadJustification( - "invalid precommit ancestries in grandpa justification with unused headers".to_string()).into()); + "invalid precommit ancestries in grandpa justification with unused headers".to_string())); } Ok(()) diff --git a/substrate/client/finality-grandpa/src/lib.rs b/substrate/client/finality-grandpa/src/lib.rs index ac677bf3f3..c499912043 100644 --- a/substrate/client/finality-grandpa/src/lib.rs +++ b/substrate/client/finality-grandpa/src/lib.rs @@ -74,11 +74,8 @@ use sp_consensus::{SelectChain, BlockImport}; use sp_core::Pair; use sp_utils::mpsc::{tracing_unbounded, TracingUnboundedReceiver}; use sc_telemetry::{telemetry, CONSENSUS_INFO, CONSENSUS_DEBUG}; -use serde_json; use parking_lot::RwLock; -use sp_finality_tracker; - use finality_grandpa::Error as GrandpaError; use finality_grandpa::{voter, BlockNumberOps, voter_set::VoterSet}; @@ -474,7 +471,7 @@ impl GenesisAuthoritySetProvider for Arc( Ok(info.finalized_number) } })) - .map_err(|err| sp_consensus::Error::InherentData(err.into())) + .map_err(|err| sp_consensus::Error::InherentData(err)) } else { Ok(()) } @@ -731,7 +728,7 @@ pub fn run_grandpa_voter( let curr = authorities.current_authorities(); let mut auths = curr.iter().map(|(p, _)| p); let maybe_authority_id = authority_id(&mut auths, &conf.keystore) - .unwrap_or(Default::default()); + .unwrap_or_default(); telemetry!(CONSENSUS_INFO; "afg.authority_set"; "authority_id" => maybe_authority_id.to_string(), @@ -841,7 +838,7 @@ where set_id: persistent_data.authority_set.set_id(), authority_set: persistent_data.authority_set.clone(), consensus_changes: persistent_data.consensus_changes.clone(), - voter_set_state: persistent_data.set_state.clone(), + voter_set_state: persistent_data.set_state, metrics: metrics.as_ref().map(|m| m.environment.clone()), _phantom: PhantomData, }); @@ -868,7 +865,7 @@ where let authority_id = is_voter(&self.env.voters, &self.env.config.keystore) .map(|ap| ap.public()) - .unwrap_or(Default::default()); + .unwrap_or_default(); telemetry!(CONSENSUS_DEBUG; "afg.starting_new_voter"; "name" => ?self.env.config.name(), @@ -914,12 +911,12 @@ where global_comms, last_completed_round.number, last_completed_round.votes.clone(), - last_completed_round.base.clone(), + last_completed_round.base, last_finalized, ); // Repoint shared_voter_state so that the RPC endpoint can query the state - if let None = self.shared_voter_state.reset(voter.voter_state()) { + if self.shared_voter_state.reset(voter.voter_state()).is_none() { info!(target: "afg", "Timed out trying to update shared GRANDPA voter state. \ RPC endpoints may return stale data." diff --git a/substrate/client/finality-grandpa/src/light_import.rs b/substrate/client/finality-grandpa/src/light_import.rs index e9ca94ce98..b63c6f0bd7 100644 --- a/substrate/client/finality-grandpa/src/light_import.rs +++ b/substrate/client/finality-grandpa/src/light_import.rs @@ -169,7 +169,7 @@ impl FinalityProofImport if *pending_number > chain_info.finalized_number && *pending_number <= chain_info.best_number { - out.push((pending_hash.clone(), *pending_number)); + out.push((*pending_hash, *pending_number)); } } @@ -253,7 +253,7 @@ fn do_import_block( J: ProvableJustification, { let hash = block.post_hash(); - let number = block.header.number().clone(); + let number = *block.header.number(); // we don't want to finalize on `inner.import_block` let justification = block.justification.take(); @@ -263,7 +263,7 @@ fn do_import_block( let mut imported_aux = match import_result { Ok(ImportResult::Imported(aux)) => aux, Ok(r) => return Ok(r), - Err(e) => return Err(ConsensusError::ClientImport(e.to_string()).into()), + Err(e) => return Err(ConsensusError::ClientImport(e.to_string())), }; match justification { @@ -435,7 +435,7 @@ fn do_import_justification( hash, ); - return Err(ConsensusError::ClientImport(e.to_string()).into()); + return Err(ConsensusError::ClientImport(e.to_string())); }, Ok(justification) => { trace!( diff --git a/substrate/client/finality-grandpa/src/observer.rs b/substrate/client/finality-grandpa/src/observer.rs index ab06f06280..e00bfec44c 100644 --- a/substrate/client/finality-grandpa/src/observer.rs +++ b/substrate/client/finality-grandpa/src/observer.rs @@ -111,7 +111,7 @@ fn grandpa_observer( Err(e) => return future::err(e.into()), }; - if let Some(_) = validation_result.ghost() { + if validation_result.ghost().is_some() { let finalized_hash = commit.target_hash; let finalized_number = commit.target_number; @@ -189,7 +189,7 @@ where client, network, persistent_data, - config.keystore.clone(), + config.keystore, voter_commands_rx ); diff --git a/substrate/client/finality-grandpa/src/until_imported.rs b/substrate/client/finality-grandpa/src/until_imported.rs index 6f76ce3fa8..90f99519a7 100644 --- a/substrate/client/finality-grandpa/src/until_imported.rs +++ b/substrate/client/finality-grandpa/src/until_imported.rs @@ -258,7 +258,7 @@ impl Stream for UntilImported { // new block imported. queue up all messages tied to that hash. if let Some((_, _, messages)) = this.pending.remove(¬ification.hash) { - let canon_number = notification.header.number().clone(); + let canon_number = *notification.header.number(); let ready_messages = messages.into_iter() .filter_map(|m| m.wait_completed(canon_number)); @@ -359,7 +359,7 @@ impl BlockUntilImported for SignedMessage { } } - return Ok(DiscardWaitOrReady::Wait(vec![(target_hash, target_number, msg)])) + Ok(DiscardWaitOrReady::Wait(vec![(target_hash, target_number, msg)])) } fn wait_completed(self, canon_number: NumberFor) -> Option { @@ -430,7 +430,7 @@ impl BlockUntilImported for BlockGlobalMessage { let mut query_known = |target_hash, perceived_number| -> Result { // check integrity: all votes for same hash have same number. let canon_number = match checked_hashes.entry(target_hash) { - Entry::Occupied(entry) => entry.get().number().clone(), + Entry::Occupied(entry) => *entry.get().number(), Entry::Vacant(entry) => { if let Some(number) = status_check.block_number(target_hash)? { entry.insert(KnownOrUnknown::Known(number));