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

[Application] Refactor app module to fetch on-chain params #555

Merged
merged 90 commits into from
May 30, 2024

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented May 28, 2024

Summary

Refactor the application module to remove all usage of the NumBlocksPerSession stand-in constant, in favor of fetching the on-chain param. This involves adding the shared module as a dependency.

Issue

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 May 15, 2024 14:10
@bryanchriswhite bryanchriswhite marked this pull request as ready for review May 28, 2024 10:00
Copy link

ellipsis-dev bot commented May 28, 2024

Your free trial has expired. To keep using Ellipsis, sign up at https://app.ellipsis.dev for $20/seat/month or reach us at [email protected]

Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Will re-review the entire PR on the next round but my main feedback is:

  1. s/queryHeight/blockHeight/
  2. s/GetDefaultX/GetXForDefaultParams

Lmk wyt

x/shared/session.go Outdated Show resolved Hide resolved
x/shared/session.go Show resolved Hide resolved
x/application/types/expected_keepers.go Show resolved Hide resolved
x/shared/session.go Outdated Show resolved Hide resolved
…ues/517/refactor/app-module

* issues/517/refactor/session-params_relayminer:
  [Relay Mining] Relay Mining math helpers (#549)
  [Relay Mining] Scaffold the RelayMiningDifficulty type (#548)
…-module

* pokt/main:
  [Relayminer] Query for on-chain session param `num_blocks_per_session` (#538)
@bryanchriswhite bryanchriswhite changed the base branch from issues/517/refactor/session-params_relayminer to main May 29, 2024 07:58
@bryanchriswhite bryanchriswhite requested a review from Olshansk May 29, 2024 08:00
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Optional comment updates

x/shared/session.go Outdated Show resolved Hide resolved
x/shared/session.go Outdated Show resolved Hide resolved
x/shared/session.go Outdated Show resolved Hide resolved
x/shared/session.go Outdated Show resolved Hide resolved
@bryanchriswhite bryanchriswhite merged commit e83b902 into main May 30, 2024
10 checks passed
@bryanchriswhite

This comment was marked as outdated.

bryanchriswhite added a commit that referenced this pull request May 30, 2024
…g-client

* pokt/main:
  [Application] Refactor app module to fetch on-chain params (#555)
  [Relayminer] Fix: relayminer on time sessions (#550)
@bryanchriswhite bryanchriswhite deleted the issues/517/refactor/app-module branch May 30, 2024 10:53
bryanchriswhite added a commit that referenced this pull request May 31, 2024
…ssues/516/feat/claim-window-close-offset-blocks

* issues/516/feat/claim-window-open-offset-blocks:
  test: ValidateClaimWindowOpenOffsetBlocks
  [On-chain] chore: add single param updates to `shared` module (#560)
  Empty commit
  [Code Health] chore: resolve & cleanup outstanding `TODO_TECHDEBT(#517)` comments (#559)
  [Session Module] refactor: session module fetches on-chain params (#557)
  [Ring Client] refactor: ring client to use on-chain params (#556)
  [LoadTest] Switch funding account to key name instead of address (#562)
  [Application] Refactor app module to fetch on-chain params (#555)
  [Relayminer] Fix: relayminer on time sessions (#550)
  Empty commit
bryanchriswhite added a commit that referenced this pull request May 31, 2024
…issues/516/refactor/relayminer

* issues/516/feat/claim-window-close-offset-blocks:
  test: ValidateClaimWindowCloseOffsetBlocks
  test: ValidateClaimWindowOpenOffsetBlocks
  [On-chain] chore: add single param updates to `shared` module (#560)
  Empty commit
  [Code Health] chore: resolve & cleanup outstanding `TODO_TECHDEBT(#517)` comments (#559)
  [Session Module] refactor: session module fetches on-chain params (#557)
  [Ring Client] refactor: ring client to use on-chain params (#556)
  [LoadTest] Switch funding account to key name instead of address (#562)
  [Application] Refactor app module to fetch on-chain params (#555)
  [Relayminer] Fix: relayminer on time sessions (#550)
  Empty commit
okdas pushed a commit that referenced this pull request Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application Changes related to the Application actor 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.

[Protocol] Add a NumBlocksPerSession governance parameter
2 participants