Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linter fixes #27

Merged
merged 6 commits into from
Mar 14, 2022
Merged

Linter fixes #27

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8400d95
Add rust-toolchain.toml set to nightly toolchain.
cryptoquick Feb 9, 2022
5dd510f
Fix some minor clippy lints.
cryptoquick Feb 9, 2022
880b01b
Fix remainder of lints. Allow a linter rule for enum tuple struct whe…
cryptoquick Feb 9, 2022
f896030
Remove clippy exception by implementing CString error handling for si…
cryptoquick Feb 22, 2022
e156005
Revert AccountStep enum member refactor.
cryptoquick Feb 22, 2022
ddb6f06
Peg toolchain to stable 1.59.
cryptoquick Mar 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions descriptors/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
// Coding conventions
#![recursion_limit = "256"]
#![deny(dead_code, /* missing_docs, */ warnings)]
#![allow(clippy::init_numbered_fields)]

//! General workflow for working with ScriptPubkey* types:
//! ```text
Expand Down
33 changes: 18 additions & 15 deletions hd/src/indexes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,10 @@ pub enum AccountStep {
#[from(u8)]
#[from(u16)]
#[from]
Normal(UnhardenedIndex),
Normal {
/// Unhardened derivation 0-based index value
index: UnhardenedIndex,
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not entirely get why do we need this. Is it something suggested by the linter?

This is a breaking change which will stop downstream projects from compiling, and will require a major release. Are we really that much needing it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was originally suggested by the linter, but if that was a breaking put it back. It's basically caused by the error I mentioned and documented here:
LNP-BP/client_side_validation#54

But also, it's weird, once I put it back, the error is gone, which may also invalidate that issue. Good catch, either way, thank you. I wouldn't want to introduce bad code, nor would I want to waste your time.


/// Derivation segment is defined by a hardened index
Hardened {
Expand Down Expand Up @@ -470,7 +473,7 @@ impl SegmentIndexes for AccountStep {
fn from_index(index: impl Into<u32>) -> Result<Self, bip32::Error> {
let index = index.into();
Ok(UnhardenedIndex::from_index(index)
.map(Self::Normal)
.map(|index| Self::Normal { index })
.unwrap_or_else(|_| {
Self::hardened(
HardenedIndex::from_index(index)
Expand All @@ -482,7 +485,7 @@ impl SegmentIndexes for AccountStep {
#[inline]
fn first_index(&self) -> u32 {
match self {
AccountStep::Normal(index) => SegmentIndexes::first_index(index),
AccountStep::Normal { index } => SegmentIndexes::first_index(index),
AccountStep::Hardened { index, .. } => SegmentIndexes::first_index(index),
}
}
Expand All @@ -497,31 +500,31 @@ impl SegmentIndexes for AccountStep {
#[inline]
fn first_derivation_value(&self) -> u32 {
match self {
AccountStep::Normal(index) => index.first_derivation_value(),
AccountStep::Normal { index } => index.first_derivation_value(),
AccountStep::Hardened { index, .. } => index.first_derivation_value(),
}
}

#[inline]
fn checked_add_assign(&mut self, add: impl Into<u32>) -> Option<u32> {
match self {
AccountStep::Normal(index) => index.checked_add_assign(add),
AccountStep::Normal { index } => index.checked_add_assign(add),
AccountStep::Hardened { index, .. } => index.checked_add_assign(add),
}
}

#[inline]
fn checked_sub_assign(&mut self, sub: impl Into<u32>) -> Option<u32> {
match self {
AccountStep::Normal(index) => index.checked_sub_assign(sub),
AccountStep::Normal { index } => index.checked_sub_assign(sub),
AccountStep::Hardened { index, .. } => index.checked_sub_assign(sub),
}
}

#[inline]
fn is_hardened(&self) -> bool {
match self {
AccountStep::Normal(_) => false,
AccountStep::Normal { .. } => false,
AccountStep::Hardened { .. } => true,
}
}
Expand All @@ -530,7 +533,7 @@ impl SegmentIndexes for AccountStep {
impl Display for AccountStep {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
match self {
AccountStep::Normal(index) => Display::fmt(index, f),
AccountStep::Normal { index } => Display::fmt(index, f),
AccountStep::Hardened {
index,
xpub_ref: XpubRef::Unknown,
Expand Down Expand Up @@ -567,9 +570,9 @@ impl TryFrom<ChildNumber> for AccountStep {
/// values to be in range, we need to re-test them here
fn try_from(child_number: ChildNumber) -> Result<Self, Self::Error> {
Ok(match child_number {
ChildNumber::Normal { index } => {
AccountStep::Normal(UnhardenedIndex::from_index(index)?)
}
ChildNumber::Normal { index } => AccountStep::Normal {
index: UnhardenedIndex::from_index(index)?,
},
ChildNumber::Hardened { index } => {
AccountStep::hardened(HardenedIndex::from_index(index)?)
}
Expand All @@ -584,7 +587,7 @@ impl From<AccountStep> for ChildNumber {
impl From<&AccountStep> for ChildNumber {
fn from(value: &AccountStep) -> Self {
match value {
AccountStep::Normal(index) => ChildNumber::Normal {
AccountStep::Normal { index } => ChildNumber::Normal {
index: index.first_index(),
},
AccountStep::Hardened { index, .. } => ChildNumber::Hardened {
Expand All @@ -604,7 +607,7 @@ impl TryFrom<AccountStep> for UnhardenedIndex {

fn try_from(value: AccountStep) -> Result<Self, Self::Error> {
match value {
AccountStep::Normal(index) => Ok(index),
AccountStep::Normal { index } => Ok(index),
AccountStep::Hardened { index, .. } => {
Err(bip32::Error::InvalidChildNumber(index.first_index()))
}
Expand All @@ -617,7 +620,7 @@ impl TryFrom<AccountStep> for HardenedIndex {

fn try_from(value: AccountStep) -> Result<Self, Self::Error> {
match value {
AccountStep::Normal(index) => {
AccountStep::Normal { index } => {
Err(bip32::Error::InvalidChildNumber(index.first_index()))
}
AccountStep::Hardened { index, .. } => Ok(index),
Expand All @@ -642,7 +645,7 @@ pub enum TerminalStep {
#[display(inner)]
Range(IndexRangeList<UnhardenedIndex>),

/// Wildcrard implying full range of unhardened indexes
/// Wildcard implying full range of unhardened indexes
#[display("*")]
Wildcard,
}
Expand Down
1 change: 1 addition & 0 deletions hd/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
// Coding conventions
#![recursion_limit = "256"]
#![deny(dead_code, missing_docs, warnings)]
#![allow(clippy::init_numbered_fields)]

#[macro_use]
extern crate amplify;
Expand Down
1 change: 1 addition & 0 deletions libbitcoin/src/signer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ impl string_result_t {
pub fn success(data: impl ToString) -> string_result_t {
let (code, details) = match CString::new(data.to_string()) {
Ok(s) => (error_t::success, result_details_t { data: s.into_raw() }),
#[allow(clippy::needless_borrow)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why just not remove the borrow here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because the CString NulError is of a different type, and there was some case where Clippy couldn't make sense of things.

Works:

#[allow(clippy::needless_borrow)]
Err(err) => (error_t::invalid_result_data, (&err).into()),

Does not work:

Err(err) => (error_t::invalid_result_data, err.into()),

Why:

the trait bound `signer::result_details_t: std::convert::From<std::ffi::NulError>` is not satisfied
the following implementations were found:
  <signer::result_details_t as std::convert::From<&E>>
required because of the requirements on the impl of `std::convert::Into<signer::result_details_t>` for `std::ffi::NulError`

However, doing the CString conversion manually works:

            Err(err) => (error_t::invalid_result_data, result_details_t {
                data: CString::new(err.to_string())
                    .expect("Null byte in string_result_t success code doc comments")
                    .into_raw(),
            }),

This was referencing the code in the from impl here:
https://github.com/LNP-BP/descriptor-wallet/pull/27/files#diff-d3399c793447433124719f22f76f514e89f4bff0280e4675af35ae071b376ee2L179

But that's less-than-ideal, since it's essentially identical code to the From implementation, duplicated over. It's not so simple to implement the error type for CStrings. Anyway, I've pushed that code, let me know if you have a better approach.

Err(err) => (error_t::invalid_result_data, (&err).into()),
};
string_result_t { code, details }
Expand Down
2 changes: 1 addition & 1 deletion psbt/src/sign/signer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ where
}
(CompositeDescrType::Wsh, Some(witness_script))
| (CompositeDescrType::ShWsh, Some(witness_script)) => {
sig_hasher.segwit_signature_hash(index, &witness_script, spent_value, sighash_type)?
sig_hasher.segwit_signature_hash(index, witness_script, spent_value, sighash_type)?
}
(CompositeDescrType::Wsh, None) | (CompositeDescrType::ShWsh, None) => {
return Err(SignInputError::NoWitnessScript)
Expand Down
2 changes: 2 additions & 0 deletions rust-toolchain.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[toolchain]
channel = "nightly"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, not sure we want to default to the nightly channel

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed, as I tried setting this back to stable, it caused the issue I mentioned I had fixed by reverting to the old code mentioend in the comment here:
#27 (comment)

I'm not sure we can get this to pass clippy lints on stable at this time without also making breaking changes to dependents.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we can again move to stable with the latest 0.59 release

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's true, how convenient :)

14 changes: 4 additions & 10 deletions scripts/src/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ pub(crate) mod test {
sk[2] = (i >> 16) as u8;

ret.push(secp256k1::PublicKey::from_secret_key(
&secp256k1::SECP256K1,
secp256k1::SECP256K1,
&secp256k1::SecretKey::from_slice(&sk[..]).unwrap(),
));
}
Expand All @@ -198,7 +198,7 @@ pub(crate) mod test {
}

pub(crate) fn no_keys_or_hashes_suite(proc: fn(LockScript) -> ()) {
let sha_hash = sha256::Hash::hash(&"(nearly)random string".as_bytes());
let sha_hash = sha256::Hash::hash("(nearly)random string".as_bytes());
let dummy_hashes: Vec<hash160::Hash> = (1..13)
.map(|i| hash160::Hash::from_inner([i; 20]))
.collect();
Expand Down Expand Up @@ -364,10 +364,7 @@ pub(crate) mod test {
#[test]
fn test_script_parse_complex_keys() {
complex_keys_suite(|lockscript, keys| {
assert_eq!(
lockscript.extract_pubkeys::<Segwitv0>().unwrap(),
keys.clone()
);
assert_eq!(lockscript.extract_pubkeys::<Segwitv0>().unwrap(), keys);
assert_eq!(
lockscript.extract_pubkey_hash_set::<Segwitv0>().unwrap(),
(BTreeSet::from_iter(keys), BTreeSet::new())
Expand All @@ -387,10 +384,7 @@ pub(crate) mod test {
#[test]
fn test_script_parse_complex_script() {
complex_suite(|lockscript, keys| {
assert_eq!(
lockscript.extract_pubkeys::<Segwitv0>().unwrap(),
keys.clone()
);
assert_eq!(lockscript.extract_pubkeys::<Segwitv0>().unwrap(), keys);
assert_eq!(
lockscript.extract_pubkeyset::<Segwitv0>().unwrap(),
BTreeSet::from_iter(keys)
Expand Down
7 changes: 4 additions & 3 deletions slip132/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,13 +359,14 @@ impl KeyApplication {
}
let bip48_purpose = ChildNumber::Hardened { index: 48 };
if path.len() >= 4 && path[0] == bip48_purpose {
return match path[3] {
match path[3] {
ChildNumber::Hardened { index: 1 } => Some(KeyApplication::NestedMultisig),
ChildNumber::Hardened { index: 2 } => Some(KeyApplication::SegWitMiltisig),
_ => None,
};
}
} else {
None
}
return None;
}

pub fn to_derivation_path(&self) -> Option<DerivationPath> {
Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
// Coding conventions
#![recursion_limit = "256"]
#![deny(dead_code, /* missing_docs, */ warnings)]
#![allow(clippy::init_numbered_fields)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably this will go away after resolving LNP-BP/client_side_validation#54

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you pls check that we still need this with the latest 0.59 stable release?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this allow statement presents this warning/error on 1.59:

Screenshot from 2022-03-07 05-09-12


#[macro_use]
extern crate amplify;
Expand Down