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

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

a60c642a54d685450aa9e117d854c624108ff7e5 Refactor satisfy* functions (Tobin C. Harding) e1e95d38f53485d205eced9e5fe040e9d00cf1cc Compress patter match in script_size (Tobin C. Harding) 23897d11be018bba68eb640c397fa928abe17570 Put the two Miniscript impl blocks together (Tobin C. Harding) 5fd22a6ac611c0810f2b2af3238db7fe0f1a389b Move Miniscript satisfy* functions into main block (Tobin C. Harding) 32342de7f4213ccffeb4c9279ae15197927f5087 Move Miniscript max_satisfaction* to main block (Tobin C. Harding) e3c1b7c3a7968d7686f3bc0e1fdd4d6ce55cce2d Move Miniscript encode/script_size into main block (Tobin C. Harding) dbf9a62e89b5269ec988983b1339c78d4c87ce03 Combine Miniscript conversion and constructor blocks (Tobin C. Harding) 6c5db91a779d1d4ab78011e2ee5379323fac7869 Move Miniscript impl block under type (Tobin C. Harding) fd09756d1a629dfff8c4736354d764ccd048ca27 Make Miniscript cmp/ord/hash docs uniform (Tobin C. Harding) 1d1ac370d6a294080fb703199d66a3bfec971a4b Move Miniscript Hash impl underneath others (Tobin C. Harding) ccb05b5ae4308964ca0801475402d0826e84fdf2 Improve rustdocs on Miniscript type (Tobin C. Harding) Pull request description: 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. ACKs for top commit: apoelstra: ACK a60c642a54d685450aa9e117d854c624108ff7e5 Tree-SHA512: 2fb555f5f399fd037b177c6d2e1224915d303c6bc13de8eb2d51143e48caf7a61e6bdd8bf0cbdac7fb95a29c00ef13bf8fe0aac1a3b9d9ca47ce59e63ced041e

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Clean up `miniscript/mod.rs` #583

Clean up `miniscript/mod.rs` #583

tcharding commented Jul 27, 2023 •

edited

Loading