[Testing] Unit tests, E2E tests, logging and other Fixes / "Touchups" (…

…#253)

This is aiming to fix multiple issues in E2E tests and just general QoL improvements:

1. Adding a comment in `relay.feature` about the need to run `make supplier1_stake && make app1_stake` until we fix #180 (known issue)
2. Replacing the reference to host `sequencer-poktroll-sequencer` with `localhost` (see first screenshot below)
3. Fixed a bug leading us to assert against a `MsgSubmitProof` event instead of a `MsgCreateClaim` event (see second screenshot below)
4. Fixed on-chain logging under `x/*` by replacing `logger\.(Info|Error)\("([^"]+)"(?:, (.*))?\)` with `logger.$1(fmt.Sprintf("$2", $3))` so it works as expected
5. Some quality-of-life comments & TODOs
6. Minor additions to unit tests from #220

.github/workflows-helpers/run-e2e-test-job-template.yaml

@@ -31,6 +31,8 @@ spec:
                   name: celestia-secret
             - name: POCKET_NODE
               value: tcp://${NAMESPACE}-sequencer:36657
+            - name: SEQUENCER_RPC_ENDPOINT
+              value: ${NAMESPACE}-sequencer:36657
             - name: E2E_DEBUG_OUTPUT
               value: "false" # Flip to true to see the command and result of the execution
             - name: POKTROLLD_HOME

Dockerfile.dev

@@ -23,6 +23,5 @@ RUN ignite chain init --skip-proto
 EXPOSE 8545
 EXPOSE 8546
 EXPOSE 8547
-EXPOSE 8548
 
 ENTRYPOINT ["ignite"]

Tiltfile

@@ -33,6 +33,7 @@ if localnet_config["helm_chart_local_repo"]["enabled"]:
     print("Using local helm chart repo " + helm_chart_local_repo)
     chart_prefix = helm_chart_local_repo + "/charts/"
 
+
 # Import files into Kubernetes ConfigMap
 def read_files_from_directory(directory):
     files = listdir(directory)
@@ -158,7 +159,7 @@ k8s_resource(
     "relayminers",
     labels=["blockchains"],
     resource_deps=["sequencer"],
-    port_forwards=["8548", "40005"],
+    port_forwards=["8545", "40005"],
 )
 k8s_resource(
     "appgateservers",

app/app.go

@@ -110,6 +110,8 @@ import (
 	ibckeeper "github.com/cosmos/ibc-go/v7/modules/core/keeper"
 	solomachine "github.com/cosmos/ibc-go/v7/modules/light-clients/06-solomachine"
 	ibctm "github.com/cosmos/ibc-go/v7/modules/light-clients/07-tendermint"
+	"github.com/spf13/cast"
+
 	appparams "github.com/pokt-network/poktroll/app/params"
 	"github.com/pokt-network/poktroll/docs"
 	applicationmodule "github.com/pokt-network/poktroll/x/application"
@@ -133,7 +135,6 @@ import (
 	tokenomicsmodule "github.com/pokt-network/poktroll/x/tokenomics"
 	tokenomicsmodulekeeper "github.com/pokt-network/poktroll/x/tokenomics/keeper"
 	tokenomicsmoduletypes "github.com/pokt-network/poktroll/x/tokenomics/types"
-	"github.com/spf13/cast"
 )
 
 const (

e2e/tests/node.go

@@ -118,9 +118,9 @@ func (p *pocketdBin) runCurlPostCmd(rpcUrl string, service string, data string,
 	dataStr := fmt.Sprintf("%s", data)
 	urlStr := fmt.Sprintf("%s/%s", rpcUrl, service)
 	base := []string{
-		"-v",                                   // verbose output
-		"-sS",                                  // silent with error
-		"POST",                                 // HTTP method
+		"-v",         // verbose output
+		"-sS",        // silent with error
+		"-X", "POST", // HTTP method
 		"-H", "Content-Type: application/json", // HTTP headers
 		"--data", dataStr, urlStr, // POST data
 	}

e2e/tests/relay.feature

@@ -1,5 +1,7 @@
 Feature: Relay Namespace
 
+    # TODO_TECHDEBT(@Olshansk, #180): This test requires you to run `make supplier1_stake && make app1_stake` first
+    # As a shorter workaround, we can also add steps that stake the application and supplier as part of the scenario.
     Scenario: App can send relay to Supplier
         Given the user has the pocketd binary installed
         And the application "app1" is staked for service "anvil"

e2e/tests/session_steps_test.go

@@ -11,20 +11,19 @@ import (
 	"time"
 
 	abci "github.com/cometbft/cometbft/abci/types"
-	"github.com/stretchr/testify/require"
-
 	"github.com/pokt-network/poktroll/pkg/client/events"
 	"github.com/pokt-network/poktroll/pkg/either"
 	"github.com/pokt-network/poktroll/pkg/observable"
 	"github.com/pokt-network/poktroll/pkg/observable/channel"
 	"github.com/pokt-network/poktroll/testutil/testclient"
 	suppliertypes "github.com/pokt-network/poktroll/x/supplier/types"
+	"github.com/stretchr/testify/require"
 )
 
 const (
 	createClaimTimeoutDuration   = 10 * time.Second
 	eitherEventsReplayBufferSize = 100
-	msgClaimSenderQueryFmt       = "tm.event='Tx' AND message.sender='%s'"
+	msgClaimSenderQueryFmt       = "tm.event='Tx' AND message.sender='%s' AND message.action='/pocket.supplier.MsgCreateClaim'"
 	testServiceId                = "anvil"
 	eitherEventsBzReplayObsKey   = "eitherEventsBzReplayObsKey"
 	preExistingClaimsKey         = "preExistingClaimsKey"
@@ -86,17 +85,21 @@ func (s *suite) AfterTheSupplierCreatesAClaimForTheSessionForServiceForApplicati
 func (s *suite) TheClaimCreatedBySupplierForServiceForApplicationShouldBePersistedOnchain(supplierName, serviceId, appName string) {
 	ctx := context.Background()
 
-	claimsRes, err := s.supplierQueryClient.AllClaims(ctx, &suppliertypes.QueryAllClaimsRequest{
+	allClaimsRes, err := s.supplierQueryClient.AllClaims(ctx, &suppliertypes.QueryAllClaimsRequest{
 		Filter: &suppliertypes.QueryAllClaimsRequest_SupplierAddress{
 			SupplierAddress: accNameToAddrMap[supplierName],
 		},
 	})
 	require.NoError(s, err)
-	require.NotNil(s, claimsRes)
+	require.NotNil(s, allClaimsRes)
 
 	// Assert that the number of claims has increased by one.
 	preExistingClaims := s.scenarioState[preExistingClaimsKey].([]suppliertypes.Claim)
-	require.Len(s, claimsRes.Claim, len(preExistingClaims)+1)
+	// NB: We are avoiding the use of require.Len here because it provides unreadable output
+	// TODO_TECHDEBT: Due to the speed of the blocks of the LocalNet sequencer, along with the small number
+	// of blocks per session, multiple claims may be created throughout the duration of the test. Until
+	// these values are appropriately adjusted
+	require.Greater(s, len(allClaimsRes.Claim), len(preExistingClaims), "number of claims must have increased")
 
 	// TODO_IMPROVE: assert that the root hash of the claim contains the correct
 	// SMST sum. The sum can be retrieved by parsing the last 8 bytes as a
@@ -106,7 +109,7 @@ func (s *suite) TheClaimCreatedBySupplierForServiceForApplicationShouldBePersist
 	// TODO_IMPROVE: add assertions about serviceId and appName and/or incorporate
 	// them into the scenarioState key(s).
 
-	claim := claimsRes.Claim[0]
+	claim := allClaimsRes.Claim[0]
 	require.Equal(s, accNameToAddrMap[supplierName], claim.SupplierAddress)
 }
 
@@ -118,9 +121,9 @@ func (s *suite) TheSupplierHasServicedASessionWithRelaysForServiceForApplication
 
 	// Query for any existing claims so that we can compensate for them in the
 	// future assertions about changes in on-chain claims.
-	claimsRes, err := s.supplierQueryClient.AllClaims(ctx, &suppliertypes.QueryAllClaimsRequest{})
+	allClaimsRes, err := s.supplierQueryClient.AllClaims(ctx, &suppliertypes.QueryAllClaimsRequest{})
 	require.NoError(s, err)
-	s.scenarioState[preExistingClaimsKey] = claimsRes.Claim
+	s.scenarioState[preExistingClaimsKey] = allClaimsRes.Claim
 
 	// Construct an events query client to listen for tx events from the supplier.
 	msgSenderQuery := fmt.Sprintf(msgClaimSenderQueryFmt, accNameToAddrMap[supplierName])

go.mod

@@ -24,6 +24,7 @@ require (
 	github.com/athanorlabs/go-dleq v0.1.0
 	github.com/cometbft/cometbft v0.37.2
 	github.com/cometbft/cometbft-db v0.8.0
+	github.com/cosmos/cosmos-proto v1.0.0-beta.2
 	github.com/cosmos/cosmos-sdk v0.47.3
 	github.com/cosmos/gogoproto v1.4.11
 	github.com/cosmos/ibc-go/v7 v7.1.0
@@ -45,6 +46,7 @@ require (
 	go.uber.org/multierr v1.11.0
 	golang.org/x/crypto v0.15.0
 	golang.org/x/sync v0.5.0
+	google.golang.org/genproto/googleapis/api v0.0.0-20230913181813-007df8e322eb
 	google.golang.org/grpc v1.59.0
 	gopkg.in/yaml.v2 v2.4.0
 )
@@ -88,7 +90,6 @@ require (
 	github.com/containerd/cgroups v1.1.0 // indirect
 	github.com/coreos/go-systemd/v22 v22.5.0 // indirect
 	github.com/cosmos/btcutil v1.0.5 // indirect
-	github.com/cosmos/cosmos-proto v1.0.0-beta.2 // indirect
 	github.com/cosmos/go-bip39 v1.0.0 // indirect
 	github.com/cosmos/gogogateway v1.2.0 // indirect
 	github.com/cosmos/iavl v0.20.0 // indirect
@@ -284,7 +285,6 @@ require (
 	google.golang.org/api v0.143.0 // indirect
 	google.golang.org/appengine v1.6.7 // indirect
 	google.golang.org/genproto v0.0.0-20230913181813-007df8e322eb // indirect
-	google.golang.org/genproto/googleapis/api v0.0.0-20230913181813-007df8e322eb // indirect
 	google.golang.org/genproto/googleapis/rpc v0.0.0-20230920204549-e6e6cdab5c13 // indirect
 	google.golang.org/protobuf v1.31.0 // indirect
 	gopkg.in/ini.v1 v1.67.0 // indirect

localnet/kubernetes/values-relayminer.yaml

@@ -1,4 +1,3 @@
 config:
   query_node_url: tcp://sequencer-poktroll-sequencer:36657
   network_node_url: tcp://sequencer-poktroll-sequencer:36657
-

pkg/client/block/block.go

@@ -27,9 +27,9 @@ func (blockEvent *cometBlockEvent) Hash() []byte {
 	return blockEvent.Block.LastBlockID.Hash.Bytes()
 }
 
-// newCometBlockEventFactoryFn is a factory function that returns a functon
+// newCometBlockEventFactoryFn is a factory function that returns a function
 // that attempts to deserialize the given bytes into a comet block.
-// if the resulting block has a height of zero, assume the event was not a block
+// If the resulting block has a height of zero, assume the event was not a block
 // event and return an ErrUnmarshalBlockEvent error.
 func newCometBlockEventFactoryFn() events.NewEventsFn[client.Block] {
 	return func(blockMsgBz []byte) (client.Block, error) {
@@ -38,7 +38,9 @@ func newCometBlockEventFactoryFn() events.NewEventsFn[client.Block] {
 			return nil, err
 		}
 
-		// If msg does not match the expected format then the block's height has a zero value.
+		// The header height should never be zero. If it is, it means that blockMsg
+		// does not match the expected format which led unmarshaling to fail,
+		// and blockHeader.height to have a default value.
 		if blockMsg.Block.Header.Height == 0 {
 			return nil, events.ErrEventsUnmarshalEvent.
 				Wrapf("with block data: %s", string(blockMsgBz))

pkg/client/block/client.go

@@ -15,11 +15,11 @@ const (
 	// the chain.
 	// See: https://docs.cosmos.network/v0.47/learn/advanced/events#default-events
 	committedBlocksQuery = "tm.event='NewBlock'"
-	// TODO_TECHDEBT/TODO_FUTURE: add a `blocksReplayLimit` field to the block
-	// client struct that defaults to this but can be overridden via an option
-	// in future work.
+
 	// defaultBlocksReplayLimit is the number of blocks that the replay
 	// observable returned by LastNBlocks() will be able to replay.
+	// TODO_TECHDEBT/TODO_FUTURE: add a `blocksReplayLimit` field to the blockClient
+	// struct that defaults to this but can be overridden via an option.
 	defaultBlocksReplayLimit = 100
 )
 

pkg/client/delegation/client.go

@@ -15,11 +15,11 @@ const (
 	// See: https://docs.cosmos.network/v0.47/learn/advanced/events#subscribing-to-events
 	// And: https://docs.cosmos.network/v0.47/learn/advanced/events#default-events
 	delegationEventQuery = "message.action='pocket.application.EventRedelegation'"
-	// TODO_TECHDEBT/TODO_FUTURE: add a `blocksReplayLimit` field to the block
-	// client struct that defaults to this but can be overridden via an option
-	// in future work.
+
 	// defaultRedelegationsReplayLimit is the number of redelegations that the
 	// replay observable returned by LastNRedelegations() will be able to replay.
+	// TODO_TECHDEBT/TODO_FUTURE: add a `redelegationsReplayLimit` field to the `delegationClient`
+	// struct that defaults to this but can be overridden via an option.
 	defaultRedelegationsReplayLimit = 100
 )
 

pkg/client/events/replay_client_example_test.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 
 	"cosmossdk.io/depinject"
-
 	"github.com/pokt-network/poktroll/pkg/client/events"
 	"github.com/pokt-network/poktroll/pkg/observable"
 	"github.com/pokt-network/poktroll/pkg/polylog"
@@ -23,6 +22,8 @@ const (
 	replayObsBufferSize = 1
 )
 
+var _ EventType = (*eventType)(nil)
+
 // Define an interface to represent the onchain event
 type EventType interface {
 	GetName() string // Illustrative only; arbitrary interfaces are supported.

pkg/client/interface.go

@@ -111,7 +111,7 @@ type Block interface {
 
 // Redelegation is an interface which wraps the EventRedelegation event
 // emitted by the application module.
-// See: proto/pocket/application/types/event.proto#EventRedelegatio
+// See: proto/pocket/application/types/event.proto#EventRedelegation
 type Redelegation interface {
 	GetAppAddress() string
 	GetGatewayAddress() string

pkg/client/supplier/client.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/pokt-network/poktroll/pkg/client"
 	"github.com/pokt-network/poktroll/pkg/client/keyring"
+	"github.com/pokt-network/poktroll/pkg/polylog"
 	sessiontypes "github.com/pokt-network/poktroll/x/session/types"
 	suppliertypes "github.com/pokt-network/poktroll/x/supplier/types"
 )
@@ -66,6 +67,8 @@ func (sClient *supplierClient) SubmitProof(
 	sessionHeader sessiontypes.SessionHeader,
 	proof *smt.SparseMerkleClosestProof,
 ) error {
+	logger := polylog.Ctx(ctx)
+
 	proofBz, err := proof.Marshal()
 	if err != nil {
 		return err
@@ -82,6 +85,16 @@ func (sClient *supplierClient) SubmitProof(
 		return err
 	}
 
+	// TODO_IMPROVE: log details related to what & how much is being proven
+	logger.Info().
+		Fields(map[string]any{
+			"supplier_addr": sClient.signingKeyAddr.String(),
+			"app_addr":      sessionHeader.ApplicationAddress,
+			"session_id":    sessionHeader.SessionId,
+			"service":       sessionHeader.Service.Id,
+		}).
+		Msg("submitted a new proof")
+
 	return <-errCh
 }
 
@@ -93,6 +106,8 @@ func (sClient *supplierClient) CreateClaim(
 	sessionHeader sessiontypes.SessionHeader,
 	rootHash []byte,
 ) error {
+	logger := polylog.Ctx(ctx)
+
 	msg := &suppliertypes.MsgCreateClaim{
 		SupplierAddress: sClient.signingKeyAddr.String(),
 		SessionHeader:   &sessionHeader,
@@ -104,8 +119,17 @@ func (sClient *supplierClient) CreateClaim(
 		return err
 	}
 
-	err = <-errCh
-	return err
+	// TODO_IMPROVE: log details related to how much is claimed
+	logger.Info().
+		Fields(map[string]any{
+			"supplier_addr": sClient.signingKeyAddr.String(),
+			"app_addr":      sessionHeader.ApplicationAddress,
+			"session_id":    sessionHeader.SessionId,
+			"service":       sessionHeader.Service.Id,
+		}).
+		Msg("created a new claim")
+
+	return <-errCh
 }
 
 // validateConfigAndSetDefaults attempts to get the address from the keyring

pkg/client/supplier/client_test.go

@@ -8,19 +8,21 @@ import (
 
 	"cosmossdk.io/depinject"
 	"github.com/golang/mock/gomock"
-	"github.com/pokt-network/smt"
-	"github.com/stretchr/testify/require"
-
-	"github.com/pokt-network/poktroll/testutil/mockclient"
-
 	"github.com/pokt-network/poktroll/pkg/client/keyring"
 	"github.com/pokt-network/poktroll/pkg/client/supplier"
+	"github.com/pokt-network/poktroll/testutil/mockclient"
 	"github.com/pokt-network/poktroll/testutil/testclient/testkeyring"
 	"github.com/pokt-network/poktroll/testutil/testclient/testtx"
 	sessiontypes "github.com/pokt-network/poktroll/x/session/types"
+	sharedtypes "github.com/pokt-network/poktroll/x/shared/types"
+	"github.com/pokt-network/smt"
+	"github.com/stretchr/testify/require"
 )
 
-var testSigningKeyName = "test_signer"
+const (
+	testSigningKeyName = "test_signer"
+	testService        = "test_service"
+)
 
 func TestNewSupplierClient(t *testing.T) {
 	ctrl := gomock.NewController(t)
@@ -85,7 +87,7 @@ func TestSupplierClient_CreateClaim(t *testing.T) {
 	require.NoError(t, err)
 
 	txCtxMock, _ := testtx.NewAnyTimesTxTxContext(t, keyring)
-	txClientMock := testtx.NewOneTimeDelayedSignAndBroadcastTxClient(t, signAndBroadcastDelay)
+	txClientMock := testtx.NewOneTimeDelayedSignAndBroadcastTxClient(t, ctx, signAndBroadcastDelay)
 
 	signingKeyOpt := supplier.WithSigningKeyName(testAppKey.Name)
 	deps := depinject.Supply(
@@ -102,6 +104,9 @@ func TestSupplierClient_CreateClaim(t *testing.T) {
 		ApplicationAddress:      testAppAddr.String(),
 		SessionStartBlockHeight: 0,
 		SessionId:               "",
+		Service: &sharedtypes.Service{
+			Id: testService,
+		},
 	}
 
 	go func() {
@@ -141,7 +146,7 @@ func TestSupplierClient_SubmitProof(t *testing.T) {
 	require.NoError(t, err)
 
 	txCtxMock, _ := testtx.NewAnyTimesTxTxContext(t, keyring)
-	txClientMock := testtx.NewOneTimeDelayedSignAndBroadcastTxClient(t, signAndBroadcastDelay)
+	txClientMock := testtx.NewOneTimeDelayedSignAndBroadcastTxClient(t, ctx, signAndBroadcastDelay)
 
 	signingKeyOpt := supplier.WithSigningKeyName(testAppKey.Name)
 	deps := depinject.Supply(
@@ -157,6 +162,9 @@ func TestSupplierClient_SubmitProof(t *testing.T) {
 		ApplicationAddress:      testAppAddr.String(),
 		SessionStartBlockHeight: 0,
 		SessionId:               "",
+		Service: &sharedtypes.Service{
+			Id: testService,
+		},
 	}
 
 	kvStore, err := smt.NewKVStore("")

pkg/relayer/proxy/synchronous.go

@@ -8,6 +8,7 @@ import (
 	"net/url"
 
 	sdkerrors "cosmossdk.io/errors"
+
 	"github.com/pokt-network/poktroll/pkg/polylog"
 	"github.com/pokt-network/poktroll/pkg/relayer"
 	"github.com/pokt-network/poktroll/x/service/types"