-
Notifications
You must be signed in to change notification settings - Fork 224
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
rpc: event subscription management for RPC client #516
Changes from 51 commits
9df13ed
a01883d
a03d0ab
73c2626
8650db1
cf67215
20cce0a
92cae5f
40e7de2
4634e48
82cf571
52c36d0
1089e8d
5994999
1ef2305
a364187
15d5765
7e6d15d
da0247a
cafa8d7
9ad109b
ecb099f
cf56f7b
ae66f90
d3d2c32
f01d13d
279913b
6d35180
2228b71
8bc9f7f
f017d27
233fd62
36e2969
83d4835
ab4ba9b
b160e65
86ed3d9
fd01278
3eadccd
a4c5a4d
1c82638
772bcd0
1d08a7d
3048155
5e9e351
99dae2a
ba2ca31
b4253c6
b9e240e
99a321f
3e38b0b
58d52ef
e218dd6
ee70331
e7bb6a1
a891c21
c22a5da
1753566
4db8d9e
e293693
e7403a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,9 +2,11 @@ | |
|
||
use contracts::{contract_trait, post}; | ||
use serde::{Deserialize, Serialize}; | ||
use thiserror::Error; | ||
|
||
#[cfg(feature = "rpc-client")] | ||
use tendermint_rpc as rpc; | ||
#[cfg(feature = "rpc-client")] | ||
use tendermint_rpc::Client; | ||
use thiserror::Error; | ||
|
||
use crate::types::{Height, LightBlock, PeerId}; | ||
|
||
|
@@ -29,6 +31,7 @@ impl From<Height> for AtHeight { | |
/// I/O errors | ||
#[derive(Clone, Debug, Error, PartialEq, Serialize, Deserialize)] | ||
pub enum IoError { | ||
#[cfg(feature = "rpc-client")] | ||
/// Wrapper for a `tendermint::rpc::Error`. | ||
#[error(transparent)] | ||
IoError(#[from] rpc::Error), | ||
|
@@ -128,7 +131,7 @@ mod prod { | |
peer: PeerId, | ||
height: AtHeight, | ||
) -> Result<TMSignedHeader, IoError> { | ||
let rpc_client = self.rpc_client_for(peer); | ||
let rpc_client = self.rpc_client_for(peer)?; | ||
|
||
let res = block_on( | ||
async { | ||
|
@@ -161,7 +164,7 @@ mod prod { | |
}; | ||
|
||
let res = block_on( | ||
self.rpc_client_for(peer).validators(height), | ||
self.rpc_client_for(peer)?.validators(height), | ||
peer, | ||
self.timeout, | ||
)?; | ||
|
@@ -172,10 +175,11 @@ mod prod { | |
} | ||
} | ||
|
||
// TODO(thane): Generalize over client transport (instead of using HttpClient directly). | ||
ebuchman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#[pre(self.peer_map.contains_key(&peer))] | ||
fn rpc_client_for(&self, peer: PeerId) -> rpc::Client { | ||
fn rpc_client_for(&self, peer: PeerId) -> Result<rpc::HttpClient, IoError> { | ||
let peer_addr = self.peer_map.get(&peer).unwrap().to_owned(); | ||
rpc::Client::new(peer_addr) | ||
Ok(rpc::HttpClient::new(peer_addr).map_err(IoError::from)?) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe pedantic but shouldn't the PeerId type already hold a valid address and hence this shouldn't need to return an error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically a Tendermint We could do away with the need for an error here by requiring host/port details instead of a |
||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ authors = [ | |
] | ||
|
||
description = """ | ||
tenndermint-rpc contains the core types returned by a Tendermint node's RPC endpoint. | ||
tendermint-rpc contains the core types returned by a Tendermint node's RPC endpoint. | ||
All networking related features are feature guarded to keep the dependencies small in | ||
cases where only the core types are needed. | ||
""" | ||
|
@@ -24,9 +24,13 @@ description = """ | |
all-features = true | ||
|
||
[features] | ||
default = ["client"] | ||
client = ["async-tungstenite", "futures", "http", "hyper", "tokio"] | ||
secp256k1 = ["tendermint/secp256k1"] | ||
default = [] | ||
client = [ "async-trait", "futures" ] | ||
secp256k1 = [ "tendermint/secp256k1" ] | ||
subscription = [ "tokio" ] | ||
transport_http = [ "http", "hyper", "tokio" ] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this should just be consolidated with client? How much benefit do we really get by opting for the transport_mock over the transport_http ? |
||
transport_mock = [ "tokio" ] | ||
transport_websocket = [ "async-tungstenite", "tokio" ] | ||
|
||
[dependencies] | ||
bytes = "0.5" | ||
|
@@ -38,8 +42,9 @@ tendermint = { version = "0.16.0", path = "../tendermint" } | |
thiserror = "1" | ||
uuid = { version = "0.8", default-features = false } | ||
|
||
async-tungstenite = { version="0.5", features = ["tokio-runtime"], optional = true } | ||
async-tungstenite = { version="0.8", features = ["tokio-runtime"], optional = true } | ||
async-trait = { version = "0.1", optional = true } | ||
futures = { version = "0.3", optional = true } | ||
http = { version = "0.2", optional = true } | ||
hyper = { version = "0.13", optional = true } | ||
tokio = { version = "0.2", features = ["macros"], optional = true } | ||
tokio = { version = "0.2", features = ["macros", "fs", "sync"], optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might want to preserve this
default-features = false
and list the features above (line 26)?Also what's the significant of the transport_http feature? When would someone not want it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the
transport_http
feature, I'm not sure. Are there instances where someone may want to exclusively use WebSocket-based subscription, but not perform any other requests via the HTTP interface?Also, when we implement a WebSocket-based RPC client for the rest of the RPC methods (i.e. those currently only supported via HTTP), would we want to include the HTTP client by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a question about dependencies. I would expect websockets to include a superset of the http dependencies, or at least the majority ++. If there's any extra deps in the http client that are not in the websocket client I'd expect them to be minimal and so the cost of just bundling them would be small.
So,
Probably, but not sure it's worth the feature distinction to have websockets without the http.
That said we may want to preserve http without websockets.
So I would imagine three layers of use cases:
Hopefully that would simplify things. In general for now I think we should air on the side of simplicity and consolidation rather than flexibility and choice, unless we have strong existing demand/reason for flexibility in certain places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the
HttpClient
requires the use of thehyper
andhttp
crates, whereas the WebSocket-based subscription client doesn't.If you'd like to simplify, can I suggest doing away with the
subscription
feature and shortening the feature names:default
client
async-trait
,futures
,tokio
http
HttpClient
(implementsClient
)http
,hyper
,tokio
websocket
WebSocketSubscriptionClient
, and in future aWebSocketClient
, which will implement theClient
andSubscriptionClient
traits and will supersede theWebSocketSubscriptionClient
async-tungstenite
,tokio
mock
MockClient
andMockSubscriptionClient
tokio
Perhaps it's worth it here to rename the existing
WebSocketSubscriptionClient
toWebSocketClient
, so its name doesn't change in future?Also, we could do away with the mock clients if you want. They were useful in my testing, but it's up to downstream users (e.g. IBC) to say if they'd see value in such a mock client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For posterity, we've simplified this even further now, as per #569