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

bump elements and related dep #90

Merged
merged 4 commits into from
Oct 8, 2024

Conversation

RCasatta
Copy link
Collaborator

@RCasatta RCasatta commented Sep 25, 2024

apoelstra, there are some places where I need guidance

Also, I still need to check CI, just tested local cargo check and cargo test are working for now

Ok(Pegin::new(fed_desc, elem_desc))
// let fed_desc = crate::BtcDescriptor::<Pk>::from_tree(&ms_expr)?;

// let fed_desc = BtcDescriptor::<Pk>::from_tree(&ms_expr)?;
Copy link
Collaborator Author

@RCasatta RCasatta Sep 25, 2024

Choose a reason for hiding this comment

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

this line fails because variant or associated item cannot be called on Descriptor<Pk> due to unsatisfied trait bounds and I don't get what I can do about that

Copy link
Member

Choose a reason for hiding this comment

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

In the impl_from_tree macro we have a ton of bounds on Pk. Upstream these have all been wrapped up into the FromStrKey trait which you need to update the macro to use instead.

If you change this line to call bitcoin_miniscript::expression::FromTree::from_tree you will get a much better error message from the compiler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in e6724fc

@RCasatta
Copy link
Collaborator Author

just realized we need a simplicity dep bump before this https://github.com/BlockstreamResearch/rust-simplicity

I left separated commit for now for easier review, will squash in 1 commit at the end

uncomputable added a commit to BlockstreamResearch/rust-simplicity that referenced this pull request Oct 1, 2024
92e0972 ci: avoid fail fast in toolchains tests (Riccardo Casatta)
c22f9de bump bitcoin dep and related, bump version to 0.3.0 (Riccardo Casatta)

Pull request description:

  Required for ElementsProject/elements-miniscript#90

ACKs for top commit:
  apoelstra:
    ACK 92e0972 successfully ran local tests
  uncomputable:
    ACK 92e0972

Tree-SHA512: a4ef036136557e5b539a0a52bf46930ebf02e0243603d00c69b98ec3f00e8caf1c0660e00f4c5195fe5ab4ad65b08573970cbf1deee6ae8419d6e818a1aeb33f
@RCasatta RCasatta force-pushed the bump_elements_dep branch 3 times, most recently from 3fec3dd to e40f8db Compare October 2, 2024 15:33
src/descriptor/tr.rs Outdated Show resolved Hide resolved
@RCasatta RCasatta marked this pull request as ready for review October 2, 2024 16:05
@RCasatta RCasatta requested a review from apoelstra October 3, 2024 15:50
@apoelstra
Copy link
Member

Sorry for the long delay. There is a clippy issue I'd like you to fix

error: useless conversion to the same type: `miniscript::RelLockTime`
   --> src/descriptor/pegin/legacy_pegin.rs:148:17
    |
148 |                 bitcoin_miniscript::RelLockTime::try_from(timelock).expect("TODO"),
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: consider removing `bitcoin_miniscript::RelLockTime::try_from()`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
    = note: `-D clippy::useless-conversion` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::useless_conversion)]`

Not sure why CI is not complaining about this.

@RCasatta
Copy link
Collaborator Author

RCasatta commented Oct 8, 2024

clippy fixed, note that now that I checked I've hit also the following one, not sure if you didn't mention this because we want to allow it or what

warning: in an `if` condition, avoid complex blocks or closures with blocks; instead, move the block or closure higher and bind it with a `let`
    --> src/descriptor/mod.rs:1019:36
     |
1019 |           if !self.for_any_key(|key| {
     |  ____________________________________^
1020 | |             // All multipath keys must have the same number of indexes at the "multi-index"
1021 | |             // step. So we can return early if we already populated the vector.
1022 | |             if !descriptors.is_empty() {
...    |
1034 | |             }
1035 | |         }) {
     | |_________^
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#blocks_in_conditions
     = note: `#[warn(clippy::blocks_in_conditions)]` on by default

@apoelstra
Copy link
Member

I don't see that one, but I do see several related to doccomments. I'm not sure what's up. But sounds like we should just do a followup with clippy fixes rather than iterating on this one. Clippy wasn't happy on master either.

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 6ed3bbb

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 6ed3bbb

@RCasatta RCasatta merged commit 6ed3bbb into ElementsProject:master Oct 8, 2024
17 checks passed
apoelstra added a commit that referenced this pull request Oct 8, 2024
6ed3bbb avoid useless conversion of the same type (Riccardo Casatta)
79f3109 bump elements and related dep (Riccardo Casatta)
b6ffda3 ci: let other toolchain finish if one is failing (Riccardo Casatta)
42375e4 bump MSRV 1.58 -> 1.63 (Riccardo Casatta)

Pull request description:

  apoelstra, there are some places where I need guidance

  Also, I still need to check CI, just tested local `cargo check` and `cargo test` are working for now

ACKs for top commit:
  apoelstra:
    ACK 6ed3bbb

Tree-SHA512: 4d3d5a44c509a0df81527d73ccad1e384c251e28a351b01aae3c19b97b4ce75afa91c6f4cf3b987c7fb8feabfe5abe51fb103a8dfb27493eaa8bf3a74e0b6af4
RCasatta added a commit that referenced this pull request Oct 8, 2024
c5257e2 bump version to 0.4.0 and upgrade changelog (Riccardo Casatta)

Pull request description:

  on top of #90

ACKs for top commit:
  apoelstra:
    ACK c5257e2 successfully ran local tests

Tree-SHA512: 4d3db3c80232acad1505269ba8272449dcf9b00c6781f62d3bc52ff4c5c2cd23cc1f3b68442b784008f04e846a76f4098b1abc4413088c18b81c7766f4ef4401
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.

2 participants