Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improved formatting for bitcoin'd rpc error messages #133

Merged
merged 4 commits into from
Dec 12, 2024
Merged

Improved formatting for bitcoin'd rpc error messages #133

merged 4 commits into from
Dec 12, 2024

Conversation

shesek
Copy link
Collaborator

@shesek shesek commented Dec 3, 2024

Resolves #132.

@shesek shesek mentioned this pull request Dec 3, 2024
Closed
shesek added 2 commits December 3, 2024 05:19
@shesek
Include full RPC error messages in HTTP replies
e0ae00c 
Using the `From<errors::Error> for HttpError` conversion, which
uses the full Error `Display` instead of just its `description()`.

Resolves romanz#132.
@shesek
Improve RPC errors Display
a66615f 
Example with new formatting:

  sendrawtransaction RPC error -22: TX decode failed. Make sure the tx has at least one input.

Instead of:

  sendrawtransaction RPC error -22: {"code":-22,"message":"TX decode failed. Make sure the tx has at least one input."}
@shesek shesek force-pushed the 202411-rpc-errors branch from 1250b19 to a66615f Compare December 3, 2024 03:19
@RCasatta
Copy link
Collaborator

RCasatta commented Dec 4, 2024

LGTM, is it feasible to add a test asserting on error string in tests/rest.rs ?

@philippem
Copy link
Collaborator

I see the expected behaviour now, but please add a test for this in tests/rest.rs

client:

% curl -X POST localhost:3001/tx
sendrawtransaction RPC error -22: TX decode failed. Make sure the tx has at least one input.%

electrs:

INFO handle POST /tx
DEBUG RPC HTTP 500 error: {"result":null,"error":{"code":-22,"message":"TX decode failed. Make sure the tx has at least one input."},"id":136}
WARN errors::Error: Error(RpcError(-22, "TX decode failed. Make sure the tx has at least one input.", "sendrawtransaction"), State { next_error: None, backtrace: InternalBacktrace { backtrace: None } })
WARN HttpError(400, "sendrawtransaction RPC error -22: TX decode failed. Make sure the tx has at least one input.")

philippem
philippem previously approved these changes Dec 7, 2024
View reviewed changes
@shesek
Copy link
Collaborator Author

shesek commented Dec 12, 2024

is it feasible to add a test asserting on error string in tests/rest.rs ?

Yes, added in ee58814, including a test for a successful POST /tx request.

@shesek
Fix Elements dynafed tests
4878e0a 
They were broken by the test added in the last commit, which mined an
extra block and caused the `GET /block/:hash` tests for Elements to run
against a Compact params blocks instead of a Full one.

This commit fixes it by running the tests against blocks 1 & 2 which
are known to be Full & Compact respectively.
RCasatta
RCasatta approved these changes Dec 12, 2024
View reviewed changes
Copy link
Collaborator

@RCasatta RCasatta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack 4878e0a

@RCasatta RCasatta merged commit 4878e0a into Blockstream:new-index Dec 12, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RCasatta RCasatta RCasatta approved these changes

@philippem philippem philippem left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TX post RPC errors not passed to client
3 participants
@shesek @RCasatta @philippem