From 71da2aa4d1296132b588efe3e4fce9ed157dc400 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 23 Jun 2022 13:45:16 +1000 Subject: [PATCH 01/10] Remove redundant field names clippy emits two warnings of form: warning: redundant field names in struct initialization Remove the redundant field names. --- src/internal_macros.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/internal_macros.rs b/src/internal_macros.rs index f8e6b16..fd77908 100644 --- a/src/internal_macros.rs +++ b/src/internal_macros.rs @@ -276,7 +276,7 @@ macro_rules! serde_struct_human_string_impl { )* let ret = $name { - $($fe: $fe),* + $($fe),* }; Ok(ret) @@ -312,7 +312,7 @@ macro_rules! serde_struct_human_string_impl { )* let ret = $name { - $($fe: $fe),* + $($fe),* }; Ok(ret) From 99ac11e22714903361475125294fe55455d18c86 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 23 Jun 2022 13:47:22 +1000 Subject: [PATCH 02/10] Allow clippy::collapsible_else_if clippy emits: warning: this `else { if .. }` block can be collapsed In this instance the code is more readable how it is, we should ignore clippy. Add compiler directive to quieten warning. --- src/util/key.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/key.rs b/src/util/key.rs index 3ea2106..bbb9e0a 100644 --- a/src/util/key.rs +++ b/src/util/key.rs @@ -411,6 +411,7 @@ impl<'de> ::serde::Deserialize<'de> for PrivateKey { #[cfg(feature = "serde")] #[cfg_attr(docsrs, doc(cfg(feature = "serde")))] +#[allow(clippy::collapsible_else_if)] // Aids readability. impl ::serde::Serialize for PublicKey { fn serialize(&self, s: S) -> Result { if s.is_human_readable() { From 7ef7eef0cb1931f6d8910ad27b09c0c22504eafe Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 23 Jun 2022 13:49:16 +1000 Subject: [PATCH 03/10] Remove unnecessary 'static lifetime clippy emits a bunch of: warning: statics have by default a `'static` lifetime Remove the unnecessary 'static lifetimes. --- src/blockdata/transaction.rs | 2 +- src/util/key.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/blockdata/transaction.rs b/src/blockdata/transaction.rs index 7859d19..90b96bc 100644 --- a/src/blockdata/transaction.rs +++ b/src/blockdata/transaction.rs @@ -1662,7 +1662,7 @@ mod benches { use crate::hashes::hex::FromHex; use test::{black_box, Bencher}; - const SOME_TX: &'static str = "0100000001a15d57094aa7a21a28cb20b59aab8fc7d1149a3bdbcddba9c622e4f5f6a99ece010000006c493046022100f93bb0e7d8db7bd46e40132d1f8242026e045f03a0efe71bbb8e3f475e970d790221009337cd7f1f929f00cc6ff01f03729b069a7c21b59b1736ddfee5db5946c5da8c0121033b9b137ee87d5a812d6f506efdd37f0affa7ffc310711c06c7f3e097c9447c52ffffffff0100e1f505000000001976a9140389035a9225b3839e2bbf32d826a1e222031fd888ac00000000"; + const SOME_TX: &str = "0100000001a15d57094aa7a21a28cb20b59aab8fc7d1149a3bdbcddba9c622e4f5f6a99ece010000006c493046022100f93bb0e7d8db7bd46e40132d1f8242026e045f03a0efe71bbb8e3f475e970d790221009337cd7f1f929f00cc6ff01f03729b069a7c21b59b1736ddfee5db5946c5da8c0121033b9b137ee87d5a812d6f506efdd37f0affa7ffc310711c06c7f3e097c9447c52ffffffff0100e1f505000000001976a9140389035a9225b3839e2bbf32d826a1e222031fd888ac00000000"; #[bench] pub fn bench_transaction_size(bh: &mut Bencher) { diff --git a/src/util/key.rs b/src/util/key.rs index bbb9e0a..a2c6a86 100644 --- a/src/util/key.rs +++ b/src/util/key.rs @@ -550,9 +550,9 @@ mod tests { fn test_key_serde() { use serde_test::{Configure, Token, assert_tokens}; - static KEY_WIF: &'static str = "cVt4o7BGAig1UXywgGSmARhxMdzP5qvQsxKkSsc1XEkw3tDTQFpy"; - static PK_STR: &'static str = "039b6347398505f5ec93826dc61c19f47c66c0283ee9be980e29ce325a0f4679ef"; - static PK_STR_U: &'static str = "\ + static KEY_WIF: &str = "cVt4o7BGAig1UXywgGSmARhxMdzP5qvQsxKkSsc1XEkw3tDTQFpy"; + static PK_STR: &str = "039b6347398505f5ec93826dc61c19f47c66c0283ee9be980e29ce325a0f4679ef"; + static PK_STR_U: &str = "\ 04\ 9b6347398505f5ec93826dc61c19f47c66c0283ee9be980e29ce325a0f4679ef\ 87288ed73ce47fc4f5c79d19ebfa57da7cff3aff6e819e4ee971d86b5e61875d\ From 1c3dee4bbc5b9090ed0dc94a7c583d5b4308f4e7 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 23 Jun 2022 13:54:34 +1000 Subject: [PATCH 04/10] Use custom digit grouping clippy emits a bunch of: warning: digits grouped inconsistently by underscores We have a custom grouping elsewhere in this file 10_000_000_00 sats == 10 BTC Fix up all instances of large sats amount to uniformly using this format and add compiler directives where needed to shoosh clippy. --- src/util/amount.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/util/amount.rs b/src/util/amount.rs index e2eff86..8ca6a16 100644 --- a/src/util/amount.rs +++ b/src/util/amount.rs @@ -1622,9 +1622,9 @@ mod tests { assert_eq!(p("1.1", btc), Ok(Amount::from_sat(1_100_000_00))); assert_eq!(p("100", sat), Ok(Amount::from_sat(100))); assert_eq!(p("55", sat), Ok(Amount::from_sat(55))); - assert_eq!(p("5500000000000000000", sat), Ok(Amount::from_sat(5_500_000_000_000_000_000))); + assert_eq!(p("5500000000000000000", sat), Ok(Amount::from_sat(55_000_000_000_000_000_00))); // Should this even pass? - assert_eq!(p("5500000000000000000.", sat), Ok(Amount::from_sat(5_500_000_000_000_000_000))); + assert_eq!(p("5500000000000000000.", sat), Ok(Amount::from_sat(55_000_000_000_000_000_00))); assert_eq!( p("12345678901.12345678", btc), Ok(Amount::from_sat(12_345_678_901__123_456_78)) @@ -2006,6 +2006,7 @@ mod tests { #[cfg(feature = "serde")] #[test] + #[allow(clippy::inconsistent_digit_grouping)] // Group to show 100,000,000 sats per bitcoin. fn serde_as_btc() { use serde_json; @@ -2041,6 +2042,7 @@ mod tests { #[cfg(feature = "serde")] #[test] + #[allow(clippy::inconsistent_digit_grouping)] // Group to show 100,000,000 sats per bitcoin. fn serde_as_btc_opt() { use serde_json; @@ -2054,8 +2056,8 @@ mod tests { } let with = T { - amt: Some(Amount::from_sat(2__500_000_00)), - samt: Some(SignedAmount::from_sat(-2__500_000_00)), + amt: Some(Amount::from_sat(2_500_000_00)), + samt: Some(SignedAmount::from_sat(-2_500_000_00)), }; let without = T { amt: None, @@ -2085,6 +2087,7 @@ mod tests { #[cfg(feature = "serde")] #[test] + #[allow(clippy::inconsistent_digit_grouping)] // Group to show 100,000,000 sats per bitcoin. fn serde_as_sat_opt() { use serde_json; @@ -2098,8 +2101,8 @@ mod tests { } let with = T { - amt: Some(Amount::from_sat(2__500_000_00)), - samt: Some(SignedAmount::from_sat(-2__500_000_00)), + amt: Some(Amount::from_sat(2_500_000_00)), + samt: Some(SignedAmount::from_sat(-2_500_000_00)), }; let without = T { amt: None, From 0c4b1417c49a4d51f977d7fe7b1b0b9c0f0f3433 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 23 Jun 2022 13:56:28 +1000 Subject: [PATCH 05/10] Remove unneeded reference clippy emits: warning: this expression creates a reference which is immediately dereferenced by the compiler As suggested, remove the explicit reference. --- src/blockdata/script.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/blockdata/script.rs b/src/blockdata/script.rs index ab141fc..99b0747 100644 --- a/src/blockdata/script.rs +++ b/src/blockdata/script.rs @@ -1085,7 +1085,7 @@ impl serde::Serialize for Script { if serializer.is_human_readable() { serializer.serialize_str(&format!("{:x}", self)) } else { - serializer.serialize_bytes(&self.as_bytes()) + serializer.serialize_bytes(self.as_bytes()) } } } From dd7e074fd10c3560eaafe1693055add226bd6175 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 23 Jun 2022 13:57:31 +1000 Subject: [PATCH 06/10] Remove unneeded clone clippy emits: warning: using `clone` on type `blockdata::transaction::OutPoint` which implements the `Copy` trait Remove unneeded call to `clone`. --- src/blockdata/transaction.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/blockdata/transaction.rs b/src/blockdata/transaction.rs index 90b96bc..e397ef1 100644 --- a/src/blockdata/transaction.rs +++ b/src/blockdata/transaction.rs @@ -631,7 +631,7 @@ impl Transaction { if let Some(output) = spent(&input.previous_output) { output.script_pubkey.verify_with_flags(idx, crate::Amount::from_sat(output.value), tx.as_slice(), flags)?; } else { - return Err(script::Error::UnknownSpentOutput(input.previous_output.clone())); + return Err(script::Error::UnknownSpentOutput(input.previous_output)); } } Ok(()) From ad080cf3f2cd1fb9d851f2d4cb13ed62448364e8 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 23 Jun 2022 13:58:29 +1000 Subject: [PATCH 07/10] Remove unnecessary ? operator clippy emits: warning: question mark operator is useless here As suggested, remove the `?` operator. --- src/util/amount.rs | 4 ++-- src/util/psbt/mod.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/amount.rs b/src/util/amount.rs index 8ca6a16..ce7757c 100644 --- a/src/util/amount.rs +++ b/src/util/amount.rs @@ -1310,7 +1310,7 @@ pub mod serde { } fn des_btc<'d, D: Deserializer<'d>>(d: D) -> Result { use serde::de::Error; - Ok(Amount::from_btc(f64::deserialize(d)?).map_err(D::Error::custom)?) + Amount::from_btc(f64::deserialize(d)?).map_err(D::Error::custom) } } @@ -1338,7 +1338,7 @@ pub mod serde { } fn des_btc<'d, D: Deserializer<'d>>(d: D) -> Result { use serde::de::Error; - Ok(SignedAmount::from_btc(f64::deserialize(d)?).map_err(D::Error::custom)?) + SignedAmount::from_btc(f64::deserialize(d)?).map_err(D::Error::custom) } } diff --git a/src/util/psbt/mod.rs b/src/util/psbt/mod.rs index c616d2d..2b56ca7 100644 --- a/src/util/psbt/mod.rs +++ b/src/util/psbt/mod.rs @@ -269,7 +269,7 @@ mod display_from_str { fn from_str(s: &str) -> Result { let data = ::base64::decode(s).map_err(PsbtParseError::Base64Encoding)?; - Ok(encode::deserialize(&data).map_err(PsbtParseError::PsbtEncoding)?) + encode::deserialize(&data).map_err(PsbtParseError::PsbtEncoding) } } } From 362d439d8c21125bc3ccacfe4f896639658555d9 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 23 Jun 2022 14:07:02 +1000 Subject: [PATCH 08/10] Enable clippy on CI Add a clippy configuration file configuring the MSRV. Add a github actions job to run clippy on CI. Please note the job does _not_ use `--all-targets` because doing so causes: ``` error[E0554]: `#![feature]` may not be used on the stable release channel --> src/lib.rs:46:54 | 46 | #![cfg_attr(all(test, feature = "unstable"), feature(test))] ``` --- .github/workflows/rust.yml | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 8d37c2f..4173353 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -107,3 +107,18 @@ jobs: CARGO_TARGET_THUMBV7M_NONE_EABI_RUNNER: "qemu-system-arm -cpu cortex-m3 -machine mps2-an385 -nographic -semihosting-config enable=on,target=native -kernel" run: cd embedded && cargo run --target thumbv7m-none-eabi + Clippy: + name: Clippy + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - uses: actions-rs/toolchain@v1 + with: + profile: minimal + toolchain: stable + override: true + - run: rustup component add clippy + - uses: actions-rs/cargo@v1 + with: + command: clippy + args: --all-features -- -D warnings From 1ce2750f415a64efaa3ee6414f290c94ea236c1f Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Tue, 7 Jun 2022 15:54:26 +1000 Subject: [PATCH 09/10] Add githooks directory Add a `githooks` directory. Copy the sample pre-commit hook into it. Add a section to the README explaining how to take advantage of the githooks. The sample pre-commit hook includes checks for trailing whitespace that we are seeing occasionally in PRs. Done in preparation for adding a clippy githook. --- README.md | 11 +++++++++++ githooks/pre-commit | 46 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100755 githooks/pre-commit diff --git a/README.md b/README.md index c6cc589..7fb11cd 100644 --- a/README.md +++ b/README.md @@ -125,6 +125,17 @@ In order to speed up the review process the CI pipeline can be run locally using skipped when using `act` due to caching being unsupported at this time. We do not *actively* support `act` but will merge PRs fixing `act` issues. +### Githooks + +To assist devs in catching errors _before_ running CI we provide some githooks. If you do not +already have locally configured githooks you can use the ones in this repository by running, in the +root directory of the repository: +``` +git config --local core.hooksPath githooks/ +``` + +Alternatively add symlinks in your `.git/hooks` directory to any of the githooks we provide. + ## Policy on Altcoins/Altchains Patches which add support for non-Bitcoin cryptocurrencies by adding constants diff --git a/githooks/pre-commit b/githooks/pre-commit new file mode 100755 index 0000000..4d6dde6 --- /dev/null +++ b/githooks/pre-commit @@ -0,0 +1,46 @@ +#!/bin/sh +# +# Verify what is about to be committed. Called by "git commit" with no +# arguments. The hook should exit with non-zero status after issuing an +# appropriate message if it wants to stop the commit. + +if git rev-parse --verify HEAD >/dev/null 2>&1 +then + against=HEAD +else + # Initial commit: diff against an empty tree object + against=$(git hash-object -t tree /dev/null) +fi + +# If you want to allow non-ASCII filenames set this variable to true. +allownonascii=$(git config --bool hooks.allownonascii) + +# Redirect output to stderr. +exec 1>&2 + +# Cross platform projects tend to avoid non-ASCII filenames; prevent +# them from being added to the repository. We exploit the fact that the +# printable range starts at the space character and ends with tilde. +if [ "$allownonascii" != "true" ] && + # Note that the use of brackets around a tr range is ok here, (it's + # even required, for portability to Solaris 10's /usr/bin/tr), since + # the square bracket bytes happen to fall in the designated range. + test $(git diff --cached --name-only --diff-filter=A -z $against | + LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0 +then + cat <<\EOF +Error: Attempt to add a non-ASCII file name. + +This can cause problems if you want to work with people on other platforms. + +To be portable it is advisable to rename the file. + +If you know what you are doing you can disable this check using: + + git config hooks.allownonascii true +EOF + exit 1 +fi + +# If there are whitespace errors, print the offending file names and fail. +exec git diff-index --check --cached $against -- From 5e3835130318183409c76a705d8100586638f84c Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 23 Jun 2022 14:10:20 +1000 Subject: [PATCH 10/10] Add clippy to pre-commit githook We have a `pre-commit` githook, add to it a call to `cargo clippy`. --- githooks/pre-commit | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/githooks/pre-commit b/githooks/pre-commit index 4d6dde6..dc0fa1b 100755 --- a/githooks/pre-commit +++ b/githooks/pre-commit @@ -43,4 +43,7 @@ EOF fi # If there are whitespace errors, print the offending file names and fail. -exec git diff-index --check --cached $against -- +git diff-index --check --cached $against -- || exit 1 + +# Check that code lints cleanly. +cargo clippy --all-features -- -D warnings || exit 1