New commit struct! #181

Shivani912 · 2020-03-12T20:28:13Z

closes #145

NOTE: I've updated the commit struct here but as a result we'll have to give up compatibility with current Gaia (assuming they're still using the older commit struct)

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGES.md

Shivani912 · 2020-03-12T20:30:32Z

@liamsi There are rpc JSON tests here that are failing because of the new commit struct. I don't have pointers to the code that generated those files. Do you want to take it from here?

Also, would appreciate your early review!

liamsi · 2020-03-12T21:01:24Z

Thanks @Shivani912! I'll review and look into the failing tests tomorrow! I'm also not sure from what they where generated; the problem is that a bunch of these aren't up to date with the latest tendermint even without the commit changes.
Note to myself:
failures:

endpoints::block
endpoints::commit
endpoints::first_block

Shivani912 · 2020-03-12T21:13:56Z

@liamsi Ahan! Time for more tests then, haha 🙃

tendermint/src/account.rs

Shivani912 · 2020-03-12T21:16:26Z

Note: All the JSON files have been re-generated so they are compatible with the latest changes. Link to generator updated in code!

liamsi · 2020-03-13T17:17:01Z

It looks like the RPC tests where either copy & pasted from the documentation, or, dumped via a modified fork. I can't find any code that generated the RPC JSON tests either. @tarcieri can you confirm that these files weren't generated with some tool in tendermint/gaiad?

tarcieri · 2020-03-13T17:18:44Z

@liamsi the JSON used in the original RPC unit tests was taken from a cosmoshub-2 full node

liamsi · 2020-03-16T01:04:10Z

tendermint/tests/rpc.rs

-            .is_none());
-    }
+    // NOTE: Since the commit struct changed, the votes i.e. CommitSig no longer contains BlockID
+    // TODO: Do we still need this test?


No, we don't need that exact test anymore. But it would we good to translate it into the new semantic. It tested that the blockId is none. Which looks suspiciously to saying BlockIDFlagAbsent now. But I'll double check.

liamsi

The main changes in this PR look great and to the point. #182 will fix the failing tests and resolve a few open todos.

* initial new commit struct * update functions * fixed deserialization for new commit struct * updated links to generator + val_set_test file * Update tendermint/src/account.rs Co-Authored-By: Ismail Khoffi <Ismail.Khoffi@gmail.com> * fmt * clippy * rpc tests: grab latest example fro commit: - use https://docs.tendermint.com/master/rpc/#/Info/commit - update chain_id: cosmoshub-1 to cosmoshub-2 everywhere else - manually kept `id` a string in JSONrpc responses * Actually let's go cosmoshub-3: - grabbed from: https://rpc.cosmos.network/commit?height=2 * Fix commit test - regenerated commit.json via `curl -X GET "http://localhost:26657/commit?height=10" -H "accept: application/json"` * Fix block test - "regenerated" block.json via `curl -X GET "http://localhost:26657/block?height=10" -H "accept: application/json"` * Fix first_block test - "regenerated" block.json via `curl -X GET "http://localhost:26657/block?height=1" -H "accept: application/json" ` Co-authored-by: Shivani Joshi <joshi.shivani912@gmail.com> Co-authored-by: Shivani Joshi <46731446+Shivani912@users.noreply.github.com>

codecov-io · 2020-03-17T22:32:30Z

Codecov Report

Merging #181 into master will decrease coverage by 4.0%.
The diff coverage is 44.9%.

@@           Coverage Diff            @@
##           master    #181     +/-   ##
========================================
- Coverage    42.0%   38.0%   -4.1%     
========================================
  Files          88      98     +10     
  Lines        3051    3546    +495     
  Branches      470     563     +93     
========================================
+ Hits         1283    1348     +65     
- Misses       1431    1850    +419     
- Partials      337     348     +11

Impacted Files	Coverage Δ
tendermint/src/account.rs	`46.0% <ø> (-8.7%)`	⬇️
tendermint/src/block.rs	`0.0% <ø> (ø)`
tendermint/src/block/header.rs	`5.5% <ø> (-4.5%)`	⬇️
tendermint/src/genesis.rs	`0.0% <0.0%> (ø)`
tendermint/src/lite/types.rs	`45.1% <ø> (ø)`
tendermint/src/lite_impl/header.rs	`89.3% <ø> (-0.5%)`	⬇️
tendermint/src/rpc/endpoint/abci_query.rs	`0.0% <ø> (ø)`
tendermint/src/rpc/endpoint/block.rs	`0.0% <0.0%> (ø)`
tendermint/src/serializers.rs	`0.0% <0.0%> (ø)`
tendermint/src/block/commit_sig.rs	`28.5% <28.5%> (ø)`
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50cd585...ae3ee39. Read the comment docs.

liamsi · 2020-03-18T12:07:09Z

Nice, the tests @greg-szabo added fired 👍 (integration tests against a tendermint node that is)

greg-szabo · 2020-03-19T02:14:17Z

I looked through the failing integration tests and found the following work to do so far:

abci_info: app_version was missing, I've just added that (it's quite verbose, we might want to talk about it)
abci_info: still fails with unknown parsing error, needs more checks
block_results: missing fields: TxsResults, BeginBlockEvents, EndBlockEvents, ValidatorUpdates, ConsensusParamUpdates. The old "results" field need to be removed.
genesis, evidence Params: missing fields: max_age_num_blocks, max_age_duration. The old field "max_age" has to be removed.

Shivani912 · 2020-03-19T18:40:39Z

After a discussion with @greg-szabo, I'm closing this pull request. Since Gaia is using Tendermint v0.32 and we don't want to give up compatibility with it, the master branch will stay as is i.e. compatible with Tendermint v0.32. So, all the v0.33 related upgrades will stay in a separate branch. That means, this branch will be compatible with Tendermint v0.33.

ebuchman · 2020-03-28T21:58:08Z

@Shivani912 we reverted this decision right? We're going to update master to tendermint v0.33 and drop compatibility with current gaia?

Shivani912 · 2020-03-30T14:40:21Z

@Shivani912 we reverted this decision right? We're going to update master to tendermint v0.33 and drop compatibility with current gaia?

Yes, that's right @ebuchman. For a record, the progress is being posted in issue #184

Shivani912 requested a review from liamsi March 12, 2020 20:49

liamsi reviewed Mar 12, 2020

View reviewed changes

tendermint/src/account.rs Outdated Show resolved Hide resolved

liamsi mentioned this pull request Mar 15, 2020

rpc: incompatibilities #88

Closed

liamsi reviewed Mar 16, 2020

View reviewed changes

liamsi mentioned this pull request Mar 16, 2020

Support for 0.33 #184

Closed

Shivani912 added 4 commits March 17, 2020 17:52

initial new commit struct

bd3430e

update functions

d129ad2

fixed deserialization for new commit struct

b64840b

updated links to generator + val_set_test file

b893a25

Shivani912 force-pushed the new-commit-struct branch from cc3aacc to b893a25 Compare March 17, 2020 21:55

liamsi and others added 2 commits March 17, 2020 18:18

fix error

525b419

more json bisection tests

ae3ee39

abci_info struct app_version implemented

0e6a073

Fixed fmt

1a9a96e

Shivani912 closed this Mar 19, 2020

Shivani912 deleted the new-commit-struct branch March 19, 2020 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New commit struct! #181

New commit struct! #181

Shivani912 commented Mar 12, 2020

Shivani912 commented Mar 12, 2020

liamsi commented Mar 12, 2020 •

edited

Loading

Shivani912 commented Mar 12, 2020

Shivani912 commented Mar 12, 2020

liamsi commented Mar 13, 2020

tarcieri commented Mar 13, 2020

liamsi Mar 16, 2020

liamsi left a comment

codecov-io commented Mar 17, 2020 •

edited

Loading

liamsi commented Mar 18, 2020 •

edited

Loading

greg-szabo commented Mar 19, 2020

Shivani912 commented Mar 19, 2020 •

edited

Loading

ebuchman commented Mar 28, 2020

Shivani912 commented Mar 30, 2020

New commit struct! #181

New commit struct! #181

Conversation

Shivani912 commented Mar 12, 2020

Shivani912 commented Mar 12, 2020

liamsi commented Mar 12, 2020 • edited Loading

Shivani912 commented Mar 12, 2020

Shivani912 commented Mar 12, 2020

liamsi commented Mar 13, 2020

tarcieri commented Mar 13, 2020

liamsi Mar 16, 2020

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 17, 2020 • edited Loading

Codecov Report

liamsi commented Mar 18, 2020 • edited Loading

greg-szabo commented Mar 19, 2020

Shivani912 commented Mar 19, 2020 • edited Loading

ebuchman commented Mar 28, 2020

Shivani912 commented Mar 30, 2020

liamsi commented Mar 12, 2020 •

edited

Loading

codecov-io commented Mar 17, 2020 •

edited

Loading

liamsi commented Mar 18, 2020 •

edited

Loading

Shivani912 commented Mar 19, 2020 •

edited

Loading