chore: Address review change requests

proto/poktroll/shared/supplier.proto

@@ -10,16 +10,17 @@ import "poktroll/shared/service.proto";
 
 // Supplier is the type defining the actor in Pocket Network that provides RPC services.
 message Supplier {
-  // The Bech32 address of the owner (i.e. custodial, staker) using cosmos'
-  // ScalarDescriptor to ensure deterministic encoding.
-  // This is the address that owns the funds for staking and is the one that
-  // receives the rewards.
-  string owner_address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"];
-  // The Bech32 address of the operator (i.e. provider, non-custodial) using cosmos'
-  // ScalarDescriptor to ensure deterministic encoding.
-  // The operator address can update all the supplier's configurations, excluding
-  // the owner_address.
-  string address = 2 [(cosmos_proto.scalar) = "cosmos.AddressString"];
+  // The address of the owner (i.e. staker, custodial) that owns the funds for staking.
+  // By default, this address is the one that receives all the rewards unless owtherwise specified.
+  // The owner can initially stake the supplier and unstake it.
+  // All other configurations are managed by the operator.
+  string owner_address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // Bech32 cosmos address
+  // The operator address of the supplier that operates it.
+  // The operator address can update the supplier's configurations excluding the owner
+  // and operator addresses which do not change over the supplier's lifespan.
+  // TODO(red-0ne): Rename this to `operator_address` include all downstream
+  // variables, comments, docs, tests, etc...
+  string address = 2 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // Bech32 cosmos address
   cosmos.base.v1beta1.Coin stake = 3; // The total amount of uPOKT the supplier has staked
   repeated SupplierServiceConfig services = 4; // The service configs this supplier can support
   // The session end height at which an actively unbonding supplier unbonds its stake.

proto/poktroll/supplier/tx.proto

@@ -58,8 +58,17 @@ message MsgStakeSupplierResponse {}
 
 message MsgUnstakeSupplier {
   option (cosmos.msg.v1.signer) = "owner_address";
-  string owner_address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // The Bech32 address of the owner (i.e. custodial, staker) using cosmos' ScalarDescriptor to ensure deterministic deterministic encoding using cosmos' ScalarDescriptor to ensure deterministic deterministic encoding
-  string address = 2 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // The Bech32 address of the operator (i.e. provider, non-custodial) using cosmos' ScalarDescriptor to ensure deterministic deterministic encoding using cosmos' ScalarDescriptor to ensure deterministic deterministic encoding
+  // The address of the owner (i.e. staker, custodial) that owns the funds for staking.
+  // By default, this address is the one that receives all the rewards unless owtherwise specified.
+  // The owner can initially stake the supplier and unstake it.
+  // All other configurations are managed by the operator.
+  string owner_address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // The Bech32 address
+  // The operator address of the supplier that operates it.
+  // The operator address can update the supplier's configurations excluding the owner
+  // and operator addresses which do not change over the supplier's lifespan.
+  // TODO(red-0ne): Rename this to `operator_address` include all downstream
+  // variables, comments, docs, tests, etc...
+  string address = 2 [(cosmos_proto.scalar) = "cosmos.AddressString"]; // The Bech32 address
 }
 
 message MsgUnstakeSupplierResponse {}

x/shared/types/errors.go

@@ -8,9 +8,10 @@ import (
 
 // x/shared module sentinel errors
 var (
-	ErrSharedInvalidSigner    = sdkerrors.Register(ModuleName, 1100, "expected gov account as only signer for proposal message")
-	ErrSharedInvalidAddress   = sdkerrors.Register(ModuleName, 1101, "invalid address")
-	ErrSharedParamNameInvalid = sdkerrors.Register(ModuleName, 1102, "the provided param name is invalid")
-	ErrSharedParamInvalid     = sdkerrors.Register(ModuleName, 1103, "the provided param is invalid")
-	ErrSharedEmitEvent        = sdkerrors.Register(ModuleName, 1104, "failed to emit event")
+	ErrSharedInvalidSigner              = sdkerrors.Register(ModuleName, 1100, "expected gov account as only signer for proposal message")
+	ErrSharedInvalidAddress             = sdkerrors.Register(ModuleName, 1101, "invalid address")
+	ErrSharedParamNameInvalid           = sdkerrors.Register(ModuleName, 1102, "the provided param name is invalid")
+	ErrSharedParamInvalid               = sdkerrors.Register(ModuleName, 1103, "the provided param is invalid")
+	ErrSharedEmitEvent                  = sdkerrors.Register(ModuleName, 1104, "failed to emit event")
+	ErrSharedUnauthorizedSupplierUpdate = sdkerrors.Register(ModuleName, 1105, "unauthorized supplier update")
 )

x/shared/types/supplier.go

@@ -21,3 +21,29 @@ func (s *Supplier) IsActive(queryHeight int64) bool {
 	// will also need to be updated to reflect sessions when the supplier is not active.
 	return !s.IsUnbonding() || uint64(queryHeight) <= s.UnstakeSessionEndHeight
 }
+
+// EnsureOwner returns an error if the given address does not match supplier's owner address.
+func (s *Supplier) EnsureOwner(ownerAddress string) error {
+	if s.OwnerAddress != ownerAddress {
+		return ErrSharedUnauthorizedSupplierUpdate.Wrapf(
+			"msg.OwnerAddress %q != provided address %q",
+			s.OwnerAddress,
+			ownerAddress,
+		)
+	}
+
+	return nil
+}
+
+// EnsureOperator returns an error if the given address does not match supplier's operator address.
+func (s *Supplier) EnsureOperator(operatorAddress string) error {
+	if s.Address != operatorAddress {
+		return ErrSharedUnauthorizedSupplierUpdate.Wrapf(
+			"msg.OperatorAddress %q != provided address %q",
+			s.OwnerAddress,
+			operatorAddress,
+		)
+	}
+
+	return nil
+}

x/supplier/config/errors.go

@@ -15,5 +15,5 @@ var (
 	ErrSupplierConfigInvalidURL            = sdkerrors.Register(types.ModuleName, 2105, "invalid endpoint url in supplier config")
 	ErrSupplierConfigEmptyContent          = sdkerrors.Register(types.ModuleName, 2106, "empty supplier config content")
 	ErrSupplierConfigInvalidStake          = sdkerrors.Register(types.ModuleName, 2107, "invalid stake amount in supplier config")
-	ErrSupplierConfigInvalidOwnerAddress   = sdkerrors.Register(types.ModuleName, 2108, "missing owner address in supplier config")
+	ErrSupplierConfigInvalidAddress        = sdkerrors.Register(types.ModuleName, 2108, "missing owner address in supplier config")
 )

x/supplier/config/supplier_configs_reader.go

@@ -9,6 +9,7 @@ import (
 
 	sharedhelpers "github.com/pokt-network/poktroll/x/shared/helpers"
 	sharedtypes "github.com/pokt-network/poktroll/x/shared/types"
+	"github.com/pokt-network/poktroll/x/supplier/types"
 )
 
 // YAMLStakeConfig is the structure describing the supplier stake config file.
@@ -42,6 +43,19 @@ type SupplierStakeConfig struct {
 	Services        []*sharedtypes.SupplierServiceConfig
 }
 
+// EnsureOwner ensures that the config owner address matches the provided address.
+func (cfg *SupplierStakeConfig) EnsureOwner(ownerAddress string) error {
+	if cfg.OwnerAddress != ownerAddress {
+		return types.ErrSupplierInvalidAddress.Wrapf(
+			"owner address %q in the stake config file does not match the address provided %q",
+			cfg.OperatorAddress,
+			ownerAddress,
+		)
+	}
+
+	return nil
+}
+
 // ParseSupplierServiceConfig parses the stake config file into a SupplierServiceConfig.
 func ParseSupplierConfigs(configContent []byte) (*SupplierStakeConfig, error) {
 	var stakeConfig *YAMLStakeConfig
@@ -57,14 +71,19 @@ func ParseSupplierConfigs(configContent []byte) (*SupplierStakeConfig, error) {
 
 	// Validate required owner address.
 	if _, err := sdk.AccAddressFromBech32(stakeConfig.OwnerAddress); err != nil {
-		return nil, ErrSupplierConfigInvalidOwnerAddress.Wrap("invalid owner address")
+		return nil, ErrSupplierConfigInvalidAddress.Wrap("invalid owner address")
 	}
 
 	// If the operator address is not set, default it to the owner address.
 	if stakeConfig.OperatorAddress == "" {
 		stakeConfig.OperatorAddress = stakeConfig.OwnerAddress
 	}
 
+	// Validate operator address.
+	if _, err := sdk.AccAddressFromBech32(stakeConfig.OperatorAddress); err != nil {
+		return nil, ErrSupplierConfigInvalidAddress.Wrap("invalid operator address")
+	}
+
 	// Validate the stake amount
 	if len(stakeConfig.StakeAmount) == 0 {
 		return nil, ErrSupplierConfigInvalidStake.Wrap("stake amount cannot be empty")

x/supplier/keeper/msg_server_stake_supplier.go

@@ -40,12 +40,12 @@ func (k msgServer) StakeSupplier(ctx context.Context, msg *types.MsgStakeSupplie
 	var coinsToEscrow sdk.Coin
 	supplier, isSupplierFound := k.GetSupplier(ctx, msg.Address)
 
-	// Retain the previous operator address to check whether it changes after updating the supplier
-	var previousOperatorAddr string
 	if !isSupplierFound {
 		logger.Info(fmt.Sprintf("Supplier not found. Creating new supplier for address %q", msg.Address))
-		// Ensure that only supplier owner is able to stake.
-		if err := ensureMsgSenderIsSupplierOwner(msg); err != nil {
+		supplier = k.createSupplier(ctx, msg)
+
+		// Ensure that only the owner can stake a new supplier.
+		if err := supplier.EnsureOwner(msg.Sender); err != nil {
 			logger.Error(fmt.Sprintf(
 				"owner address %q in the message does not match the msg sender address %q",
 				msg.OwnerAddress,
@@ -54,24 +54,36 @@ func (k msgServer) StakeSupplier(ctx context.Context, msg *types.MsgStakeSupplie
 
 			return nil, err
 		}
-		supplier = k.createSupplier(ctx, msg)
+
 		coinsToEscrow = *msg.Stake
 	} else {
 		logger.Info(fmt.Sprintf("Supplier found. About to try updating supplier with address %q", msg.Address))
 
-		// Only the owner can update the supplier's owner and operator addresses.
-		if ownerOrOperatorAddressesChanged(msg, &supplier) {
-			if err := ensureMsgSenderIsSupplierOwner(msg); err != nil {
-				logger.Error("only the owner can update the supplier's owner and operator addresses")
+		// Ensure that only the operator can update the supplier info.
+		if err := supplier.EnsureOperator(msg.Sender); err != nil {
+			logger.Error(fmt.Sprintf(
+				"operator address %q in the message does not match the msg sender address %q",
+				msg.Address,
+				msg.Sender,
+			))
+			return nil, err
+		}
+
+		// Ensure that the owner addresses is not changed.
+		if err := supplier.EnsureOwner(msg.OwnerAddress); err != nil {
+			logger.Error("updating the supplier's owner address forbidden")
 
-				return nil, err
-			}
+			return nil, err
+		}
+
+		// Ensure that the operator addresses is not changed.
+		if err := supplier.EnsureOperator(msg.Address); err != nil {
+			logger.Error("updating the supplier's operator address forbidden")
+
+			return nil, err
 		}
 
 		currSupplierStake := *supplier.Stake
-		// Get the previous operator address to update the store by deleting the old
-		// key if it has been changed.
-		previousOperatorAddr = supplier.Address
 		if err = k.updateSupplier(ctx, &supplier, msg); err != nil {
 			logger.Error(fmt.Sprintf("could not update supplier for address %q due to error %v", msg.Address, err))
 			return nil, err
@@ -93,9 +105,9 @@ func (k msgServer) StakeSupplier(ctx context.Context, msg *types.MsgStakeSupplie
 	}
 
 	// Retrieve the account address of the supplier owner
-	supplierOwnerAddress, err := sdk.AccAddressFromBech32(msg.OwnerAddress)
+	supplierOwnerAddress, err := sdk.AccAddressFromBech32(supplier.OwnerAddress)
 	if err != nil {
-		logger.Error(fmt.Sprintf("could not parse address %q", msg.OwnerAddress))
+		logger.Error(fmt.Sprintf("could not parse address %q", supplier.OwnerAddress))
 		return nil, err
 	}
 
@@ -107,10 +119,6 @@ func (k msgServer) StakeSupplier(ctx context.Context, msg *types.MsgStakeSupplie
 	}
 	logger.Info(fmt.Sprintf("Successfully escrowed %v coins from %q to %q module account", coinsToEscrow, supplierOwnerAddress, types.ModuleName))
 
-	// Remove the previous supplier key if the operator address has changed
-	if hasDifferentOperatorAddr(previousOperatorAddr, &supplier) {
-		k.RemoveSupplier(ctx, previousOperatorAddr)
-	}
 	// Update the Supplier in the store
 	k.SetSupplier(ctx, supplier)
 	logger.Info(fmt.Sprintf("Successfully updated supplier stake for supplier: %+v", supplier))
@@ -138,15 +146,6 @@ func (k msgServer) updateSupplier(
 	supplier *sharedtypes.Supplier,
 	msg *types.MsgStakeSupplier,
 ) error {
-	// Check if the the msg owner address is the same as the current owner
-	if msg.OwnerAddress != supplier.OwnerAddress {
-		return types.ErrSupplierUnauthorized.Wrapf("msg OwnerAddress %q != supplier owner address %q", msg.OwnerAddress, supplier.OwnerAddress)
-	}
-
-	// Operator address should be already validated in `msg.ValidateBasic`.
-	// TODO_CONSIDERATION: Delay the operator address change until the next session.
-	supplier.Address = msg.Address
-
 	// Validate that the stake is not being lowered
 	if msg.Stake == nil {
 		return types.ErrSupplierInvalidStake.Wrapf("stake amount cannot be nil")
@@ -166,30 +165,3 @@ func (k msgServer) updateSupplier(
 
 	return nil
 }
-
-// hasDifferentOperatorAddr checks if the new operator address differs from the old one.
-func hasDifferentOperatorAddr(oldOperatorAddress string, supplier *sharedtypes.Supplier) bool {
-	if oldOperatorAddress == "" {
-		return false
-	}
-
-	return oldOperatorAddress != supplier.Address
-}
-
-// ensureMsgSenderIsSupplierOwner returns an error if the message sender is not the supplier owner.
-func ensureMsgSenderIsSupplierOwner(msg *types.MsgStakeSupplier) error {
-	if msg.OwnerAddress == msg.Sender {
-		types.ErrSupplierUnauthorized.Wrapf(
-			"owner address %q in the message does not match the msg sender address %q",
-			msg.OwnerAddress,
-			msg.Sender,
-		)
-	}
-
-	return nil
-}
-
-// ownerOrOperatorAddressesChanged checks if the owner or operator addresses have changed.
-func ownerOrOperatorAddressesChanged(msg *types.MsgStakeSupplier, supplier *sharedtypes.Supplier) bool {
-	return msg.OwnerAddress != supplier.OwnerAddress || msg.Address != supplier.Address
-}

x/supplier/keeper/msg_server_unstake_supplier.go

@@ -38,12 +38,14 @@ func (k msgServer) UnstakeSupplier(
 	}
 
 	// Ensure that the message sender is the supplier owner.
-	if supplier.OwnerAddress != msg.OwnerAddress {
+	if err := supplier.EnsureOwner(msg.OwnerAddress); err != nil {
 		logger.Error(fmt.Sprintf(
 			"owner address %q in the message does not match the supplier's owner address %q",
 			msg.OwnerAddress,
 			supplier.OwnerAddress,
 		))
+
+		return nil, err
 	}
 
 	logger.Info(fmt.Sprintf("Supplier found. Unstaking supplier for address %s", msg.Address))