Add ADR document on errors #513
Conversation
WalkthroughThis update introduces a new document, Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional Context UsedLanguageTool (14)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
|
||
| ## Context and Problem Statement | ||
|
|
||
| The tool we use to produce language bindings for bdk-ffi libraries is [uniffi]. While the library is powerful, it also comes with some caveats. Those come into play when attempting to expose Rust errors; we must choose between simple enums that have variants but not data associated with them (for example a `TxidError` would not be able to return the specific txid that triggered the error), or more complex objects that do have data fields, but no ability to control the error message returned to the user. What's more, while Kotlin and Java users expect a `message` field on an `Exception` type, this field becomes an empty string if the object has no data, and just a strignified version of the fields if it does have any. |
There was a problem hiding this comment.
Correct the spelling mistake in "strignified" to "stringified".
- just a strignified version of the fields if it does have any.
+ just a stringified version of the fields if it does have any.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| The tool we use to produce language bindings for bdk-ffi libraries is [uniffi]. While the library is powerful, it also comes with some caveats. Those come into play when attempting to expose Rust errors; we must choose between simple enums that have variants but not data associated with them (for example a `TxidError` would not be able to return the specific txid that triggered the error), or more complex objects that do have data fields, but no ability to control the error message returned to the user. What's more, while Kotlin and Java users expect a `message` field on an `Exception` type, this field becomes an empty string if the object has no data, and just a strignified version of the fields if it does have any. | |
| The tool we use to produce language bindings for bdk-ffi libraries is [uniffi]. While the library is powerful, it also comes with some caveats. Those come into play when attempting to expose Rust errors; we must choose between simple enums that have variants but not data associated with them (for example a `TxidError` would not be able to return the specific txid that triggered the error), or more complex objects that do have data fields, but no ability to control the error message returned to the user. What's more, while Kotlin and Java users expect a `message` field on an `Exception` type, this field becomes an empty string if the object has no data, and just a stringified version of the fields if it does have any. |
|
|
||
| ## Decision Drivers | ||
|
|
||
| Some options were considered and explored in detail in [issue #509]. Some aspects of this decisions include: |
There was a problem hiding this comment.
Replace "this decisions" with "these decisions" to correct grammatical agreement.
- Some aspects of this decisions include:
+ Some aspects of these decisions include:Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| Some options were considered and explored in detail in [issue #509]. Some aspects of this decisions include: | |
| Some options were considered and explored in detail in [issue #509]. Some aspects of these decisions include: |
| ## Decision Drivers | ||
|
|
||
| Some options were considered and explored in detail in [issue #509]. Some aspects of this decisions include: | ||
| - Expectations from devs using the libraries in different languages (for example Kotlin users expect a `message` field on the exception, whereas Swift users do not) |
There was a problem hiding this comment.
Consider adding a comma after "for example" for better readability.
- (for example Kotlin users expect a `message` field on the exception, whereas Swift users do not)
+ (for example, Kotlin users expect a `message` field on the exception, whereas Swift users do not)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - Expectations from devs using the libraries in different languages (for example Kotlin users expect a `message` field on the exception, whereas Swift users do not) | |
| - Expectations from devs using the libraries in different languages (for example, Kotlin users expect a `message` field on the exception, whereas Swift users do not) |
Ensure lists are surrounded by blank lines.
- - Expectations from devs using the libraries in different languages (for example Kotlin users expect a `message` field on the exception, whereas Swift users do not)
+
+ - Expectations from devs using the libraries in different languages (for example Kotlin users expect a `message` field on the exception, whereas Swift users do not)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - Expectations from devs using the libraries in different languages (for example Kotlin users expect a `message` field on the exception, whereas Swift users do not) | |
| - Expectations from devs using the libraries in different languages (for example Kotlin users expect a `message` field on the exception, whereas Swift users do not) | |
|
|
||
| Some options were considered and explored in detail in [issue #509]. Some aspects of this decisions include: | ||
| - Expectations from devs using the libraries in different languages (for example Kotlin users expect a `message` field on the exception, whereas Swift users do not) | ||
| - Ease of maintaining the errors as they evolve and we expose more and more of the Rust BDK ecosystem |
There was a problem hiding this comment.
Consider adding a comma before "and" to connect two independent clauses.
- as they evolve and we expose more and more of the Rust BDK ecosystem
+ as they evolve, and we expose more and more of the Rust BDK ecosystemCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - Ease of maintaining the errors as they evolve and we expose more and more of the Rust BDK ecosystem | |
| - Ease of maintaining the errors as they evolve, and we expose more and more of the Rust BDK ecosystem |
|
|
||
| We have decided to leverage two different approaches that uniffi offers for exposing errors, and using the most appropriate one for each situation. | ||
|
|
||
| In the case where errors do not require passing back data, we opt for the simpler to maintain option, which provides better, costumized error message with a `message` field provided by the `thiserror` library. Those errors cannot have fields. For example: |
There was a problem hiding this comment.
Correct the spelling mistake in "costumized" to "customized".
- which provides better, costumized error message with a `message` field provided by the `thiserror` library.
+ which provides better, customized error message with a `message` field provided by the `thiserror` library.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| In the case where errors do not require passing back data, we opt for the simpler to maintain option, which provides better, costumized error message with a `message` field provided by the `thiserror` library. Those errors cannot have fields. For example: | |
| In the case where errors do not require passing back data, we opt for the simpler to maintain option, which provides better, customized error message with a `message` field provided by the `thiserror` library. Those errors cannot have fields. For example: |
| }; | ||
| ``` | ||
|
|
||
| ### Rust |
There was a problem hiding this comment.
Ensure headings are surrounded by blank lines.
- ### Rust
+ ### RustCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| ### Rust | |
| ### Rust |
| In the case where errors do not require passing back data, we opt for the simpler to maintain option, which provides better, costumized error message with a `message` field provided by the `thiserror` library. Those errors cannot have fields. For example: | ||
| #### UDL | ||
|
|
||
| ```txt |
There was a problem hiding this comment.
Ensure fenced code blocks are surrounded by blank lines.
- ```txt
+
+ ```txt| ``` | ||
|
|
||
| #### Rust | ||
| ```rust |
There was a problem hiding this comment.
Ensure fenced code blocks are surrounded by blank lines.
- ```rust
+
+ ```rust| In the case where complex types should be returned as part of the error, we use the more complex interface UDL type and return the data, at the cost of more explicit messages. For example: | ||
|
|
||
| ### UDL | ||
| ```txt |
There was a problem hiding this comment.
Ensure fenced code blocks are surrounded by blank lines.
- ```txt
+
+ ```txt| ``` | ||
|
|
||
| ### Rust | ||
| ```rust |
There was a problem hiding this comment.
Ensure fenced code blocks are surrounded by blank lines.
- ```rust
+
+ ```rust
|
Great idea. I'm going to ruminate on this a bit over the next week (and add comments/thoughts as needed) |
|
|
||
| ## Context and Problem Statement | ||
|
|
||
| The tool we use to produce language bindings for bdk-ffi libraries is [uniffi]. While the library is powerful, it also comes with some caveats. Those come into play when attempting to expose Rust errors; we must choose between simple enums that have variants but not data associated with them (for example a `TxidError` would not be able to return the specific txid that triggered the error), or more complex objects that do have data fields, but no ability to control the error message returned to the user. What's more, while Kotlin and Java users expect a `message` field on an `Exception` type, this field becomes an empty string if the object has no data, and just a strignified version of the fields if it does have any. |
There was a problem hiding this comment.
if you want to touch on swift real quick here we can add something like this at the end of the Context and Problem Statement section:
"Swift, in contrast, can optionally leverage something like the LocalizedError protocol to provide custom and localized descriptions."
| ## Decision Drivers | ||
|
|
||
| Some options were considered and explored in detail in [issue #509]. Some aspects of this decisions include: | ||
| - Expectations from devs using the libraries in different languages (for example Kotlin users expect a `message` field on the exception, whereas Swift users do not) |
There was a problem hiding this comment.
"whereas Swift users do not" could be "whereas Swift users do not expect message but might expect localizedDescription"
There was a problem hiding this comment.
verbiage on this suggestion might change now with mozilla/uniffi-rs#2116
| @@ -0,0 +1,59 @@ | |||
| # Errors | |||
|
|
|||
| Returning meaningful errors is important for users of our libraries. The Rust bitcoin ecosystem uses descriptive and data-driven errors, and we would like to stay as close to them as possible when building language bindings for these libraries. | |||
There was a problem hiding this comment.
what do you think of something like this for this section, it includes what you already have but just sandwiches some extra "error handling philosophy" in between:
Returning meaningful errors is important for users of our libraries. Libraries return errors and applications decide if and how those errors are formatted and displayed to users. Our goal as a library is to produce meaningful and structured error types which allows applications to easily differentiate various error cases. The Rust bitcoin ecosystem uses descriptive and data-driven errors, and we would like to stay as close to them as possible when building language bindings for these libraries.
|
Still interested in adding this as an ADR? I think we are solidified enough per #509 that the way we do it now is the "standard", so I'm cool with getting this merged if you are good with that (obviously after rebase, maybe fixing small nits/comments, etc). |
This PR is very much up for review and discussion. I figured that a good way to lay out correctly a solution would be to write it out. We can hash out the specifics directly on this PR.