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

[Code Health] refactor: block query client interface #616

Merged
merged 101 commits into from
Jun 24, 2024

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Jun 18, 2024

Summary

Define a new interface which is a subset of cometclient.CometRPC and only contains members that poktroll depends on.

Issue

We were depending on a cometbft interface which was much larger than we need. This came while working on #402 in an integration test. Defining our own interface for this purpose is more "SOLID".

Type of change

Select one or more:

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

Testing

Documentation changes (only if making doc changes)

  • make docusaurus_start; only needed if you make doc changes

Local Testing (only if making code changes)

  • Unit Tests: make go_develop_and_test
  • LocalNet E2E Tests: make test_e2e
  • See quickstart guide for instructions

PR Testing (only if making code changes)

  • DevNet E2E Tests: Add the devnet-test-e2e label to the PR.
    • THIS IS VERY EXPENSIVE, so only do it after all the reviews are complete.
    • Optionally run make trigger_ci if you want to re-trigger tests without any code changes
    • If tests fail, try re-running failed tests only using the GitHub UI as shown here

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have commented my code
  • I have performed a self-review of my own code; both comments & source code
  • I create and reference any new tickets, if applicable
  • I have left TODOs throughout the codebase, if applicable

bryanchriswhite and others added 30 commits June 6, 2024 12:25
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
…request_probability

* pokt/main:
  [Proof Module] feat: on-chain proof window validation (#588)
…requirement_threshold

* pokt/main:
  [Proof Module] Add `proof_request_probability` param (#593)
Co-authored-by: Daniel Olshansky <[email protected]>
@bryanchriswhite bryanchriswhite force-pushed the issues/402/refactor/block-query-client branch from b4f0108 to 2ed7f2c Compare June 20, 2024 08:13
@bryanchriswhite bryanchriswhite force-pushed the issues/402/refactor/block-query-client branch from 2ed7f2c to 70b13c2 Compare June 20, 2024 08:17
@bryanchriswhite bryanchriswhite marked this pull request as ready for review June 20, 2024 08:18
@bryanchriswhite bryanchriswhite changed the base branch from issues/579/chore/telemetry to main June 20, 2024 08:43
@@ -297,3 +298,10 @@ type SharedQueryClient interface {
// the session that includes queryHeight opens.
GetProofWindowOpenHeight(ctx context.Context, queryHeight int64) (int64, error)
}

// BlockQueryClient defines an interface that enables the querying of
Copy link
Member

Choose a reason for hiding this comment

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

LGTM. No issues. Just calling out something below but not looking for a response.

Looks like we're already diverging from [1] w.r.t Naming (e.g. BlockClient vs BlockQueryClient)

I know:

  • It has different uses
  • It has different contexts
  • Functionality differs

Just calling it out.

cc @adshmh @red-0ne

[1] https://github.com/pokt-network/shannon-sdk/blob/d6cb20a34a2e50ea26f196f7c671e60a66bbc015/block.go

Copy link
Contributor

Choose a reason for hiding this comment

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

Just note that shannon-sdk's block client and poktroll's block client will diverge in functionality.

While the former will mainly be used to query block heights, the later wants the full block to get block events and txs.

So they are definitely not interchangeable.

@bryanchriswhite bryanchriswhite added push-image CI related - pushes images to ghcr.io devnet-test-e2e labels Jun 24, 2024
Copy link

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks.

You may need to run make trigger_ci to submit an empty commit that'll trigger the tests.

GCP workloads (requires changing the namespace to 616)
Grafana network dashboard for devnet-issue-{issue-id}

Copy link

The image is going to be pushed after the next commit.

You can use make trigger_ci to push an empty commit.

If you also want to run E2E tests, please add devnet-test-e2e label.

@bryanchriswhite
Copy link
Contributor Author

E2E tests passed locally but failed to run in CI:
image

@bryanchriswhite bryanchriswhite merged commit 60fda01 into main Jun 24, 2024
9 of 10 checks passed
@bryanchriswhite bryanchriswhite deleted the issues/402/refactor/block-query-client branch June 24, 2024 10:00
bryanchriswhite added a commit that referenced this pull request Jun 26, 2024
…02/refactor/relayminer

* pokt/issues/402/feat/rng:
  [Code Health] refactor: tx hash encoding (#617)
  [Docs] Relay session & claim/proof diagrams (#478)
  Empty commit
  [Code Health] refactor: block query client interface (#616)
  [TODO] chore: cleanup done TODOs (#627)
  [TODOs] chore: cleanup validate session ID TODOs (#608)
  [LocalNet] Switch to CosmosSDK defaults (#609)
  [Relay Mining] Emit events and refactor test helpers (#610)
  [Docs] Minor improvements to `adding_params.md` (#623)
  [Telemetry] chore: add probabilistic proof telemetry (#605)
bryanchriswhite added a commit that referenced this pull request Jun 26, 2024
…period

* pokt/main:
  [Code Health] refactor: random number generation (#618)
  [Code Health] refactor: tx hash encoding (#617)
  [Docs] Relay session & claim/proof diagrams (#478)
  [Code Health] refactor: block query client interface (#616)
bryanchriswhite added a commit that referenced this pull request Jun 27, 2024
…aim-proof-distribution

* issues/402/refactor/relayminer: (21 commits)
  fix: unflake and speed up tests
  chore: speed up & improve tests
  chore: update comments
  chore: simplify
  [E2E] Add logging output from the init-container step (#635)
  fix: failing test
  Empty commit
  [Code Health] refactor: random number generation (#618)
  chore: review improvements
  chore: review improvments
  [Code Health] refactor: tx hash encoding (#617)
  [Docs] Relay session & claim/proof diagrams (#478)
  Empty commit
  [Code Health] refactor: block query client interface (#616)
  chore: review feedback improvements
  [TODO] chore: cleanup done TODOs (#627)
  [TODOs] chore: cleanup validate session ID TODOs (#608)
  [LocalNet] Switch to CosmosSDK defaults (#609)
  [Relay Mining] Emit events and refactor test helpers (#610)
  [Docs] Minor improvements to `adding_params.md` (#623)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Cleans up some code devnet devnet-test-e2e push-image CI related - pushes images to ghcr.io
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[Proof] Claim & proof distribution validation
3 participants