Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inline descriptor inner types #587

Closed
wants to merge 2 commits into from

Conversation

tcharding
Copy link
Member

Patch 1 is trivial cleanup

From commit log of patch 2

The `ShInner` and `WshInner` enum's provide no benefit that I can
discern. Both include only tuple variants with private tuple fields. We
can therefore inline the enums into `Sh` and `Wsh` respectively with no
loss of type safety. Doing so marginally simplifies the code, the
biggest benefit is it stops devs wondering why there is an inner type.

@tcharding tcharding force-pushed the 08-01-rm-inner branch 2 times, most recently from c4d1957 to bbf5ee9 Compare August 7, 2023 08:22
apoelstra
apoelstra previously approved these changes Aug 7, 2023
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK bbf5ee9

@apoelstra
Copy link
Member

This is a breaking change but hopefully not too disruptive of one. There isn't really any way to do a deprecation stage.

Will let @sanket1729 take a look before merging.

@tcharding
Copy link
Member Author

This is a breaking change but hopefully not too disruptive of one.

On that topic, whats your view on the status of miniscript, you said in the past that the 1.0 release was a mistake, do you consider the crate is in the same state as rust-bitcoin i.e., we can shake things up a bunch with the aim of trying to stabilise so we can stop making so many changes to the public API?

@apoelstra
Copy link
Member

@tcharding yeah. I think a "real" 1.0 should even be delayed until we get rid of all the recursion, make it impossible to construct invalid objects, and maybe improve the in-memory representation of things.

@tcharding
Copy link
Member Author

Cool cheers, plenty to do then.

apoelstra
apoelstra previously approved these changes Sep 22, 2023
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 21e3cbc

The `ShInner` and `WshInner` enum's provide no benefit that I can
discern. Both include only tuple variants with private tuple fields. We
can therefore inline the enums into `Sh` and `Wsh` respectively with no
loss of type safety. Doing so marginally simplifies the code, the
biggest benefit is it stops devs wondering why there is an inner type.
@tcharding
Copy link
Member Author

Rebase. Also fix a couple of match statements that used match &self to use match *self as is customary.

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tcharding, this was done on purpose to prohibit invalid use of constructors. If these are directly used in enums, user can directly create Wsh::Ms(invalid minisript). An invalid miniscript could be something that is not does have a root level type B among other things.

The purpose of shifting to inners was to guard access strictly via constructors.

@tcharding
Copy link
Member Author

Ahhh, so my commit log shows the problem

The ShInner and WshInner enum's provide no benefit that I can discern.

The problem is with me, there is a difference, as you state - maintaining the invariant. My bad.

Closing, thanks.

@tcharding tcharding closed this Sep 26, 2023
@tcharding
Copy link
Member Author

tcharding commented Sep 26, 2023

Ahhh I feel kind of vindicated, it is not immediately apparent that this function maintains the [undocumented] invariants.

    /// Create a new p2sh descriptor with the raw miniscript
    pub fn new(ms: Miniscript<Pk, Legacy>) -> Result<Self, Error> {
        // do the top-level checks
        Legacy::top_level_checks(&ms)?;
        Ok(Self { inner: ShInner::Ms(ms) })
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants