Skip to content

refactor: have a structured IndexOutOfBoundsError type#302

Merged
ValuedMammal merged 1 commit intobitcoindevkit:masterfrom
chaitika:refactor--index-out-of-bounds-error-type
Sep 20, 2025
Merged

refactor: have a structured IndexOutOfBoundsError type#302
ValuedMammal merged 1 commit intobitcoindevkit:masterfrom
chaitika:refactor--index-out-of-bounds-error-type

Conversation

@chaitika
Copy link
Copy Markdown
Contributor

@chaitika chaitika commented Aug 21, 2025 •
edited by ValuedMammal
Loading

Description

Fixes #267

Notes to the reviewers

I can rebase to master after #298 is merged to fix fmt errors.

Changelog notice

Added
- newtype `IndexOutOfBoundsError`

Changed
- `PolicyError::IndexOutOfRange` now wraps a `IndexOutOfBoundsError`
- `SignerError::InputIndexOutOfRange` now wraps a `IndexOutOfBoundsError`

Checklists

All Submissions:

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@chaitika chaitika requested a review from ValuedMammal as a code owner August 21, 2025 08:26
@coveralls
Copy link
Copy Markdown

coveralls commented Aug 21, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 17246072754

Details

  • 27 of 43 (62.79%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.07%) to 84.843%
Changes Missing Coverage Covered Lines Changed/Added Lines %
wallet/src/descriptor/policy.rs 8 11 72.73%
wallet/src/types.rs 3 8 37.5%
wallet/src/wallet/signer.rs 13 21 61.9%
Totals Coverage Status
Change from base Build 17132535147: -0.07%
Covered Lines: 6689
Relevant Lines: 7884
💛 - Coveralls

ValuedMammal
ValuedMammal reviewed Aug 23, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@ValuedMammal ValuedMammal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Concept ACK

📌 I really like that you were able to find places where the new type could be substituted in. If this change were to only add the IndexOutOfBoundsError then I think we could accept it straight away, but since it also touches the public API by changing the PolicyError and SignerError enums, then it may need to be pushed to 3.0. That said, it may not be worth adding features to policy.rs since development has shifted to the miniscript::plan module.

@ValuedMammal ValuedMammal added this to the Wallet 3.0.0 milestone Aug 23, 2025
@ValuedMammal ValuedMammal moved this to Needs Review in BDK Wallet Aug 23, 2025
@110CodingP
Copy link
Copy Markdown
Contributor

110CodingP commented Aug 26, 2025

Ack 1e86fc1!
Ran the tests and checked that the correct error message is produced (by removing the should_panic attribute in the psbt tests.)

@chaitika chaitika force-pushed the refactor--index-out-of-bounds-error-type branch from 1e86fc1 to 42d9b90 Compare August 26, 2025 17:43
ValuedMammal
ValuedMammal approved these changes Sep 20, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK 42d9b90

@chaitika I updated the PR description with the changelog notice.

@ValuedMammal ValuedMammal merged commit 964d714 into bitcoindevkit:master Sep 20, 2025
20 checks passed
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Wallet Sep 20, 2025
@ValuedMammal ValuedMammal mentioned this pull request Mar 4, 2026
Closed
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ValuedMammal ValuedMammal ValuedMammal approved these changes

Assignees

@chaitika chaitika

Labels

None yet

Projects

BDK Wallet
Archived in project

Milestone

Wallet 3.0.0

Development

Successfully merging this pull request may close these issues.

Have a structured IndexOutOfBoundsError type

4 participants

@chaitika @coveralls @110CodingP @ValuedMammal