Merge rust-bitcoin/rust-miniscript#733: Remove TranslatePk trait and 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

Author: heap-coder

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>,