Prepare for using new bech32 release #1975
Conversation
tcharding
force-pushed
the
08-04-address-prep
branch
from
f641cd4
to
fb8cb50
Compare
tcharding
added
P-high
| } | ||
| } | ||
|
|
||
| impl From<base58::Error> for ParseError { |
There was a problem hiding this comment.
I don't think it is approriate to create From<base58::Error> for ParseError. Can we wrap this in a struct struct AddrBase58Error(base58::Error). And then in the enum we would have variant as Base58(AddrBase58Error) instead of Base58(base58::Error).
Finally, we can implement From<AddrBase58Error> for ParseError.
There was a problem hiding this comment.
Thanks for the review. I don't understand your concern, can you elaborate please? Why can't we put the base58::Error directly in a variant, this is "standard" error handling boilerplate?
There was a problem hiding this comment.
Consider the following code.
fn foo() -> ParseError {
let sender_name = base58::decode(...)?; // Some other usage of base58 completely unrelated to address module
let addr_parsed = Address::from_str(&base58_str)?; // Error related to base58 in address module.
}In the current implementation, the above code would compile both of them will go into address::ParseError().
But sender name is really not related to address.
There was a problem hiding this comment.
Fair point but this same applies for every error we have, right? Just to clarify foo in your example is a function in downstream code that is returning an error from our code? Or are you saying that we might add foo in rust-bitcoin at some later stage and inadvertently create the situation you describe?
Said another way, I don't think the From imples are the problem, I think the foo() -> ParseError function is buggy because it returns the wrong error type. Am I understanding you correctly?
There was a problem hiding this comment.
Fair point but this same applies for every error we have, right?
Agreed. I no longer have the above concern. Will do a detailed review later.
There was a problem hiding this comment.
No worries, thanks man.
tcharding
force-pushed
the
08-04-address-prep
branch
from
fb8cb50
to
4451b9b
Compare
tcharding
force-pushed
the
08-04-address-prep
branch
from
4451b9b
to
dd1dbb9
Compare
|
Rebase only, no other changes. |
In preparation for splitting out the error code move `address.rs` to `address/mod.rs`. File move only, no other changes.
Split the error code out of `address/mod.rs` and into `address/error.rs`. Code move only, no changes other than to imports/exports etc. to make it build.
`Error` is in this file, no need for `self::Error`.
In an effort to reduce the number of lines of code import the error variants locally within the `Display` impl on `Error`. Refactor only, no logic changes.
There is no need to use the general `address::Error` when parsing an address type, there is only one error path.
The `address::Error` is module level general, we can make the code easier to maintain and easier to stabalize by splitting the parse error out of the general error. Create a `ParseError` that is returned by `FromStr for Address`. Remove the now unused variants from the general `address::Error`.
The BIP-173 and BIP-350 test vectors are implemented in `rust-bech32`, no need to duplicate those tests here.
These docs do not add that much value, we do not typically bother documenting `From` and `TryFrom` implementations because they are super well known and its obvious from the function signature what is going on.
tcharding
force-pushed
the
08-04-address-prep
branch
from
dd1dbb9
to
52f2332
Compare
|
Rebased to pick up recent pinning fix, no other changes. |
|
This PR is basically just refactoring and a some improvements to error handling. It has been open for over a month with an ack from @apoelstra. Other activity was partial review by @sanket1729 and all his concerns were explicitly ok'ed by him above. As such it is a candidate for the "refactor carve-out" and may be merged at any time. |
In preparation for depending on the recently released version of
rust-bech32do a bunch of preparatory fixes.addressmodule error handling as we are doing else where at the momentWitnessVersionThis PR is the first 8 patches of #1951