[Supplier] feat: persistence proofs in submit proof msg handler (2) #328

bryanchriswhite · 2024-01-17T11:19:11Z

Summary

Human Summary

Validate and persist proofs received in MsgSubmitProof message by the supplier module.
~~Simplify ReplayEventsClient to reduce number of generic parameters~~
(merged in [Off-chain] Simplify TxClient with EventsQueryClient #330)

Issue

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

Type of change

Select one or more:

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

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

Co-authored-by: Redouane Lakrache <[email protected]>

h5law

Nice work, looks great - saw a pattern for error handling in messages emerge left some comments on that but overall this is coming along great!

e2e/tests/session.feature

e2e/tests/session_steps_test.go

h5law · 2024-01-23T19:38:00Z

x/supplier/keeper/msg_server_create_claim.go

@@ -25,7 +28,7 @@ func (k msgServer) CreateClaim(goCtx context.Context, msg *suppliertypes.MsgCrea
 		msg.GetSupplierAddress(),
 	)
 	if err != nil {
-		return nil, err
+		return nil, status.Error(codes.InvalidArgument, err.Error())


Is this something we should be doing in message handlers, over returning an error from types/errors.go? Or is it specific for this use case that we are not doing so?

(see: #328 (comment))

h5law · 2024-01-23T19:48:25Z

x/supplier/keeper/msg_server_submit_proof.go


 	if err := msg.ValidateBasic(); err != nil {
-		return nil, err
+		return nil, status.Error(codes.InvalidArgument, err.Error())


This seems like a pattern you have found, is it worth updating the codebase post migration to do this for all message handlers? Should we create an issue for this?

It looks like it stops us from using require.ErrorIs / require.ErrorAs so some tests will need to be updated too.

The CLI should ALWAYS return a gRPC status error (i.e. status.Error(...)). In addition to consistency, this is an important aspect of separating client-facing errors from internal errors. Details of internal errors should only appear in logs that the operator sees and NEVER be sent to the client to avoid leaking potentially sensitive information.

This pattern fell out of the refactoring that was necessary in #266/#250. The tests in those branches take this into consideration (and will be re-applied in subsequent work).

IIRC the reason I pushed usage of status.Error all the way down into the keeper was because there was a lot of redundancy appearing in the CLI code involving wrapping keeper errors in the same status code error. Given that the only consumer of the keepers should be the CLI, or one another, it didn't seem inappropriate to make that simplification. I don't have a strong opinion about it though.

h5law · 2024-01-23T19:52:00Z

x/supplier/types/message_create_claim.go

 	if err != nil {
-		return sdkerrors.Wrapf(ErrSupplierInvalidAddress, "invalid supplierAddress address (%s)", err)
+		return sdkerrors.Wrapf(ErrSupplierInvalidAddress, "%s", msg.GetSupplierAddress())


Suggested change

return sdkerrors.Wrapf(ErrSupplierInvalidAddress, "%s", msg.GetSupplierAddress())

return ErrSupplierInvalidAddress.Wrapf("%s", msg.GetSupplierAddress())

Same for all the others

Thanks for calling this out! 🙌

I thought I got all of these but I either missed some or there were some reversions between the cherry-picking, etc.

After looking into this for second:

It seems like we have lots of usages of sdkerrors.Wrapf() across the modules (none of which are in tests). This leads me to believe that we might have picked this up from the ignite generator templates (in whatever ignite version was used).

I would prefer to batch this out in a separate PR after v0.50 migration, in case we do decide to transplant regenerate module code.

Captured in #347.

h5law · 2024-01-23T19:54:05Z

x/supplier/types/message_submit_proof.go

 	if err != nil {
-		return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid supplierAddress address (%s)", err)
+		return sdkerrors.ErrInvalidAddress.Wrapf(


Suggested change

return sdkerrors.ErrInvalidAddress.Wrapf(

return ErrSupplierInvalidAddress.Wrapf(

I think you should define this type something like invalid supplier address

I created #348 to capture the higher-level concerns that I see as in play here.

In this particular case, my rationale for using sdkerrors.ErrInvalidAddress over any of our own error variables is that the usage here is identical to that of the what I interpreted as its intended usage, as indicated by its actual usage in cosmos-sdk code (i.e. in cases where sdk.AccAddressFromBech32() returns an error in #ValidateBasic() methods).

I could imagine a middle-of-the-road alternative as us defining an alias to that error, such that we reference the alias instead. However, this begs the question, "in which module should that error variable be defined?"

x/supplier/types/message_submit_proof.go

h5law · 2024-01-23T19:56:01Z

x/supplier/types/message_submit_proof_test.go

+				},
+				Proof: testClosestMerkleProof,
+			},
+			expectedErr: sdkerrors.ErrInvalidAddress.Wrapf(


See the messages above about defining a type for this specific to the supplier module

This must match whatever error variable we decide to go with in the above. Let's continue the conversation there (#328 (comment)).

Olshansk

Last minor set of comments in addition to those left by others

e2e/tests/session_steps_test.go

x/supplier/keeper/msg_server_submit_proof.go

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

Co-authored-by: Redouane Lakrache <[email protected]>

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

…ersistence-2 * pokt/main: Update upload-pages-artifact.yml Update upload-pages-artifact.yml (#346) Fix upload-pages-artifact.yml (#345) [Service] Add AddServiceFee and enforce it in the message handler (#337)

Olshansk

Looks good from my end!

I know @h5law has some open comments for current/future work, so definitely should wait for a 👍 from his end as well.

…328) - Validate and persist proofs received in `MsgSubmitProof` message by the supplier module. - ~~Simplify `ReplayEventsClient` to reduce number of generic parameters~~ *(merged in #330)* --- Co-authored-by: Daniel Olshansky <[email protected]> Co-authored-by: Redouane Lakrache <[email protected]> Co-authored-by: h5law <[email protected]> Co-authored-by: h5law <[email protected]>

bryanchriswhite added supplier Changes related to the Supplier actor off-chain Off-chain business logic labels Jan 17, 2024

bryanchriswhite added this to the Shannon TestNet milestone Jan 17, 2024

bryanchriswhite self-assigned this Jan 17, 2024

bryanchriswhite added on-chain On-chain business logic and removed off-chain Off-chain business logic labels Jan 17, 2024

bryanchriswhite and others added 7 commits January 17, 2024 12:39

refactor: off-chain session validation

8ab21b6

refactor: proof CLI

10df226

refactor: proof store indices

4a22787

fixup! refactor: off-chain session validation

3127f1b

fixup! fixup! refactor: off-chain session validation

84ffced

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

255a677

chore: review feedback improvement

de6f735

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

bryanchriswhite force-pushed the issues/141/refactor/proof-store-indices-2 branch from cce5a68 to 3665b5e Compare January 17, 2024 11:39

bryanchriswhite added 2 commits January 17, 2024 12:43

chore: remove unneeded whitespace

189ad8c

chore: update go.mod

08d68ab

bryanchriswhite force-pushed the issues/141/refactor/proof-store-indices-2 branch from 3665b5e to 08d68ab Compare January 17, 2024 11:44

bryanchriswhite added 13 commits January 17, 2024 12:48

feat: proof persistence

db47b1a

refactor: session fixture test helpers

a7f4c5b

test: submit proof message handling

0435959

chore: update Proof protobuf usage

d3a9daf

test: proof cli querying

3c76f0f

todo: stub claim/proof creation tests

f34c4f1

chore: validate claim for proof

32920cd

refactor: proof msg handler errors

1116b21

refactor: create claim error & tests

c6e14bb

wip: post cherry-pick fixes

a0d4960

chore: add TODO to session.feature

fffcf86

test: E2E proof persistence

8b03fcc

refactor: simplify EventsReplayClient

064a1b3

bryanchriswhite and others added 3 commits January 23, 2024 08:47

Merge branch 'main' into issues/141/feat/proof-persistence-2

fe62959

Merge branch 'main' into issues/141/feat/proof-persistence-2

351dc18

chore: review feedback improvements

36be615

Co-authored-by: Redouane Lakrache <[email protected]>

bryanchriswhite linked an issue Jan 23, 2024 that may be closed by this pull request

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

Closed

8 tasks

h5law suggested changes Jan 23, 2024

View reviewed changes

Olshansk requested changes Jan 23, 2024

View reviewed changes

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

x/supplier/keeper/msg_server_submit_proof.go Show resolved Hide resolved

x/supplier/keeper/msg_server_submit_proof.go Show resolved Hide resolved

bryanchriswhite mentioned this pull request Jan 25, 2024

[Code Health] Simplify error object usage #347

Closed

5 tasks

bryanchriswhite and others added 2 commits January 25, 2024 10:39

chore: review feedback improvements

dafc425

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

chore: review feedback improvement

0ed19fe

Co-authored-by: Redouane Lakrache <[email protected]>

bryanchriswhite requested review from Olshansk, h5law and red-0ne January 25, 2024 09:52

bryanchriswhite and others added 2 commits January 25, 2024 11:10

chore: review feedback improvement

862bf4b

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

Merge remote-tracking branch 'pokt/main' into issues/141/feat/proof-p…

49888cd

…ersistence-2 * pokt/main: Update upload-pages-artifact.yml Update upload-pages-artifact.yml (#346) Fix upload-pages-artifact.yml (#345) [Service] Add AddServiceFee and enforce it in the message handler (#337)

bryanchriswhite force-pushed the issues/141/feat/proof-persistence-2 branch from fa3a6e0 to 49888cd Compare January 25, 2024 10:11

fix: session start block height in test

52ac55e

Olshansk approved these changes Jan 25, 2024

View reviewed changes

bryanchriswhite added 3 commits January 26, 2024 08:43

Merge branch 'main' into issues/141/feat/proof-persistence-2

380b49d

Merge branch 'main' into issues/141/feat/proof-persistence-2

780baa6

Merge branch 'main' into issues/141/feat/proof-persistence-2

5e45ac6

h5law mentioned this pull request Jan 30, 2024

[☂️ Tooling, Migration] Umbrella ticket for cosmos-sdk v0.50 migration #240

Closed

2 tasks

h5law approved these changes Jan 30, 2024

View reviewed changes

Olshansk mentioned this pull request Feb 1, 2024

[Tokenomics] Integrate SettleSessionAccounting w/ the Claim & Proof Lifecycle #359

Closed

6 tasks

Merge branch 'main' into issues/141/feat/proof-persistence-2

9fc6ad4

Olshansk merged commit 1b4a8d5 into main Feb 1, 2024
9 checks passed

bryanchriswhite mentioned this pull request Feb 2, 2024

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

Closed

11 tasks

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

[Supplier] feat: persistence proofs in submit proof msg handler (2) #328

[Supplier] feat: persistence proofs in submit proof msg handler (2) #328

bryanchriswhite commented Jan 17, 2024 •

edited

Loading

h5law left a comment

h5law Jan 23, 2024

bryanchriswhite Jan 25, 2024

h5law Jan 23, 2024

bryanchriswhite Jan 25, 2024 •

edited

Loading

h5law Jan 23, 2024

bryanchriswhite Jan 25, 2024

bryanchriswhite Jan 25, 2024 •

edited

Loading

bryanchriswhite Jan 25, 2024

h5law Jan 23, 2024

bryanchriswhite Jan 25, 2024

bryanchriswhite Jan 25, 2024

h5law Jan 23, 2024

bryanchriswhite Jan 25, 2024

Olshansk left a comment

Olshansk left a comment

	return sdkerrors.Wrapf(ErrSupplierInvalidAddress, "%s", msg.GetSupplierAddress())
	return ErrSupplierInvalidAddress.Wrapf("%s", msg.GetSupplierAddress())

	return sdkerrors.ErrInvalidAddress.Wrapf(
	return ErrSupplierInvalidAddress.Wrapf(

[Supplier] feat: persistence proofs in submit proof msg handler (2) #328

[Supplier] feat: persistence proofs in submit proof msg handler (2) #328

Conversation

bryanchriswhite commented Jan 17, 2024 • edited Loading

Summary

Human Summary

Issue

Type of change

Testing

Sanity Checklist

h5law 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 Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryanchriswhite Jan 25, 2024 • edited Loading

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

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

Olshansk left a comment

Choose a reason for hiding this comment

bryanchriswhite commented Jan 17, 2024 •

edited

Loading

bryanchriswhite Jan 25, 2024 •

edited

Loading

bryanchriswhite Jan 25, 2024 •

edited

Loading