Remove TranslatePk trait and clean up Translator trait
#733
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is the last of the uses of TranslatePk.
sanket1729
previously approved these changes
Aug 30, 2024
There was a problem hiding this comment.
ACK 269f44d.
Definitely makes the translate API easier to use.
|
@apoelstra, the integration tests also need to be updated. |
apoelstra
force-pushed
the
2024-08--translate-pk
branch
from
269f44d
to
380a0d3
Compare
|
Current CI failure is because of bad doctest links. My local CI should be catching this. Will investigate.. |
|
Got it. I was forcing Will confirm that my local checks fail here, and then fix the PR. |
We actually just empty it and deprecate it in an attempt to minimize breakage. People *implementing* the trait will break anyway because the next commit will have me changing the Translator trait. But the typical case is that somebody imports the trait, then calls .translate_pk on some object, and we want that usecase to continue working with only warnings.
This is an annoying breaking change for users of the Translator trait but I think it greatly improves the ergonomics of using the trait. Rather than having it be parameterized over 3 types, it is now parameterized over just one (the "source pk type"). This matches how this trait is used in practice -- you typically have a miniscript/policy/whatever with a keytype Pk, and you want to use a translator from Pk to "whatever the translator maps to" with "whatever error the translator yields". So the only type parameter you really need to type is Pk; the others are irrelevant, and making the user name and type them is annoying. Since this eliminates the need to explicitly write out the error types except when actually implementing the trait, this also changes a ton of error types from () to Infallible, which is more efficient and correct.
apoelstra
force-pushed
the
2024-08--translate-pk
branch
from
380a0d3
to
36b0659
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This may be too much of a breaking change and an instance of rust-bitcoin/rust-bitcoin#3166, but the existing traits are really annoying to use. But this change is not essential to any of my other work so feel free to concept NACK it.
There are two changes here:
TranslatePktrait which had no business existing. It attempts to be generic over "things whose keys can be translated", but is missing impls (on keys themselves, for example) and anyway it is basically impossible to usefully be generic over this trait since it has an associatedOutputtype which is unconstrained except by a comment saying "this must beSelf<Q>. Really, the only purpose of this trait is to force users to writeuse miniscript::TranslatePkevery time they want access to thetranslate_pkmethod.Translatorfrom generics to associated types. This makes it far more ergonomic to implement and require the trait, since you no longer need to write 3 separate generics everywhere when two are implied. (Actually, all three are implied, including the sourcePktype, but in practice users want to constrain this to match an existing key type that they've got. So I left it as a generic rather than an associated type.)