taproot: Split errors up#2886
Conversation
|
In 1be14146dcef2c306b5d6b5d7390fb80047088dd Both |
|
989595b270aa4808905fbf66ca2812bbf842ed51 looks good otherwise! Definitely an improvement. |
989595b to
dc46eba
Compare
|
Hey @jamillambert this is a pretty standard error cleanup PR, if you are interested in the direction we are going with regards to error handling this PR would be a good one to review and understand. If you find error handling boring then don't sweat it, its mainly interesting as a pure engineering problem because there is a whole bunch of nuance in how the Rust language treats enums, enum variant fields, and |
dc46eba to
005f566
Compare
I was/am unsure whether it should be "Merkle tree" or "Merkle Tree" - I went with the later. Fixed also "Huffman Tree". Please note there is one error message changed in the first patch to remove the two sentences. |
In the docstring can be "Merkle tree" (see Wikipedia - Merkle tree for example). |
jamillambert
left a comment
There was a problem hiding this comment.
I am not sure I understand completely why only 2 of the 5 TaprootError variants were split out. Is it because of the functions that only return one of these, whereas the other variants are returned by functions that can return other variants too and therefore need to be general?
But in terms of achieving what is described I don't see any issues.
bitcoin/src/taproot/mod.rs
Outdated
There was a problem hiding this comment.
Should 'tap tree' be 'Taproot tree'?
There was a problem hiding this comment.
Either that or "taptree" which is one word.
bitcoin/src/taproot/mod.rs
Outdated
There was a problem hiding this comment.
Missed capitalization of Taproot
There was a problem hiding this comment.
Legend, thanks. Will fix.
Yes that is correct. The idea, and if you notice this violated anywhere it should be fixed, is that for every function that returns an enum error type all variants of the error type are returned by the function. This is something that we have been working towards over the last year or so, it is highly likely there are still some that need fixing. |
005f566 to
a4ea84e
Compare
|
I've dropped the first patch that was attempting to fix the capitalization of error messages because there are a whole bunch of anomalies in the |
|
Added #2913 to track the spelling/capitalization stuff that was dropped. |
Kixunil
left a comment
There was a problem hiding this comment.
Other than the doc comment issue ACK 12baa0b891. Feel free to one-ACK after it's fixed without other changes.
bitcoin/src/taproot/mod.rs
Outdated
There was a problem hiding this comment.
Unrelated but I think error messages should say what's wrong not how it should be right.
There was a problem hiding this comment.
Ah, interesting. I think all the error messages in general need work but definitely the ones in taproot.
bitcoin/src/taproot/mod.rs
Outdated
There was a problem hiding this comment.
Might be nice to newtype the other variants too.
There was a problem hiding this comment.
Added an additional patch, good reviewing - thanks.
bitcoin/src/taproot/mod.rs
Outdated
There was a problem hiding this comment.
This doc comment is unrelated.
Currently there are a couple of errors in the `taproot` module that are too general, resulting in functions that return a general error type when a specific one would do. Split two errors out and use them for for enum variants and function returns as possible.
Add two more error types so that the `TaprootError` has all its variants strongly typed.
The formatter doesn't touch this line but its not uniform with the surrounding code.
a4ea84e to
0c9223a
Compare
|
Needs ack from @Kixunil please because changes since last ack are non-trivial. No rush on this one its just error handling stuff. |
Currently there are a couple of errors in the
taprootmodule that are too general, resulting in functions that return a general error type when a specific one would do.Split two errors out and use them for for enum variants and function returns as possible.
Done as part of #2883