update TranslatePk again #493
Conversation
|
These look great! Only nits are that I think in a couple cases you could've cleaned up some I'm not thrilled about some of the |
Perhaps, it might be time for clippy in rust-miniscript? It is great is suggesting idiomatic things when possible. Given that it is already existing in rust-bitcoin, we can add it here. |
sanket1729
force-pushed
the
translate_err
branch
from
2b40679 to
dc01037
Compare
|
Rebased. But based on #537 |
|
Rebased after #537 |
src/lib.rs
Outdated
| /// # Errors | ||
| /// | ||
| /// This function will return an error if the Error is OutError. | ||
| pub fn try_into_translator_err(self) -> Result<E, Self> { |
There was a problem hiding this comment.
In a3019c7:
Since you exclusively use this function to assert, I think you should just call it unwrap_translator_err and have it panic if it gets an OuterError. I find the current name try_into_translator_err hard to understand, and each time you call it you've got a separate variant of expect("outer errors can't happen") which is also hard to understand.
There was a problem hiding this comment.
I think the point of try into is to explicitly have this message. There are different reasons why this might not fail. BIP32 derivations/translations we know that are not going to be fail etc.
And having a string message as a small comment is helpful rather than unwrap thing because it at least forces me to think and write why it might not fail
There was a problem hiding this comment.
Can we call it expect_translator_err and take a string?
|
ack a3019c7 except for the one comment. |
|
Added one commit to address the issue. |
Miniscript should only be called with two constructors. from_ast and from_components_unchecked
Each context has slightly different rules on what Pks are allowed in descriptors Legacy/Bare does not allow x_only keys SegwitV0 does not allow uncompressed keys and x_only keys Tapscript does not allow uncompressed keys Note that String types are allowed everywhere
This fixes of the long last standing bug in rust-miniscript that allowed creating unsound miniscript using the translate APIs
|
Fixed and force pushed |
b90cdda Change try_into_translator_err to expect_translator_err (sanket1729) be07a9c Update TranslatePk again (sanket1729) ee8de9a Cleanly check validity rules for DescriptorKeys (sanket1729) fa2e8b4 Cleanup Miniscript constructors (sanket1729) Pull request description: This PR has lot of cleanups that I had planned but did not have time for a long time. - All places now use constructors that do invariant checking on the struct being created. - Translate* APIs had a soundness bug you could create uncompressed keys in segwit descriptors etc. ACKs for top commit: apoelstra: ACK b90cdda Tree-SHA512: d1c7001650b55c018d6e395628cb3cb8e5d15540c24b696946d370202e6ccc82b376b7f18c1138fdbd83a550fef0e39dc8b44fd670019dc4854b87a67cdc41c7
This PR has lot of cleanups that I had planned but did not have time for a long time.