[Payload] Refactoring how null (i.e. empty Data) payloads are returned (#382)

- Introduced `EmptyErrorPayload()` to replace `protocol.Payload{}` returns with unique error data value
- Add `SHOULD NEVER HAPPEN` to a few log messages
- Ensure `Payload` struct is fully hydrated

Author: Olshansk

gateway/request_context.go

@@ -333,7 +333,7 @@ func (rc *requestContext) BroadcastAllObservations() {
 		}
 
 		// The service request context contains all the details the QoS needs to update its internal metrics about endpoint(s), which it should use to build
-		// the observation.QoSObservations struct.
+		// the qosobservations.Observations struct.
 		// This ensures that separate PATH instances can communicate and share their QoS observations.
 		// The QoS context will be nil if the target service ID is not specified correctly by the request.
 		var qosObservations qosobservations.Observations

protocol/payload.go

@@ -4,14 +4,9 @@ import (
 	sharedtypes "github.com/pokt-network/poktroll/x/shared/types"
 )
 
-// TODO_TECHDEBT(@adshmh): use an interface here that returns the serialized form of the request, with the following requirements:
-// 1. Payload should return the serialized form of the request to be delivered to the backend service,
-// i.e. the onchain service to which the protocol endpoint proxies relay requests.
-// 2. Use an enum to represent the underlying spec/standard, e.g. REST/JSONRPC/gRPC/etc.
-//
-// Payload currently supports HTTP(s) requests to blockchain services
-// TODO_DOCUMENT(@adshmh): add more examples, e.g. for RESTful services, as support for more types of services
-// is added.
+// Payload represents the HTTP(s) requests proxied between clients and backend services.
+// TODO_DOCUMENT(@adshmh): Add more examples (e.g. for RESTful services)
+// TODO_IMPROVE(@adshmh): Use an interface here that returns the serialized form of the request.
 type Payload struct {
 	Data            string
 	Method          string
@@ -23,4 +18,20 @@ type Payload struct {
 	// - RPCType_COMET_BFT: CometBFT RPC (typically port 26657)
 	// - RPCType_JSON_RPC: EVM JSON-RPC (typically port 8545)
 	RPCType sharedtypes.RPCType
-}
+}
+
+// EmptyPayload returns an empty payload struct.
+// It should only be used when an error is encountered and the actual request cannot be
+// proxied, parsed or otherwise processed.
+func EmptyErrorPayload() Payload {
+	return Payload{
+		// This is an intentional placeholder to distinguish errors in retrieving payloads
+		// from explicit empty payloads set by PATH.
+		Data:            "PATH_EmptyErrorPayload",
+		Method:          "",
+		Path:            "",
+		Headers:         map[string]string{},
+		TimeoutMillisec: 0,
+		RPCType:         sharedtypes.RPCType_UNKNOWN_RPC,
+	}
+}

qos/cosmos/error_context.go

@@ -80,8 +80,8 @@ func (ec *errorContext) GetObservations() qosobservations.Observations {
 // It logs a warning and returns nil.
 // Implements the gateway.RequestQoSContext interface.
 func (ec *errorContext) GetServicePayload() protocol.Payload {
-	ec.logger.Warn().Msg("Invalid usage: errorContext.GetServicePayload() should never be called.")
-	return protocol.Payload{}
+	ec.logger.Warn().Msg("SHOULD NEVER HAPPEN: errorContext.GetServicePayload() should never be called.")
+	return protocol.EmptyErrorPayload()
 }
 
 // UpdateWithResponse should never be called.
@@ -91,14 +91,14 @@ func (ec *errorContext) UpdateWithResponse(endpointAddr protocol.EndpointAddr, e
 	ec.logger.With(
 		"endpoint_addr", endpointAddr,
 		"endpoint_response_len", len(endpointSerializedResponse),
-	).Warn().Msg("Invalid usage: errorContext.UpdateWithResponse() should never be called.")
+	).Warn().Msg("SHOULD NEVER HAPPEN: errorContext.UpdateWithResponse() should never be called.")
 }
 
 // UpdateWithResponse should never be called.
 // It logs a warning and returns a failing selector that logs a warning on all selection attempts.
 // Implements the gateway.RequestQoSContext interface.
 func (ec *errorContext) GetEndpointSelector() protocol.EndpointSelector {
-	ec.logger.Warn().Msg("Invalid usage: errorContext.GetEndpointSelector() should never be called.")
+	ec.logger.Warn().Msg("SHOULD NEVER HAPPEN: errorContext.GetEndpointSelector() should never be called.")
 
 	return errorTrackingSelector{
 		logger: ec.logger,
@@ -119,7 +119,7 @@ type errorTrackingSelector struct {
 func (ets errorTrackingSelector) Select(endpoints protocol.EndpointAddrList) (protocol.EndpointAddr, error) {
 	ets.logger.With(
 		"num_endpoints", len(endpoints),
-	).Warn().Msg("Invalid usage: errorTrackingSelector.Select() should never be called.")
+	).Warn().Msg("SHOULD NEVER HAPPEN: errorTrackingSelector.Select() should never be called.")
 
 	return protocol.EndpointAddr(""), errInvalidSelectorUsage
 }
@@ -131,7 +131,7 @@ func (ets errorTrackingSelector) SelectMultiple(endpoints protocol.EndpointAddrL
 	ets.logger.With(
 		"num_endpoints", len(endpoints),
 		"num_endpoints_to_select", numEndpoints,
-	).Warn().Msg("Invalid usage: errorTrackingSelector.SelectMultiple() should never be called.")
+	).Warn().Msg("SHOULD NEVER HAPPEN: errorTrackingSelector.SelectMultiple() should never be called.")
 
 	return nil, errInvalidSelectorUsage
 }

qos/cosmos/request_validator_jsonrpc.go

@@ -19,6 +19,8 @@ const (
 	// maximum length of the error message stored in request validation failure observations and logs.
 	maxErrMessageLen = 1000
 
+	// defaultJSONRPCRequestTimeoutMillisec is the default timeout when sending a request to a Cosmos blockchain endpoint.
+	// TODO_IMPROVE(@adshmh): Support method level specific timeouts and allow the user to configure them.
 	defaultJSONRPCRequestTimeoutMillisec = 10_000
 )
 
@@ -106,22 +108,23 @@ func (rv *requestValidator) buildJSONRPCRequestContext(
 	}, true
 }
 
+// buildJSONRPCServicePayload builds a protocol payload for a JSONRPC request.
 func buildJSONRPCServicePayload(rpcType sharedtypes.RPCType, jsonrpcReq jsonrpc.Request) (protocol.Payload, error) {
 	// DEV_NOTE: marshaling the request, rather than using the original payload, is necessary.
 	// Otherwise, a request missing `id` field could fail.
 	// See the Request struct in `jsonrpc` package for the details.
 	reqBz, err := json.Marshal(jsonrpcReq)
 	if err != nil {
-		return protocol.Payload{}, err
+		return protocol.EmptyErrorPayload(), err
 	}
 
 	return protocol.Payload{
-		Data: string(reqBz),
-		// JSONRPC always uses POST
-		Method:          http.MethodPost,
+		Data:            string(reqBz),
+		Method:          http.MethodPost, // JSONRPC always uses POST
+		Path:            "",              // JSONRPC does not use paths
+		Headers:         map[string]string{},
 		TimeoutMillisec: defaultJSONRPCRequestTimeoutMillisec,
-		// Add the RPCType hint, so protocol sets correct HTTP headers for the endpoint.
-		RPCType: rpcType,
+		RPCType:         rpcType, // Add the RPCType hint the so protocol sets correct HTTP headers for the endpoint.
 	}, nil
 }
 

qos/cosmos/request_validator_rest.go

@@ -4,16 +4,18 @@ import (
 	"errors"
 	"net/url"
 
+	"github.com/pokt-network/poktroll/pkg/polylog"
+	sharedtypes "github.com/pokt-network/poktroll/x/shared/types"
+
 	"github.com/buildwithgrove/path/gateway"
 	qosobservations "github.com/buildwithgrove/path/observation/qos"
 	"github.com/buildwithgrove/path/protocol"
 	"github.com/buildwithgrove/path/qos"
 	"github.com/buildwithgrove/path/qos/jsonrpc"
-	"github.com/pokt-network/poktroll/pkg/polylog"
-	sharedtypes "github.com/pokt-network/poktroll/x/shared/types"
 )
 
 // Default timeout for REST requests
+// TODO_IMPROVE(@adshmh): Support method level specific timeouts and allow the user to configure them.
 const defaultRESTRequestTimeoutMillisec = 30000
 
 // validateRESTRequest validates a REST request by:
@@ -124,11 +126,10 @@ func buildRESTServicePayload(
 	return protocol.Payload{
 		Data:            string(httpRequestBody),
 		Method:          httpRequestMethod,
+		Path:            path,
+		Headers:         map[string]string{},
 		TimeoutMillisec: defaultRESTRequestTimeoutMillisec,
-		// Add the RPCType hint, so protocol sets correct HTTP headers for the endpoint.
-		RPCType: rpcType,
-		// Set the request path, including raw query, if used.
-		Path: path,
+		RPCType:         rpcType, // Add the RPCType hint, so protocol sets correct HTTP headers for the endpoint.
 	}
 }
 

qos/error_context.go

@@ -68,8 +68,8 @@ func (rec *RequestErrorContext) GetObservations() qosobservations.Observations {
 // It logs a warning and returns nil.
 // Implements the gateway.RequestQoSContext interface.
 func (rec *RequestErrorContext) GetServicePayload() protocol.Payload {
-	rec.Logger.Warn().Msg("Invalid usage: RequestErrorContext.GetServicePayload() should never be called.")
-	return protocol.Payload{}
+	rec.Logger.Warn().Msg("SHOULD NEVER HAPPEN: RequestErrorContext.GetServicePayload() should never be called.")
+	return protocol.EmptyErrorPayload()
 }
 
 // UpdateWithResponse should never be called.
@@ -79,14 +79,14 @@ func (rec *RequestErrorContext) UpdateWithResponse(endpointAddr protocol.Endpoin
 	rec.Logger.With(
 		"endpoint_addr", endpointAddr,
 		"endpoint_response_len", len(endpointSerializedResponse),
-	).Warn().Msg("Invalid usage: RequestErrorContext.UpdateWithResponse() should never be called.")
+	).Warn().Msg("SHOULD NEVER HAPPEN: RequestErrorContext.UpdateWithResponse() should never be called.")
 }
 
 // UpdateWithResponse should never be called.
 // It logs a warning and returns a failing selector that logs a warning on all selection attempts.
 // Implements the gateway.RequestQoSContext interface.
 func (rec *RequestErrorContext) GetEndpointSelector() protocol.EndpointSelector {
-	rec.Logger.Warn().Msg("Invalid usage: RequestErrorContext.GetEndpointSelector() should never be called.")
+	rec.Logger.Warn().Msg("SHOULD NEVER HAPPEN: RequestErrorContext.GetEndpointSelector() should never be called.")
 
 	return errorTrackingSelector{
 		logger: rec.Logger,
@@ -107,7 +107,7 @@ type errorTrackingSelector struct {
 func (ets errorTrackingSelector) Select(endpoints protocol.EndpointAddrList) (protocol.EndpointAddr, error) {
 	ets.logger.With(
 		"num_endpoints", len(endpoints),
-	).Warn().Msg("Invalid usage: errorTrackingSelector.Select() should never be called.")
+	).Warn().Msg("SHOULD NEVER HAPPEN: errorTrackingSelector.Select() should never be called.")
 
 	return protocol.EndpointAddr(""), errInvalidSelectorUsage
 }
@@ -116,7 +116,7 @@ func (ets errorTrackingSelector) Select(endpoints protocol.EndpointAddrList) (pr
 // It logs a warning and returns an invalid usage error.
 // Implements the protocol.EndpointSelector interface.
 func (ets errorTrackingSelector) SelectMultiple(endpoints protocol.EndpointAddrList, numEndpoints uint) (protocol.EndpointAddrList, error) {
-	ets.logger.Warn().Msg("Invalid usage: errorTrackingSelector.SelectMultiple() should never be called.")
+	ets.logger.Warn().Msg("SHOULD NEVER HAPPEN: errorTrackingSelector.SelectMultiple() should never be called.")
 
 	return nil, errInvalidSelectorUsage
 }

qos/evm/context.go

@@ -5,15 +5,16 @@ import (
 	"net/http"
 
 	"github.com/pokt-network/poktroll/pkg/polylog"
+	sharedtypes "github.com/pokt-network/poktroll/x/shared/types"
 
 	"github.com/buildwithgrove/path/gateway"
 	qosobservations "github.com/buildwithgrove/path/observation/qos"
 	"github.com/buildwithgrove/path/protocol"
 	"github.com/buildwithgrove/path/qos/jsonrpc"
 )
 
-// TODO_MVP(@adshmh): Support individual configuration of timeout for every service that uses EVM QoS.
-// The default timeout when sending a request to an EVM blockchain endpoint.
+// defaultServiceRequestTimeoutMillisec is the default timeout when sending a request to an EVM blockchain endpoint.
+// TODO_IMPROVE(@adshmh): Support method level specific timeouts and allow the user to configure them.
 const defaultServiceRequestTimeoutMillisec = 10_500
 
 // requestContext provides the support required by the gateway
@@ -93,28 +94,22 @@ type requestContext struct {
 	endpointSelectionMetadata EndpointSelectionMetadata
 }
 
-// TODO_MVP(@adshmh): Ensure the JSONRPC request struct
-// can handle all valid service requests.
+// GetServicePayload implements the gateway.RequestQoSContext interface.
+// TODO_MVP(@adshmh): Ensure the JSONRPC request struct can handle all valid service requests.
 func (rc requestContext) GetServicePayload() protocol.Payload {
 	reqBz, err := json.Marshal(rc.jsonrpcReq)
 	if err != nil {
-		// TODO_MVP(@adshmh): find a way to guarantee this never happens,
-		// e.g. by storing the serialized form of the JSONRPC request
-		// at the time of creating the request context.
-		return protocol.Payload{}
+		rc.logger.Error().Err(err).Msg("SHOULD RARELY HAPPEN: requestContext.GetServicePayload() should never fail marshaling the JSONRPC request.")
+		return protocol.EmptyErrorPayload()
 	}
 
 	return protocol.Payload{
-		Data: string(reqBz),
-		// Method is alway POST for EVM-based blockchains.
-		Method: http.MethodPost,
-
-		// Path field is not used for EVM-based blockchains.
-
-		// TODO_IMPROVE: adjust the timeout based on the request method:
-		// An endpoint may need more time to process certain requests,
-		// as indicated by the request's method and/or parameters.
+		Data:            string(reqBz),
+		Method:          http.MethodPost, // Method is alway POST for EVM-based blockchains.
+		Path:            "",              // Path field is not used for EVM-based blockchains.
+		Headers:         map[string]string{},
 		TimeoutMillisec: defaultServiceRequestTimeoutMillisec,
+		RPCType:         sharedtypes.RPCType_JSON_RPC,
 	}
 }
 

qos/evm/error_context.go

@@ -80,8 +80,8 @@ func (ec *errorContext) GetObservations() qosobservations.Observations {
 // It logs a warning and returns nil.
 // Implements the gateway.RequestQoSContext interface.
 func (ec *errorContext) GetServicePayload() protocol.Payload {
-	ec.logger.Warn().Msg("Invalid usage: errorContext.GetServicePayload() should never be called.")
-	return protocol.Payload{}
+	ec.logger.Warn().Msg("SHOULD NEVER HAPPEN: errorContext.GetServicePayload() should never be called.")
+	return protocol.EmptyErrorPayload()
 }
 
 // UpdateWithResponse should never be called.
@@ -91,14 +91,14 @@ func (ec *errorContext) UpdateWithResponse(endpointAddr protocol.EndpointAddr, e
 	ec.logger.With(
 		"endpoint_addr", endpointAddr,
 		"endpoint_response_len", len(endpointSerializedResponse),
-	).Warn().Msg("Invalid usage: errorContext.UpdateWithResponse() should never be called.")
+	).Warn().Msg("SHOULD NEVER HAPPEN: errorContext.UpdateWithResponse() should never be called.")
 }
 
 // UpdateWithResponse should never be called.
 // It logs a warning and returns a failing selector that logs a warning on all selection attempts.
 // Implements the gateway.RequestQoSContext interface.
 func (ec *errorContext) GetEndpointSelector() protocol.EndpointSelector {
-	ec.logger.Warn().Msg("Invalid usage: errorContext.GetEndpointSelector() should never be called.")
+	ec.logger.Warn().Msg("SHOULD NEVER HAPPEN: errorContext.GetEndpointSelector() should never be called.")
 
 	return errorTrackingSelector{
 		logger: ec.logger,
@@ -119,7 +119,7 @@ type errorTrackingSelector struct {
 func (ets errorTrackingSelector) Select(endpoints protocol.EndpointAddrList) (protocol.EndpointAddr, error) {
 	ets.logger.With(
 		"num_endpoints", len(endpoints),
-	).Warn().Msg("Invalid usage: errorTrackingSelector.Select() should never be called.")
+	).Warn().Msg("SHOULD NEVER HAPPEN: errorTrackingSelector.Select() should never be called.")
 
 	return protocol.EndpointAddr(""), errInvalidSelectorUsage
 }

qos/noop/context.go

@@ -3,6 +3,8 @@ package noop
 import (
 	"net/http"
 
+	sharedtypes "github.com/pokt-network/poktroll/x/shared/types"
+
 	"github.com/buildwithgrove/path/gateway"
 	qosobservations "github.com/buildwithgrove/path/observation/qos"
 	"github.com/buildwithgrove/path/protocol"
@@ -55,7 +57,10 @@ func (rc *requestContext) GetServicePayload() protocol.Payload {
 	payload := protocol.Payload{
 		Data:            string(rc.httpRequestBody),
 		Method:          rc.httpRequestMethod,
+		Path:            "", // set below
+		Headers:         map[string]string{},
 		TimeoutMillisec: rc.endpointResponseTimeoutMillisec,
+		RPCType:         sharedtypes.RPCType_UNKNOWN_RPC,
 	}
 	if rc.httpRequestPath != "" {
 		payload.Path = rc.httpRequestPath

qos/solana/context.go

@@ -6,6 +6,7 @@ import (
 	"net/http"
 
 	"github.com/pokt-network/poktroll/pkg/polylog"
+	sharedtypes "github.com/pokt-network/poktroll/x/shared/types"
 
 	"github.com/buildwithgrove/path/gateway"
 	qosobservations "github.com/buildwithgrove/path/observation/qos"
@@ -15,8 +16,8 @@ import (
 )
 
 const (
-	// The default timeout when sending a request to
-	// a Solana blockchain endpoint.
+	// defaultServiceRequestTimeoutMillisec is the default timeout when sending a request to a Solana blockchain endpoint.
+	// TODO_IMPROVE(@adshmh): Support method level specific timeouts and allow the user to configure them.
 	defaultServiceRequestTimeoutMillisec = 15_000
 )
 
@@ -80,23 +81,17 @@ type requestContext struct {
 func (rc requestContext) GetServicePayload() protocol.Payload {
 	reqBz, err := json.Marshal(rc.JSONRPCReq)
 	if err != nil {
-		// TODO_MVP(@adshmh): find a way to guarantee this never happens,
-		// e.g. by storing the serialized form of the JSONRPC request
-		// at the time of creating the request context.
-		return protocol.Payload{}
+		rc.logger.Error().Err(err).Msg("SHOULD RARELY HAPPEN: requestContext.GetServicePayload() should never fail marshaling the JSONRPC request.")
+		return protocol.EmptyErrorPayload()
 	}
 
 	return protocol.Payload{
-		Data: string(reqBz),
-		// Method is alway POST for Solana.
-		Method: http.MethodPost,
-
-		// Path field is not used for Solana.
-
-		// TODO_IMPROVE: adjust the timeout based on the request method:
-		// An endpoint may need more time to process certain requests,
-		// as indicated by the request's method and/or parameters.
+		Data:            string(reqBz),
+		Method:          http.MethodPost, // Method is alway POST for Solana.
+		Path:            "",              // Path field is not used for Solana.
+		Headers:         map[string]string{},
 		TimeoutMillisec: defaultServiceRequestTimeoutMillisec,
+		RPCType:         sharedtypes.RPCType_JSON_RPC,
 	}
 }