[Session,Service] Ensure SessionHeader and Service basic validation. #782

red-0ne · 2024-09-03T17:51:35Z

Summary

This PR addresses the following TODO_BETAs:

TODO_BETA: Call msg.GetSessionHeader().ValidateBasic() once its implemented

TODO_BETA: Ensure this is used everywhere a SessionHeader is validated.

TODO_TECHDEBT(BETA): Refactor to a Service#ValidateBasic method.

It ensures that SessionHeader#ValidateBasic() is covering all usage of SessionHeader both on-chain and off-chain.
Implements Service#ValidateBasic which is a sub-check of SessionHeader#ValidateBasic
Refactors some tests to align with the new shape of the errors returned.

Type of change

Select one or more:

Testing

Documentation: make docusaurus_start; only needed if you make doc changes
Unit Tests: make go_develop_and_test
LocalNet E2E Tests: make test_e2e
DevNet E2E Tests: Add the devnet-test-e2e label to the PR.

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

x/proof/keeper/msg_server_submit_proof_test.go

x/proof/types/message_create_claim.go

x/proof/types/message_submit_proof.go

x/proof/types/message_submit_proof_test.go

Olshansk · 2024-09-05T17:59:59Z

x/shared/types/service.go

+		return ErrSharedInvalidService.Wrapf("invalid service name: %s", s.Name)
+	}
+
+	return nil


Please ensure the other fields (e.g. owner, CUPR, etc...) aren't empty and/or valid as well.

This might require adding unit tests.

Service evolved. Good catch!

Adding these fields to the validation breaks a lot of code and tests because we're creating services with Id only in a lot of places, Instead, I propose to replace these Services with Ids in a follow-up PR as stated in service.proto

// TODO_MAINNET: Avoid embedding the full Service because we just need the ID.

I estimate it could be done in 1 day. WDYT?

How about this:

Add TODO_BETA/TODO_BLOCKER(@red-0ne): ... in the validation function explaining this situation

Update the PR and merge it in

Iterate on the ServiceId thing next week

…dation' into HEAD

bryanchriswhite

Nice one! 🚀

x/proof/keeper/msg_server_submit_proof_test.go

x/proof/types/message_submit_proof_test.go

x/session/keeper/session_hydrator.go

x/session/types/session_header.go

x/shared/helpers/service_configs.go

…-basic-validation

github-actions · 2024-09-09T15:09:37Z

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 782)
Grafana network dashboard for devnet-issue-782

…-basic-validation

x/shared/types/service.go

…-basic-validation

…ke-transfer * pokt/main: [Tooling, Code Health] refactor: split Makefile into multiple files (#816) [Tooling] fix and add makefile targets (#814) [Docs] Add a debugging tip: using transaction hashes (#786) [Refactor] Replace embedded Service with ServiceId (#802) [Session,Service] Ensure SessionHeader and Service basic validation. (#782) [Tokenomics] Preparation for Global Mint Reimbursement Request (#755) [Quick PR][LocalNet] Turn off minimum-gas-prices (#805) [GATEWAY POC] A couple helpers to help deploy a PATH gateway (#801) [Documentation] Cosmovisor (#768) [CI] Add cosmovisor to container images (#796) Use a descriptive error if service ID is not found (#787) [SessionManager] Implement off-chain proof params usage (#765) [SDK] Update shannon sdk dependency (#800) [E2E Test] Remove redundant logging (#798)

…actor/transfer-msg_period-param * issues/657/feat/app-stake-transfer: [Tooling, Code Health] refactor: split Makefile into multiple files (#816) [Tooling] fix and add makefile targets (#814) [Docs] Add a debugging tip: using transaction hashes (#786) [Refactor] Replace embedded Service with ServiceId (#802) [Session,Service] Ensure SessionHeader and Service basic validation. (#782) [Tokenomics] Preparation for Global Mint Reimbursement Request (#755) [Quick PR][LocalNet] Turn off minimum-gas-prices (#805) [GATEWAY POC] A couple helpers to help deploy a PATH gateway (#801) [Documentation] Cosmovisor (#768) [CI] Add cosmovisor to container images (#796) Use a descriptive error if service ID is not found (#787) [SessionManager] Implement off-chain proof params usage (#765) [SDK] Update shannon sdk dependency (#800) [E2E Test] Remove redundant logging (#798)

…ues/657/chore/app-transfer-period * issues/657/refactor/transfer-msg_period-param: [Tooling, Code Health] refactor: split Makefile into multiple files (#816) [Tooling] fix and add makefile targets (#814) [Docs] Add a debugging tip: using transaction hashes (#786) [Refactor] Replace embedded Service with ServiceId (#802) [Session,Service] Ensure SessionHeader and Service basic validation. (#782) [Tokenomics] Preparation for Global Mint Reimbursement Request (#755) [Quick PR][LocalNet] Turn off minimum-gas-prices (#805) [GATEWAY POC] A couple helpers to help deploy a PATH gateway (#801) [Documentation] Cosmovisor (#768) [CI] Add cosmovisor to container images (#796) Use a descriptive error if service ID is not found (#787) [SessionManager] Implement off-chain proof params usage (#765) [SDK] Update shannon sdk dependency (#800) [E2E Test] Remove redundant logging (#798)

…782) ## Summary This PR addresses the following `TODO_BETA`s: > `TODO_BETA: Call `msg.GetSessionHeader().ValidateBasic()` once its implemented` > `TODO_BETA: Ensure this is used everywhere a SessionHeader is validated.` > `TODO_TECHDEBT(BETA): Refactor to a `Service#ValidateBasic` method.` * It ensures that `SessionHeader#ValidateBasic()` is covering all usage of `SessionHeader` both on-chain and off-chain. * Implements `Service#ValidateBasic` which is a sub-check of `SessionHeader#ValidateBasic` * Refactors some tests to align with the new shape of the errors returned. ## Type of change Select one or more: - [ ] New feature, functionality or library - [ ] Bug fix - [x] Code health or cleanup - [ ] Documentation - [ ] Other (specify) ## Testing - [ ] **Documentation**: `make docusaurus_start`; only needed if you make doc changes - [x] **Unit Tests**: `make go_develop_and_test` - [x] **LocalNet E2E Tests**: `make test_e2e` - [ ] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR. ## Sanity Checklist - [x] I have tested my changes using the available tooling - [x] I have commented my code - [x] 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 --------- Co-authored-by: Daniel Olshansky <[email protected]>

red-0ne added 2 commits September 3, 2024 09:47

chore: Session header and service basic validation

3cba149

fix: Adapt test after refactor

124b660

red-0ne added service Anything related to general-purpose RPC service support session Changes related to Session management off-chain Off-chain business logic on-chain On-chain business logic code health Cleans up some code labels Sep 3, 2024

red-0ne requested review from bryanchriswhite and Olshansk September 3, 2024 17:51

red-0ne self-assigned this Sep 3, 2024

chore: Fix godoc comment

2994415

okdas added the consensus-breaking IMPORTANT! If the PR with this tag is merged, next release WILL HAVE TO BE an upgrade. label Sep 3, 2024

Olshansk added this to the Shannon Beta TestNet Launch milestone Sep 5, 2024

Merge branch 'main' into chore/session-service-basic-validation

8028d01

Olshansk requested changes Sep 5, 2024

View reviewed changes

red-0ne added 4 commits September 6, 2024 22:08

WIP: ValidteBasic

9bc4442

Merge remote-tracking branch 'origin/main' into HEAD

1fbc638

fix: Revert service validate basic

281edd5

Merge remote-tracking branch 'origin/chore/session-service-basic-vali…

a6c6643

…dation' into HEAD

red-0ne requested a review from Olshansk September 7, 2024 00:47

bryanchriswhite requested changes Sep 9, 2024

View reviewed changes

red-0ne added 2 commits September 9, 2024 16:49

chore: Address review change requests

0a7f927

Merge remote-tracking branch 'origin/main' into chore/session-service…

8464b16

…-basic-validation

red-0ne added the devnet-test-e2e label Sep 9, 2024

github-actions bot added devnet push-image CI related - pushes images to ghcr.io labels Sep 9, 2024

red-0ne requested a review from bryanchriswhite September 9, 2024 15:13

red-0ne added 2 commits September 9, 2024 17:38

Empty commit

6ba8e41

Merge remote-tracking branch 'origin/main' into chore/session-service…

cc0f6fd

…-basic-validation

Merge branch 'main' into chore/session-service-basic-validation

6e84ff9

Olshansk approved these changes Sep 9, 2024

View reviewed changes

x/shared/types/service.go Show resolved Hide resolved

Merge remote-tracking branch 'origin/main' into chore/session-service…

141d723

…-basic-validation

bryanchriswhite approved these changes Sep 10, 2024

View reviewed changes

red-0ne merged commit 614ad77 into main Sep 10, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Session,Service] Ensure SessionHeader and Service basic validation. #782

[Session,Service] Ensure SessionHeader and Service basic validation. #782

red-0ne commented Sep 3, 2024

Olshansk Sep 5, 2024

Olshansk Sep 5, 2024

red-0ne Sep 6, 2024

red-0ne Sep 6, 2024 •

edited

Loading

Olshansk Sep 6, 2024

bryanchriswhite left a comment

github-actions bot commented Sep 9, 2024

[Session,Service] Ensure SessionHeader and Service basic validation. #782

[Session,Service] Ensure SessionHeader and Service basic validation. #782

Conversation

red-0ne commented Sep 3, 2024

Summary

Type of change

Testing

Sanity Checklist

Olshansk Sep 5, 2024

Choose a reason for hiding this comment

Olshansk Sep 5, 2024

Choose a reason for hiding this comment

red-0ne Sep 6, 2024

Choose a reason for hiding this comment

red-0ne Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Olshansk Sep 6, 2024

Choose a reason for hiding this comment

bryanchriswhite left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 9, 2024

red-0ne Sep 6, 2024 •

edited

Loading