Merge #733: Remove TranslatePk trait and clean up Translator trait

36b0659 translator: make target pk and error associated types (Andrew Poelstra)
c330d0b remove the TranslatePk trait (Andrew Poelstra)
6e1d6bb descriptor: stop using TranslatePk in mod.rs (Andrew Poelstra)
b80296b descriptor: stop using TranslatePk in tr.rs (Andrew Poelstra)
f89cf25 descriptor: stop using TranslatePk in sh.rs (Andrew Poelstra)
4654b98 descriptor: stop using TranslatePk in segwitv0.rs (Andrew Poelstra)
9c91586 descriptor: stop using TranslatePk in bare.rs (Andrew Poelstra)
e73a141 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 36b0659

Tree-SHA512: 0f7989925af2f9857146eb00e41a81ec1bbaf4c94b0003230835d3979ccb7913d8c958f1293f501e36ddcbaf81e8a88ada261371d9b79c4c85fec3ee195ffc07

Author: apoelstra

bitcoind-tests/tests/setup/test_util.rs

@@ -26,7 +26,7 @@ use bitcoin::secp256k1;
 use miniscript::descriptor::{SinglePub, SinglePubKey};
 use miniscript::{
     bitcoin, hash256, Descriptor, DescriptorPublicKey, Error, Miniscript, ScriptContext,
-    TranslatePk, Translator,
+    Translator,
 };
 use rand::RngCore;
 use secp256k1::XOnlyPublicKey;
@@ -155,8 +155,11 @@ pub fn parse_insane_ms<Ctx: ScriptContext>(
 #[derive(Debug, Clone)]
 struct StrDescPubKeyTranslator<'a>(usize, &'a PubData);
 
-impl<'a> Translator<String, DescriptorPublicKey, ()> for StrDescPubKeyTranslator<'a> {
-    fn pk(&mut self, pk_str: &String) -> Result<DescriptorPublicKey, ()> {
+impl<'a> Translator<String> for StrDescPubKeyTranslator<'a> {
+    type TargetPk = DescriptorPublicKey;
+    type Error = core::convert::Infallible;
+
+    fn pk(&mut self, pk_str: &String) -> Result<Self::TargetPk, Self::Error> {
         let avail = !pk_str.ends_with('!');
         if avail {
             self.0 += 1;
@@ -181,22 +184,22 @@ impl<'a> Translator<String, DescriptorPublicKey, ()> for StrDescPubKeyTranslator
         }
     }
 
-    fn sha256(&mut self, sha256: &String) -> Result<sha256::Hash, ()> {
+    fn sha256(&mut self, sha256: &String) -> Result<sha256::Hash, Self::Error> {
         let sha = sha256::Hash::from_str(sha256).unwrap();
         Ok(sha)
     }
 
-    fn hash256(&mut self, hash256: &String) -> Result<hash256::Hash, ()> {
+    fn hash256(&mut self, hash256: &String) -> Result<hash256::Hash, Self::Error> {
         let hash256 = hash256::Hash::from_str(hash256).unwrap();
         Ok(hash256)
     }
 
-    fn ripemd160(&mut self, ripemd160: &String) -> Result<ripemd160::Hash, ()> {
+    fn ripemd160(&mut self, ripemd160: &String) -> Result<ripemd160::Hash, Self::Error> {
         let ripemd160 = ripemd160::Hash::from_str(ripemd160).unwrap();
         Ok(ripemd160)
     }
 
-    fn hash160(&mut self, hash160: &String) -> Result<hash160::Hash, ()> {
+    fn hash160(&mut self, hash160: &String) -> Result<hash160::Hash, Self::Error> {
         let hash160 = hash160::Hash::from_str(hash160).unwrap();
         Ok(hash160)
     }
@@ -208,8 +211,11 @@ impl<'a> Translator<String, DescriptorPublicKey, ()> for StrDescPubKeyTranslator
 #[derive(Debug, Clone)]
 struct StrTranslatorLoose<'a>(usize, &'a PubData);
 
-impl<'a> Translator<String, DescriptorPublicKey, ()> for StrTranslatorLoose<'a> {
-    fn pk(&mut self, pk_str: &String) -> Result<DescriptorPublicKey, ()> {
+impl<'a> Translator<String> for StrTranslatorLoose<'a> {
+    type TargetPk = DescriptorPublicKey;
+    type Error = core::convert::Infallible;
+
+    fn pk(&mut self, pk_str: &String) -> Result<Self::TargetPk, Self::Error> {
         let avail = !pk_str.ends_with('!');
         if avail {
             self.0 += 1;
@@ -238,22 +244,22 @@ impl<'a> Translator<String, DescriptorPublicKey, ()> for StrTranslatorLoose<'a>
         }
     }
 
-    fn sha256(&mut self, sha256: &String) -> Result<sha256::Hash, ()> {
+    fn sha256(&mut self, sha256: &String) -> Result<sha256::Hash, Self::Error> {
         let sha = sha256::Hash::from_str(sha256).unwrap();
         Ok(sha)
     }
 
-    fn hash256(&mut self, hash256: &String) -> Result<hash256::Hash, ()> {
+    fn hash256(&mut self, hash256: &String) -> Result<hash256::Hash, Self::Error> {
         let hash256 = hash256::Hash::from_str(hash256).unwrap();
         Ok(hash256)
     }
 
-    fn ripemd160(&mut self, ripemd160: &String) -> Result<ripemd160::Hash, ()> {
+    fn ripemd160(&mut self, ripemd160: &String) -> Result<ripemd160::Hash, Self::Error> {
         let ripemd160 = ripemd160::Hash::from_str(ripemd160).unwrap();
         Ok(ripemd160)
     }
 
-    fn hash160(&mut self, hash160: &String) -> Result<hash160::Hash, ()> {
+    fn hash160(&mut self, hash160: &String) -> Result<hash160::Hash, Self::Error> {
         let hash160 = hash160::Hash::from_str(hash160).unwrap();
         Ok(hash160)
     }

examples/big.rs

@@ -18,7 +18,7 @@ use miniscript::policy::{Concrete, Liftable};
 use miniscript::psbt::PsbtExt;
 use miniscript::{
     translate_hash_fail, DefiniteDescriptorKey, Descriptor, DescriptorPublicKey, MiniscriptKey,
-    TranslatePk, Translator,
+    Translator,
 };
 use secp256k1::Secp256k1;
 fn main() {
@@ -82,12 +82,15 @@ struct StrPkTranslator {
     pk_map: HashMap<String, XOnlyPublicKey>,
 }
 
-impl Translator<String, XOnlyPublicKey, ()> for StrPkTranslator {
-    fn pk(&mut self, pk: &String) -> Result<XOnlyPublicKey, ()> {
+impl Translator<String> for StrPkTranslator {
+    type TargetPk = XOnlyPublicKey;
+    type Error = ();
+
+    fn pk(&mut self, pk: &String) -> Result<XOnlyPublicKey, Self::Error> {
         self.pk_map.get(pk).copied().ok_or(())
     }
 
     // We don't need to implement these methods as we are not using them in the policy.
     // Fail if we encounter any hash fragments. See also translate_hash_clone! macro.
-    translate_hash_fail!(String, XOnlyPublicKey, ());
+    translate_hash_fail!(String, XOnlyPublicKey, Self::Error);
 }

examples/taproot.rs

@@ -8,7 +8,7 @@ use miniscript::bitcoin::secp256k1::rand;
 use miniscript::bitcoin::{Network, WitnessVersion};
 use miniscript::descriptor::DescriptorType;
 use miniscript::policy::Concrete;
-use miniscript::{translate_hash_fail, Descriptor, Miniscript, Tap, TranslatePk, Translator};
+use miniscript::{translate_hash_fail, Descriptor, Miniscript, Tap, Translator};
 
 // Refer to https://github.com/sanket1729/adv_btc_workshop/blob/master/workshop.md#creating-a-taproot-descriptor
 // for a detailed explanation of the policy and it's compilation
@@ -17,14 +17,17 @@ struct StrPkTranslator {
     pk_map: HashMap<String, XOnlyPublicKey>,
 }
 
-impl Translator<String, XOnlyPublicKey, ()> for StrPkTranslator {
-    fn pk(&mut self, pk: &String) -> Result<XOnlyPublicKey, ()> {
+impl Translator<String> for StrPkTranslator {
+    type TargetPk = XOnlyPublicKey;
+    type Error = ();
+
+    fn pk(&mut self, pk: &String) -> Result<XOnlyPublicKey, Self::Error> {
         self.pk_map.get(pk).copied().ok_or(())
     }
 
     // We don't need to implement these methods as we are not using them in the policy.
     // Fail if we encounter any hash fragments. See also translate_hash_clone! macro.
-    translate_hash_fail!(String, XOnlyPublicKey, ());
+    translate_hash_fail!(String, XOnlyPublicKey, Self::Error);
 }
 
 fn main() {

src/descriptor/bare.rs

@@ -23,7 +23,7 @@ use crate::prelude::*;
 use crate::util::{varint_len, witness_to_scriptsig};
 use crate::{
     BareCtx, Error, ForEachKey, FromStrKey, Miniscript, MiniscriptKey, Satisfier, ToPublicKey,
-    TranslateErr, TranslatePk, Translator,
+    TranslateErr, Translator,
 };
 
 /// Create a Bare Descriptor. That is descriptor that is
@@ -92,6 +92,14 @@ impl<Pk: MiniscriptKey> Bare<Pk> {
         let scriptsig_len = self.ms.max_satisfaction_size()?;
         Ok(4 * (varint_len(scriptsig_len) + scriptsig_len))
     }
+
+    /// Converts the keys in the script from one type to another.
+    pub fn translate_pk<T>(&self, t: &mut T) -> Result<Bare<T::TargetPk>, TranslateErr<T::Error>>
+    where
+        T: Translator<Pk>,
+    {
+        Bare::new(self.ms.translate_pk(t)?).map_err(TranslateErr::OuterError)
+    }
 }
 
 impl<Pk: MiniscriptKey + ToPublicKey> Bare<Pk> {
@@ -190,21 +198,6 @@ impl<Pk: MiniscriptKey> ForEachKey<Pk> for Bare<Pk> {
     }
 }
 
-impl<P, Q> TranslatePk<P, Q> for Bare<P>
-where
-    P: MiniscriptKey,
-    Q: MiniscriptKey,
-{
-    type Output = Bare<Q>;
-
-    fn translate_pk<T, E>(&self, t: &mut T) -> Result<Bare<Q>, TranslateErr<E>>
-    where
-        T: Translator<P, Q, E>,
-    {
-        Bare::new(self.ms.translate_pk(t)?).map_err(TranslateErr::OuterError)
-    }
-}
-
 /// A bare PkH descriptor at top level
 #[derive(Clone, Ord, PartialOrd, Eq, PartialEq, Hash)]
 pub struct Pkh<Pk: MiniscriptKey> {
@@ -260,6 +253,18 @@ impl<Pk: MiniscriptKey> Pkh<Pk> {
         note = "Use max_weight_to_satisfy instead. The method to count bytes was redesigned and the results will differ from max_weight_to_satisfy. For more details check rust-bitcoin/rust-miniscript#476."
     )]
     pub fn max_satisfaction_weight(&self) -> usize { 4 * (1 + 73 + BareCtx::pk_len(&self.pk)) }
+
+    /// Converts the keys in a script from one type to another.
+    pub fn translate_pk<T>(&self, t: &mut T) -> Result<Pkh<T::TargetPk>, TranslateErr<T::Error>>
+    where
+        T: Translator<Pk>,
+    {
+        let res = Pkh::new(t.pk(&self.pk)?);
+        match res {
+            Ok(pk) => Ok(pk),
+            Err(e) => Err(TranslateErr::OuterError(Error::from(e))),
+        }
+    }
 }
 
 impl<Pk: MiniscriptKey + ToPublicKey> Pkh<Pk> {
@@ -391,22 +396,3 @@ impl<Pk: FromStrKey> core::str::FromStr for Pkh<Pk> {
 impl<Pk: MiniscriptKey> ForEachKey<Pk> for Pkh<Pk> {
     fn for_each_key<'a, F: FnMut(&'a Pk) -> bool>(&'a self, mut pred: F) -> bool { pred(&self.pk) }
 }
-
-impl<P, Q> TranslatePk<P, Q> for Pkh<P>
-where
-    P: MiniscriptKey,
-    Q: MiniscriptKey,
-{
-    type Output = Pkh<Q>;
-
-    fn translate_pk<T, E>(&self, t: &mut T) -> Result<Self::Output, TranslateErr<E>>
-    where
-        T: Translator<P, Q, E>,
-    {
-        let res = Pkh::new(t.pk(&self.pk)?);
-        match res {
-            Ok(pk) => Ok(pk),
-            Err(e) => Err(TranslateErr::OuterError(Error::from(e))),
-        }
-    }
-}

src/descriptor/key.rs

@@ -1003,9 +1003,7 @@ impl DefiniteDescriptorKey {
     /// always return a compressed key
     ///
     /// Will return an error if the descriptor key has any hardened derivation steps in its path. To
-    /// avoid this error you should replace any such public keys first with [`translate_pk`].
-    ///
-    /// [`translate_pk`]: crate::TranslatePk::translate_pk
+    /// avoid this error you should replace any such public keys first with [`crate::Descriptor::translate_pk`].
     pub fn derive_public_key<C: Verification>(
         &self,
         secp: &Secp256k1<C>,