Skip to content

Remove PartialEq impl from Error #741

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

Merged
merged 3 commits into from
Sep 8, 2024
Merged

Remove PartialEq impl from Error #741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4f52fb7
introduce `StaticDebugAndDisplay` blanket trait, use it for all errors
apoelstra Aug 30, 2024
3649850
tests: remove all uses of Eq on error types
apoelstra Aug 31, 2024
57bbde8
remove PartialEq bound from top-level Error type
apoelstra Aug 30, 2024
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
25 changes: 15 additions & 10 deletions src/blanket_traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ use core::{fmt, hash};

use crate::MiniscriptKey;

/// Auxiliary trait indicating that a type implements both `Debug` and `Display`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Auxiliary trait indicating that a type implements both `Debug` and `Display`.
/// Auxiliary trait indicating that a type implements `'static`, `Debug` and `Display`.

Copy link
Member Author

Choose a reason for hiding this comment

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

"implements 'static" isn't the right terminology ... if we want to say this would say it "is 'static".

But IMHO it's fine as-is. The name of the trait exactly describes what it does.

Copy link
Member

Choose a reason for hiding this comment

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

Works for me.

pub trait StaticDebugAndDisplay: fmt::Debug + fmt::Display + 'static {}

impl<T: fmt::Debug + fmt::Display + 'static> StaticDebugAndDisplay for T {}

/// Blanket trait describing a key where all associated types implement `FromStr`,
/// and all `FromStr` errors can be displayed.
pub trait FromStrKey:
Expand All @@ -36,7 +41,7 @@ pub trait FromStrKey:
+ fmt::Debug
+ hash::Hash;
/// Dummy type. Do not use.
type _Sha256FromStrErr: fmt::Debug + fmt::Display;
type _Sha256FromStrErr: StaticDebugAndDisplay;
/// Dummy type. Do not use.
type _Hash256: FromStr<Err = Self::_Hash256FromStrErr>
+ Clone
Expand All @@ -46,7 +51,7 @@ pub trait FromStrKey:
+ fmt::Debug
+ hash::Hash;
/// Dummy type. Do not use.
type _Hash256FromStrErr: fmt::Debug + fmt::Display;
type _Hash256FromStrErr: StaticDebugAndDisplay;
/// Dummy type. Do not use.
type _Ripemd160: FromStr<Err = Self::_Ripemd160FromStrErr>
+ Clone
Expand All @@ -56,7 +61,7 @@ pub trait FromStrKey:
+ fmt::Debug
+ hash::Hash;
/// Dummy type. Do not use.
type _Ripemd160FromStrErr: fmt::Debug + fmt::Display;
type _Ripemd160FromStrErr: StaticDebugAndDisplay;
/// Dummy type. Do not use.
type _Hash160: FromStr<Err = Self::_Hash160FromStrErr>
+ Clone
Expand All @@ -66,9 +71,9 @@ pub trait FromStrKey:
+ fmt::Debug
+ hash::Hash;
/// Dummy type. Do not use.
type _Hash160FromStrErr: fmt::Debug + fmt::Display;
type _Hash160FromStrErr: StaticDebugAndDisplay;
/// Dummy type. Do not use.
type _FromStrErr: fmt::Debug + fmt::Display;
type _FromStrErr: StaticDebugAndDisplay;
}

impl<T> FromStrKey for T
Expand All @@ -78,11 +83,11 @@ where
Self::Hash256: FromStr,
Self::Ripemd160: FromStr,
Self::Hash160: FromStr,
<Self as FromStr>::Err: fmt::Debug + fmt::Display,
<<Self as MiniscriptKey>::Sha256 as FromStr>::Err: fmt::Debug + fmt::Display,
<Self::Hash256 as FromStr>::Err: fmt::Debug + fmt::Display,
<Self::Ripemd160 as FromStr>::Err: fmt::Debug + fmt::Display,
<Self::Hash160 as FromStr>::Err: fmt::Debug + fmt::Display,
<Self as FromStr>::Err: StaticDebugAndDisplay,
<<Self as MiniscriptKey>::Sha256 as FromStr>::Err: StaticDebugAndDisplay,
<Self::Hash256 as FromStr>::Err: StaticDebugAndDisplay,
<Self::Ripemd160 as FromStr>::Err: StaticDebugAndDisplay,
<Self::Hash160 as FromStr>::Err: StaticDebugAndDisplay,
{
type _Sha256 = <T as MiniscriptKey>::Sha256;
type _Sha256FromStrErr = <<T as MiniscriptKey>::Sha256 as FromStr>::Err;
Expand Down
2 changes: 1 addition & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ pub trait ForEachKey<Pk: MiniscriptKey> {

/// Miniscript
#[derive(Debug, PartialEq)]
#[derive(Debug)]
pub enum Error {
/// Opcode appeared which is not part of the script subset
InvalidOpcode(Opcode),
Expand Down
16 changes: 11 additions & 5 deletions src/miniscript/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1472,13 +1472,16 @@ mod tests {
fn duplicate_keys() {
// You cannot parse a Miniscript that has duplicate keys
let err = Miniscript::<String, Segwitv0>::from_str("and_v(v:pk(A),pk(A))").unwrap_err();
assert_eq!(err, Error::AnalysisError(crate::AnalysisError::RepeatedPubkeys));
assert!(matches!(err, Error::AnalysisError(crate::AnalysisError::RepeatedPubkeys)));

// ...though you can parse one with from_str_insane
let ok_insane =
Miniscript::<String, Segwitv0>::from_str_insane("and_v(v:pk(A),pk(A))").unwrap();
// ...but this cannot be sanity checked.
assert_eq!(ok_insane.sanity_check().unwrap_err(), crate::AnalysisError::RepeatedPubkeys);
assert!(matches!(
ok_insane.sanity_check().unwrap_err(),
crate::AnalysisError::RepeatedPubkeys
));
// ...it can be lifted, though it's unclear whether this is a deliberate
// choice or just an accident. It seems weird given that duplicate public
// keys are forbidden in several other places.
Expand All @@ -1492,7 +1495,10 @@ mod tests {
"and_v(v:and_v(v:older(4194304),pk(A)),and_v(v:older(1),pk(B)))",
)
.unwrap_err();
assert_eq!(err, Error::AnalysisError(crate::AnalysisError::HeightTimelockCombination));
assert!(matches!(
err,
Error::AnalysisError(crate::AnalysisError::HeightTimelockCombination)
));

// Though you can in an or() rather than and()
let ok_or = Miniscript::<String, Segwitv0>::from_str(
Expand All @@ -1512,10 +1518,10 @@ mod tests {
ok_insane.sanity_check().unwrap_err(),
crate::AnalysisError::HeightTimelockCombination
);
assert_eq!(
assert!(matches!(
ok_insane.lift().unwrap_err(),
Error::LiftError(crate::policy::LiftError::HeightTimelockCombination)
);
));
}

#[test]
Expand Down
Loading