Run clippy from the test script #1079

tcharding · 2022-06-30T00:45:00Z

Currently we run clippy in CI using a github action. The invocation has a couple of shortcomings

it does not lint the tests (this requires --all-targets)
it does not lint the examples

I could not find a way to lint the examples without explicitly linting each example by name.

This PR does the following:

Fix clippy issues (patch 1-4)
Move the clippy control to test.sh
Add an env var DO_LINT to control it
Remove the separate CI job
Run the linter during the Test job using the stable toolchain.
Run clippy with --all-features AND --all-targets (only recently made possible).
Run each example explicitly

Thanks to dunxen for noticing all the errors in my psbt example on review and prompting me to work out why clippy was not running :)

tcharding · 2022-07-19T06:04:50Z

Ha win! It found a bunch of warnings already, will fix and re-spin.

dunxen

Nice :)
This LGTM (with the linting addressed)

cd2369b Run ecdsa-psbt example in test script (Tobin C. Harding) 6967c0e Add psbt example (Tobin Harding) Pull request description: Add an example PSBT workflow. The workflow we simulate is that of a setup using a watch-only online wallet (contains only public keys) and a cold-storage wallet (contains the private keys). We create and update a PSBT using the watch-only wallet then pass the PSBT to the cold-storage wallet to sign. Partially resolves #892 (more done in #935). ## Note This PR includes a sub-module in `examples/psbt.rs` that implements ECDSA signing. This will hopefully eventually be merged into the main crate by way of #957. We have three PRs that add examples/tests of PSBTs that need to do signing, in order to help us design a good AP in #957 I think it would be beneficial to complete and merge these three PRs. 1. This PR (PSBT ECDSA example) 2. PBST ECDSA integration test: #935 3. PSBT taproot example: #999 Note to self, this will need a change to `test.sh` if #1079 merges. ACKs for top commit: apoelstra: ACK cd2369b sanket1729: ACK cd2369b. The example is clean, you can address the comments in followups. Tree-SHA512: c4fb8ec631bf8bfc30534e8974b1f6c4bb7cc6def165a4ee2bb7aa73f5aa7fdc11d2000ca25792a4b534b2c5bf1592efe542ada14a9d702c7dfacbaa808f3aea

apoelstra · 2022-07-20T16:47:44Z

Looks good! I notice that the commit message for f43542c says you don't lint the tests or examples, as you would need --all-targets or explicitly enumerating the examples ... but it looks from the diff that you do do that.'

apoelstra

ACK f43542c

tcharding · 2022-07-20T22:19:00Z

I notice that the commit message for f43542c says you don't lint the tests or examples, as you would need --all-targets or explicitly enumerating the examples ... but it looks from the diff that you do do that.'

Drats, that's stale. I have a tendency to not read my own commit logs during rebase, chance to lift my game :)

tcharding · 2022-07-21T00:48:43Z

oh, turns out it wasn't stale, just hard to read. I didn't bother changing the commit log and force pushing but I did update the PR description.

tcharding · 2022-07-21T00:56:35Z

~~pfff! I just see that --all-targets has been changed to --targets=all, have to rebase now :(~~ Bizzare, its only when running cargo tree

cargo tree --target=all
cargo clippy --all-targets

tcharding · 2022-07-21T23:20:59Z

If #999 goes in before this, then we will need to add an additional call to clippy to lint the taproot-psbt example.

tcharding · 2022-07-25T03:59:50Z

Changes in force push:

rebased, fixing multiple merge conflicts
Add clippy lint of new ecdsa-psbt example

apoelstra

ACK 5134ba3

Kixunil

The first commit looks wrong, the rest looks OK.

Kixunil · 2022-07-25T14:54:14Z

src/util/misc.rs

-        let msg_hash = super::signed_msg_hash(&message);
-        let msg = secp256k1::Message::from(msg_hash);
+        let msg_hash = super::signed_msg_hash(message);
+        let msg = secp256k1::Message::from_slice(&msg_hash).expect("message");


What? This looks like a step back.

Wow, I don't know how I missed this.

Oh boy, that's a bad mistake, thanks @Kixunil. I did not check over my rebase well enough.

Clippy emits various warnings of type: warning: this expression creates a reference which is immediately dereferenced by the compiler As suggested, remove the unnecessary explicit reference.

Clippy emits: warning: used `assert_eq!` with a literal bool As suggested, use `assert` instead of `assert_eq`.

Clippy emits various warnings of form: warning: redundant clone As suggested, remove the redundant calls to `clone`.

Clippy emits two warnings of form: warning: redundant field names in struct initialization Remove the redundant field names and use struct short initialization form.

Currently we run clippy in CI using a github action. The invocation has a couple of shortcomings 1. it does not lint the tests (this requires `--all-targets`) 2. it does not lint the examples I could not find a way to lint the examples without explicitly linting each example by name. Move the clippy control to `test.sh` and add an env var `DO_LINT` to control it. Remove the explicit CI job and run the linter during the `Test` job using the stable toolchain.

tcharding · 2022-07-25T22:49:55Z

Changes in force push: Remove re-introduction of Message::from_slice from patch one. No other changes.

apoelstra

ACK 74f3a5a

Kixunil

ACK 74f3a5a

74f3a5a Run clippy from the test script (Tobin C. Harding) aa8109a Use struct field short form (Tobin C. Harding) d1a0540 Remove redundant calls to clone (Tobin C. Harding) 1964925 Use assert instead of assert_eq (Tobin C. Harding) 3173ef9 Remove unnecessary explicit reference (Tobin C. Harding) Pull request description: Currently we run clippy in CI using a github action. The invocation has a couple of shortcomings 1. it does not lint the tests (this requires `--all-targets`) 2. it does not lint the examples I could not find a way to lint the examples without explicitly linting each example by name. **This PR does the following:** - Fix clippy issues (patch 1-4) - Move the clippy control to `test.sh` - Add an env var `DO_LINT` to control it - Remove the separate CI job - Run the linter during the `Test` job using the stable toolchain. - Run clippy with ` --all-features` AND `--all-targets` (only recently made possible). - Run each example explicitly Thanks to dunxen for noticing all the errors in my psbt example on review and prompting me to work out why clippy was not running :) ACKs for top commit: apoelstra: ACK 74f3a5a Kixunil: ACK 74f3a5a Tree-SHA512: 56dc6262144f4caa5efa6fdc46aeecf7bddc050ef134a639b31a34d4c5e01abcdeda18a00f4ded443866bbdfc982b4e5b67b0089639e0c253e207f0b54777f57

tcharding mentioned this pull request Jun 30, 2022

Add psbt watch-only/cold-storage example #940

Merged

tcharding marked this pull request as draft June 30, 2022 01:23

tcharding force-pushed the 06-30-clippy-in-test-script branch from 851df5c to 9e777c1 Compare July 19, 2022 05:53

tcharding marked this pull request as ready for review July 19, 2022 05:53

dunxen previously approved these changes Jul 19, 2022

View reviewed changes

tcharding dismissed dunxen’s stale review via f43542c July 20, 2022 03:03

tcharding force-pushed the 06-30-clippy-in-test-script branch from 9e777c1 to f43542c Compare July 20, 2022 03:03

apoelstra previously approved these changes Jul 20, 2022

View reviewed changes

tcharding mentioned this pull request Jul 21, 2022

examples: Add taproot PSBT example workflow #999

Merged

tcharding added the no release notes mention label Jul 25, 2022

tcharding dismissed apoelstra’s stale review via 5134ba3 July 25, 2022 03:59

tcharding force-pushed the 06-30-clippy-in-test-script branch from f43542c to 5134ba3 Compare July 25, 2022 03:59

apoelstra previously approved these changes Jul 25, 2022

View reviewed changes

Kixunil reviewed Jul 25, 2022

View reviewed changes

tcharding added 5 commits July 26, 2022 08:46

Remove unnecessary explicit reference

3173ef9

Clippy emits various warnings of type: warning: this expression creates a reference which is immediately dereferenced by the compiler As suggested, remove the unnecessary explicit reference.

Use assert instead of assert_eq

1964925

Clippy emits: warning: used `assert_eq!` with a literal bool As suggested, use `assert` instead of `assert_eq`.

Remove redundant calls to clone

d1a0540

Clippy emits various warnings of form: warning: redundant clone As suggested, remove the redundant calls to `clone`.

Use struct field short form

aa8109a

Clippy emits two warnings of form: warning: redundant field names in struct initialization Remove the redundant field names and use struct short initialization form.

tcharding dismissed apoelstra’s stale review via 74f3a5a July 25, 2022 22:49

tcharding force-pushed the 06-30-clippy-in-test-script branch from 5134ba3 to 74f3a5a Compare July 25, 2022 22:49

apoelstra approved these changes Jul 25, 2022

View reviewed changes

Kixunil approved these changes Jul 26, 2022

View reviewed changes

apoelstra merged commit 92b6211 into rust-bitcoin:master Jul 26, 2022

tcharding deleted the 06-30-clippy-in-test-script branch July 27, 2022 00:54

Kixunil added this to the 0.29.0 milestone Aug 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run clippy from the test script #1079

Run clippy from the test script #1079

tcharding commented Jun 30, 2022 •

edited by apoelstra

tcharding commented Jul 19, 2022

dunxen left a comment

apoelstra commented Jul 20, 2022

apoelstra left a comment

tcharding commented Jul 20, 2022

tcharding commented Jul 21, 2022

tcharding commented Jul 21, 2022 •

edited

tcharding commented Jul 21, 2022

tcharding commented Jul 25, 2022

apoelstra left a comment

Kixunil left a comment

Kixunil Jul 25, 2022

apoelstra Jul 25, 2022

tcharding Jul 25, 2022

tcharding commented Jul 25, 2022

apoelstra left a comment

Kixunil left a comment

Run clippy from the test script #1079

Run clippy from the test script #1079

Conversation

tcharding commented Jun 30, 2022 • edited by apoelstra

tcharding commented Jul 19, 2022

dunxen left a comment

Choose a reason for hiding this comment

apoelstra commented Jul 20, 2022

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Jul 20, 2022

tcharding commented Jul 21, 2022

tcharding commented Jul 21, 2022 • edited

tcharding commented Jul 21, 2022

tcharding commented Jul 25, 2022

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

Kixunil Jul 25, 2022

Choose a reason for hiding this comment

apoelstra Jul 25, 2022

Choose a reason for hiding this comment

tcharding Jul 25, 2022

Choose a reason for hiding this comment

tcharding commented Jul 25, 2022

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

tcharding commented Jun 30, 2022 •

edited by apoelstra

tcharding commented Jul 21, 2022 •

edited