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

chore: fix rpc consensus_state height_vote_set #2696

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

albttx
Copy link
Member

@albttx albttx commented Aug 14, 2024

It seems there is an error on amino parsing RoundStateSimple.Votes

Before

{
  "jsonrpc": "2.0",
  "id": "",
  "result": {
    "round_state": {
      "height/round/step": "810255/0/1",
      "start_time": "2024-08-14T10:48:40.822715298Z",
      "proposal_block_hash": null,
      "locked_block_hash": null,
      "valid_block_hash": null,
      "height_vote_set": {}
    }
  }
}

After

{
  "jsonrpc": "2.0",
  "id": "",
  "result": {
    "round_state": {
      "height/round/step": "87/0/4",
      "start_time": "2024-08-14T10:46:41.086066222Z",
      "proposal_block_hash": "RzxP5+qPE3m2TmSIPToKlXZSQrsbPpZaVDHBipWceTs=",
      "locked_block_hash": null,
      "valid_block_hash": null,
      "height_vote_set": [
        {
          "round": "0",
          "prevotes": [
            "Vote{0:0E33DB6910E5 87/00/1(Prevote) 473C4FE7EA8F D1B277E50809 @ 2024-08-14T10:46:41.115790322Z}",
            "Vote{1:102A3FB16690 87/00/1(Prevote) 473C4FE7EA8F 05871AA71823 @ 2024-08-14T10:46:41.010870819Z}",
            "nil-Vote"
          ],
          "prevotes_bit_array": "BA{3:xx_} 2/3 = 0.67",
          "precommits": [
            "nil-Vote",
            "nil-Vote",
            "nil-Vote"
          ],
          "precommits_bit_array": "BA{3:___} 0/3 = 0.00"
        },
        {
          "round": "1",
          "prevotes": [
            "nil-Vote",
            "nil-Vote",
            "nil-Vote"
          ],
          "prevotes_bit_array": "BA{3:___} 0/3 = 0.00",
          "precommits": [
            "nil-Vote",
            "nil-Vote",
            "nil-Vote"
          ],
          "precommits_bit_array": "BA{3:___} 0/3 = 0.00"
        }
      ]
    }
  }

After the fix, i realized that tendermint1 latest version is using json too on this field

https://github.com/tendermint/tendermint/blob/35581cf54ec436b8c37fabb43fdaa3f48339a170/rpc/jsonrpc/types/types.go#L187-L200

@albttx albttx self-assigned this Aug 14, 2024
@github-actions github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Aug 14, 2024
@albttx albttx force-pushed the fix/tm2-rpc-consensus-state branch from 8176ccb to c5653e1 Compare August 14, 2024 10:50
@albttx albttx force-pushed the fix/tm2-rpc-consensus-state branch from c5653e1 to 1e1efc9 Compare August 14, 2024 10:51
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.15%. Comparing base (c606efe) to head (d6f4813).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2696      +/-   ##
==========================================
- Coverage   60.15%   60.15%   -0.01%     
==========================================
  Files         561      561              
  Lines       74999    74999              
==========================================
- Hits        45119    45112       -7     
- Misses      26505    26509       +4     
- Partials     3375     3378       +3     
Flag Coverage Δ
contribs/gnodev 60.58% <ø> (-0.82%) ⬇️
contribs/gnofaucet 14.46% <ø> (-0.86%) ⬇️
gnovm 64.13% <ø> (ø)
misc/genstd 80.54% <ø> (ø)
misc/logos 20.23% <ø> (+0.35%) ⬆️
tm2 62.03% <100.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@albttx albttx changed the title Fix/tm2 rpc consensus state chore: fix rpc consensus_state height_vote_set Aug 14, 2024
@@ -194,7 +194,7 @@ func NewRPCSuccessResponse(id JSONRPCID, res any) RPCResponse {

if res != nil {
var js []byte
js, err := amino.MarshalJSON(res)
js, err := json.Marshal(res)
Copy link
Member

Choose a reason for hiding this comment

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

Can we figure out why the Amino JSON is causing an empty object? What is the type of res?
Maybe we just need to register the package / type with Amino

It's now causing tests to fail, most likely because we use Amino JSON to unmarshal all responses / requests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related
Projects
Status: In Progress
Status: In Review
Development

Successfully merging this pull request may close these issues.

2 participants