fix: Show InputIndexOutOfRange original error

[ItoroD]

Description

Resolves #1096

Notes to the reviewers

IndexOutOfBoundsError is being returned directly with no concatenation from bdk-wallet. See here and the error itself

Documentation

• bdk_wallet
• bitcoin
• uniffi
• Other:

Changelog

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing
• I've added exactly one changelog:* label
• I've linked the relevant upstream docs or specs above

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

[j-kon]

Two follow-ups before merge: this changes the exported error variant shape, so changelog: breaking appears more appropriate than changelog: fixed; the branch also needs rebasing onto current master before its CI result is current.

[j-kon]

Could we expose e.index and e.len as numeric fields instead of collapsing them into e.to_string()? #1096 specifically asks to preserve the offending index, but error_message makes foreign callers parse upstream display text. Since this already changes the exported variant shape and this repository supports numeric error fields, { index: u64, length: u64 } would provide a stable FFI contract while retaining the same Display message.

[ItoroD]

Thanks for the review. Did you try out the error display in any of the bindings? As I think this shows the index and the length clearly at least from the test in this PR.

but error_message makes foreign callers parse upstream display text.

Can you clarify ?

[j-kon]

Yes, the current display message does show both the index and the length clearly.

My point was more about the API exposed to the bindings rather than the human-readable error message itself.

With the current approach:

InputIndexOutOfRange { error_message: String }

a Swift/Kotlin/etc. caller gets the values only as part of a string like:

"Index out of bounds: index 3 is greater than or equal to length 1"

So if the binding user actually needs the offending index programmatically, they would have to parse that display string.

Upstream already exposes these as structured fields on IndexOutOfBoundsError:

• index
• len

So I was thinking we could preserve them directly in the FFI error, something along the lines of:

InputIndexOutOfRange {
index: u64,
length: u64,
}

while keeping the same Display message with:

#[error("Index out of bounds: index {index} is greater than or equal to length {length}")]

That way the error still reads exactly as it does now for humans, but binding callers also get stable numeric values without depending on the wording of the upstream Display implementation.

I haven't run the generated bindings for this PR specifically, so I'm not saying the current display is broken. The current approach works fine if the goal is only to improve the error message. I was mainly thinking about #1096's request to preserve the offending index as usable error data for binding consumers.

[ItoroD]

#1096 request to preserve offending index was because index nor length was not displayed at all. It complete dropped the error message coming from upstream and gives its own fixed error display "input index out of range". My changes now carry the actual error from upstream which contains the len and index.

Preserving offending index is not necessarily saying we should expose the InputIndexOutOfRange as a strong type, its saying the len and index should not be discard from bindings layer. Note that we have many other errors where we use the { error_message: String } instead of using the type itself. We typically are not trying to expose all errors from upstream as types. That has not been the pattern at least (I stand to be corrected). But I certainly see your point about the users having to parse the string if they need to fetch the index and all.

[j-kon]

That makes sense, and I agree that error_message: String is already an established pattern for several wrapped upstream errors in bdk-ffi.

My concern was mainly that #1096 specifically talks about preserving the offending index, and since this variant is already changing shape, exposing index/len would make that information programmatically usable instead of only visible in the display string.

But I don't think this needs to turn into a broader change in how bdk-ffi models upstream errors. If the preferred convention here is to preserve the upstream display through error_message, I'm fine with that approach.

The other two things from my review still apply though: this is an API break, so changelog: breaking seems appropriate, and the branch still needs to be rebased onto current master.

[ItoroD]

Yes, changelog updated. It will be rebased when we are nearly ready to merge them. i.e (after non-breaking changes are merged)

[j-kon]

“Yes, changelog updated. It will be rebased when we are nearly ready to merge them. i.e (after non-breaking changes are merged)”