Rework translate API to allow associated types to be translated #426

sanket1729 · 2022-06-05T23:52:01Z

The Translate API currently does not support translating the hashes and timelocks. Ideally, we want to write an HTLC as or( and( alice_key, older(timeout), and(bob_key, sha256(hash))).
But it is impossible to use placeholders for hash, and timelock. It is only possible for keys.

This is a significant API change, but I think it is for good. The diff is large, but reviewing lib.rs is the main diff, other diffs are just chasing compiler errors

sanket1729 · 2022-06-06T06:02:57Z

@tcharding, you may be interested in the latest commit. rustfmt on nightly suggested new changes all of a sudden. Both the tips HEAD and HEAD~1 from "Misc rustfmt check" 2e9bd32 pass the cargo +nightly fmt --all -- --check

apoelstra

utACK 2e9bd32

apoelstra · 2022-06-06T18:43:50Z

As discussed in another channel, I am happy with this approach. I agree the Translator trait might involve some boilerplate for a lot of users, but the PkTranslate etc ones help with this, and anyway the existing API also involves boilerplate, often with crazy inscrutable syntax like <_, _, hash160::Hash, _> and with names like translate_pk1 translate_pk2 etc.

tcharding

Nice work bro! I love the simplification of all the translate traits. I had a good long play with this. I pushed 5 patches to a branch on my fork with the same name rework_translate. Feel free to use them, ignore them, or steal whatever parts you like. 4 of them are trivial, docs and things. In one patch I remove the PkTranslator trait, includes full justification in the commit log.

Cheers

tcharding · 2022-06-06T23:41:34Z

src/macros.rs

+    };
+}
+
+/// Macro for implementing FromTree trait. This avoids copying all the Pk::Associated type bounds


Suggested change

/// Macro for implementing FromTree trait. This avoids copying all the Pk::Associated type bounds

/// Macro for implementing FromStr trait. This avoids copying all the Pk::Associated type bounds

If we want to be super fussy could add code backticks also to this and the comment on impl_from_tree.

tcharding · 2022-06-06T23:44:02Z

src/macros.rs

+        impl<Pk $(, $gen)*> core::str::FromStr for $name
+        where
+            Pk: MiniscriptKey + core::str::FromStr,
+            Pk::Hash : core::str::FromStr,


Beep deep dot, I'm a bot, filling in for rustfmt :)

Suggested change

Pk::Hash : core::str::FromStr,

Pk::Hash: core::str::FromStr,

Here and in the other macro too.

tcharding · 2022-06-07T00:00:08Z

src/test_utils.rs

+/// Translate from a String MiniscriptKey type to bitcoin::PublicKey
+/// If the hashmap is populated, this will lookup for keys in HashMap
+/// Otherwise, this will return a translation to a random key
+/// Only used in testing


This line is a bit redundant, this whole module is only used in testing.

tcharding · 2022-06-07T00:01:26Z

src/test_utils.rs

+    }
+
+    fn f_sha256(&mut self, _sha256: &String) -> Result<sha256::Hash, ()> {
+        // hard coded value


This comment does not add any value :)

sanket1729 · 2022-06-07T02:12:53Z

@tcharding, I looked at the branch. I will pick the four commits modulo but the one that removes the PkTranslator one. Soon, we will also have associated types for Sha256, Hash256, Ripemd160, Hash160, Older, After. After this, PkTranslator API will have many functions and the PkTranslator would make more sense.

The purpose of the macros is to avoid 20-line pre-conditions on each function. Every time we add a new associated type, I needed to add the bounds in multiple places across many files.

tcharding · 2022-06-07T03:38:56Z

No sweat, cheers.

sanket1729 · 2022-06-07T04:32:06Z

Thanks a lot for the suggestions and improvements. Will address them shortly.

sanket1729 · 2022-06-07T20:12:32Z

Addressed all suggestions and cherry-picked the 4 commits as mentioned above.

tcharding

I like the result of these macros, not having the trait bounds everywhere, but they are not pretty. I had a play around and could not come up with anything better though. Someone with better macro skills than me can probably get rid of the trailing comma requirements. A couple of meta comments:

The changes in commit f483089 Fix some macro bugs for 1.41.1 are not specific to Rust 1.41.1 as the title suggests, probably should be squashed.
Commit 2ffa5ea Rename Sha256Hash type to Sha256 could be squashed
All the commits I did can be squashed into the original work, I don't need attribution
The rustfmt change would be cleaner at the front of the PR because its totally unrelated

So I'd probably expect this PR to be three patches

The rustfmt one
Introduce impl macros
Introduce Translator trait

tcharding · 2022-06-07T22:47:15Z

src/descriptor/tr.rs

-    fn parse_tr_script_spend(tree: &expression::Tree) -> Result<TapTree<Pk>, Error> {
+    fn parse_tr_script_spend(tree: &expression::Tree,) -> Result<TapTree<Pk>, Error> {


It would be nice if we could work out how to make the arg list not require a trailing comma, my macro-foo is not strong enough though.

I also tried and couldn't do it.

Should be possible. Will work on it

tcharding · 2022-06-07T23:18:17Z

src/macros.rs

+    };
+}
+
+/// Macro for implementing FromTree trait. This avoids copying all the Pk::Associated type bounds


This comment has cut'n'paste glitch - FromTree

apoelstra

ACK a086861

Maybe rustfmt nightly changed something. But it looks like both versions are accepted by --check check

It is not possible to add a supertrait(and a blanket implemnetation) that captures associated types currently. The where clauses must be carried around everywhere. Use a macro instead

Overhaul TranslatePk triat This is merely fixing compiler errors and creating structs wherever necessary. This in preperation to allow associated enums for Hashes and Timelocks. Users can then write "hash(H)" and translate API(TBD in future commits will deal with it) Also adds fields for Sha256 associated data

sanket1729 · 2022-06-08T23:57:15Z

Squashed and force pushed. Only one line diff fixing the nit. @apoelstra

sanket1729:~/rust-miniscript$ git diff 8e5b09ba3b47b99d9e44e389a3b81adcb5580aed a086861
diff --git a/src/macros.rs b/src/macros.rs
index 7f59aa9..066beae 100644
--- a/src/macros.rs
+++ b/src/macros.rs
@@ -72,7 +72,7 @@ macro_rules! impl_from_str {
     };
 }
 
-/// Macro for impl Struct with associated bounds. This avoids copying all the Pk::Associated type bounds
+/// Macro for implementing FromTree trait. This avoids copying all the Pk::Associated type bounds
 /// throughout the codebase.
 macro_rules! impl_block_str {
     ($(;$gen:ident; $gen_con:ident, )* $name: ty,

tcharding

ACK 8e5b09b

tcharding · 2022-06-09T05:20:51Z

Its clean now! Thanks for putting up with my picky reviews :)

apoelstra

ACK 8e5b09b

This is again a breaking change because we remove the trait impl from `Terminal`, but again I don't think the trait impl should've existed. This also exposed an "off-label" use of `real_translate_pk` to convert context types as well as pk types, which I apparently explicitly reviewed and ACKed when reviewing rust-bitcoin#426, but which in retrospect looks a little funny. Rename this function to translate_pk_ctx so it's clear that it's doing two jobs.

sanket1729 force-pushed the rework_translate branch 2 times, most recently from 397b38e to acbef2f Compare June 6, 2022 05:44

apoelstra previously approved these changes Jun 6, 2022

View reviewed changes

tcharding reviewed Jun 7, 2022

View reviewed changes

sanket1729 dismissed apoelstra’s stale review via 34a3e18 June 7, 2022 19:44

sanket1729 force-pushed the rework_translate branch from 34a3e18 to a086861 Compare June 7, 2022 20:11

tcharding reviewed Jun 7, 2022

View reviewed changes

apoelstra previously approved these changes Jun 8, 2022

View reviewed changes

sanket1729 dismissed apoelstra’s stale review via b7899b6 June 8, 2022 23:53

sanket1729 force-pushed the rework_translate branch from a086861 to b7899b6 Compare June 8, 2022 23:53

Misc rustfmt change

c43129c

Maybe rustfmt nightly changed something. But it looks like both versions are accepted by --check check

sanket1729 force-pushed the rework_translate branch from b7899b6 to 4193e5a Compare June 8, 2022 23:54

sanket1729 added 2 commits June 8, 2022 16:56

Add impl_from_tree!

851120e

It is not possible to add a supertrait(and a blanket implemnetation) that captures associated types currently. The where clauses must be carried around everywhere. Use a macro instead

sanket1729 force-pushed the rework_translate branch from 4193e5a to 8e5b09b Compare June 8, 2022 23:56

tcharding approved these changes Jun 9, 2022

View reviewed changes

apoelstra approved these changes Jun 9, 2022

View reviewed changes

sanket1729 merged commit fce14d0 into rust-bitcoin:master Jun 9, 2022

sanket1729 mentioned this pull request Jun 10, 2022

Fix TapTree depth check bug #429

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework translate API to allow associated types to be translated #426

Rework translate API to allow associated types to be translated #426

sanket1729 commented Jun 5, 2022

sanket1729 commented Jun 6, 2022

apoelstra left a comment

apoelstra commented Jun 6, 2022

tcharding left a comment •

edited

Loading

tcharding Jun 6, 2022

tcharding Jun 6, 2022

tcharding Jun 7, 2022

tcharding Jun 7, 2022

sanket1729 commented Jun 7, 2022 •

edited

Loading

tcharding commented Jun 7, 2022

sanket1729 commented Jun 7, 2022

sanket1729 commented Jun 7, 2022

tcharding left a comment

tcharding Jun 7, 2022

apoelstra Jun 8, 2022

sanket1729 Jun 8, 2022

tcharding Jun 7, 2022

apoelstra left a comment

sanket1729 commented Jun 8, 2022 •

edited

Loading

tcharding left a comment

tcharding commented Jun 9, 2022

apoelstra left a comment

	/// Macro for implementing FromTree trait. This avoids copying all the Pk::Associated type bounds
	/// Macro for implementing FromStr trait. This avoids copying all the Pk::Associated type bounds

		fn parse_tr_script_spend(tree: &expression::Tree) -> Result<TapTree<Pk>, Error> {
		fn parse_tr_script_spend(tree: &expression::Tree,) -> Result<TapTree<Pk>, Error> {

Rework translate API to allow associated types to be translated #426

Rework translate API to allow associated types to be translated #426

Conversation

sanket1729 commented Jun 5, 2022

sanket1729 commented Jun 6, 2022

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Jun 6, 2022

tcharding left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 commented Jun 7, 2022 • edited Loading

tcharding commented Jun 7, 2022

sanket1729 commented Jun 7, 2022

sanket1729 commented Jun 7, 2022

tcharding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 commented Jun 8, 2022 • edited Loading

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented Jun 9, 2022

apoelstra left a comment

Choose a reason for hiding this comment

tcharding left a comment •

edited

Loading

sanket1729 commented Jun 7, 2022 •

edited

Loading

sanket1729 commented Jun 8, 2022 •

edited

Loading