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

fix: Status now serializes to json #529

Closed
wants to merge 3 commits into from

Conversation

ericswanson-dfinity
Copy link
Member

@ericswanson-dfinity ericswanson-dfinity commented Mar 11, 2024

Description

Status now serializes to json everything in the values map, without the enum types

Part of https://dfinity.atlassian.net/browse/SDK-1457

Prior, serializing to json looked like this:

{
  "impl_hash": "a380266e4b6977b7cce447aa5f784efdba0bb45820d51f39b9e7908f7fbb1aa1",
  "replica_health_status": "healthy",
  "root_key": [ 48, 129, 130 ],
  "values": {
    "certified_height": {
      "Integer": 948674
    },
    "ic_api_version": {
      "String": "0.18.0"
    },
    "impl_hash": {
      "String": "a380266e4b6977b7cce447aa5f784efdba0bb45820d51f39b9e7908f7fbb1aa1"
    },
    "impl_version": {
      "String": "0.9.0"
    },
    "replica_health_status": {
      "String": "healthy"
    },
    "root_key": {
      "Bytes": [
        48,
        129,
        130
      ]
    }
  }
}

How Has This Been Tested?

Changed the test to build the Status from cbor, in order to populate the values the way that actually happens. Also tested locally with dfx ping:

$ dfx ping
{"certified_height":953309,"ic_api_version":"0.18.0","impl_hash":"a380266e4b6977b7cce447aa5f784efdba0bb45820d51f39b9e7908f7fbb1aa1","impl_version":"0.9.0","replica_health_status":"healthy","root_key":[48,129,130,48,29,6,13,43,6,1,4,1,130,220,124,5,3,1,2,1,6,12,43,6,1,4,1,130,220,124,5,3,2,1,3,97,0,141,112,44,49,204,10,52,103,112,50,213,52,220,171,58,218,10,234,157,59,85,7,80,99,161,109,225,44,206,32,66,199,182,184,50,240,80,243,131,149,39,55,219,247,20,71,75,63,16,9,217,12,111,97,242,112,231,229,216,169,214,145,220,61,185,28,221,211,199,38,126,171,26,207,140,210,43,188,187,142,140,38,131,73,28,195,49,117,148,138,203,72,51,45,93,5]}

Checklist:

  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@ericswanson-dfinity ericswanson-dfinity requested a review from a team as a code owner March 11, 2024 20:28
@ericswanson-dfinity ericswanson-dfinity enabled auto-merge (squash) March 11, 2024 20:43
@lwshang
Copy link
Contributor

lwshang commented Mar 11, 2024

The original "bug report" was that dfx ping didn't print out valid JSON. The reason is that we print the Status directly without specifying a serializing format, i.e. JSON. Then the Display impl was used.

The JSON text in the description above was indeed valid JSON and can be parsed by standard JSON tools. So I don't think it is necessary to make this custom serializer for Status.

And since the /api/v2/status endpoint returns freeform CBOR data, the types of each field is important and should be included in the serialization.


Another thing I want to point out is which fields should be pre-defined in Status.
The interface-spec only mentions root_key. Any other fields are "additional implementation-specific fields". IMO, we should remove impl_version and replica_health_status field in Status.


And there is indeed a problem in the serialization that the pre-defined fields like root_key are also included in the CBOR value map which is certainly redundant.

@ericswanson-dfinity ericswanson-dfinity deleted the ens/sdk-1457-ping-output-json branch March 12, 2024 20:49
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.

2 participants