-
Notifications
You must be signed in to change notification settings - Fork 3
Align test specification with bitcoinkernel.h exported symbols
#7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
fast feedback : please also update the documentation => ### Error Response
```json
{
"id": "unique-request-id",
"error": {
"type": "error_category",
"variant": "specific_error"
}
}Error response fields:
|
|
Oops, forgot to adapt the spec; thanks @janb84! |
janb84
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK cc128f1
lgtm and no mayor issues implementing the changes om my .NET handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this alignment, I think it's definitely an improvement. I've thought a bit more about my earlier suggestion wrt the Response format, and I think it can be improved further.
A Response format could look like:
{
"id": str,
"result": optional[any],
"error": optional[error]
}
For result:
- if a function return a
value,resultis specified as"result": value - if a function returns a
nullptr,resultmust benull - if a function doesn't return (it is
void, or the function errored),resultmust benull
For error:
- if a function doesn't "throw",
errormust benull - if a function throws,
errormust be defined, with"error": {}being the base, catch-all exception, that may be further specified, e.g. with acodeobject (that is already in the spec) - if
erroris not null,successmust be null
Note: null is equivalent to the key being omitted from the object.
A couple of examples:
Scenario A: Function returns a value (e.g., btck_script_pubkey_verify -> true)
{
"id": "req-1",
"result": true
}Scenario B: Function is void or returns nullptr on success, but doesn't throw
{
"id": "req-2",
"result": null # can also be omitted
}Scenario C: Specific Error (e.g., verification failed with code)
{
"id": "req-3",
"error": {
"code": {
"type": "btck_ScriptVerifyStatus",
"member": "ERROR_INVALID_FLAGS_COMBINATION"
}
}
}Scenario D: Generic/Unspecified Error
{
"id": "req-4",
"error": {}
}|
Thank you for the review @stickies-v! I like the improvement you are suggesting; besides the Regarding your comment #7 (comment) on whether to treat invalid scripts (when the request is valid) as success cases: I think it comes down to how to interpret the C API a bit. I understand how you are reading into this though: we are either successful in verifying (valid/invalid script) or we are not (error/throw), and it makes sense. I am OK with applying the change here, though it seems it contradicts the Rust/Go/.NET approaches (edit: also the python approach now that I am looking) of treating everything but the valid script as an error/exception. And with this change applied, these bindings have to translate back the invalid error cases to non-errors for the handler implementation. |
This eliminates usage of the "VERIFY_ALL_PRE_TAPROOT" flag which is not exported from `libbitcoinkernel`
cc128f1 to
0f1e00d
Compare
|
Rebased and implemented the semantics suggested here #7 (review) - Thanks @stickies-v! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update btck_script_pubkey_verify test cases to match the C function signature and parameter names from bitcoinkernel.h: - Method name: Use `btck_script_pubkey_verify` instead of `script_pubkey.verify` - Parameters: Rename fields to match C API: - `script_pubkey_hex` → `script_pubkey` - `tx_hex` → `tx_to` - `value` → `amount` (in spent_outputs array)
Drop test cases for errors not exported by `bitcoinkernel.h` (`TxInputIndex`, `SpentOutputsMismatch`, `InvalidFlags`) to align with C API capabilities.
Also refactor response validation to be aligned with the improved semantics: - Success case: result contains the return value (or null for void/nullptr), error must be null/omitted - Error case: result must be null/omitted, error contains error details Additional changes: - Replace Error.type/variant with Error.code.type/member - Split validation into separate validateResponseForSuccess/Error functions - Update test cases to reflect new format
Adapt Makefile to also run unit tests with `make test`
0f1e00d to
dd5c930
Compare
|
Applied @stickies-v's #7 (comment) for further alignment with the C API naming and tested the new pre-release I think I will be merging this in a few hours (if no one sees any further issues) to be able to rebase and merge #9 too. Then hopefully all changes can be exercised with the release |
janb84
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK dd5c930
Good update, nice to have unit tests now also !
|
Updated stickies-v/py-bitcoinkernel#42 to the latest force-push, no more comments on the alignment! Didn't review the code, but everything seems to work well. For a future improvement, I think it might be worth considering using JSON Schema (or even OpenRPC) to document the interface in a more systematic way, but that's out of scope for this PR. |
dd5c930 to
bda7ae4
Compare
That would be nice! Especially if we can have something that allows auto-generating code for encoding/decoding the JSON data on the handler side. |
bda7ae4 to
e8ddf75
Compare
Added schema.json in #10 |
Addresses #5.
Pre-release
v0.0.2-alpha.1can be used to test these changes (as I have used and tested in stringintech/go-bitcoinkernel@f03919c)