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

fix(getblocktemplate): change error format in proposals #6044

Merged
merged 6 commits into from
Feb 2, 2023
Merged

fix(getblocktemplate): change error format in proposals #6044

merged 6 commits into from
Feb 2, 2023

Conversation

oxarbitrage
Copy link
Contributor

Motivation

We want to match zcashd error format in getblocktemplate proposal mode responses.

Close #5981

Solution

Create kabeb-case errors for ProposalResponse from zebra json.

Review

Anyone can review.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@oxarbitrage oxarbitrage requested a review from a team as a code owner January 27, 2023 22:00
@oxarbitrage oxarbitrage requested review from arya2 and removed request for a team January 27, 2023 22:00
@github-actions github-actions bot added the C-enhancement Category: This is an improvement label Jan 27, 2023
@oxarbitrage oxarbitrage changed the title change error format in proposals fix(getblocktemplate): change error format in proposals Jan 27, 2023
@oxarbitrage oxarbitrage self-assigned this Jan 27, 2023
@oxarbitrage oxarbitrage added P-Medium ⚡ A-rpc Area: Remote Procedure Call interfaces A-compatibility Area: Compatibility with other nodes or wallets, or standard rules labels Jan 27, 2023
@codecov
Copy link

codecov bot commented Jan 27, 2023 •
edited
Loading

Codecov Report

Merging #6044 (377025f) into main (00e7418) will increase coverage by 0.17%.
The diff coverage is n/a.

❗ Current head 377025f differs from pull request most recent head 1ddb575. Consider uploading reports for the commit 1ddb575 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #6044      +/-   ##
==========================================
+ Coverage   78.03%   78.21%   +0.17%     
==========================================
  Files         312      312              
  Lines       39036    39417     +381     
==========================================
+ Hits        30463    30830     +367     
- Misses       8573     8587      +14

arya2
arya2 reviewed Jan 28, 2023
View reviewed changes
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

This looks great, thank you!

zebra-rpc/src/methods/get_block_template_rpcs/types/get_block_template/proposal.rs Show resolved Hide resolved
zebra-rpc/src/methods/get_block_template_rpcs/types/get_block_template/proposal.rs Outdated Show resolved Hide resolved
zebra-rpc/Cargo.toml Outdated Show resolved Hide resolved
zebra-rpc/src/methods/get_block_template_rpcs/types/get_block_template/proposal.rs Outdated Show resolved Hide resolved
teor2345
teor2345 previously approved these changes Jan 29, 2023
View reviewed changes
Copy link
Collaborator

@teor2345 teor2345 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think all the changes we've suggested are optional.

zebra-rpc/src/methods/get_block_template_rpcs/types/get_block_template/proposal.rs Show resolved Hide resolved
zebra-rpc/src/methods/get_block_template_rpcs/types/get_block_template/proposal.rs Show resolved Hide resolved
zebra-rpc/src/methods/get_block_template_rpcs/types/get_block_template/proposal.rs Outdated Show resolved Hide resolved
zebra-rpc/src/methods/get_block_template_rpcs/types/get_block_template/proposal.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 mentioned this pull request Jan 30, 2023
Closed
@github-actions github-actions bot added the C-bug Category: This is a bug label Feb 1, 2023
arya2
arya2 previously approved these changes Feb 1, 2023
View reviewed changes
teor2345
teor2345 approved these changes Feb 1, 2023
View reviewed changes
Copy link
Collaborator

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks!

@oxarbitrage
Copy link
Contributor Author

There is an error in the CI but i think is unrelated to this PR so i am retrying.

Error is for macos only in the v4_with_modified_joinsplit_is_rejected test: https://github.com/ZcashFoundation/zebra/actions/runs/4068002665/jobs/7006036670#step:14:1929

@teor2345 teor2345 mentioned this pull request Feb 1, 2023
Closed
@teor2345
Copy link
Collaborator

teor2345 commented Feb 1, 2023

There is an error in the CI but i think is unrelated to this PR so i am retrying.

Error is for macos only in the v4_with_modified_joinsplit_is_rejected test: https://github.com/ZcashFoundation/zebra/actions/runs/4068002665/jobs/7006036670#step:14:1929

This seems to be bug #5823, which is unrelated.

mergify bot added a commit that referenced this pull request Feb 1, 2023
@mergify
Merge of #6044
44e89d6
@mergify mergify bot mentioned this pull request Feb 1, 2023
Closed
@mergify mergify bot merged commit b327d5b into main Feb 2, 2023
@mergify mergify bot deleted the issue5981 branch February 2, 2023 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 approved these changes

@arya2 arya2 arya2 left review comments

Assignees

@oxarbitrage oxarbitrage

Labels
A-compatibility Area: Compatibility with other nodes or wallets, or standard rules A-rpc Area: Remote Procedure Call interfaces C-bug Category: This is a bug C-enhancement Category: This is an improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use the same format as zcashd for getblocktemplate proposal mode errors
3 participants
@oxarbitrage @teor2345 @arya2