Skip to content

Conversation

ValuedMammal
Copy link
Collaborator

@ValuedMammal ValuedMammal commented Sep 24, 2025

Bump bdk_wallet MSRV to 1.85.0.

Restructure repository to be a single rust project.

Small cleanups to .github/workflows by moving away from unmaintained actions-rs/toolchain in favor of dtolnay/rust-toolchain.

Follow-ups

  • Address zizmor code scanning results
  • Use rust edition 2024. May have to remove some "unnecessary binding modifiers"
  • Consider updating .github/CODEOWNERS

fix #289

Notes to the reviewers

Changelog notice

Changed

- Raise project MSRV to 1.85.0
- ci: Migrate to `dtolnay/rust-toolchain`
- ci: Add zizmor.yml config

Checklists

All Submissions:

@coveralls
Copy link

coveralls commented Sep 24, 2025

Pull Request Test Coverage Report for Build 18041569734

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 85.211%

Totals Coverage Status
Change from base Build 17957725258: 0.4%
Covered Lines: 3895
Relevant Lines: 4571

💛 - Coveralls

@ValuedMammal ValuedMammal moved this to In Progress in BDK Wallet Sep 24, 2025
@ValuedMammal ValuedMammal added this to the Wallet 3.0.0 milestone Sep 24, 2025
@ValuedMammal ValuedMammal added the chore Non-coding related work label Sep 24, 2025
@ValuedMammal ValuedMammal self-assigned this Sep 24, 2025
Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

Concept ACK. See comment on workspace. Good move away from the deprecated action-rs/rust-toolchain action.

This is to avoid a clash with wallet/examples when
the workspace goes away.
Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

I'm personally not a fan of the "pin the hash on the actions" that zizmor recommends here.

If you want to address the zizmor warning you're getting here, you could do what we do for bdk-ffi. You add a file called zizmor.yml at the root of the .github/ directory with the following:

# This is a configuration file for the zizmor action defined in .github/workflows/zizmor.yaml

rules:
  # Disable the rule requiring all actions be pinned to a specific hash
  unpinned-uses:
    config:
      policies:
        "*": any

Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

Tested ACK 6260274.

I'm getting one little clippy warning locally:

error: the following explicit lifetimes could be elided: 'a
    --> src/wallet/mod.rs:1192:25
     |
1192 |     pub fn transactions<'a>(&'a self) -> impl Iterator<Item = WalletTx<'a>> + 'a {
     |                         ^^   ^^                                        ^^     ^^
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
     = note: `-D clippy::needless-lifetimes` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(clippy::needless_lifetimes)]`
help: elide the lifetimes
     |
1192 -     pub fn transactions<'a>(&'a self) -> impl Iterator<Item = WalletTx<'a>> + 'a {
1192 +     pub fn transactions(&self) -> impl Iterator<Item = WalletTx<'_>> + '_ {

But this is related to my default toolchain being 1.85. The warning disappears if I build using 1.89. See #315 for my recommendation on how to fix that sort of issue. Not required for this PR, just pointing it out.

@thunderbiscuit
Copy link
Member

Also note that I think the rust-version file was there to help the actions-rs/toolchain action (that's a file it uses to decide on rust compiler version). Since you've moved away from this action it might be possible to remove the file entirely (or at least we should know why we need to keep it if another tool/workflow/dev is using it).

@ValuedMammal
Copy link
Collaborator Author

In 2a5c492 I added an exception to the unpinned-uses rule specifically for dtolnay/rust-toolchain. One slight problem is that the upstream repo only has one v1 tag that seems to be continually overwritten (i.e. insecure from a code scanning perspective). We could pin it to a hash but that would require more hands-on involvement if we ever want to update it. If there's alternatives to dtolnay for installing a rust toolchain we could explore that too.

@ValuedMammal ValuedMammal marked this pull request as ready for review September 26, 2025 14:04
@ValuedMammal ValuedMammal moved this from In Progress to Needs Review in BDK Wallet Sep 26, 2025
Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

ACK eceb1a3. Nice cleanup.


<div align="center">
<h1>BDK Wallet</h1>
<h1>BDK</h1>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

📌 Accidentally clobbered when I merged the two READMEs together. Same on line 4.

@ValuedMammal ValuedMammal merged commit e2cf34d into bitcoindevkit:master Sep 29, 2025
16 checks passed
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Wallet Sep 29, 2025
@ValuedMammal ValuedMammal deleted the chore/bump_msrv branch September 29, 2025 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Non-coding related work

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Upgrade MSRV to 1.85

3 participants