Fix types conversion error variants#522
Fix types conversion error variants#522jamillambert wants to merge 2 commits intorust-bitcoin:masterfrom
Conversation
types/src/v17/mining/error.rs
Outdated
| /// Conversion of the `hash` field failed. | ||
| Hash(hex::HexToArrayError), | ||
| /// Conversion of the `wtxid` field failed. | ||
| Wtxid(hex::HexToArrayError), |
There was a problem hiding this comment.
This one is interesting. We re-name the hash field to wtixid in model. I re-checked and believe that is the correct thing to do but I'm not sure about the error variant re-name. Since its done already I guess its ok but it is introducing a precedent [0]. At a minimum we probably should update the docs to be:
/// Conversion of the `hash` (to `wtxid`) field failed.
And likewise in the Display impl output.
[0] Or do we do this already in other places?
There was a problem hiding this comment.
I changed it back to hash and updated the docs and Display message. I also did this for tx to unsigned_tx in raw_transaction.
types/src/v17/mining/into.rs
Outdated
| consensus::encode::deserialize_hex::<Transaction>(&self.data).map_err(E::Data)?; | ||
| let txid = self.txid.parse::<Txid>().map_err(E::Txid)?; | ||
| let wtxid = self.hash.parse::<Wtxid>().map_err(E::Hash)?; | ||
| let wtxid = self.hash.parse::<Wtxid>().map_err(E::Wtxid)?; |
There was a problem hiding this comment.
This is jarring to read (for me personally) now because my eyes expect self.hash to map to E::Hash. Just a comment, I'm open to retraining my eyes.
There was a problem hiding this comment.
Changed to hash with an updated message.
|
I'm ok to merge as is, will wait for you to respond before doing so. |
jamillambert
force-pushed
the
0226-error-variants
branch
from
43ea5f0 to
008117d
Compare
The error types for `hidden` v30 were exact copies of the v29 types. Remove the `Error` module from v30 and reuse the v29 types instead.
Each error variant should be for an individual field. Add new variants where one variant was used for more than one field. Rename existing variants and update error messages to match the field names.
jamillambert
force-pushed
the
0226-error-variants
branch
from
008117d to
9ca8e04
Compare
|
I changed the error variant names to match the version specific type instead of the model type. I also updated the docs and Display messages for the ones that change name. This is not consistent in all RPCs, opened #526 to follow up. I also noticed an error that was redefined in v30 but there were no changes from v29 so I removed the v30. |
2 participants
Each error variant should be for an individual field. Use an LLM to go through all into.rs files to address the issue, spend longer than doing the job from scratch myself fixing all its errors.
Closes #521