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

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

Merged
merged 38 commits into from
Dec 7, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
cebfce1
chore: add testkeyring pkg
bryanchriswhite Nov 30, 2023
586e3a4
chore: update go.mod
bryanchriswhite Nov 29, 2023
825d0c3
refactor: add session keeper dependency to supplier keeper
bryanchriswhite Nov 29, 2023
d9eb488
refactor: add mock session keeper to supplier keeper testutil
bryanchriswhite Nov 29, 2023
0049e3b
refactor: supplier keeper testutil usage
bryanchriswhite Nov 29, 2023
5180140
chore: add claim session validation
bryanchriswhite Nov 29, 2023
4dfe48a
test: claim message validation unit coverage
bryanchriswhite Nov 29, 2023
5721b51
test: e2e session lifecycle
bryanchriswhite Nov 29, 2023
65d92e3
chore: add `ApplicationModuleGenesisStateWithAccounts` testutil
bryanchriswhite Nov 30, 2023
17fb3f1
wip: fix claims show/list tests
bryanchriswhite Nov 30, 2023
91815d1
fix: bug in session supplier hydration
bryanchriswhite Dec 1, 2023
d28c74c
wip: fixing tests
bryanchriswhite Dec 1, 2023
304d51f
wip: fixing tests
bryanchriswhite Dec 1, 2023
d02d5d7
chore: update mock node flag with in-tilt host
bryanchriswhite Dec 1, 2023
d191d39
wip: debugging
bryanchriswhite Dec 1, 2023
51cabc3
wip: debugging
bryanchriswhite Dec 1, 2023
c3926b1
chore: self-review improvements
bryanchriswhite Dec 1, 2023
7814b00
chore: review feedback improvements
bryanchriswhite Dec 4, 2023
79fdefa
chore: add TODO
bryanchriswhite Dec 4, 2023
7c08f69
fixup! chore: update mock node flag with in-tilt host
bryanchriswhite Dec 4, 2023
7ab73a9
chore: review feedback improvements
bryanchriswhite Dec 4, 2023
2361013
chore: review feedback improvements
bryanchriswhite Dec 4, 2023
1142628
wip
bryanchriswhite Dec 4, 2023
aedfb6e
chore: chore: review feedback improvements
bryanchriswhite Dec 4, 2023
9d5f62b
Merge remote-tracking branch 'pokt/main' into issues/140/chore/testke…
bryanchriswhite Dec 4, 2023
a2773c9
Merge branch 'issues/140/chore/testkeyring' into issues/140/chore/cla…
bryanchriswhite Dec 4, 2023
4e25816
fix: post-merge
bryanchriswhite Dec 4, 2023
4508e15
chore: review feedback improvements
bryanchriswhite Dec 4, 2023
a46d196
chore: remove todo
bryanchriswhite Dec 4, 2023
2f5e5ff
chore: self-review improvements
bryanchriswhite Dec 4, 2023
1ce457a
chore: re
bryanchriswhite Dec 5, 2023
b49441c
chore: review feedback improvements
bryanchriswhite Dec 5, 2023
a21ab57
chore: chore: review feedback improvements
bryanchriswhite Dec 5, 2023
b21f843
fix: test
bryanchriswhite Dec 5, 2023
7fa8c07
Merge remote-tracking branch 'pokt/main' into issues/140/chore/testke…
bryanchriswhite Dec 6, 2023
b176b7a
Merge branch 'issues/140/chore/testkeyring' into issues/140/chore/cla…
bryanchriswhite Dec 6, 2023
e58cd33
fix: disambiguate `CometLocalWebsocketURL` & `CometLocalTCPURL`
bryanchriswhite Dec 6, 2023
80e7386
Merge remote-tracking branch 'pokt/main' into issues/140/chore/claim-…
bryanchriswhite Dec 7, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions e2e/tests/init_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ func TestMain(m *testing.M) {

type suite struct {
gocuke.TestingT
// TODO_TECHDEBT: rename to `poktrolld`.
pocketd *pocketdBin
scenarioState map[string]any // temporary state for each scenario
cdc codec.Codec
Expand All @@ -85,6 +86,7 @@ func TestFeatures(t *testing.T) {
gocuke.NewRunner(t, &suite{}).Path(flagFeaturesPath).Run()
}

// TODO_TECHDEBT: rename `pocketd` to `poktrolld`.
func (s *suite) TheUserHasThePocketdBinaryInstalled() {
s.TheUserRunsTheCommand("help")
}
Expand Down
1 change: 1 addition & 0 deletions e2e/tests/session.feature
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ Feature: Session Namespace
Then the claim created by supplier "supplier1" for service "svc1" for application "app1" should be persisted on-chain
# TODO_IMPROVE: ...
# And an event should be emitted...
# TODO_INCOMPLETE: add step(s) for proof validation.
6 changes: 3 additions & 3 deletions e2e/tests/session_steps_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ import (
"strings"
"time"

abci "github.com/cometbft/cometbft/abci/types"
"github.com/stretchr/testify/require"

eventsquery "github.com/pokt-network/poktroll/pkg/client/events_query"
"github.com/pokt-network/poktroll/pkg/client/tx"
"github.com/pokt-network/poktroll/pkg/either"
"github.com/pokt-network/poktroll/pkg/observable"
"github.com/pokt-network/poktroll/pkg/observable/channel"
Expand All @@ -34,7 +34,7 @@ func (s *suite) AfterTheSupplierCreatesAClaimForTheSessionForServiceForApplicati
var ctx, done = context.WithCancel(context.Background())

// TODO_CONSIDERATION: if this test suite gets more complex, it might make
Olshansk marked this conversation as resolved.
Show resolved Hide resolved
// sense to refactor this key into a function that takes serviceId and appName
// sense to refactor this key into a function that takes serviceId and appName
// as arguments and returns the key.
eitherEventsBzReplayObs := s.scenarioState[eitherEventsBzReplayObsKey].(observable.ReplayObservable[either.Bytes])

Expand All @@ -50,7 +50,7 @@ func (s *suite) AfterTheSupplierCreatesAClaimForTheSessionForServiceForApplicati
}

// Unmarshal event data into a TxEventResponse object.
txEvent := &tx.TxEvent{}
txEvent := &abci.TxResult{}
err = json.Unmarshal(eventBz, txEvent)
require.NoError(s, err)

Expand Down
15 changes: 5 additions & 10 deletions pkg/client/tx/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ import (
"fmt"
"sync"

"cosmossdk.io/api/tendermint/abci"
"cosmossdk.io/depinject"
abciTypes "github.com/cometbft/cometbft/abci/types"
comettypes "github.com/cometbft/cometbft/types"
cosmostypes "github.com/cosmos/cosmos-sdk/types"
"go.uber.org/multierr"
Expand Down Expand Up @@ -87,15 +87,10 @@ type (

// TxEvent is used to deserialize incoming websocket messages from
// the transactions subscription.
type TxEvent struct {
// Tx is the binary representation of the tx hash.
Tx []byte `json:"tx"`
Result TxResult `json:"result"`
}

type TxResult struct {
Events []abciTypes.Event `json:"events"`
}
//
Copy link
Member

Choose a reason for hiding this comment

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

So much cleaner!

// TODO_CONSIDERATION: either expose this via an interface and unexport this type,
// or remove it altogether.
type TxEvent = abci.TxResult

// NewTxClient attempts to construct a new TxClient using the given dependencies
// and options.
Expand Down
31 changes: 16 additions & 15 deletions testutil/supplier/fixtures.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

Ty for making the change!

type AppSupplierPair struct {
AppAddr string
SupplierAddr string
}

// NewSessionFixturesWithPairings creates a map of session fixtures where the key
// is the application address and the value is the session fixture. App/supplier
// addresses are expected to be provided in alternating order (as pairs). The same
Expand All @@ -29,34 +35,27 @@ type SessionsByAppAddress map[string]sessiontypes.Session
func NewSessionFixturesWithPairings(
t *testing.T,
service *sharedtypes.Service,
appAndSupplierAddrPairs ...string,
appSupplierPairs ...AppSupplierPair,
) SessionsByAppAddress {
t.Helper()

if len(appAndSupplierAddrPairs)%2 != 0 {
t.Fatalf("expected even number of app and supplier address pairs, got %d", len(appAndSupplierAddrPairs))
}

// Initialize the session fixtures map.
sessionFixturesByAppAddr := make(SessionsByAppAddress)

// Iterate over the app and supplier address pairs (two indices at a time),
// and create a session fixture for each app address.
for i := 0; i < len(appAndSupplierAddrPairs); i += 2 {
appAddr := appAndSupplierAddrPairs[i]
application := newApplication(t, appAddr, service)

supplierAddr := appAndSupplierAddrPairs[i+1]
supplier := newSupplier(t, supplierAddr, service)
for _, appSupplierPair := range appSupplierPairs {
application := newApplication(t, appSupplierPair.AppAddr, service)
supplier := newSupplier(t, appSupplierPair.SupplierAddr, service)

if session, ok := sessionFixturesByAppAddr[appAddr]; ok {
if session, ok := sessionFixturesByAppAddr[appSupplierPair.AppAddr]; ok {
session.Suppliers = append(session.Suppliers, supplier)
continue
}

sessionFixturesByAppAddr[appAddr] = sessiontypes.Session{
sessionFixturesByAppAddr[appSupplierPair.AppAddr] = sessiontypes.Session{
Header: &sessiontypes.SessionHeader{
ApplicationAddress: appAddr,
ApplicationAddress: appSupplierPair.AppAddr,
Service: service,
SessionStartBlockHeight: testBlockHeight,
SessionId: testSessionId,
Expand All @@ -67,14 +66,15 @@ func NewSessionFixturesWithPairings(
NumBlocksPerSession: testBlocksPerSession,
Application: application,
Suppliers: []*sharedtypes.Supplier{
newSupplier(t, supplierAddr, service),
newSupplier(t, appSupplierPair.SupplierAddr, service),
},
}
}

return sessionFixturesByAppAddr
}

// newSuppliers configures a supplier for the services provided and nil endpoints.
func newSupplier(t *testing.T, addr string, services ...*sharedtypes.Service) *sharedtypes.Supplier {
bryanchriswhite marked this conversation as resolved.
Show resolved Hide resolved
t.Helper()

Expand All @@ -93,6 +93,7 @@ func newSupplier(t *testing.T, addr string, services ...*sharedtypes.Service) *s
}
}

// newApplication configures an application for the services provided.
func newApplication(t *testing.T, addr string, services ...*sharedtypes.Service) *apptypes.Application {
bryanchriswhite marked this conversation as resolved.
Show resolved Hide resolved
t.Helper()

Expand Down
35 changes: 20 additions & 15 deletions x/supplier/client/cli/helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,19 @@ import (
"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/client/flags"
"github.com/cosmos/cosmos-sdk/codec"
types3 "github.com/cosmos/cosmos-sdk/codec/types"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"github.com/cosmos/cosmos-sdk/crypto/keyring"
"github.com/cosmos/cosmos-sdk/testutil/cli"
types4 "github.com/cosmos/cosmos-sdk/types"
testcli "github.com/cosmos/cosmos-sdk/testutil/cli"
sdktypes "github.com/cosmos/cosmos-sdk/types"
"github.com/stretchr/testify/require"

"github.com/pokt-network/poktroll/cmd/pocketd/cmd"
"github.com/pokt-network/poktroll/testutil/network"
"github.com/pokt-network/poktroll/testutil/testkeyring"
types5 "github.com/pokt-network/poktroll/x/application/types"
types2 "github.com/pokt-network/poktroll/x/session/types"
apptypes "github.com/pokt-network/poktroll/x/application/types"
sessiontypes "github.com/pokt-network/poktroll/x/session/types"
sharedtypes "github.com/pokt-network/poktroll/x/shared/types"
cli2 "github.com/pokt-network/poktroll/x/supplier/client/cli"
"github.com/pokt-network/poktroll/x/supplier/client/cli"
"github.com/pokt-network/poktroll/x/supplier/types"
)

Expand Down Expand Up @@ -65,8 +65,6 @@ func networkWithClaimObjects(
sessionCount int,
supplierCount int,
appCount int,
// TODO_THIS_COMMIT: hook up to genesis state generation...
serviceCount int,
) (net *network.Network, claims []types.Claim) {
t.Helper()

Expand Down Expand Up @@ -113,7 +111,7 @@ func networkWithClaimObjects(

// Add supplier and application module genesis states to the network config.
cfg.GenesisState[types.ModuleName] = supplierGenesisBuffer
cfg.GenesisState[types5.ModuleName] = appGenesisBuffer
cfg.GenesisState[apptypes.ModuleName] = appGenesisBuffer

// Construct the network with the configuration.
net = network.New(t, cfg)
Expand Down Expand Up @@ -159,6 +157,8 @@ func networkWithClaimObjects(
return net, claims
}

// encodeSessionHeader returns a base64 encoded string of a json
// serialized session header.
func encodeSessionHeader(
bryanchriswhite marked this conversation as resolved.
Show resolved Hide resolved
t *testing.T,
appAddr string,
Expand All @@ -167,18 +167,19 @@ func encodeSessionHeader(
) string {
t.Helper()

argSessionHeader := &types2.SessionHeader{
argSessionHeader := &sessiontypes.SessionHeader{
ApplicationAddress: appAddr,
SessionStartBlockHeight: sessionStartHeight,
SessionId: sessionId,
SessionEndBlockHeight: sessionStartHeight + numBlocksPerSession,
Service: &sharedtypes.Service{Id: testServiceId},
}
cdc := codec.NewProtoCodec(types3.NewInterfaceRegistry())
cdc := codec.NewProtoCodec(codectypes.NewInterfaceRegistry())
sessionHeaderBz := cdc.MustMarshalJSON(argSessionHeader)
return base64.StdEncoding.EncodeToString(sessionHeaderBz)
}

// createClaim sends a tx using the test CLI to create an on-chain claim
func createClaim(
bryanchriswhite marked this conversation as resolved.
Show resolved Hide resolved
t *testing.T,
net *network.Network,
Expand All @@ -201,16 +202,17 @@ func createClaim(
fmt.Sprintf("--%s=%s", flags.FlagFrom, supplierAddr),
fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation),
fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync),
fmt.Sprintf("--%s=%s", flags.FlagFees, types4.NewCoins(types4.NewCoin(net.Config.BondDenom, math.NewInt(10))).String()),
fmt.Sprintf("--%s=%s", flags.FlagFees, sdktypes.NewCoins(sdktypes.NewCoin(net.Config.BondDenom, math.NewInt(10))).String()),
}

responseRaw, err := cli.ExecTestCLICmd(ctx, cli2.CmdCreateClaim(), args)
responseRaw, err := testcli.ExecTestCLICmd(ctx, cli.CmdCreateClaim(), args)
require.NoError(t, err)
var responseJson map[string]interface{}
err = json.Unmarshal(responseRaw.Bytes(), &responseJson)
require.NoError(t, err)
require.Equal(t, float64(0), responseJson["code"], "code is not 0 in the response: %v", responseJson)

// TODO_TECHDEBT: Forward the actual claim in the response once the response is updated to return it.
return &types.Claim{
bryanchriswhite marked this conversation as resolved.
Show resolved Hide resolved
SupplierAddress: supplierAddr,
SessionId: sessionId,
Expand All @@ -219,6 +221,9 @@ func createClaim(
}
}

// getSessionId sends a query using the test CLI to get a session for the inputs provided.
// It is assumed that the supplierAddr will be in that session based on the test design, but this
// is insured in this function before it's successfully returned.
func getSessionId(
bryanchriswhite marked this conversation as resolved.
Show resolved Hide resolved
t *testing.T,
net *network.Network,
Expand All @@ -229,8 +234,8 @@ func getSessionId(
t.Helper()
ctx := context.TODO()

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

Choose a reason for hiding this comment

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

Thoughts on having the caller pass in this local constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Let's hold off 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙏

BlockHeight: sessionStartHeight,
Expand Down
3 changes: 0 additions & 3 deletions x/supplier/client/cli/query_claim_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,11 @@ func TestClaim_Show(t *testing.T) {
sessionCount := 1
supplierCount := 3
appCount := 3
serviceCount := 1

net, claims := networkWithClaimObjects(
t, sessionCount,
appCount,
supplierCount,
serviceCount,
)

ctx := net.Validators[0].ClientCtx
Expand Down Expand Up @@ -106,7 +104,6 @@ func TestClaim_List(t *testing.T) {
t, sessionCount,
supplierCount,
appCount,
serviceCount,
)

ctx := net.Validators[0].ClientCtx
Expand Down
6 changes: 1 addition & 5 deletions x/supplier/keeper/msg_server_create_claim.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package keeper

import (
"context"
"log"

sdk "github.com/cosmos/cosmos-sdk/types"

Expand Down Expand Up @@ -60,9 +59,6 @@ func (k msgServer) CreateClaim(goCtx context.Context, msg *suppliertypes.MsgCrea
)
}
bryanchriswhite marked this conversation as resolved.
Show resolved Hide resolved

// TODO_TECHDEBT(#181): refactor once structured logging is available.
log.Printf("DEBUG: validated claim with sessionId %q for supplier %q", sessionRes.Session.SessionId, msg.GetSupplierAddress())

logger.
With(
"session_id", sessionRes.GetSession().GetSessionId(),
Expand Down Expand Up @@ -100,6 +96,6 @@ func (k msgServer) CreateClaim(goCtx context.Context, msg *suppliertypes.MsgCrea
).
Debug("created claim")

// TODO_CONSIDERATION: perhaps it would be useful to return the claim in the response.
// TODO: return the claim in the response.
return &suppliertypes.MsgCreateClaimResponse{}, nil
}
24 changes: 15 additions & 9 deletions x/supplier/keeper/msg_server_create_claim_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,20 @@ import (
const testServiceId = "svc1"

func TestMsgServer_CreateClaim_Success(t *testing.T) {
appAddr, supplierAddr := sample.AccAddress(), sample.AccAddress()
appSupplierPair := supplier.AppSupplierPair{
AppAddr: sample.AccAddress(),
SupplierAddr: sample.AccAddress(),
}
service := &sharedtypes.Service{Id: testServiceId}
sessionFixturesByAddr := supplier.NewSessionFixturesWithPairings(t, service, appAddr, supplierAddr)
sessionFixturesByAddr := supplier.NewSessionFixturesWithPairings(t, service, appSupplierPair)

supplierKeeper, sdkCtx := keepertest.SupplierKeeper(t, sessionFixturesByAddr)
srv := keeper.NewMsgServerImpl(*supplierKeeper)
ctx := sdk.WrapSDKContext(sdkCtx)

claimMsg := newTestClaimMsg(t)
claimMsg.SupplierAddress = supplierAddr
claimMsg.SessionHeader.ApplicationAddress = appAddr
claimMsg.SupplierAddress = appSupplierPair.SupplierAddr
claimMsg.SessionHeader.ApplicationAddress = appSupplierPair.AppAddr

createClaimRes, err := srv.CreateClaim(ctx, claimMsg)
require.NoError(t, err)
Expand All @@ -48,8 +51,11 @@ func TestMsgServer_CreateClaim_Success(t *testing.T) {

func TestMsgServer_CreateClaim_Error(t *testing.T) {
service := &sharedtypes.Service{Id: testServiceId}
appAddr, supplierAddr := sample.AccAddress(), sample.AccAddress()
sessionFixturesByAppAddr := supplier.NewSessionFixturesWithPairings(t, service, appAddr, supplierAddr)
appSupplierPair := supplier.AppSupplierPair{
AppAddr: sample.AccAddress(),
SupplierAddr: sample.AccAddress(),
}
sessionFixturesByAppAddr := supplier.NewSessionFixturesWithPairings(t, service, appSupplierPair)

supplierKeeper, sdkCtx := keepertest.SupplierKeeper(t, sessionFixturesByAppAddr)
srv := keeper.NewMsgServerImpl(*supplierKeeper)
Expand All @@ -64,8 +70,8 @@ func TestMsgServer_CreateClaim_Error(t *testing.T) {
desc: "on-chain session ID must match claim msg session ID",
claimMsgFn: func(t *testing.T) *types.MsgCreateClaim {
msg := newTestClaimMsg(t)
msg.SupplierAddress = supplierAddr
msg.SessionHeader.ApplicationAddress = appAddr
msg.SupplierAddress = appSupplierPair.SupplierAddr
msg.SessionHeader.ApplicationAddress = appSupplierPair.AppAddr
msg.SessionHeader.SessionId = "invalid_session_id"

return msg
Expand All @@ -76,7 +82,7 @@ func TestMsgServer_CreateClaim_Error(t *testing.T) {
desc: "claim msg supplier address must be in the session",
claimMsgFn: func(t *testing.T) *types.MsgCreateClaim {
msg := newTestClaimMsg(t)
msg.SessionHeader.ApplicationAddress = appAddr
msg.SessionHeader.ApplicationAddress = appSupplierPair.AppAddr

// Overwrite supplier address to one not included in the session fixtures.
msg.SupplierAddress = sample.AccAddress()
Expand Down
Loading