Skip to content

Commit

Permalink
chore: more lints (#762)
Browse files Browse the repository at this point in the history 
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](rust-lang/cargo#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.
  • Loading branch information
@ntn-x2
ntn-x2 authored Oct 25, 2024
1 parent f057961 commit 09b67ae
Show file tree
Hide file tree
Showing 107 changed files with 889 additions and 661 deletions.
76 changes: 73 additions & 3 deletions .cargo/config.toml
View file
Original file line number Diff line number Diff line change
@@ -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",
Expand All @@ -12,15 +68,29 @@ rustflags = [
"-Dclippy::index_refutable_slice",
"-Dclippy::indexing_slicing",
"-Dclippy::lossy_float_literal",
"-Dclippy::modulo_arithmetic",
"-Dclippy::panic",
"-Dclippy::string_slice",
"-Dclippy::todo",
"-Dclippy::unimplemented",
"-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",
]
3 changes: 1 addition & 2 deletions .github/workflows/check-code.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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') }}
Expand Down Expand Up @@ -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
Expand Down
116 changes: 57 additions & 59 deletions crates/assets/src/asset.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,8 @@ pub mod v1 {
where
I: AsRef<[u8]> + Into<Vec<u8>>,
{
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 [{},{}]",
Expand All @@ -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.");
Expand All @@ -145,27 +145,27 @@ 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(
reference, identifier,
)))
}
// "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(
reference, identifier,
)))
}
// Generic yet valid asset IDs
_ => GenericAssetId::from_utf8_encoded(input).map(Self::Generic),
_ => GenericAssetId::from_utf8_encoded(input_ref).map(Self::Generic),
}
}
}
Expand Down Expand Up @@ -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,
}
}

Expand All @@ -315,10 +312,10 @@ pub mod v1 {
where
I: AsRef<[u8]> + Into<Vec<u8>>,
{
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
})?;
Expand Down Expand Up @@ -359,7 +356,7 @@ pub mod v1 {

// Getters
impl Slip44Reference {
pub fn inner(&self) -> &U256 {
pub const fn inner(&self) -> &U256 {
&self.0
}
}
Expand All @@ -383,9 +380,9 @@ pub mod v1 {
where
I: AsRef<[u8]> + Into<Vec<u8>>,
{
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(|_| {
Expand All @@ -407,7 +404,7 @@ pub mod v1 {

// Getters
impl EvmSmartContractFungibleReference {
pub fn inner(&self) -> &[u8] {
pub const fn inner(&self) -> &[u8] {
&self.0
}
}
Expand All @@ -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<EvmSmartContractNonFungibleIdentifier> {
pub const fn identifier(&self) -> &Option<EvmSmartContractNonFungibleIdentifier> {
&self.1
}
}
Expand All @@ -457,10 +454,10 @@ pub mod v1 {
where
I: AsRef<[u8]> + Into<Vec<u8>>,
{
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)
Expand All @@ -470,7 +467,7 @@ pub mod v1 {
})?;

Ok(Self(
Vec::<u8>::from(input)
Vec::<u8>::from(input_ref)
.try_into()
.map_err(|_| IdentifierError::InvalidFormat)?,
))
Expand Down Expand Up @@ -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<Result> to Result<Option> and bubble Err case up, keeping Ok(Option) for
// successful cases.
id: identifier.map_or(Ok(None), |id| GenericAssetIdentifier::from_utf8_encoded(id).map(Some))?,
id: encoded_identifier
.map_or(Ok(None), |id| GenericAssetIdentifier::from_utf8_encoded(id).map(Some))?,
}),
_ => Err(Error::InvalidFormat),
}
Expand All @@ -533,13 +531,13 @@ pub mod v1 {

// Getters
impl GenericAssetId {
pub fn namespace(&self) -> &GenericAssetNamespace {
pub const fn namespace(&self) -> &GenericAssetNamespace {
&self.namespace
}
pub fn reference(&self) -> &GenericAssetReference {
pub const fn reference(&self) -> &GenericAssetReference {
&self.reference
}
pub fn id(&self) -> &Option<GenericAssetIdentifier> {
pub const fn id(&self) -> &Option<GenericAssetIdentifier> {
&self.id
}
}
Expand All @@ -557,19 +555,19 @@ pub mod v1 {
where
I: AsRef<[u8]> + Into<Vec<u8>>,
{
let input = input.as_ref();
check_namespace_length_bounds(input)?;
let input_ref = input.as_ref();
check_namespace_length_bounds(input_ref)?;

input.iter().try_for_each(|c| {
if !matches!(c, b'-' | b'a'..=b'z' | b'0'..=b'9') {
input_ref.iter().try_for_each(|c| {
if !matches!(*c, b'-' | b'a'..=b'z' | b'0'..=b'9') {
log::trace!("Provided input has some invalid values as expected by a generic asset namespace.");
Err(NamespaceError::InvalidFormat)
} else {
Ok(())
}
})?;
Ok(Self(
Vec::<u8>::from(input)
Vec::<u8>::from(input_ref)
.try_into()
.map_err(|_| NamespaceError::InvalidFormat)?,
))
Expand Down Expand Up @@ -607,19 +605,19 @@ pub mod v1 {
where
I: AsRef<[u8]> + Into<Vec<u8>>,
{
let input = input.as_ref();
check_reference_length_bounds(input)?;
let input_ref = input.as_ref();
check_reference_length_bounds(input_ref)?;

input.iter().try_for_each(|c| {
if !matches!(c, b'-' | b'.' | b'%' | b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9') {
input_ref.iter().try_for_each(|c| {
if !matches!(*c, b'-' | b'.' | b'%' | b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9') {
log::trace!("Provided input has some invalid values as expected by a generic asset reference.");
Err(ReferenceError::InvalidFormat)
} else {
Ok(())
}
})?;
Ok(Self(
Vec::<u8>::from(input)
Vec::<u8>::from(input_ref)
.try_into()
.map_err(|_| ReferenceError::InvalidFormat)?,
))
Expand Down Expand Up @@ -657,19 +655,19 @@ pub mod v1 {
where
I: AsRef<[u8]> + Into<Vec<u8>>,
{
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| {
if !matches!(c, b'-' | b'.' | b'%' | b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9') {
input_ref.iter().try_for_each(|c| {
if !matches!(*c, b'-' | b'.' | b'%' | b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9') {
log::trace!("Provided input has some invalid values as expected by a generic asset identifier.");
Err(IdentifierError::InvalidFormat)
} else {
Ok(())
}
})?;
Ok(Self(
Vec::<u8>::from(input)
Vec::<u8>::from(input_ref)
.try_into()
.map_err(|_| IdentifierError::InvalidFormat)?,
))
Expand Down
Loading

0 comments on commit 09b67ae

Please sign in to comment.