10000 Add custom error for invalid parsing of address types by nph4rd · Pull Request #1091 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add custom error for invalid parsing of address types #1091

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

Jump to bottom
Merged
merged 1 commit into from
Jul 12, 2022
Merged

Add custom error for invalid parsing of address types #1091

merged 1 commit into from
Jul 12, 2022

Conversation

nph4rd
Copy link
Contributor
@nph4rd nph4rd commented Jul 10, 2022 •
edited
Loading

Adds a custom error UnknownAddressType(_) which is returned in cases where an unknown address type tries to be parsed via
FromStr. This provides more context for the error, since it contains the String that tried to be parsed.

Closes #1064

@nph4rd nph4rd mentioned this pull request Jul 10, 2022
Closed
tcharding
tcharding reviewed Jul 11, 2022
View reviewed changes
Copy link
Member
@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! High level it looks nice, I suggested a few minor improvements.

src/util/address.rs
Err(Error::InvalidAddressType("invalid".to_string()))
);
}

Copy link
Member
@tcharding tcharding Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Admittedly our test practices are a bit loosey-goosey but this one could be made super clean by doing

    #[test]
    fn valid_address_parses_correctly() {
        let addr = AddressType::from_str("p2tr").expect("false negative while parsing address");
        assert_eq!(addr, AddressType::P2tr);
    }

    #[test]
    fn invalid_address_parse_error() {
        let got = AddressType::from_str("invalid");
        let want = Err(Error::InvalidAddressType("invalid".to_string()));
        assert_eq!(got, want);
    }

Copy link
Member

Choose a reason for hiding this comment

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

Bad github-foo, I did not add enough context when doing this suggestion, my bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got you! Yeah, this is done too.

Copy link
Contributor Author
@nph4rd nph4rd Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I was actually thinking about iterating over all of the valid address types in the test, but didn't do it in the end because I thought it to be slightly over-kill. Wdyt? I've left it as you suggested, but happy to add that if it seems worth it! :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tempting but not sure if worth it either.

@nph4rd
Copy link
Contributor Author
nph4rd commented Jul 11, 2022

@tcharding - cheers for the review! That was quick!

I've made the changes you requested 👍 It should look better now!

@nph4rd nph4rd requested a review from tcharding July 11, 2022 02:15
@tcharding
Copy link
Member
tcharding commented Jul 11, 2022 •
edited
Loading

Sweet, thanks mate. Can you squash it all down into a single commit and force push please. In general if you make changes to a PR after review that effect a previous commit in the PR then you can just squash it. Unless there is some extra reason not to.

Kixunil
Kixunil reviewed Jul 11, 2022
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Looks good. Some bike shedding: maybe UnknownAddressType is better because there could be a new one in the future and it wouldn't be invalid, just not known to this library.

src/util/address.rs
Err(Error::InvalidAddressType("invalid".to_string()))
);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tempting but not sure if worth it either.

@nph4rd
Copy link
Contributor Author
nph4rd commented Jul 11, 2022

maybe UnknownAddressType is better because there could be a new one in the future and it wouldn't be invalid, just not known to this library

Good point @Kixunil! Perhaps the display for the error should also be changed from: "invalid address type: '{}'" to something like: "unknown address type: '{}' is either invalid or not supported in rust-bitcoin".

What do you think about this?

@nph4rd
Copy link
Contributor Author
nph4rd commented Jul 11, 2022 •
edited
Loading

Can you squash it all down into a single commit and force push please. In general if you make changes to a PR after review that effect a previous commit in the PR then you can just squash it

@tcharding - Of course! Will do, after addressing @Kixunil's change requests. Thanks for the tip 👍

@nph4rd
Add custom error for unknown address type parsing
19ba7ec 
Adds a custom error `UnknownAddressType(_)` which is returned in
cases where an unknown address type tries to be parsed via
`FromStr`. This provides more context for the error.

For more info see [1].

[1] - `https://github.com/rust-bitcoin/rust-bitcoin/issues/1064`
@nph4rd nph4rd force-pushed the add-invalid-address-type-err branch from 0f85476 to 19ba7ec Compare July 11, 2022 14:00
@nph4rd
Copy link
Contributor Author
nph4rd commented Jul 11, 2022

Force-pushed to 19ba7ec. This includes the changes requested by @tcharding; namely:

  • Removal of unnecessary &.
  • Re-ordering in the Error impl.
  • Split of the test.

Also, added some new changes requested by @Kixunil:

  • Replaced to_string with to_owned().
  • Renamed error from InvalidAddressType to UnknownAddressType and changed the relevant comments. For example, display is now: "unknown address type: '{}' is either invalid or not supported in rust-bitcoin"

@nph4rd nph4rd requested a review from Kixunil July 11, 2022 14:06
Kixunil
Kixunil approved these changes Jul 11, 2022
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Perfect!

ACK 19ba7ec

@Kixunil Kixunil added API break This PR requires a version bump for the next release one ack PRs that have one ACK, so one more can progress them error handling Issues and PRs related to error handling, mainly error refactoring epic labels Jul 11, 2022
dunxen
dunxen approved these changes Jul 11, 2022
View reviewed changes
Copy link
Contributor
@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

ACK 19ba7ec

tcharding
tcharding approved these changes Jul 11, 2022
View reviewed changes
Copy link
Member
@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 19ba7ec

apoelstra
apoelstra approved these changes Jul 12, 2022
View reviewed changes
Copy link
Member
@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 19ba7ec

@apoelstra apoelstra merged commit 3f27131 into rust-bitcoin:master Jul 12, 2022
ChallengeDev210 pushed a commit to ChallengeDev210/rust-bitcoin that referenced this pull request Aug 1, 2022
@apoelstra
Merge rust-bitcoin/rust-bitcoin#1091: Add custom error for invalid pa…
3a338a1 
…rsing of address types

19ba7ec Add custom error for unknown address type parsing (Arturo Marquez)

Pull request description:

  Adds a custom error `UnknownAddressType(_)` which is returned in cases where an unknown address type tries to be parsed via
  `FromStr`. This provides more context for the error, since it contains the `String` that tried to be parsed.

  Closes rust-bitcoin/rust-bitcoin#1064

ACKs for top commit:
  Kixunil:
    ACK 19ba7ec
  dunxen:
    ACK 19ba7ec
  tcharding:
    ACK 19ba7ec
  apoelstra:
    ACK 19ba7ec

Tree-SHA512: 2f94eb2e122c1acafcb8c852f7bfe22cb3725806541ae40f82d3a882011fb911ce8fc430153ebea4066f43c5a51813359a4c9b95d2a9077ce8f6dcaa58eee6fd
@Kixunil Kixunil added this to the 0.29.0 milestone Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil approved these changes

@apoelstra apoelstra apoelstra approved these changes

@dunxen dunxen dunxen approved these changes

@tcharding tcharding tcharding approved these changes

Assignees
No one assigned
Labels
API break This PR requires a version bump for the next release error handling Issues and PRs related to error handling, mainly error refactoring epic one ack PRs that have one ACK, so one more can progress them
Projects
None yet
Milestone
0.29.0
Development

Successfully merging this pull request may close these issues.

AddressType FromStr impl returns bad error type
5 participants
@nph4rd @tcharding @Kixunil @apoelstra @dunxen
0