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

Tendermint RPC 0.34 compatibility API #1143

Closed
wants to merge 16 commits into from

Conversation

mzabaluev
Copy link
Contributor

@mzabaluev mzabaluev commented Jun 10, 2022

Add a feature-gated v0_34 module in the tendermint-rpc crate, providing compatibility with Tendermint nodes using the RPC protocol as implemented in Tendermint Go version 0.34.

The v0_34 module provides APIs that exhibit the protocol differences, mainly due to the changes in the subscription event data structure:

  • Event
  • Subscription
  • SubscriptionClient
  • WebSocketClient
  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Added entry in .changelog/

Sorry, something went wrong.

@mzabaluev mzabaluev added the rpc label Jun 10, 2022
@mzabaluev mzabaluev requested review from romac and thanethomson June 10, 2022 09:33
Start on the tendermint 0.34 legacy support, gated by "tendermint-0.34"
feature.
The most naive copy of the implementation, swapping out Event and
Subscription* for their v0_34 counterparts.
This will be the basis for core reuse between WebSocketClient for
different protocol versions.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Share all subscription management code between the two
WebSocketClient implementations by making it generic about the
event type.
Test the parsing of v0_34::event::Event in the kvstore_fixtures tests,
and tell the v0_34::WebSocketClient test to load the v0.34 fixtures.
@mzabaluev mzabaluev force-pushed the mikhail/tendermint-0.34-compat branch from 865661a to 781dbfa Compare June 10, 2022 12:20
@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2022

Codecov Report

Merging #1143 (de97d2a) into master (b3d31a5) will increase coverage by 0.2%.
The diff coverage is 62.3%.

@@           Coverage Diff            @@
##           master   #1143     +/-   ##
========================================
+ Coverage    64.7%   65.0%   +0.2%     
========================================
  Files         232     238      +6     
  Lines       15667   16046    +379     
========================================
+ Hits        10142   10432    +290     
- Misses       5525    5614     +89     
Impacted Files Coverage Δ
light-client/src/errors.rs 84.2% <ø> (ø)
rpc/src/client.rs 4.9% <ø> (ø)
rpc/src/client/sync.rs 73.6% <0.0%> (-13.9%) ⬇️
rpc/src/lib.rs 100.0% <ø> (ø)
rpc/src/v0_34/endpoint/tx.rs 14.2% <14.2%> (ø)
rpc/src/v0_34/event.rs 40.0% <40.0%> (ø)
rpc/src/client/transport/websocket/plumbing.rs 46.3% <46.3%> (ø)
rpc/src/v0_34/client/subscription.rs 50.0% <50.0%> (ø)
rpc/src/v0_34/serializers/hash_base64.rs 52.1% <52.1%> (ø)
rpc/src/client/transport/router.rs 75.8% <59.0%> (+0.8%) ⬆️
... and 11 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 b3d31a5...de97d2a. Read the comment docs.

The encoding of the hash field in the request has changed, so
we need to use a legacy endpoint struct.
@mzabaluev mzabaluev marked this pull request as ready for review June 13, 2022 10:46
The websocket client is properly enabled by the "websocket-client",
feature, in addition to the whole v0_34 module enabled by the
"tendermint-0.34" feature.
Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

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

Wow, this seems like a substantial amount of work @mzabaluev, thanks so much! I didn't realize we'd need to make the client machinery generic over the data structures that differ between Tendermint versions, but it totally makes sense.

Two questions, and it'd be great to also have a changelog entry for this, but otherwise LGTM 👍

rpc/Cargo.toml Outdated Show resolved Hide resolved
/// A client that exclusively provides [`Event`] subscription capabilities,
/// without any other RPC method support.
#[async_trait]
pub trait SubscriptionClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

One of the benefits of this approach here is that the API can evolve across different versions of Tendermint, but one of the drawbacks is that callers need to refer to Tendermint version-specific client traits.

An alternative approach here would be to have one SubscriptionClient trait for all supported versions of Tendermint, and have it be generic over the Event type produced by the subscription client. Then the caller could also be generic over the event type. I'd be interested to know whether you considered and ruled out such an approach? (I haven't actually done the hard work here, so I'm not aware yet of the ripple effects throughout the codebase of such an approach.)

Copy link
Contributor Author

@mzabaluev mzabaluev Jun 14, 2022

Choose a reason for hiding this comment

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

I think it's possible to introduce Subscription and/or Event as associated types for the unified SubscriptionClient trait, but I was not sure if the more substantial API change is desirable. Currently, v0_34 can be treated like a bolt-on with no complications to the definitions elsewhere. In the relayer, we'll have to explicitly specialize for both event types anyway, because parsing for old subscription events has to be different (it is a good thing to have this changed for 0.35).

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense 👍 I'd say we should keep your changes as-is then, and we can always look at merging the traits at some point in a future version of tendermint-rs if necessary.

Renamed from "tendermint-0.34" to "v0_34", to mirror the name
of the top-level module.
@mzabaluev
Copy link
Contributor Author

mzabaluev commented Jun 14, 2022

In a meeting with folks from the Tendermint team at Interchain, it has been suggested that we don't need to expose the pre-0.35 event data structure and its separate subscription stream API, what we can instead do is transform the "flattened" key-value map into a list of ABCI events using the knowledge of how Tendermint 0.34 nodes encode event types and tags into the flattened map.

With this, there are fewer reasons to provide a statically different v0_34 API. The remaining ones that come to my mind are:

  • Not hiding dynamically switched behaviour that would be needed to support 0.35+ and 0.34 dialects through the single API. The performance impact of that should not be something to worry about, though.
  • Statically expose the lack of /block_by_hash endpoint in 0.34, by having a v0_34::Client trait that lacks the block_by_hash method. Not sure how useful that would be in practice.
  • As long as 0.34 support is feature-gated, altering behaviour via compile-time features is a bit of an anti-pattern.

@mzabaluev
Copy link
Contributor Author

I'm closing this for now due to lack of urgent need for this change (given non-adoption of Tendermint 0.35 in the upcoming release of Cosmos SDK) and the alternative idea of transforming the pre-0.35 RPC subscription event data into the uniform current API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants