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

rpc: Add structured querying for event subscriptions #584

Merged
merged 13 commits into from
Oct 7, 2020
Merged

Conversation

thanethomson
Copy link
Contributor

@thanethomson thanethomson commented Sep 24, 2020

Partially addresses #582.

This PR adds structured querying to RPC event subscription, which replaces the String-based interface for subscription.

No query parsing is implemented yet, as it's not clear yet whether we need it at the moment and we need to figure out how to translate the query PEG from the Go version of Tendermint.

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

This commit adds structured querying to RPC event subscription, which
replaces the `String`-based interface for subscription.

No query parsing is implemented yet.

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
This merges `master` into `rpc/query` while also introducing a locking
mechanism in the tests such that no two integration tests that perform
transaction subscriptions to a live Tendermint node can run at the same
time.

Without this locking mechanism, Tokio executes the two tests
simultaneously, which causes non-deterministic results.

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson thanethomson marked this pull request as ready for review October 6, 2020 20:30
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@6a28a90). Click here to learn what that means.
The diff coverage is 70.5%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #584   +/-   ##
========================================
  Coverage          ?   41.8%           
========================================
  Files             ?     164           
  Lines             ?   11704           
  Branches          ?    2649           
========================================
  Hits              ?    4898           
  Misses            ?    6438           
  Partials          ?     368           
Impacted Files Coverage Δ
rpc/src/client/subscription.rs 72.3% <0.0%> (ø)
rpc/src/event.rs 11.6% <0.0%> (ø)
tendermint/tests/integration.rs 0.0% <0.0%> (ø)
rpc/src/query.rs 75.2% <75.2%> (ø)
rpc/src/client/transport/websocket.rs 72.2% <83.3%> (ø)
rpc/src/client/transport/mock/subscription.rs 78.7% <85.7%> (ø)

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 6a28a90...39e8a79. Read the comment docs.

@romac
Copy link
Member

romac commented Oct 7, 2020

@thanethomson Great stuff!

I took the liberty pushing a few non-controversial changes (imho):

The last commit (27fe514) merges the Operation and Condition structs together, which I personally find simpler even though it distributes the key into each variant of the enum. I don't feel strongly about this and only did this as a suggestion so let me know if you'd rather keep the original version and I'll revert the commit.

@romac
Copy link
Member

romac commented Oct 7, 2020

Looks like ecdsa fails to build on Nightly. /cc @tony-iqlusion

2020-10-07T08:16:01.4214668Z    Compiling ecdsa v0.8.3
2020-10-07T08:16:01.8028615Z error[E0277]: cannot multiply `&NonZeroScalar<C>` to `<C as ProjectiveArithmetic>::ProjectivePoint`
2020-10-07T08:16:01.8035228Z ##[error]  --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/ecdsa-0.8.3/src/sign.rs:90:58
2020-10-07T08:16:01.8043918Z    |
2020-10-07T08:16:01.8044748Z 90 |             public_key: (C::ProjectivePoint::generator() * &self.secret_scalar).to_affine(),
2020-10-07T08:16:01.8045596Z    |                                                          ^ no implementation for `<C as ProjectiveArithmetic>::ProjectivePoint * &NonZeroScalar<C>`
2020-10-07T08:16:01.8046174Z    |
2020-10-07T08:16:01.8046773Z    = help: the trait `Mul<&NonZeroScalar<C>>` is not implemented for `<C as ProjectiveArithmetic>::ProjectivePoint`
2020-10-07T08:16:01.8047526Z help: consider further restricting the associated type
2020-10-07T08:16:01.8047864Z    |
2020-10-07T08:16:01.8048427Z 87 |         AffinePoint<C>: Clone + Debug, <C as ProjectiveArithmetic>::ProjectivePoint: Mul<&NonZeroScalar<C>>
2020-10-07T08:16:01.8049018Z    |                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2020-10-07T08:16:01.8049204Z 
2020-10-07T08:16:01.8476530Z error: aborting due to previous error
2020-10-07T08:16:01.8477619Z ##[error]aborting due to previous error
2020-10-07T08:16:01.8479396Z For more information about this error, try `rustc --explain E0277`.
2020-10-07T08:16:01.8636042Z error: could not compile `ecdsa`

@thanethomson
Copy link
Contributor Author

Excellent, thanks @romac! Looks great - I like the simplification 👍

Are there any clippy lints we can enable that can warn us about there being more idiomatic ways to implement specific pieces of code?

@romac
Copy link
Member

romac commented Oct 7, 2020

Not to my knowledge. Closest lint I could find is https://rust-lang.github.io/rust-clippy/master/#inherent_to_string but it would not have helped in this case.

I guess there are valid use cases for implementing ToString or defining a From instance directly which is why Clippy does not warn about that, or perhaps those lints are just currently missing.

@tony-iqlusion
Copy link
Collaborator

tony-iqlusion commented Oct 7, 2020

My best guess for the ecdsa breakage on nightly is something broken with Deref rules.

I can try to repro it locally and see if there's a quick fix to work around the issue. This might also be worth reporting upstream.

Edit: reproduced the issue locally and confirmed that an explicit deref fixed the issue. I can push up a fix (which might make the behavior a bit more explicit/straightforward in general)

@thanethomson thanethomson merged commit 460a492 into master Oct 7, 2020
@thanethomson thanethomson deleted the rpc/query branch October 7, 2020 15:31
@tony-iqlusion
Copy link
Collaborator

Plausible cause of the nightly regression: rust-lang/rust#77638

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.

4 participants