From 09b67ae0e46888d7bdd43c05ce41cb1dc8bbeb8b Mon Sep 17 00:00:00 2001 From: Antonio Date: Fri, 25 Oct 2024 10:20:25 +0300 Subject: [PATCH] chore: more lints (#762) Add more lints to our codebase. I have taken all lints that are allowed by default, and made either either warnings or strong denies. **I have not added any lints from the [pedantic](https://rust-lang.github.io/rust-clippy/master/index.html?groups=pedantic) group, as those tend to be opinionated and I don't think should be enforced on a shared codebase.** Unfortunately, there is an [open cargo issue](https://github.com/rust-lang/cargo/issues/8170) which makes it impossible to enable/disable lints based on the `#[cfg(test)]` or `#[cfg(feature = "...")]` features, hence I have had to enable lints for either the whole codebase (`[target.'cfg(all())']`), or for runtime-only code `[target.'cfg(target_arch = "wasm32")']`. The changeset is huge but just because I tackled all the new lints generated, so reviewing the new lints (in case some are unwanted or in case I missed some) would be sufficient, since the CI has been updated to verify all lints are properly addressed. Last but not least, I had to enable the `-Dwarnings` flag for ALL clippy invocations, so also on local machines, because it's not possible to pass custom `--cfg` either to rustc via cargo, e.g., `cargo clippy -- --cfg ci` and then have a `[target.'cfg(ci)']` entry in the config file. This might go away in future versions of cargo (we are currently on 1.74), but until then we either never fail on the CI, or we fail for everything also locally, and I think the latter is a better choice. This is also because rustc takes additional flags either from the `RUSTFLAGS` env variable or the `.cargo/config.toml` file. Hence specifying `RUSTFLAGS="-Dwarnings"` in the CI would not enable any additional lints as the env variable would take precedence over the config file, rendering them useless. --- .cargo/config.toml | 76 +++++++++- .github/workflows/check-code.yml | 3 +- crates/assets/src/asset.rs | 116 ++++++++------- crates/assets/src/chain.rs | 70 +++++----- crates/assets/src/v1.rs | 18 +-- crates/kilt-dip-primitives/Cargo.toml | 3 + .../src/merkle_proofs/v0/input_common.rs | 2 +- .../merkle_proofs/v0/provider_state/mod.rs | 27 ++-- .../src/state_proofs/mod.rs | 48 ++++--- .../nodes/dip-consumer/src/chain_spec.rs | 6 +- .../nodes/dip-consumer/src/command.rs | 14 +- dip-template/nodes/dip-consumer/src/rpc.rs | 7 +- .../nodes/dip-consumer/src/service.rs | 64 +++++---- .../nodes/dip-provider/src/chain_spec.rs | 6 +- .../nodes/dip-provider/src/command.rs | 14 +- dip-template/nodes/dip-provider/src/rpc.rs | 7 +- .../nodes/dip-provider/src/service.rs | 64 +++++---- dip-template/pallets/pallet-postit/src/lib.rs | 27 ++-- dip-template/runtimes/dip-consumer/src/dip.rs | 2 +- dip-template/runtimes/dip-consumer/src/lib.rs | 2 + dip-template/runtimes/dip-provider/src/lib.rs | 2 + integration-tests/emulated/Cargo.toml | 7 +- .../emulated/src/mock/para_chains.rs | 11 +- .../peregrine/did_pallets/attestation.rs | 2 +- .../src/tests/peregrine/did_pallets/w3n.rs | 7 +- .../spiritnet/did_pallets/attestation.rs | 2 +- .../src/tests/spiritnet/did_pallets/w3n.rs | 2 +- nodes/parachain/src/chain_spec/mod.rs | 4 + nodes/parachain/src/cli.rs | 2 +- nodes/parachain/src/command.rs | 11 +- nodes/parachain/src/rpc.rs | 8 +- nodes/parachain/src/service.rs | 57 ++++---- nodes/standalone/src/chain_spec.rs | 4 +- nodes/standalone/src/main.rs | 2 + nodes/standalone/src/rpc.rs | 9 +- nodes/standalone/src/service.rs | 6 +- pallets/attestation/src/lib.rs | 7 + pallets/attestation/src/migrations.rs | 4 +- pallets/ctype/src/lib.rs | 11 +- .../delegation/src/delegation_hierarchy.rs | 4 +- pallets/delegation/src/lib.rs | 15 +- pallets/delegation/src/migrations.rs | 4 +- pallets/delegation/src/mock.rs | 2 +- pallets/did/src/did_details.rs | 22 +-- pallets/did/src/lib.rs | 19 ++- pallets/did/src/migrations.rs | 4 +- pallets/did/src/mock_utils.rs | 2 +- pallets/did/src/origin.rs | 2 +- .../pallet-asset-switch/src/benchmarking.rs | 4 +- pallets/pallet-asset-switch/src/lib.rs | 13 +- pallets/pallet-asset-switch/src/switch.rs | 4 +- .../src/tests/set_switch_pair.rs | 2 +- .../pallet-asset-switch/src/tests/switch.rs | 2 +- .../src/xcm/query/tests/expecting_response.rs | 10 +- .../src/xcm/query/tests/on_response.rs | 2 +- .../xcm/trade/switch_pair_remote_asset/mod.rs | 8 +- .../src/xcm/trade/xcm_fee_asset/mod.rs | 2 +- pallets/pallet-configuration/Cargo.toml | 1 + .../pallet-configuration/src/benchmarking.rs | 2 +- pallets/pallet-configuration/src/lib.rs | 7 + pallets/pallet-configuration/src/mock.rs | 2 +- pallets/pallet-configuration/src/tests.rs | 6 +- pallets/pallet-deposit-storage/Cargo.toml | 2 +- pallets/pallet-deposit-storage/src/lib.rs | 7 + pallets/pallet-did-lookup/src/account.rs | 22 ++- .../src/associate_account_request.rs | 4 +- pallets/pallet-did-lookup/src/lib.rs | 11 +- pallets/pallet-did-lookup/src/migrations.rs | 4 +- .../pallet-did-lookup/src/tests/associate.rs | 35 +++-- pallets/pallet-dip-consumer/src/lib.rs | 7 + pallets/pallet-dip-consumer/src/mock.rs | 4 +- pallets/pallet-dip-provider/src/lib.rs | 11 +- pallets/pallet-inflation/src/lib.rs | 7 + pallets/pallet-inflation/src/mock.rs | 2 +- pallets/pallet-migration/src/lib.rs | 21 ++- pallets/pallet-relay-store/src/lib.rs | 10 ++ pallets/pallet-web3-names/src/lib.rs | 18 ++- pallets/pallet-web3-names/src/migrations.rs | 4 +- pallets/pallet-web3-names/src/web3_name.rs | 28 ++-- pallets/parachain-staking/src/benchmarking.rs | 25 ++-- pallets/parachain-staking/src/lib.rs | 132 ++++++++++-------- pallets/parachain-staking/src/mock.rs | 2 +- pallets/parachain-staking/src/set.rs | 9 +- .../parachain-staking/src/tests/rewards.rs | 2 +- pallets/parachain-staking/src/tests/round.rs | 4 +- pallets/parachain-staking/src/types.rs | 24 +--- pallets/public-credentials/src/lib.rs | 9 +- pallets/public-credentials/src/migrations.rs | 7 +- runtimes/common/src/authorization.rs | 20 +-- runtimes/common/src/constants.rs | 5 +- runtimes/common/src/dip/deposit/mod.rs | 3 + runtimes/common/src/dip/did/mod.rs | 4 +- runtimes/common/src/dip/merkle/mod.rs | 2 +- .../src/dip/merkle/tests/generate_proof.rs | 3 +- runtimes/common/src/dip/merkle/v0/mod.rs | 46 +++--- .../merkle/v0/tests/generate_commitment.rs | 13 +- .../src/dip/merkle/v0/tests/generate_proof.rs | 57 +++----- runtimes/common/src/dip/mock.rs | 10 +- runtimes/common/src/fees.rs | 12 +- runtimes/common/src/lib.rs | 7 +- runtimes/kestrel/src/lib.rs | 12 +- runtimes/peregrine/src/lib.rs | 14 +- runtimes/peregrine/src/xcm_config.rs | 11 +- runtimes/spiritnet/src/lib.rs | 14 +- runtimes/spiritnet/src/xcm_config.rs | 11 +- support/src/migration.rs | 2 +- support/src/traits.rs | 20 ++- 107 files changed, 889 insertions(+), 661 deletions(-) diff --git a/.cargo/config.toml b/.cargo/config.toml index b93dd3548..be3fa2d13 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -1,5 +1,61 @@ -# Deployment runtime lints -[target.'cfg(all(target_arch = "wasm32", not(test)))'] +# Unfortunately we cannot add any lints based on `cfg(test)` nor `cfg(feature = "feat")`: https://github.com/rust-lang/cargo/issues/8170. +# Hence, we can only define lints that are applied codebase-wide (including tests), and lints that are applied only to runtime code. + +# Codebase-wide lints. +[target.'cfg(all())'] +rustflags = [ + # We need to deny warnings here instead of with the `RUSTFLAGS` env since the env variable would completely override these settings -> https://github.com/rust-lang/cargo/issues/5376. + "-Dwarnings", + "-Wclippy::as_underscore", + "-Wclippy::assertions_on_result_states", + "-Wclippy::branches_sharing_code", + "-Wclippy::clear_with_drain", + "-Wclippy::clone_on_ref_ptr", + "-Wclippy::collection_is_never_read", + "-Wclippy::derive_partial_eq_without_eq", + "-Wclippy::else_if_without_else", + "-Wclippy::empty_drop", + "-Wclippy::empty_structs_with_brackets", + "-Wclippy::equatable_if_let", + "-Wclippy::if_then_some_else_none", + "-Wclippy::impl_trait_in_params", + "-Wclippy::iter_on_empty_collections", + "-Wclippy::iter_on_single_items", + "-Wclippy::iter_with_drain", + "-Wclippy::needless_collect", + "-Wclippy::needless_pass_by_ref_mut", + "-Wclippy::negative_feature_names", + "-Wclippy::option_if_let_else", + "-Wclippy::or_fun_call", + "-Wclippy::pub_without_shorthand", + "-Wclippy::redundant_clone", + "-Wclippy::redundant_type_annotations", + "-Wclippy::ref_patterns", + "-Wclippy::rest_pat_in_fully_bound_structs", + "-Wclippy::suspicious_operation_groupings", + "-Wclippy::type_repetition_in_bounds", + "-Wclippy::unnecessary_self_imports", + "-Wclippy::unnecessary_struct_initialization", + "-Wclippy::unneeded_field_pattern", + "-Wclippy::unused_peekable", + "-Wclippy::useless_let_if_seq", + "-Wclippy::wildcard_dependencies", + # TODO: Add after upgrading to 1.76 + # "-Wclippy::infinite_loop", + # TODO: Add after upgrading to 1.77 + #"-Wclippy::empty_enum_variants_with_brackets" + # TODO: Add after upgrading to 1.80 + #"-Wclippy::renamed_function_params" + # TODO: Add after upgrading to 1.81 + #"-Wclippy::cfg_not_test" + #"-Wclippy::allow_attributes", + # "-Wclippy::allow_attributes_without_reason", + # TODO: Add after upgrading to 1.83 + #"-Wclippy::unused_trait_names" +] + +# Deployment runtime lints. +[target.'cfg(target_arch = "wasm32")'] rustflags = [ "-Dclippy::arithmetic_side_effects", "-Dclippy::as_conversions", @@ -12,6 +68,7 @@ rustflags = [ "-Dclippy::index_refutable_slice", "-Dclippy::indexing_slicing", "-Dclippy::lossy_float_literal", + "-Dclippy::modulo_arithmetic", "-Dclippy::panic", "-Dclippy::string_slice", "-Dclippy::todo", @@ -19,8 +76,21 @@ rustflags = [ "-Dclippy::unreachable", "-Dclippy::unwrap_used", "-Funsafe_code", + "-Wclippy::alloc_instead_of_core", + "-Wclippy::decimal_literal_representation", + "-Wclippy::default_numeric_fallback", + "-Wclippy::error_impl_error", "-Wclippy::integer_division", - "-Wclippy::modulo_arithmetic", + "-Wclippy::let_underscore_must_use", + "-Wclippy::let_underscore_untyped", + "-Wclippy::missing_const_for_fn", + "-Wclippy::mixed_read_write_in_expression", "-Wclippy::print_stderr", "-Wclippy::print_stdout", + "-Wclippy::shadow_reuse", + "-Wclippy::shadow_same", + "-Wclippy::shadow_unrelated", + "-Wclippy::str_to_string", + "-Wclippy::string_slice", + "-Wclippy::string_to_string", ] diff --git a/.github/workflows/check-code.yml b/.github/workflows/check-code.yml index 526e8294f..3250b9ff1 100644 --- a/.github/workflows/check-code.yml +++ b/.github/workflows/check-code.yml @@ -39,7 +39,6 @@ jobs: env: # Configured by the Docker image. We can't change this unless the image does it. CARGO_HOME: /usr/local/cargo - RUSTFLAGS: -D warnings SKIP_WASM_BUILD: 1 needs: get-commit-head if: ${{ !contains(needs.get-commit-head.outputs.headCommitMsg, 'ci-skip-rust') }} @@ -71,7 +70,7 @@ jobs: save-always: true - name: Run `cargo clippy` - run: cargo clippy --locked ${{ matrix.cargo-flags }} + run: cargo clippy --locked --no-deps ${{ matrix.cargo-flags }} cargo-fmt: name: Check formatting diff --git a/crates/assets/src/asset.rs b/crates/assets/src/asset.rs index 2caf3c390..5ee17721b 100644 --- a/crates/assets/src/asset.rs +++ b/crates/assets/src/asset.rs @@ -109,8 +109,8 @@ pub mod v1 { where I: AsRef<[u8]> + Into>, { - let input = input.as_ref(); - let input_length = input.len(); + let input_ref = input.as_ref(); + let input_length = input_ref.len(); if !(MINIMUM_ASSET_ID_LENGTH..=MAXIMUM_ASSET_ID_LENGTH).contains(&input_length) { log::trace!( "Length of provided input {} is not included in the inclusive range [{},{}]", @@ -122,12 +122,12 @@ pub mod v1 { } let AssetComponents { - namespace, - reference, - identifier, - } = split_components(input); + namespace: encoded_namespace, + reference: encoded_reference, + identifier: encoded_identifier, + } = split_components(input_ref); - match (namespace, reference, identifier) { + match (encoded_namespace, encoded_reference, encoded_identifier) { // "slip44:" assets -> https://github.com/ChainAgnostic/CAIPs/blob/master/CAIPs/caip-20.md (Some(SLIP44_NAMESPACE), _, Some(_)) => { log::trace!("Slip44 namespace does not accept an asset identifier."); @@ -145,9 +145,9 @@ pub mod v1 { EvmSmartContractFungibleReference::from_utf8_encoded(erc20_reference).map(Self::Erc20) } // "erc721:" assets -> https://github.com/ChainAgnostic/CAIPs/blob/master/CAIPs/caip-22.md - (Some(ERC721_NAMESPACE), Some(erc721_reference), identifier) => { + (Some(ERC721_NAMESPACE), Some(erc721_reference), erc721_identifier) => { let reference = EvmSmartContractFungibleReference::from_utf8_encoded(erc721_reference)?; - let identifier = identifier.map_or(Ok(None), |id| { + let identifier = erc721_identifier.map_or(Ok(None), |id| { EvmSmartContractNonFungibleIdentifier::from_utf8_encoded(id).map(Some) })?; Ok(Self::Erc721(EvmSmartContractNonFungibleReference( @@ -155,9 +155,9 @@ pub mod v1 { ))) } // "erc1155:" assets-> https://github.com/ChainAgnostic/CAIPs/blob/master/CAIPs/caip-29.md - (Some(ERC1155_NAMESPACE), Some(erc1155_reference), identifier) => { + (Some(ERC1155_NAMESPACE), Some(erc1155_reference), erc1155_identifier) => { let reference = EvmSmartContractFungibleReference::from_utf8_encoded(erc1155_reference)?; - let identifier = identifier.map_or(Ok(None), |id| { + let identifier = erc1155_identifier.map_or(Ok(None), |id| { EvmSmartContractNonFungibleIdentifier::from_utf8_encoded(id).map(Some) })?; Ok(Self::Erc1155(EvmSmartContractNonFungibleReference( @@ -165,7 +165,7 @@ pub mod v1 { ))) } // Generic yet valid asset IDs - _ => GenericAssetId::from_utf8_encoded(input).map(Self::Generic), + _ => GenericAssetId::from_utf8_encoded(input_ref).map(Self::Generic), } } } @@ -276,23 +276,20 @@ pub mod v1 { /// Split the given input into its components, i.e., namespace, reference, /// and identifier, if the proper separators are found. fn split_components(input: &[u8]) -> AssetComponents { - let mut split = input.splitn(2, |c| *c == ASSET_NAMESPACE_REFERENCE_SEPARATOR); - let (namespace, reference) = (split.next(), split.next()); + let mut namespace_reference_split = input.splitn(2, |c| *c == ASSET_NAMESPACE_REFERENCE_SEPARATOR); + let (parsed_namespace, reference) = (namespace_reference_split.next(), namespace_reference_split.next()); // Split the remaining reference to extract the identifier, if present - let (reference, identifier) = if let Some(r) = reference { - let mut split = r.splitn(2, |c| *c == ASSET_REFERENCE_IDENTIFIER_SEPARATOR); + let (parsed_reference, parsed_identifier) = reference.map_or((reference, None), |r| { + let mut reference_identifier_split = r.splitn(2, |c| *c == ASSET_REFERENCE_IDENTIFIER_SEPARATOR); // Split the reference further, if present - (split.next(), split.next()) - } else { - // Return the old reference, which is None if we are at this point - (reference, None) - }; + (reference_identifier_split.next(), reference_identifier_split.next()) + }); AssetComponents { - namespace, - reference, - identifier, + namespace: parsed_namespace, + reference: parsed_reference, + identifier: parsed_identifier, } } @@ -315,10 +312,10 @@ pub mod v1 { where I: AsRef<[u8]> + Into>, { - let input = input.as_ref(); - check_reference_length_bounds(input)?; + let input_ref = input.as_ref(); + check_reference_length_bounds(input_ref)?; - let decoded = str::from_utf8(input).map_err(|_| { + let decoded = str::from_utf8(input_ref).map_err(|_| { log::trace!("Provided input is not a valid UTF8 string as expected by a Slip44 reference."); ReferenceError::InvalidFormat })?; @@ -359,7 +356,7 @@ pub mod v1 { // Getters impl Slip44Reference { - pub fn inner(&self) -> &U256 { + pub const fn inner(&self) -> &U256 { &self.0 } } @@ -383,9 +380,9 @@ pub mod v1 { where I: AsRef<[u8]> + Into>, { - let input = input.as_ref(); + let input_ref = input.as_ref(); // If the prefix is "0x" => parse the address - if let [b'0', b'x', contract_address @ ..] = input { + if let [b'0', b'x', contract_address @ ..] = input_ref { check_reference_length_bounds(contract_address)?; let decoded = hex::decode(contract_address).map_err(|_| { @@ -407,7 +404,7 @@ pub mod v1 { // Getters impl EvmSmartContractFungibleReference { - pub fn inner(&self) -> &[u8] { + pub const fn inner(&self) -> &[u8] { &self.0 } } @@ -431,11 +428,11 @@ pub mod v1 { // Getters impl EvmSmartContractNonFungibleReference { - pub fn smart_contract(&self) -> &EvmSmartContractFungibleReference { + pub const fn smart_contract(&self) -> &EvmSmartContractFungibleReference { &self.0 } - pub fn identifier(&self) -> &Option { + pub const fn identifier(&self) -> &Option { &self.1 } } @@ -457,10 +454,10 @@ pub mod v1 { where I: AsRef<[u8]> + Into>, { - let input = input.as_ref(); - check_identifier_length_bounds(input)?; + let input_ref = input.as_ref(); + check_identifier_length_bounds(input_ref)?; - input.iter().try_for_each(|c| { + input_ref.iter().try_for_each(|c| { if !c.is_ascii_digit() { log::trace!("Provided input has some invalid values as expected by a smart contract-based asset identifier."); Err(IdentifierError::InvalidFormat) @@ -470,7 +467,7 @@ pub mod v1 { })?; Ok(Self( - Vec::::from(input) + Vec::::from(input_ref) .try_into() .map_err(|_| IdentifierError::InvalidFormat)?, )) @@ -519,12 +516,13 @@ pub mod v1 { } = split_components(input.as_ref()); match (namespace, reference, identifier) { - (Some(namespace), Some(reference), identifier) => Ok(Self { - namespace: GenericAssetNamespace::from_utf8_encoded(namespace)?, - reference: GenericAssetReference::from_utf8_encoded(reference)?, + (Some(encoded_namespace), Some(encoded_reference), encoded_identifier) => Ok(Self { + namespace: GenericAssetNamespace::from_utf8_encoded(encoded_namespace)?, + reference: GenericAssetReference::from_utf8_encoded(encoded_reference)?, // Transform Option to Result