Skip to content

Conversation

chaitika
Copy link

Description

Fixes #267

Notes to the reviewers

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

Changelog notice

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

@coveralls
Copy link

coveralls commented Aug 21, 2025

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

Copy link
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.

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

Have a structured IndexOutOfBoundsError type
4 participants