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.

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

rpc: Allow empty JSON object value for consensus_param_updates in /block_results response #1441

Merged
merged 6 commits into from
Jul 11, 2024

Conversation

ljoss17
Copy link
Contributor

@ljoss17 ljoss17 commented Jul 11, 2024

Closes: #1440

This PR adds a custom deserializer for the consensus_param_updates field in the /block_results response to deserialize both null and an empty JSON object to None.

E.g. both of these will successfully deserialize to None:

  • {"consensus_param_updates": {}}

  • {"consensus_param_updates": null}

  • Referenced an issue explaining the need for the change

  • Updated all relevant documentation in docs

  • Updated all code comments where relevant

  • Wrote tests

  • Added entry in .changelog/

@ljoss17 ljoss17 added the bug Something isn't working label Jul 11, 2024
@ljoss17 ljoss17 requested a review from romac July 11, 2024 10:18
@romac
Copy link
Member

romac commented Jul 11, 2024

Thanks! Can we also fix this while we are at it?

#[serde(skip)] // FIXME: kvstore /genesis returns '{}' instead of '{app_version: "0"}'
pub version: Option<VersionParams>,

@ljoss17
Copy link
Contributor Author

ljoss17 commented Jul 11, 2024

Thanks! Can we also fix this while we are at it?

#[serde(skip)] // FIXME: kvstore /genesis returns '{}' instead of '{app_version: "0"}'
pub version: Option<VersionParams>,

Oh I missed that sorry. I'll fix it

@@ -35,6 +35,7 @@ pub mod chain;
pub mod channel;
pub mod consensus;
pub mod crypto;
pub mod deserializers;
Copy link
Member

Choose a reason for hiding this comment

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

Let's add it to the serializers instead, to help discovery.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 568f843

@romac romac merged commit 67c7923 into main Jul 11, 2024
23 checks passed
@romac romac deleted the luca_joss/allow-empty-json-consensus-params branch July 11, 2024 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rpc: Deserializing /block_results with empty JSON consensus_param_updates fails
2 participants