Add subscriptions support #225

zmanian · 2020-04-17T19:22:20Z

To do

integration tests
Typed events

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

* 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>

fix /abci_info & /genesis: - add app_version & use #[serde(default)] to deal with omitted fields in JSON - make app_state in /genesis reply optional - fix string in abci_info test (kvstore wont reply with "GaiaApp") verify tests pass via running `tendermint node --proxy_app=kvstore` and `cargo test -- --nocapture --ignored

…or CI

…int-rs into tendermint/v0.33

liamsi · 2020-05-23T10:16:15Z

tendermint/src/rpc/event_listener.rs

+        match serde_json::from_str::<JsonRPCBlockResult>(&msg.to_string()) {
+            Ok(data) => {
+                if let Some(block_result) = data.0.result {
+                    Ok(Event::JsonRPCBlockResult(Box::new(block_result)))
+                } else {
+                    // The Websocket should never send an empty block
+                    panic!("Websocket sent us an empty block")
+                }
+            }
+            Err(_) => match serde_json::from_str::<JsonRPCTransactionResult>(&msg.to_string()) {
+                Ok(data) => {
+                    if let Some(tx_result) = data.0.result {
+                        Ok(Event::JsonRPCTransactionResult(Box::new(tx_result)))
+                    } else {
+                        // The Websocket should never send an empty transaction
+                        panic!("Websocket sent us an empty transaction")
+                    }
+                }
+                Err(_) => match serde_json::from_str::<serde_json::Value>(&msg.to_string()) {
+                    Ok(json_value) => Ok(Event::GenericJSONEvent(json_value)),
+                    Err(_) => Ok(Event::GenericStringEvent(msg.to_string())),
+                },


I'm pretty sure this can be simplified in a way that serde returns the correct Event enum variant and you don't need matching here again. Non-blocking for merging this PR though.

tendermint/src/rpc/event_listener.rs

- unexport Wrapper fields

liamsi · 2020-05-23T10:58:02Z

Any idea why the integration test receives a GenericJSONEvent (which basically looks like a JsonRPCBlockResult)?
https://github.com/informalsystems/tendermint-rs/pull/225/checks?check_run_id=701830448#step:5:266

You were saying that this does not fail against your local gaia build? Which (tendermint) version does that use?

liamsi · 2020-05-23T11:30:20Z

I'll try to figure out what is going on with the integration test this afternoon and we should be ready to merge.

zmanian · 2020-05-23T13:31:39Z

So the integration test fails if Serde fails to deserialize the Block event as a expected type and instead falls through to a Generic JSON event.

zmanian · 2020-05-23T13:32:11Z

The GOZ code is running

gaiad tendermint version
tendermint: 0.33.4
abci: 0.16.2
blockprotocol: 10
p2pprotocol: 7

and the integration test appears to pass with this version.

liamsi · 2020-05-23T13:59:10Z

So the integration test fails if Serde fails to deserialize the Block event as a expected type and instead falls through to a Generic JSON event.

Exactly. The reason why it's falling through is Error("missing field events", line: 61, column: 32). But interestingly, the JSON payload has that field 🤔

I doubt this has anything to do with different tendermint versions.

This reverts commit 3269d58

liamsi · 2020-05-23T14:45:58Z

tendermint/src/rpc/event_listener.rs

+                dbg!("Error:");
+                dbg!(e);
+                dbg!("msg:");
+                dbg!(&msg.to_string());


These lines should be removed before merging.

Suggested change

dbg!("Error:");

dbg!(e);

dbg!("msg:");

dbg!(&msg.to_string());

liamsi · 2020-05-23T14:50:32Z

tendermint/src/rpc/event_listener.rs

+        match serde_json::from_str::<JsonRPCBlockResult>(&msg.to_string()) {
+            Ok(data) => {
+                let block_result = data.into_result()?;
+                Ok(Event::JsonRPCBlockResult(Box::new(block_result)))
+            }
+            Err(e) => {
+                dbg!("Error:");
+                dbg!(e);
+                dbg!("msg:");
+                dbg!(&msg.to_string());
+                match serde_json::from_str::<JsonRPCTransactionResult>(&msg.to_string()) {
+                    Ok(data) => {
+                        let tx_result = data.into_result()?;
+                        Ok(Event::JsonRPCTransactionResult(Box::new(tx_result)))
+                    }
+                    Err(_) => match serde_json::from_str::<serde_json::Value>(&msg.to_string()) {
+                        Ok(json_value) => Ok(Event::GenericJSONEvent(json_value)),
+                        Err(_) => Ok(Event::GenericStringEvent(msg.to_string())),
+                    },
+                }


I think ideally, we would not match case a lot here but instead of jsonrpc wrapping JsonRPCBlockResult, JsonRPCTransactionResult separately, we should wrap the whole Event enum and make it impl response::Response. serde can very well deal with enums so there is no need to try each variant separately. No need to block this PR on this though.

liamsi

Let's merge this and deal with these in followup issues:

https://github.com/informalsystems/tendermint-rs/pull/225/files#r429444086
#225 (comment)
#225 (comment)

Thanks @zmanian 💪 Thanks @greg-szabo for helping with the debugging too!

Shivani912 and others added 30 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

fix error

525b419

more json bisection tests

ae3ee39

abci_info struct app_version implemented

0e6a073

Fixed fmt

1a9a96e

Updated integration tests to tendermint v0.33 node

a3367f3

update block result response

d0b9039

changes to abci_info and block_results

accb997

Removed CircleCI reference and added codecov reference to Cargo

e4918c8

removing unwanted Option from DeliverTx struct fields

e0bce41

last_block_app_hash is now Option<Vec<u8>>

06d91d4

AbciInfo.last_block_app_hash is base64 encoded instead of hex

af430df

last_block_app_hash changed to Vec<u8>

0ad0537

[88] Fix JSON ID

323b44a

[192] Fix abci_info compatibility for v0.33

95902c6

[4] block_results test fix

c0c3ed7

[184][120] Tendermint v0.33 compatibility fixes + integration tests f…

3659a30

…or CI

Added one abci_query test

6c589c9

FMT + get rid of warnings

2bf6125

Relaxed genesis struct AppHash requirements from strict hex to anything

5e9b62a

Removed unused Protocol struct

0d4952d

Removed unused Protocol struct

58411ac

Merge branch 'tendermint/v0.33' of github.com:informalsystems/tenderm…

2743f8b

…int-rs into tendermint/v0.33

IdType removed and JSON ID made as enum

7649289

Duration looks better now

f0048c5

liamsi reviewed May 23, 2020

View reviewed changes

tendermint/src/rpc/event_listener.rs Outdated Show resolved Hide resolved

use rpc::response::Response and improve err handling

b11ea9a

- unexport Wrapper fields

liamsi added 4 commits May 23, 2020 13:14

Fix hyperlink in doc comment

f9b8899

Update link on error codes

4a4d9cb

Make error codes symmetric

6dbe426

Fix panic string

42b5cfc

liamsi added 2 commits May 23, 2020 15:45

Add some debug output

3193c73

Add some more debug output

81748e7

liamsi added 3 commits May 23, 2020 16:04

Type alias instead of wrapping the wrapper

fc8fc43

Revert "DRY Block, BlockData, Evidence, Parts"

0cb9ea3

This reverts commit 3269d58

DRY Block, BlockData, Evidence, Parts

fdf8873

liamsi reviewed May 23, 2020

View reviewed changes

liamsi added 6 commits May 23, 2020 17:08

Let's try an Option

31e48a8

Another Option?

86e3f6a

These events are actually optional

c2711e8

typo

0efbcb8

Let it be

04053f5

Fix match arm

9b77399

liamsi approved these changes May 23, 2020

View reviewed changes

liamsi merged commit d28c4b1 into informalsystems:master May 23, 2020

liamsi deleted the zaki-event-subscribe branch May 23, 2020 16:17

greg-szabo mentioned this pull request May 23, 2020

Event Subscription response cannot be deserialized correctly #278

Closed

liamsi mentioned this pull request May 24, 2020

Simplify event subscription and fix serialization issue (#278) #279

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add subscriptions support #225

Add subscriptions support #225

zmanian commented Apr 17, 2020 •

edited

Loading

liamsi May 23, 2020

liamsi commented May 23, 2020

liamsi commented May 23, 2020

zmanian commented May 23, 2020

zmanian commented May 23, 2020

liamsi commented May 23, 2020 •

edited

Loading

liamsi May 23, 2020

liamsi May 23, 2020

liamsi left a comment

Add subscriptions support #225

Add subscriptions support #225

Conversation

zmanian commented Apr 17, 2020 • edited Loading

liamsi May 23, 2020

Choose a reason for hiding this comment

liamsi commented May 23, 2020

liamsi commented May 23, 2020

zmanian commented May 23, 2020

zmanian commented May 23, 2020

liamsi commented May 23, 2020 • edited Loading

liamsi May 23, 2020

Choose a reason for hiding this comment

liamsi May 23, 2020

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

zmanian commented Apr 17, 2020 •

edited

Loading

liamsi commented May 23, 2020 •

edited

Loading