Clean up `miniscript/mod.rs` #583

tcharding · 2023-07-27T06:11:16Z

Make an effort to clean up the miniscript/mod.rs file. I could not see a reason for all the separate impl blocks, is there one? Please note this PR does not fix all the docs, just does ones that should not be too hard to review.

apoelstra · 2023-07-27T18:17:41Z

src/miniscript/mod.rs

@@ -59,7 +59,7 @@ pub struct Miniscript<Pk: MiniscriptKey, Ctx: ScriptContext> {
    pub ty: types::Type,
    ///Additional information helpful for extra analysis.
    pub ext: types::extra_props::ExtData,
-    /// Context PhantomData. Only accessible inside this crate
+    /// Needed because not all terminals are paramatized by script context.


In c874074

This is actually just used to simulate #[non_exhaustive] which we did not have in 1.29. We can just drop it and add #[non_exhaustive].

Did you think this was an enum? I think the comment is correct IIUC, if we remove the phantom we get build error

error[E0392]: parameter `Ctx` is never used --> src/miniscript/mod.rs:55:42 | 55 | pub struct Miniscript<Pk: MiniscriptKey, Ctx: ScriptContext> { | ^^^ unused parameter | = help: consider removing `Ctx`, referring to it in a field, or using a marker such as `PhantomData`

Oh, no, I think actually I'm just wrong. But I don't understand your comment. Ctx is clearly used in the node field, this error is just a bug in rustc (which I think has been well-known for many years). We should change the comment to clarify this.

And it is doing double-duty as a #[non_exhaustive] tag, which is fine. It prevents users from directly constructing this type.

Oh I thought it was being caused by the fact that not all variants of Terminal have Ctx. Not sure what to write there now, the original is definitely just noise though.

apoelstra · 2023-07-27T18:19:20Z

src/miniscript/mod.rs

-/// The type information and extra_properties can be deterministically determined
-/// by the ast.
+///
+/// The type information and extra_properties can be deterministically determined by the AST.


In 33c04e2:

While we're touching this, let's replace "can be deterministically determined" with "are implied" :P.

apoelstra · 2023-07-27T18:25:00Z

5182529 looks good otherwise

tcharding · 2023-07-27T19:55:18Z

Changes in force push:

Fixed docs as suggested
Added a couple of lines of whitespace that were missing from the original refactorings
Fixed formatting in the last patch

tcharding · 2023-07-27T19:56:54Z

... and fixed the rebase

tcharding · 2023-07-27T19:58:07Z

And rebased on master to pick up the pinning fix.

apoelstra · 2023-07-27T20:01:00Z

55e65f3 is good except I'd like to change the comment on the phantom.

The `Hash` trait is related to PartialEq, Eq, PartialOrd, Ord - as such put it next to them.

Improve and make the docs all the same.

Move the `Miniscript` impl block (with the constructors in in) to be underneath the struct. Refactor only, no logic changes.

Combine the two impl blocks that contain constructors and conversion functions. Refactor only, no logic changes.

Move the `encode` and `script_size` functions to the main impl block. Refactor only, no logic changes.

Move the two `max_satisfaction*` functions to the main impl block. Refactor only, no logic changes.

Move the two `satisfy*` functions to the main impl block. Refactor only, no logic changes.

There are now two impl blocks for `Miniscript` with different generics, put one directly under the other. To match the diff this should be said "move trait impls below the second Miniscript impl block".

Compress match arms using `|`, this makes the code more terse and easier to quickly see if terminal variants return the expected length (for the trivial ones at least).

Reduce duplicate code by factoring out a helper function. Refactor only, no logic changes.

tcharding · 2023-07-27T21:05:33Z

I just dropped the phantom doc change altogether.

apoelstra

ACK a60c642

apoelstra reviewed Jul 27, 2023

View reviewed changes

tcharding force-pushed the 07-27-miniscript-mod-cleanup branch 2 times, most recently from 6775430 to 3150427 Compare July 27, 2023 19:54

tcharding force-pushed the 07-27-miniscript-mod-cleanup branch from 3150427 to 447412a Compare July 27, 2023 19:56

tcharding force-pushed the 07-27-miniscript-mod-cleanup branch from 447412a to 55e65f3 Compare July 27, 2023 19:57

tcharding added 11 commits July 28, 2023 07:04

Improve rustdocs on Miniscript type

ccb05b5

Move Miniscript Hash impl underneath others

1d1ac37

The `Hash` trait is related to PartialEq, Eq, PartialOrd, Ord - as such put it next to them.

Make Miniscript cmp/ord/hash docs uniform

fd09756

Improve and make the docs all the same.

Move Miniscript impl block under type

6c5db91

Move the `Miniscript` impl block (with the constructors in in) to be underneath the struct. Refactor only, no logic changes.

Combine Miniscript conversion and constructor blocks

dbf9a62

Combine the two impl blocks that contain constructors and conversion functions. Refactor only, no logic changes.

Move Miniscript encode/script_size into main block

e3c1b7c

Move the `encode` and `script_size` functions to the main impl block. Refactor only, no logic changes.

Move Miniscript max_satisfaction* to main block

32342de

Move the two `max_satisfaction*` functions to the main impl block. Refactor only, no logic changes.

Move Miniscript satisfy* functions into main block

5fd22a6

Move the two `satisfy*` functions to the main impl block. Refactor only, no logic changes.

Put the two Miniscript impl blocks together

23897d1

There are now two impl blocks for `Miniscript` with different generics, put one directly under the other. To match the diff this should be said "move trait impls below the second Miniscript impl block".

Compress patter match in script_size

e1e95d3

Compress match arms using `|`, this makes the code more terse and easier to quickly see if terminal variants return the expected length (for the trivial ones at least).

Refactor satisfy* functions

a60c642

Reduce duplicate code by factoring out a helper function. Refactor only, no logic changes.

tcharding force-pushed the 07-27-miniscript-mod-cleanup branch from 55e65f3 to a60c642 Compare July 27, 2023 21:05

apoelstra approved these changes Jul 28, 2023

View reviewed changes

apoelstra merged commit d1051cb into rust-bitcoin:master Jul 28, 2023

tcharding deleted the 07-27-miniscript-mod-cleanup branch August 7, 2023 07:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Clean up `miniscript/mod.rs` #583

Clean up `miniscript/mod.rs` #583

Uh oh!

tcharding commented Jul 27, 2023 •

edited

Loading

Uh oh!

apoelstra Jul 27, 2023

Uh oh!

tcharding Jul 27, 2023

Uh oh!

apoelstra Jul 27, 2023

Uh oh!

tcharding Jul 27, 2023

Uh oh!

apoelstra Jul 27, 2023

Uh oh!

apoelstra commented Jul 27, 2023

Uh oh!

tcharding commented Jul 27, 2023

Uh oh!

tcharding commented Jul 27, 2023

Uh oh!

tcharding commented Jul 27, 2023

Uh oh!

apoelstra commented Jul 27, 2023

Uh oh!

tcharding commented Jul 27, 2023

Uh oh!

apoelstra left a comment

Uh oh!

Uh oh!

Clean up miniscript/mod.rs #583

Clean up miniscript/mod.rs #583

Uh oh!

Conversation

tcharding commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra Jul 27, 2023

Choose a reason for hiding this comment

Uh oh!

tcharding Jul 27, 2023

Choose a reason for hiding this comment

Uh oh!

apoelstra Jul 27, 2023

Choose a reason for hiding this comment

Uh oh!

tcharding Jul 27, 2023

Choose a reason for hiding this comment

Uh oh!

apoelstra Jul 27, 2023

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Jul 27, 2023

Uh oh!

tcharding commented Jul 27, 2023

Uh oh!

tcharding commented Jul 27, 2023

Uh oh!

tcharding commented Jul 27, 2023

Uh oh!

apoelstra commented Jul 27, 2023

Uh oh!

tcharding commented Jul 27, 2023

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Clean up `miniscript/mod.rs` #583

Clean up `miniscript/mod.rs` #583

tcharding commented Jul 27, 2023 •

edited

Loading