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

Stabilization roadmap? #649

Closed
Kixunil opened this issue Sep 10, 2021 · 5 comments
Closed

Stabilization roadmap? #649

Kixunil opened this issue Sep 10, 2021 · 5 comments
Labels
1.0 Issues and PRs required or helping to stabilize the API

Comments

@Kixunil
Copy link
Collaborator

Kixunil commented Sep 10, 2021

Lack of stability of this crate may be a deterrent from using it (and consequently Rust) in Bitcoin applications. Is there any desire to stabilize it?

If yes, what are the things that should be changed/done before going stable?

What I can think of right now:

@dr-orlovsky
Copy link
Collaborator

Clearly, the first and foremost - complete taproot implementation, which will take a while

Rust 2018
#[non_exhaustive] on Error, AddressType and possibly more

Requires MSRV version bump, which is also an API break and should be probably stated separately

@Kixunil
Copy link
Collaborator Author

Kixunil commented Sep 10, 2021

I felt that going to to 1.0 is pretty obvious break but no harm in writing it down. :)

@sanket1729
Copy link
Member

I think we can think of this only after we stabilize rust-secp256k1. Because every major version change in rust-secp256k1 would require a version change here.

@Kixunil Kixunil added the 1.0 Issues and PRs required or helping to stabilize the API label Feb 9, 2022
@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 9, 2022

I've created 1.0 label for labeling things that are required or should be done before 1.0. This can help people who would like to focus on stabilizing the crate.

@tcharding
Copy link
Member

Stabalization of all crates in our stack is the primary focus right now (and for the last year or so). This issue adds no additional information now. Closing.

yancyribbens pushed a commit to yancyribbens/rust-bitcoin that referenced this issue Mar 23, 2024
…irely

e971e77 types: remove error path from sub-fragment parameter to threshold() (Andrew Poelstra)
d4dc226 types: make a ton of constructors const (Andrew Poelstra)
fdea7f6 types: drop `Property` trait entirely (Andrew Poelstra)
b25023d compiler: remove a bunch of unused error paths (Andrew Poelstra)
eb854aa compiler: stop using Property trait in CompilerExtData (Andrew Poelstra)
602fd29 types: remove all Result returns for ExtData (Andrew Poelstra)
b6c3ca8 types: don't implement Property for ExtData. (Andrew Poelstra)
87294ff types: don't implement Property for Malleability (Andrew Poelstra)
dd0978d types: don't implement Property for Correctness (Andrew Poelstra)
9620380 clippy: fix a couple nits (Andrew Poelstra)
3c1c896 blanket_traits: remove crate:: in a bunch of places (Andrew Poelstra)
9280609 blanket_traits: add some more bounds (Andrew Poelstra)

Pull request description:

  Something of a followup to rust-bitcoin#649, which specifically removes the `type_check` method from `Property`.

  Basically, this whole trait is confused. Originally it was supposed to express "a recursively defined property of a Miniscript" which could be defined by a couple constructors and then automatically computed for any Miniscript. In fact, the trait required a separate constructor for every single Miniscript fragment so nothing was "automatic" except sometimes reusing code between the different hash fragments and between `older`/`after`.

  Furthermore, every implementor of this trait had slightly different requirements, meaning that there were `unreachable!()`s throughout the code, functions that were never called (e.g. `Type::from_sha256` passing through to `Correctness::from_sha256` and `Malleability::from_sha256` when all three of those types implemented the function by calling `from_hash`, which *also* was defined by `Type::from_hash` calling `Correctness::from_hash` and `Malleability::from_hash`. So much boilerplate) and many instances of methods being implemented but ignoring arguments, or (worse) being generic over `Pk`/`Ctx` and ignoring those types entirely, or returning `Result` but always returing the `Ok` variant.

  So basically, there was no value in being generic over the trait and the trait wasn't even a generalization of any of the types that implemented it.

  **Adding to this** having the trait prevents us from having constfns in our type checking code, which sucks because we have a lot of common fixed fragments. It also prevents us from returning different error types from different parts of the typechecker, which is my specific goal in removing the trait.

  This PR has a lot of commits but most of them are mechanical and/or small.

ACKs for top commit:
  sanket1729:
    ACK e971e77. Looks a lot cleaner. Thanks for these changes. It's nice that we no longer have the artificial error paths because the parent trait had the definition to encompass all possible implementations.

Tree-SHA512: 38c254caf938e5c3f84c1f0007e0bfc93180e678aef3968cb8f16ad1544722a0f7cfda2c014056446f0091b198edb12c91c0424df64f68c31f5d316fac28425c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 Issues and PRs required or helping to stabilize the API
Projects
None yet
Development

No branches or pull requests

4 participants