Export Display trait on errors

[thunderbiscuit]

This brings the Display trait we create using the thiserror::Error macro to the bindings. We've been implementing the Display trait for all this time, but not passing it on to the bindings!

Note that the draft version of this PR implements this just for the CreateWithPersistError enum.

See related issues #509 and #964.

Todo

• Export Display and Debug on all errors
• Look at the messages to make sure they are what we want (for example I think a nice addition would be to add quotes around the message given from upstream, so that the string reads: http error with status code 404 and message 'Block not found' instead of http error with status code 404 and message Block not found. Just an idea.

In Practice

Here is what this means in practice:

Kotlin

The default toString() method on Exceptions (className + ": " + message) is overriden by the Display trait. You loose the className when the exception is printed out, but you gain the human-readable message we maintain in the crate. If you want the Exception type, you can still print it. You keep the strongly typed fields (in the example below your status field is a UShort), and the message field is just a stringified concatenation of all the fields. This means you have to know that on BDK exceptions, the human-readable message is not in fact in the message field like you expect on typical Kotlin errors, but instead in the toString() method.

@Test  
fun `Esplora header height exception`() {  
    try {  
        val esploraClient = EsploraClient(ESPLORA_REGTEST_URL)  
        esploraClient.getBlockHash(10000000u)  
    } catch (e: Exception) {
	    println("Type:     ${e::class.qualifiedName}") 
        println("Message:  ${e.message}")  
        println("toString: ${e.toString()}")  
    }
}
// The exception type is:
// org.bitcoindevkit.EsploraException.HttpResponse

// The exception.message field is:
// status=404, errorMessage=Block not found
   
// The exception.toString() method returns:
// http error with status code 404 and message Block not found

Swift

In Swift, two new fields are added to the errors: description and debugDescription. The description field now holds the human-readable messages we craft on the structs using thiserror. The `debugDescription is just the Debug trait on Rust. Note that this is not available in Kotlin, as there is not equivalent "debug" print method or field.

    func testNewAddress() throws {
        do {
            let persister = try Persister.newInMemory()
            let _ = try Wallet(
                descriptor: descriptor,
                changeDescriptor: changeDescriptor,
                network: .regtest,
                persister: persister
            )
        } catch let error as CreateWithPersistError {
        print("\n========================================")
        print("🔴 CAUGHT ERROR")
        print("========================================")
        print("print(error):         \(error)")
        print("----------------------------------------")
        print("localizedDescription: \(error.localizedDescription)")
        print("----------------------------------------")
        print("errorDescription:     \(error.errorDescription)")
        print("----------------------------------------")
        print("description:          \(error.description)")
        print("----------------------------------------")
        print("debugDescription:     \(error.debugDescription)")
        print("----------------------------------------")
        print("dump(error):")
        dump(error)
        print("========================================\n")
        throw error
        }
    }

========================================
🔴 CAUGHT ERROR
========================================
print(error):         Descriptor(errorMessage: "External and internal descriptors are the same")
----------------------------------------
localizedDescription: BitcoinDevKit.CreateWithPersistError.Descriptor(errorMessage: "External and internal descriptors are the same")
----------------------------------------
errorDescription:     Optional("BitcoinDevKit.CreateWithPersistError.Descriptor(errorMessage: \"External and internal descriptors are the same\")")
----------------------------------------
description:          the loaded changeset cannot construct wallet: External and internal descriptors are the same
----------------------------------------
debugDescription:     Descriptor { error_message: "External and internal descriptors are the same" }
----------------------------------------
dump(error):
▿ BitcoinDevKit.CreateWithPersistError.Descriptor
  ▿ Descriptor: (1 element)
    - errorMessage: "External and internal descriptors are the same"
========================================

Changelog

## Added
- [Swift]: Errors get new `description` and `debugDescription` fields. `description` defers to the Display trait and `debugDescription` defers to the `Debug` trait.
- [Kotlin]: Exceptions now use a customized `toString()` method with a human-readable message populated by the `Display` trait.

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

[ItoroD]

@thunderbiscuit This is ineresting for the swift side I can see the difference this is making. For kotlin, are you saying we will have to implement a custom display method (Just to clarify).

Edit - I guess not cause that will then affect all bindings.

[reez]

Concept ACK

[j-kon]

Since the PR title/changelog describe exporting Display on errors broadly, should this export be applied to the other uniffi::Error enums in this file too, or should the PR scope be narrowed to CreateWithPersistError only? Right now this is the only error type getting Debug/Display exported to the bindings.

[thunderbiscuit]

Yep I should add it for all errors! Initially I just wanted to open this up for discussion and write up the PR description going into details. But now I'm ready to add it everywhere and will push to this PR soon.

[j-kon]

Thanks for updating this to cover the other error enums too, that addresses the scope question from my earlier review.

One follow-up question: should this PR add at least one binding-facing test/example for the generated error display behavior? Since the PR description calls out different downstream behavior in Kotlin and Swift, it may be useful to assert one representative error path so we know the exported Display/Debug behavior stays covered.

[thunderbiscuit]

That's a great idea. Let me add a few tests right now.

[j-kon]

Thanks for updating this to cover the other error enums too. That addresses my scope question.

[thunderbiscuit]

I added 2 tests showcasing how the exceptions behave and look like from the point of view of a Kotlin dev. @reez would you like me to add some for the Swift side as well?

In short:

1. The exception when printed out now gives you what you would typically expect be in the message field. These are good and human-readable. They are the strings we've had in the thiserror macro for a long time.
2. If there are any fields on the Rust enum, they also get populated in the Exception, and can be queried/printed.
3. The message field is actually just a concatenation of all the other fields on the type. If there are no fields, then the message string is empty.

This is, I think, an improvement over what we have currently. My only beef left with it is that at least on the JVM side, your stack traces are now a bit odd, and provide a different type of information than you're used to. I need to see if this is the case for other languages as well.

Example

With this PR:

org.bitcoindevkit.test               E  ----- begin exception -----
org.bitcoindevkit.test               E  the word count 13 is not supported
                                            at org.bitcoindevkit.FfiConverterTypeBip39Error.read(bdk.kt:25268)
                                            at org.bitcoindevkit.FfiConverterTypeBip39Error.read(bdk.kt:25263)
...

Before, you'd get a different stack trace because the toString method is typically of the following format: ExceptionClassName: message:

org.bitcoindevkit.test               E  ----- begin exception -----
org.bitcoindevkit.test               E  org.bitcoindevkit.Bip39Exception$BadWordCount: wordCount=13
                                            at org.bitcoindevkit.FfiConverterTypeBip39Error.read(bdk.kt:25156)
                                            at org.bitcoindevkit.FfiConverterTypeBip39Error.read(bdk.kt:25151)

[reez]

I added 2 tests showcasing how the exceptions behave and look like from the point of view of a Kotlin dev. @reez would you like me to add some for the Swift side as well?

Thanks for asking, from my POV I think the Kotlin ones cover this PR since Kotlin’s toString() and Swift’s description expose the same Rust Display output. The only other minor thing is Swift’s debugDescription is unique but I don’t really think a separate Swift test is necessary or blocking based on that.