[On-chain, Relayminer] chore: add claim msg validation #236

bryanchriswhite · 2023-11-29T09:37:19Z

Summary

Human Summary

Adds validation logic around the claim's session during claim creation. The session ID must match that as determined by an on-chain query of the session given the same inputs. Additionally, the supplier list for the session must contain the supplier whose address is referenced in the claim.

AI Summary

Summary generated by Reviewpad on 07 Dec 23 10:58 UTC

This pull request includes various changes across multiple files. Here is a summary of the changes:

supplier.go (in testutil/keeper package):
- Added imports for supplier and supplier/mocks packages.
- Added import for sessiontypes package from github.com/pokt-network/poktroll/x/session/types.
- Modified the SupplierKeeper function to accept an additional parameter sessionByAppAddr of type supplier.SessionsByAppAddress.
- Added implementation for GetSession method of the mockSessionKeeper mock.
- Added a call to k.SupplySessionKeeper(mockSessionKeeper).
relay_verifier.go (in pkg/relayer/proxy package):
- Added import for sdkerrors package.
- Added import for ring_secp256k1 package.
- Removed import for github.com/cometbft/cometbft/crypto package.
- Removed import for github.com/pokt-network/poktroll/x/session/types package.
- Modified the VerifyRelayRequest function to handle missing application address from the relay request.
- Changed the method of obtaining signable bytes of the relay request to GetSignableBytesHash.
- Changed the method of verifying relay request's signature using ringSig.
- Modified the code for verifying relay request session using GetSession method of rp.sessionQuerier.
- Improved error message when session ID does not match relay request's session ID.
relay_builders.go:
- Added a debug log message when there is a failure in unmarshaling the relay request.
ring_signer.go:
- Changed the Sign function to accept an array of 32 bytes for the message argument instead of a byte slice.
sessiontypes package:
- Added SessionKeeper interface which includes the GetSession method.
- Updated the go:generate mockgen command to include the SessionKeeper interface in the mock generation.
testqueryclients/appquerier.go:
- Updated the import paths for the mockclient and types packages.
- Replaced the context parameter in the GetApplication function with an underscore.
- Updated the return type of the GetApplication function to use the apptypes type.
- Updated the variable names within the GetApplication function.
- Updated the error type returned in case of an application not found to apptypes.ErrAppNotFound.
app.go:
- Changes to the New function related to the ordering of code and addition of comments.
interface.go:
- Added SupplierQueryClient interface.
- Added SessionQueryClient interface.
session.feature:
- Added a new feature file describing the "Session Namespace" feature.
- Includes a scenario for completing the claim/proof lifecycle for a valid session.
simple_signer.go:
- Changed the signature parameter of the Sign function to accept an array of bytes with a fixed size of 32.
logger.go (in pkg/polylog/polyzero package):
- Added the os package as an import.
- Updated the NewLogger function to provide default logger configuration.
- Added TODO comments for improving and adding new logger functions.
client.go:
- Added import of the cosmossdk.io/api/tendermint/abci package.
- Removed import of the abciTypes package from github.com/cometbft/cometbft/abci/types.
- Changed the type TxEvent to be an alias for the abci.TxResult type.
- Added comments.
proxy.go:
- Removed the import of cosmosclient package.
- Removed the import of querytypes package.
- Replaced the type suppliertypes.QueryClient with client.SupplierQueryClient.
- Replaced the type sessiontypes.QueryClient with client.SessionQueryClient.
- Removed the field clientCtx.
- Replaced the initialization of supplierQuerier with client.SupplierQueryClient.
- Replaced the initialization of sessionQuerier with client.SessionQueryClient.
- Removed the initialization of keyring.
- Added a check to ensure proxiedServicesEndpoints is not empty in the validateConfig method.
relayerproxy.go:
- Added various package imports and new struct and functions related to testing utilities and mock structures for the relayer proxy.

Issue

On-chain claim message handling should validate that the claimed session matches the session at the given claimed sesison end height; i.e. given some session inputs, ensure that the ID matches and that the supplier is in the session.

Type of change

Select one or more:

Testing

Run all unit tests: make go_develop_and_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

x/supplier/keeper/message_create_claim_test.go

x/supplier/keeper/msg_server_create_claim_test.go

x/supplier/keeper/keeper.go

app/app.go

e2e/tests/session_steps_test.go

e2e/tests/session.feature

github-actions · 2023-11-29T09:37:49Z

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

testutil/keeper/supplier.go

e2e/tests/session_steps_test.go

testutil/testkeyring/accounts.go

testutil/network/network.go

Olshansk

Posting a partial review and will finish tomorrow.

app/app.go

e2e/tests/session.feature

x/supplier/keeper/keeper.go

x/supplier/client/cli/query_claim_test.go

e2e/tests/session_steps_test.go

testutil/keeper/supplier.go

testutil/network/network.go

bryanchriswhite · 2023-12-04T12:06:33Z

testutil/testclient/localnet.go

+//
+// TODO_IMPROVE: It would be nice if the value could be set correctly based
+// on whether the test using it is running in tilt or not.
+const CometLocalWebsocketURL = "ws://sequencer-poktroll-sequencer:36657/websocket"


@okdas Does this look right to you?

@bryanchriswhite yep! That hostname will now resolve to sequencer on both LocalNet and DevNet!

@okdas Bump

Olshansk

The PR is looking really good overall. Really appreciate both the changes, and comments to set up followup work.

Left a few lingering comments but should be g2g right after!

e2e/tests/session.feature

e2e/tests/session_steps_test.go

pkg/client/tx/client.go

x/supplier/keeper/msg_server_create_claim.go

testutil/supplier/fixtures.go

x/supplier/client/cli/helpers_test.go

Olshansk · 2023-12-04T23:32:26Z

x/supplier/client/cli/helpers_test.go

+	sessionQueryClient := types2.NewQueryClient(net.Validators[0].ClientCtx)
+	res, err := sessionQueryClient.GetSession(ctx, &types2.QueryGetSessionRequest{
+		ApplicationAddress: appAddr,
+		Service:            &sharedtypes.Service{Id: testServiceId},


Thoughts on having the caller pass in this local constant?

Definitely have thoughts 😅. This is related to #236 (comment).

TL;DR, I would hold off for now.

It might be better to take a step back and think about how we would like multi-service integration tests to be designed, and then evaluate if it's compatible with our current direction (i.e. refactor and/or reuse what we have) or if we need to do that in a new test suite with more comprehensive helpers. Wdyt?

I think holding off and seeing what/when we need from multi-service integration makes sense.

Let's hold off 👍

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

Olshansk

LGTM. The last round of comments & cleanup really put the on it!

Note that the E2E tests are failing and wanted to make sure you remember the base branch here is not main (should it be?), but otherwise g2g!

Olshansk · 2023-12-05T20:07:03Z

pkg/client/tx/client.go

-type TxResult struct {
-	Events []abciTypes.Event `json:"events"`
-}
+//


So much cleaner!

Olshansk · 2023-12-05T20:07:25Z

testutil/supplier/fixtures.go

@@ -21,6 +21,12 @@ const (
 // application address and the value is the session fixture.
 type SessionsByAppAddress map[string]sessiontypes.Session

+// AppSupplierPair is a pairing of an application and a supplier address.


Ty for making the change!

Olshansk · 2023-12-05T20:08:35Z

x/supplier/client/cli/helpers_test.go

+	sessionQueryClient := types2.NewQueryClient(net.Validators[0].ClientCtx)
+	res, err := sessionQueryClient.GetSession(ctx, &types2.QueryGetSessionRequest{
+		ApplicationAddress: appAddr,
+		Service:            &sharedtypes.Service{Id: testServiceId},


I think holding off and seeing what/when we need from multi-service integration makes sense.

Let's hold off 👍

bryanchriswhite · 2023-12-06T07:32:58Z

Note that the E2E tests are failing and wanted to make sure you remember the base branch here is not main (should it be?), but otherwise g2g!

Thanks for calling this out! I typically intentionally keep the github base of PR branches the same as the git (actual) base until after approval to avoid cluttering the commit list during review. I always take note of the +/- LOC diff before and after re-github-basing (not an actual rebase operation). In the event of discrepancies and/or merge conflict resolutions I double-check everything before merging and would ask for re-review if something unexpected arises or changes since approval.

…yring * pokt/main: [Off-chain, Polylog] refactor: replace std logger usage in pkg/... with polylog (#219) [Off-chain, Polylog] feat: polylog context integration (#218) [Off-chain, Polylog] chore: add zerolog logger implementation (#211) [Off-chain, Polylog] test: add testutils (#209) [Testing] chore: add testkeyring pkg (#237)

…im-msg-validation * issues/140/chore/testkeyring: [Off-chain, Polylog] refactor: replace std logger usage in pkg/... with polylog (#219) [Off-chain, Polylog] feat: polylog context integration (#218) [Off-chain, Polylog] chore: add zerolog logger implementation (#211) [Off-chain, Polylog] test: add testutils (#209) [Testing] chore: add testkeyring pkg (#237)

(cherry picked from commit 358cb0ae8ffed76a75d83f42ef0a4ea94d185189)

…msg-validation * pokt/main: [RelayerProxy] Test relayer proxy startup (#199) [CI] Pin ignite version to 0.27.2 (#249) fix: localnet output broken 🙄 (#247) Make Relay{Request,Response}.GetSignableBytes return the signable hash (#245)

* chore: add testkeyring pkg * chore: update go.mod * refactor: add session keeper dependency to supplier keeper * refactor: add mock session keeper to supplier keeper testutil * refactor: supplier keeper testutil usage * chore: add claim session validation * test: claim message validation unit coverage * test: e2e session lifecycle * chore: add `ApplicationModuleGenesisStateWithAccounts` testutil * wip: fix claims show/list tests * fix: bug in session supplier hydration * wip: fixing tests * wip: fixing tests * chore: update mock node flag with in-tilt host * wip: debugging * wip: debugging * chore: self-review improvements * chore: review feedback improvements * chore: add TODO Co-authored-by: Daniel Olshansky <[email protected]> * fixup! chore: update mock node flag with in-tilt host * chore: review feedback improvements * chore: review feedback improvements Co-authored-by: Daniel Olshansky <[email protected]> * wip * chore: chore: review feedback improvements Co-authored-by: Daniel Olshansky <[email protected]> * fix: post-merge * chore: review feedback improvements Co-authored-by: Daniel Olshansky <[email protected]> * chore: remove todo * chore: self-review improvements * chore: re Co-authored-by: Daniel Olshansky <[email protected]> * chore: review feedback improvements Co-authored-by: Daniel Olshansky <[email protected]> * chore: chore: review feedback improvements Co-authored-by: Daniel Olshansky <[email protected]> * fix: test * fix: disambiguate `CometLocalWebsocketURL` & `CometLocalTCPURL` (cherry picked from commit 358cb0ae8ffed76a75d83f42ef0a4ea94d185189) --------- Co-authored-by: Daniel Olshansky <[email protected]>

bryanchriswhite added relayminer Changes related to the Relayminer on-chain On-chain business logic labels Nov 29, 2023

bryanchriswhite added this to the Shannon TestNet milestone Nov 29, 2023

bryanchriswhite self-assigned this Nov 29, 2023

github-actions bot reviewed Nov 29, 2023

View reviewed changes

bryanchriswhite added the devnet-test-e2e label Nov 29, 2023

github-actions bot added devnet push-image CI related - pushes images to ghcr.io labels Nov 29, 2023

bryanchriswhite commented Nov 29, 2023

View reviewed changes

testutil/keeper/supplier.go Outdated Show resolved Hide resolved

bryanchriswhite force-pushed the issues/140/chore/claim-msg-validation branch from c7e0fdd to 61c08c7 Compare November 29, 2023 11:36

github-actions bot reviewed Nov 29, 2023

View reviewed changes

e2e/tests/session_steps_test.go Outdated Show resolved Hide resolved

bryanchriswhite force-pushed the issues/140/chore/claim-msg-validation branch from 61c08c7 to a74863c Compare November 29, 2023 11:41

github-actions bot reviewed Nov 30, 2023

View reviewed changes

testutil/testkeyring/accounts.go Outdated Show resolved Hide resolved

testutil/network/network.go Outdated Show resolved Hide resolved

bryanchriswhite added 10 commits November 30, 2023 20:45

chore: add testkeyring pkg

cebfce1

chore: update go.mod

586e3a4

refactor: add session keeper dependency to supplier keeper

825d0c3

refactor: add mock session keeper to supplier keeper testutil

d9eb488

refactor: supplier keeper testutil usage

0049e3b

chore: add claim session validation

5180140

test: claim message validation unit coverage

4dfe48a

test: e2e session lifecycle

5721b51

chore: add ApplicationModuleGenesisStateWithAccounts testutil

65d92e3

wip: fix claims show/list tests

17fb3f1

bryanchriswhite force-pushed the issues/140/chore/claim-msg-validation branch from caa24cd to 17fb3f1 Compare November 30, 2023 19:47

bryanchriswhite changed the base branch from main to issues/140/chore/testkeyring November 30, 2023 19:47

Olshansk self-requested a review November 30, 2023 22:25

Olshansk requested changes Dec 1, 2023

View reviewed changes

bryanchriswhite added 2 commits December 1, 2023 09:27

fix: bug in session supplier hydration

91815d1

wip: fixing tests

d28c74c

bryanchriswhite commented Dec 4, 2023

View reviewed changes

chore: self-review improvements

2f5e5ff

bryanchriswhite marked this pull request as ready for review December 4, 2023 12:11

bryanchriswhite requested review from Olshansk and okdas December 4, 2023 12:11

Olshansk requested changes Dec 4, 2023

View reviewed changes

Olshansk mentioned this pull request Dec 5, 2023

[Off-Chain] Add EventsReplayClient Generic Event Listener for BlockClient and DelegationClient #220

Merged

10 tasks

This was linked to issues Dec 5, 2023

[Protocol] MVP implementation off the on-chain CreateClaim message handling #140

Closed

[C&P] Introduce the first session.feature #232

Closed

bryanchriswhite and others added 4 commits December 5, 2023 11:02

chore: re

1ce457a

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

chore: review feedback improvements

b49441c

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

chore: chore: review feedback improvements

a21ab57

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

fix: test

b21f843

bryanchriswhite requested a review from Olshansk December 5, 2023 11:52

Olshansk approved these changes Dec 5, 2023

View reviewed changes

bryanchriswhite mentioned this pull request Dec 6, 2023

[Protocol] MVP implementation off the on-chain SubmitProof message handling #141

Closed

8 tasks

bryanchriswhite added 3 commits December 6, 2023 10:06

fix: disambiguate CometLocalWebsocketURL & CometLocalTCPURL

e58cd33

(cherry picked from commit 358cb0ae8ffed76a75d83f42ef0a4ea94d185189)

bryanchriswhite force-pushed the issues/140/chore/claim-msg-validation branch from 893b9db to e58cd33 Compare December 6, 2023 09:29

bryanchriswhite mentioned this pull request Dec 6, 2023

[DO NOT MERGE] experiment: testing CI #248

Closed

10 tasks

bryanchriswhite changed the base branch from issues/140/chore/testkeyring to main December 7, 2023 10:58

bryanchriswhite merged commit 7d0c1c9 into main Dec 7, 2023
5 checks passed

bryanchriswhite deleted the issues/140/chore/claim-msg-validation branch December 7, 2023 11:10

bryanchriswhite removed push-image CI related - pushes images to ghcr.io devnet-test-e2e labels May 16, 2024

github-actions bot removed the devnet label May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[On-chain, Relayminer] chore: add claim msg validation #236

[On-chain, Relayminer] chore: add claim msg validation #236

bryanchriswhite commented Nov 29, 2023 •

edited by reviewpad bot

Loading

github-actions bot commented Nov 29, 2023

Olshansk left a comment

bryanchriswhite Dec 4, 2023

okdas Dec 4, 2023

Olshansk Dec 4, 2023

Olshansk left a comment

Olshansk Dec 4, 2023

bryanchriswhite Dec 5, 2023

Olshansk Dec 5, 2023

bryanchriswhite Dec 6, 2023

Olshansk left a comment

Olshansk Dec 5, 2023

Olshansk Dec 5, 2023

Olshansk Dec 5, 2023

bryanchriswhite commented Dec 6, 2023

[On-chain, Relayminer] chore: add claim msg validation #236

[On-chain, Relayminer] chore: add claim msg validation #236

Conversation

bryanchriswhite commented Nov 29, 2023 • edited by reviewpad bot Loading

Summary

Human Summary

AI Summary

Summary generated by Reviewpad on 07 Dec 23 10:58 UTC

Issue

Type of change

Testing

Sanity Checklist

github-actions bot commented Nov 29, 2023

Olshansk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryanchriswhite commented Dec 6, 2023

bryanchriswhite commented Nov 29, 2023 •

edited by reviewpad bot

Loading