Remove TranslatePk trait and clean up Translator trait
#733
Conversation
This file contains hidden or 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
Member
sanket1729
left a comment
sanket1729
left a comment
There was a problem hiding this comment.
ACK 269f44d.
Definitely makes the translate API easier to use.
Member
|
@apoelstra, the integration tests also need to be updated. |
apoelstra
force-pushed
the
2024-08--translate-pk
branch
from
269f44d to
380a0d3
Compare
Member
Author
|
Current CI failure is because of bad doctest links. My local CI should be catching this. Will investigate.. |
Member
Author
|
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
heap-coder
added a commit
to heap-coder/rust-miniscript
that referenced
this pull request
Sep 27, 2025
…d clean up `Translator` trait 36b065981a10c30f37548f5df792885d5397730f translator: make target pk and error associated types (Andrew Poelstra) c330d0b37adb8e07dda75ab41cd57fa15728bbfc remove the TranslatePk trait (Andrew Poelstra) 6e1d6bbc9cb000f8473ab62017912f316740b1c8 descriptor: stop using TranslatePk in mod.rs (Andrew Poelstra) b80296b1a7943be8f51f27673ac818e6c623ff60 descriptor: stop using TranslatePk in tr.rs (Andrew Poelstra) f89cf257564fd4c0c0e41742efc62b7fbb739eb7 descriptor: stop using TranslatePk in sh.rs (Andrew Poelstra) 4654b98d9d5e90083d6cf01d718d1df7d3302b1e descriptor: stop using TranslatePk in segwitv0.rs (Andrew Poelstra) 9c91586d2668785cf601e13458260be49e3628e1 descriptor: stop using TranslatePk in bare.rs (Andrew Poelstra) e73a141dd69e7a409419ee78d61023399ea71c04 miniscript: stop using TranslatePk (Andrew Poelstra) Pull request description: 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: * This drops the `TranslatePk` trait 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 associated `Output` type which is unconstrained except by a comment saying "this must be `Self<Q>`. Really, the only purpose of this trait is to force users to write `use miniscript::TranslatePk` every time they want access to the `translate_pk` method. * It moves two of the generics in `Translator` from 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 source `Pk` type, 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.) ACKs for top commit: sanket1729: ACK 36b065981a10c30f37548f5df792885d5397730f Tree-SHA512: 0f7989925af2f9857146eb00e41a81ec1bbaf4c94b0003230835d3979ccb7913d8c958f1293f501e36ddcbaf81e8a88ada261371d9b79c4c85fec3ee195ffc07
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.)