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.

Sign up for GitHub

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

Jump to bottom

[Supplier] refactor: proof store indices (2) #327

Merged
merged 12 commits into from
Jan 19, 2024
Merged

[Supplier] refactor: proof store indices (2) #327

merged 12 commits into from
Jan 19, 2024

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Jan 17, 2024 •
edited
Loading

Summary

Human Summary

Refactors on-chain proof store to support multiple indices/stores in the multistore (similar to claims): "primary key", "supplier address", and "session end block height"

Issue

Type of change

Select one or more:

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

Testing

  • Run all unit tests: make go_develop_and_test
  • Run E2E tests locally: make test_e2e
  • Run E2E tests on DevNet: Add the devnet-test-e2e label to the PR. This is VERY expensive, only do it after all the reviews are complete.

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 supplier Changes related to the Supplier actor off-chain Off-chain business logic labels Jan 17, 2024
@bryanchriswhite bryanchriswhite added this to the Shannon TestNet milestone Jan 17, 2024
@bryanchriswhite bryanchriswhite self-assigned this Jan 17, 2024
@bryanchriswhite bryanchriswhite added on-chain On-chain business logic devnet-test-e2e and removed off-chain Off-chain business logic labels Jan 17, 2024
@github-actions GitHub Actions
Copy link

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. If you just created a pull request, you might need to push another commit to produce a container image DevNet can utilize to spin up infrastructure. You can use make trigger_ci to push an empty commit.

@github-actions github-actions bot added devnet push-image CI related - pushes images to ghcr.io labels Jan 17, 2024
@bryanchriswhite bryanchriswhite marked this pull request as ready for review January 17, 2024 11:37
@bryanchriswhite bryanchriswhite force-pushed the issues/141/refactor/proof-store-indices-2 branch from cce5a68 to 3665b5e Compare January 17, 2024 11:39
@bryanchriswhite bryanchriswhite marked this pull request as draft January 17, 2024 11:40
@bryanchriswhite bryanchriswhite force-pushed the issues/141/refactor/proof-store-indices-2 branch from 3665b5e to 08d68ab Compare January 17, 2024 11:44
@bryanchriswhite bryanchriswhite marked this pull request as ready for review January 17, 2024 11:50
@bryanchriswhite
Copy link
Contributor Author

Supersedes #266; shelves unit & integration coverage due to excessive refactor effort required.

@bryanchriswhite bryanchriswhite changed the title [Supplier] refactor: proof store indices 2 [Supplier] refactor: proof store indices (2) Jan 17, 2024
@bryanchriswhite bryanchriswhite removed push-image CI related - pushes images to ghcr.io devnet devnet-test-e2e labels Jan 17, 2024
Olshansk
Olshansk requested changes Jan 17, 2024
View reviewed changes
x/supplier/client/cli/query_proof.go Outdated Show resolved Hide resolved
x/supplier/client/cli/query_proof.go Outdated Show resolved Hide resolved
x/supplier/client/cli/query_proof.go Outdated Show resolved Hide resolved
x/supplier/keeper/msg_server_create_claim.go Outdated Show resolved Hide resolved
x/supplier/types/query_validation.go

func (query *QueryAllProofsRequest) ValidateBasic() error {
// TODO_TECHDEBT: update function signature to receive a context.
Copy link
Member

Choose a reason for hiding this comment

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

Do we have any ValidateBasic functions doing this? I'd argue this isn't a TODO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm realizing that this is actually not possible as it would require modification to cosmos-sdk framework method signatures (which is not feasible IMHO). I'm just going to use context.Background() and remove the comment for now.

x/supplier/types/query_validation.go Outdated Show resolved Hide resolved
x/supplier/types/query_validation.go Show resolved Hide resolved
x/supplier/keeper/session.go Outdated Show resolved Hide resolved
Olshansk
Olshansk approved these changes Jan 18, 2024
View reviewed changes
x/supplier/keeper/session.go Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. If you just created a pull request, you might need to push another commit to produce a container image DevNet can utilize to spin up infrastructure. You can use make trigger_ci to push an empty commit.

@github-actions github-actions bot added devnet push-image CI related - pushes images to ghcr.io labels Jan 19, 2024
@bryanchriswhite bryanchriswhite merged commit fd67d44 into main Jan 19, 2024
10 checks passed
bryanchriswhite added a commit that referenced this pull request Jan 22, 2024
@bryanchriswhite
Merge remote-tracking branch 'pokt/main' into issues/141/refactor/pro…
7e8cb19 
…of-store-indices-2

* pokt/main:
  [Off-chain] Simplify `TxClient` with `EventsQueryClient` (#330)
  fix: add yarn.lock (#334)
  [Service] NIT followups to `AddService` in #316 (#338)
  [Service] Implement AddService with extensive tests (#316)
  [Supplier] refactor: proof store indices (2) (#327)
@bryanchriswhite bryanchriswhite mentioned this pull request Jan 22, 2024
Closed
11 tasks
@bryanchriswhite bryanchriswhite removed push-image CI related - pushes images to ghcr.io devnet-test-e2e labels May 16, 2024
okdas pushed a commit that referenced this pull request Nov 14, 2024
@bryanchriswhite @Olshansk
[Supplier] refactor: proof store indices (2) (#327)
55b7c73 
* refactor: off-chain session validation

* refactor: proof CLI

* refactor: proof store indices

* fixup! refactor: off-chain session validation

* fixup! fixup! refactor: off-chain session validation

* fixup! fixup! fixup! refactor: off-chain session validation

* chore: review feedback improvement

Co-authored-by: Daniel Olshansky <[email protected]>

* chore: remove unneeded whitespace

* chore: update go.mod

* chore: review feedback improvements

Co-authored-by: Daniel Olshansky <[email protected]>

* chore: review feedback improvements

Co-authored-by: Daniel Olshansky <[email protected]>

---------

Co-authored-by: Daniel Olshansky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Olshansk Olshansk Olshansk approved these changes

Assignees

@bryanchriswhite bryanchriswhite

Labels
on-chain On-chain business logic supplier Changes related to the Supplier actor
Projects
Shannon
Status: ✅ Done
Milestone
Shannon Private TestNet
Development

Successfully merging this pull request may close these issues.
2 participants
@bryanchriswhite @Olshansk