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

[Experiment] repro: comet query client #74

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Oct 18, 2023

Summary

Human Summary

This branch is an attempt to replace our "custom" comet RPC client for event subscriptions (using the gorilla websocket package). As noted above the QueryClient interface definition in pkg/client/interface.go, such a thing seems to exist in the cometbft repo; however, my attempts at getting it to work, for our use case at least have been unsuccessful. I think there's likely something basic that I'm misunderstanding or an assumption that I don't realize I'm making. 🤔

AI Summary

Summary generated by Reviewpad on 26 Oct 23 18:51 UTC

This pull request implements a new HTTP RPC client for the comet_query package. It introduces a new cometQueryClient struct that handles querying events from a remote CometBFT node. The client can subscribe to specific events and receive event data in the form of bytes. The implementation includes functions for starting the client, subscribing to events, and producing events. Additionally, a test is added to verify the functionality of the EventsBytes method.

Issue

Relates to:

Improve maintainability (owning less code).

Type of change

Select one or more:

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify): experiment / reproduction

Testing

  • Run all unit tests: make go_test
  • Verify Localnet manually: See the instructions [here](TODO: add link to instructions)

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have performed a self-review of my own code
  • I have commented my code, updated documentation and left TODOs throughout the codebase

@bryanchriswhite bryanchriswhite added the miner Changes related to the Miner label Oct 18, 2023
@bryanchriswhite bryanchriswhite added this to the Shannon MainNet milestone Oct 18, 2023
@bryanchriswhite bryanchriswhite self-assigned this Oct 18, 2023
@bryanchriswhite bryanchriswhite force-pushed the experiment/comet-query-client branch from ee5a992 to e02dd8f Compare October 18, 2023 14:31
@bryanchriswhite bryanchriswhite force-pushed the feat/query-client branch 8 times, most recently from 30b7a3c to 31099ec Compare October 23, 2023 20:09
@bryanchriswhite bryanchriswhite force-pushed the experiment/comet-query-client branch from e02dd8f to c84f055 Compare October 26, 2023 15:36
@bryanchriswhite bryanchriswhite changed the base branch from feat/query-client to pinned_base/experiment/comet-query-client October 26, 2023 15:36
@bryanchriswhite bryanchriswhite force-pushed the experiment/comet-query-client branch from c84f055 to 5d1fd7a Compare October 26, 2023 18:50
@bryanchriswhite bryanchriswhite force-pushed the experiment/comet-query-client branch from 5d1fd7a to 332f527 Compare October 26, 2023 18:51
@Olshansk
Copy link
Member

Olshansk commented Oct 26, 2023

@bryanchriswhite Can you link to this PR (#74) in the comments next to our implementation?

@@ -0,0 +1,96 @@
package comet_query
Copy link
Member

Choose a reason for hiding this comment

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

@bryanchriswhite You asked me to TAL at this in this comment but it's not something I've ever used or looked into so I honestly don't think there's any value I can provide.

Was there something specific you wanted me to look into or just be aware of this PR?

Copy link
Contributor Author

@bryanchriswhite bryanchriswhite Oct 27, 2023

Choose a reason for hiding this comment

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

That comment was a follow-up to #64 (comment), where you had asked about "why don't we use their client?" and #64 (comment) with a similar question about if we switch to theirs do we still need our interfaces. The main reason I wanted you to see this was to add clarity to my responses there.

@bryanchriswhite Can you link to this PR (#74) in the comments next to our implementation?

There already is a code comment next to the interface, which is why I was apologizing in that github comment. The base branch moved since opening #74 and made the PR unreadable. The code comment is a couple lines below where you had placed the github comment, it reads:

// NOTE: a branch which attempts this is available at:
// https://github.com/pokt-network/poktroll/pull/74

Do you have a strong opinion about having it closer to the implementation instead?

@Olshansk
Copy link
Member

Olshansk commented Nov 2, 2023

@bryanchriswhite Thoughts on closing this PR? We have it available as a reference (be it closed or not) moving forward.

@bryanchriswhite
Copy link
Contributor Author

@Olshansk no preference from me; whatever's most convenient for project management.

@Olshansk Olshansk closed this Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
miner Changes related to the Miner
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants