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.
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
Improve SchnorrSigHashType
#903
Improve SchnorrSigHashType
#903
Conversation
As is done in the rest of the `internal_macros` module use the fully qualified path for the `String` type. Done in preparation for using `serde_string_impl` in the `sighash` module.
86e0cb9
to
1f70983
Compare
In preparation for constructing an error outside of this module improve the `SigHashTypeParseError` by doing: - Make the field public - Rename the field to `unrecognized` to better describe its usage
1f70983
to
e04eb11
Compare
We currently implement `Display` and `FromStr` on `EcdsaSigHashType` and use them in the `serde_string_impl` macro to implement ser/de. Mirror this logic in `SchnorrSigHashType`.
e04eb11
to
35b682d
Compare
All the force pushes are me finally working out that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 35b682d. Thanks, much easier to review now that the diff is small
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 35b682d
} | ||
|
||
impl fmt::Display for SigHashTypeParseError { | ||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
write!(f, "can't recognize SIGHASH string '{}'", self.string) | ||
write!(f, "Unrecognized SIGHASH string '{}'", self.unrecognized) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: error strings usually starts with small letter, since they are appended after Error:
when printed out by default printer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet, I was wondering this. I'll rectify this as I do other clean ups.
@@ -1154,7 +1155,7 @@ mod tests { | |||
"SigHash_NONE", | |||
]; | |||
for s in sht_mistakes { | |||
assert_eq!(EcdsaSigHashType::from_str(s).unwrap_err().to_string(), format!("can't recognize SIGHASH string '{}'", s)); | |||
assert_eq!(EcdsaSigHashType::from_str(s).unwrap_err().to_string(), format!("Unrecognized SIGHASH string '{}'", s)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit as above
Implement Display/FromStr for SchnorrSigHashType
We currently implement
Display
andFromStr
onEcdsaSigHashType
and use them in theserde_string_impl
macro to implement ser/de.Mirror this logic in
SchnorrSigHashType
.Patch 1 and 2 are preparatory patches for patch 3.
Notes to reviewers
This PR has some conflicts with #898 but is pushing in the same direction, I'm happy to let 898 go in first and rebase on top.