Skip to content

Fix #1871#1873

Merged
achamayou merged 5 commits into
microsoft:masterfrom
achamayou:fix_update_state_digest_schema
Nov 13, 2020
Merged

Fix #1871#1873
achamayou merged 5 commits into
microsoft:masterfrom
achamayou:fix_update_state_digest_schema

Conversation

@achamayou

@achamayou achamayou commented Nov 10, 2020

Copy link
Copy Markdown
Member

Fix #1871

41: 16:54:14.612 | INFO     | infra.member:update_ack_state_digest:142 - [0|member0] POST /gov/ack/update_state_digest
41: 16:54:14.612 | INFO     | infra.member:update_ack_state_digest:142 - 200 @2.4 {"state_digest":"2fb5520d4d50d599f056378925de1208e2a88b81666a5cd75e1ca1ff978b8ed6"}

@achamayou achamayou requested a review from a team as a code owner November 10, 2020 11:36
Comment thread src/node/rpc/member_frontend.h Outdated
@ghost

ghost commented Nov 10, 2020

Copy link
Copy Markdown

fix_update_state_digest_schema@15406 aka 20201113.13 vs master ewma over 50 builds from 14843 to 15395
images

@achamayou

achamayou commented Nov 13, 2020

Copy link
Copy Markdown
Member Author

After discussion with @jumaffre, I'm wrapping this again in a top-level JSON object, under a single key. I think we want to apply the following principles across all node and governance RPCs:

  • JSON in/out, by default. We may accept, and return (given the right Accept:) other content types for convenience, but JSON should always be available, for:
    • Consistency when scripting/interacting from code.
    • Extensibility, adding new fields to an object is straightforward.
  • Digests are always encoded as hex-strings in JSON
  • Where we need to embed other binary values, such as quotes or requests, in JSON, they will be encoded as base64

Comment on lines +1600 to +1603
nlohmann::json j;
j["state_digest"] = ma->state_digest;

return make_success(ma->state_digest);
return make_success(j);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it possible to do return make_success(ma->state_digest); instead of constructing a JSON object?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, because the serialisation auto-unwraps state_digest, so that ends up being just a string.

@achamayou achamayou merged commit 1172765 into microsoft:master Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update state digest is incorrectly documented

2 participants