Key assignment type safety (#725)

* pb changes

* nvm dont wanna open that can of worms

* still wip

* more fixes

* almost

* builds

* helpers and fixed one file

* comments

* mas

* test fix

* fix another

* types

* smol

* un mas

* un mas

* nit

* reformat

* mas

* fix last bug

* to fix integration test

* proper way to do stringer

* Update slashing.go

* Update slashing.go

* links

* comments

* Update keeper.go

* smol

* nit

* changes to TestHandleEquivocationProposal

* merge with fixes

* merge fix

* comment

---------

Co-authored-by: MSalopek <[email protected]>

proto/interchain_security/ccv/provider/v1/genesis.proto

@@ -76,27 +76,3 @@ message ValsetUpdateIdToHeight {
     uint64 valset_update_id = 1;
     uint64 height = 2;
 }
-
-// Used to serialize the ValidatorConsumerPubKey index from key assignment
-// ValidatorConsumerPubKey: (chainID, providerAddr consAddr) -> consumerKey tmprotocrypto.PublicKey
-message ValidatorConsumerPubKey {
-    string chain_id = 1;
-    bytes provider_addr = 2;
-    tendermint.crypto.PublicKey consumer_key = 3;
-}
-
-// Used to serialize the ValidatorConsumerAddr index from key assignment
-// ValidatorByConsumerAddr: (chainID, consumerAddr consAddr) -> providerAddr consAddr
-message ValidatorByConsumerAddr {
-    string chain_id = 1;
-    bytes consumer_addr = 2;
-    bytes provider_addr = 3;
-}
-
-// Used to serialize the ConsumerAddrsToPrune index from key assignment
-// ConsumerAddrsToPrune: (chainID, vscID uint64) -> consumerAddrs AddressList
-message ConsumerAddrsToPrune {
-    string chain_id = 1;
-    uint64 vsc_id = 2;
-    AddressList consumer_addrs = 3;
-}

proto/interchain_security/ccv/provider/v1/provider.proto

@@ -104,7 +104,7 @@ message GlobalSlashEntry {
   // This field is used to obtain validator power in HandleThrottleQueues.
   // 
   // This field is not used in the store key, but is persisted in value bytes, see QueueGlobalSlashEntry.
-  bytes provider_val_cons_addr = 4;
+  ProviderConsAddress provider_val_cons_addr = 4;
 }
 
 // Params defines the parameters for CCV Provider module
@@ -143,7 +143,7 @@ message HandshakeMetadata {
   string version = 2;
 }
 
-// SlashAcks contains addesses of consumer chain validators
+// SlashAcks contains cons addresses of consumer chain validators
 // successfully slashed on the provider chain
 message SlashAcks {
   repeated string addresses = 1;
@@ -161,11 +161,6 @@ message ConsumerRemovalProposals {
   repeated ConsumerRemovalProposal pending = 1;
 }
 
-// AddressList contains a list of consensus addresses
-message AddressList {
-  repeated bytes addresses = 1;
-}
-
 message ChannelToChain {
   string channel_id = 1;
   string chain_id = 2;
@@ -196,9 +191,57 @@ message VscSendTimestamp {
   google.protobuf.Timestamp timestamp = 2 [(gogoproto.stdtime) = true, (gogoproto.nullable) = false];
 }
 
+//
+// Key assignment section
+//
+
+// A validator's assigned consensus address for a consumer chain. 
+// Note this type is for type safety within provider code, consumer code uses normal sdk.ConsAddress, 
+// since there's no notion of provider vs consumer address.
+message ConsumerConsAddress {
+  // Do not generate stringer for this type, we'll use sdk.ConsAddress.String() instead
+  option (gogoproto.goproto_stringer) = false;
+  bytes address = 1;
+}
+
+// A validator's consensus address on the provider chain
+message ProviderConsAddress {
+  // Do not generate stringer for this type, we'll use sdk.ConsAddress.String() instead
+  option (gogoproto.goproto_stringer) = false;
+  bytes address = 1;
+}
+
+// ConsumerAddressList contains a list of consumer consensus addresses
+message ConsumerAddressList {
+  repeated ConsumerConsAddress addresses = 1;
+}
+
 message KeyAssignmentReplacement {
-  bytes provider_addr = 1;
+  ProviderConsAddress provider_addr = 1;
   tendermint.crypto.PublicKey prev_c_key = 2;
   int64 power = 3;
 }
 
+// Used to serialize the ValidatorConsumerPubKey index from key assignment
+// ValidatorConsumerPubKey: (chainID, providerAddr consAddr) -> consumerKey tmprotocrypto.PublicKey
+message ValidatorConsumerPubKey {
+  string chain_id = 1;
+  ProviderConsAddress provider_addr = 2;
+  tendermint.crypto.PublicKey consumer_key = 3;
+}
+
+// Used to serialize the ValidatorConsumerAddr index from key assignment
+// ValidatorByConsumerAddr: (chainID, consumerAddr consAddr) -> providerAddr consAddr
+message ValidatorByConsumerAddr {
+  string chain_id = 1;
+  ConsumerConsAddress consumer_addr = 2;
+  ProviderConsAddress provider_addr = 3;
+}
+
+// Used to serialize the ConsumerAddrsToPrune index from key assignment
+// ConsumerAddrsToPrune: (chainID, vscID uint64) -> consumerAddrs AddressList
+message ConsumerAddrsToPrune {
+  string chain_id = 1;
+  uint64 vsc_id = 2;
+  ConsumerAddressList consumer_addrs = 3;
+}

tests/e2e/slashing.go

@@ -14,13 +14,14 @@ import (
 	clienttypes "github.com/cosmos/ibc-go/v4/modules/core/02-client/types"
 	channeltypes "github.com/cosmos/ibc-go/v4/modules/core/04-channel/types"
 	keepertestutil "github.com/cosmos/interchain-security/testutil/keeper"
+	providertypes "github.com/cosmos/interchain-security/x/ccv/provider/types"
 	tmtypes "github.com/tendermint/tendermint/types"
 
 	abci "github.com/tendermint/tendermint/abci/types"
 	"github.com/tendermint/tendermint/crypto/ed25519"
 )
 
-// TestRelayAndApplySlashPacket tests that downtime slash packets can be properly relayed
+// TestRelayAndApplyDowntimePacket tests that downtime slash packets can be properly relayed
 // from consumer to provider, handled by provider, with a VSC and jailing
 // eventually effective on consumer and provider.
 //
@@ -45,7 +46,7 @@ func (s *CCVTestSuite) TestRelayAndApplyDowntimePacket() {
 	s.Require().NoError(err)
 	pubkey, err := cryptocodec.FromTmProtoPublicKey(val.GetPubKey())
 	s.Require().Nil(err)
-	consumerConsAddr := sdk.GetConsAddress(pubkey)
+	consumerConsAddr := providertypes.NewConsumerConsAddress(sdk.GetConsAddress(pubkey))
 	// map consumer consensus address to provider consensus address
 	providerConsAddr, found := providerKeeper.GetValidatorByConsumerAddr(
 		s.providerCtx(),
@@ -54,7 +55,7 @@ func (s *CCVTestSuite) TestRelayAndApplyDowntimePacket() {
 	)
 	s.Require().True(found)
 
-	stakingVal, found := providerStakingKeeper.GetValidatorByConsAddr(s.providerCtx(), providerConsAddr)
+	stakingVal, found := providerStakingKeeper.GetValidatorByConsAddr(s.providerCtx(), providerConsAddr.ToSdkConsAddr())
 	s.Require().True(found)
 	valOldBalance := stakingVal.Tokens
 
@@ -72,7 +73,7 @@ func (s *CCVTestSuite) TestRelayAndApplyDowntimePacket() {
 	s.Require().NoError(err)
 
 	// Set outstanding slashing flag for first consumer, it's important to use the consumer's cons addr here
-	firstConsumerKeeper.SetOutstandingDowntime(s.consumerCtx(), consumerConsAddr)
+	firstConsumerKeeper.SetOutstandingDowntime(s.consumerCtx(), consumerConsAddr.ToSdkConsAddr())
 
 	// Note: RecvPacket advances two blocks. Let's say the provider is currently at height N.
 	// The received slash packet will be queued during N, and handled by the ccv module during
@@ -126,15 +127,15 @@ func (s *CCVTestSuite) TestRelayAndApplyDowntimePacket() {
 	}
 
 	// Get staking keeper's validator obj after the relayed slash packet
-	stakingValAfter, ok := providerStakingKeeper.GetValidatorByConsAddr(s.providerCtx(), providerConsAddr)
+	stakingValAfter, ok := providerStakingKeeper.GetValidatorByConsAddr(s.providerCtx(), providerConsAddr.ToSdkConsAddr())
 	s.Require().True(ok)
 
 	// check that the validator's tokens were NOT slashed on provider
 	valNewBalance := stakingValAfter.GetTokens()
 	s.Require().Equal(valOldBalance, valNewBalance)
 
 	// Get signing info for the validator
-	valSignInfo, found := providerSlashingKeeper.GetValidatorSigningInfo(s.providerCtx(), providerConsAddr)
+	valSignInfo, found := providerSlashingKeeper.GetValidatorSigningInfo(s.providerCtx(), providerConsAddr.ToSdkConsAddr())
 	s.Require().True(found)
 
 	// check that the validator is successfully jailed on provider
@@ -147,7 +148,7 @@ func (s *CCVTestSuite) TestRelayAndApplyDowntimePacket() {
 	// check that the outstanding slashing flag is reset on first consumer,
 	// since that consumer originally sent the slash packet.
 	// It's important to use the consumer's cons addr here.
-	pFlag := firstConsumerKeeper.OutstandingDowntime(s.consumerCtx(), consumerConsAddr)
+	pFlag := firstConsumerKeeper.OutstandingDowntime(s.consumerCtx(), consumerConsAddr.ToSdkConsAddr())
 	s.Require().False(pFlag)
 
 	// check that slashing packet gets acknowledged successfully
@@ -175,15 +176,15 @@ func (s *CCVTestSuite) TestRelayAndApplyDoubleSignPacket() {
 	s.Require().NoError(err)
 	pubkey, err := cryptocodec.FromTmProtoPublicKey(val.GetPubKey())
 	s.Require().Nil(err)
-	consumerConsAddr := sdk.GetConsAddress(pubkey)
+	consumerConsAddr := providertypes.NewConsumerConsAddress(sdk.GetConsAddress(pubkey))
 	// map consumer consensus address to provider consensus address
 	providerConsAddr, found := providerKeeper.GetValidatorByConsumerAddr(
 		s.providerCtx(),
 		s.consumerChain.ChainID,
 		consumerConsAddr)
 	s.Require().True(found)
 
-	stakingVal, found := providerStakingKeeper.GetValidatorByConsAddr(s.providerCtx(), providerConsAddr)
+	stakingVal, found := providerStakingKeeper.GetValidatorByConsAddr(s.providerCtx(), providerConsAddr.ToSdkConsAddr())
 	s.Require().True(found)
 	valOldBalance := stakingVal.Tokens
 
@@ -213,15 +214,15 @@ func (s *CCVTestSuite) TestRelayAndApplyDoubleSignPacket() {
 	s.Require().Len(s.providerChain.Vals.Validators, validatorsPerChain)
 
 	// Get staking keeper's validator obj after the relayed slash packet
-	stakingValAfter, ok := providerStakingKeeper.GetValidatorByConsAddr(s.providerCtx(), providerConsAddr)
+	stakingValAfter, ok := providerStakingKeeper.GetValidatorByConsAddr(s.providerCtx(), providerConsAddr.ToSdkConsAddr())
 	s.Require().True(ok)
 
 	// check that the validator's tokens were NOT slashed on provider
 	valNewBalance := stakingValAfter.GetTokens()
 	s.Require().Equal(valOldBalance, valNewBalance)
 
 	// Get signing info for the validator
-	valSignInfo, found := providerSlashingKeeper.GetValidatorSigningInfo(s.providerCtx(), providerConsAddr)
+	valSignInfo, found := providerSlashingKeeper.GetValidatorSigningInfo(s.providerCtx(), providerConsAddr.ToSdkConsAddr())
 	s.Require().True(found)
 
 	// check that the validator's unjailing time is NOT updated on provider

tests/e2e/stop_consumer.go

@@ -85,7 +85,7 @@ func (s *CCVTestSuite) TestStopConsumerChain() {
 
 				// Queue slash and vsc packet data for consumer 0, these queue entries will be removed
 				firstBundle := s.getFirstBundle()
-				globalEntry := types.NewGlobalSlashEntry(s.providerCtx().BlockTime(), firstBundle.Chain.ChainID, 7, []byte{})
+				globalEntry := types.NewGlobalSlashEntry(s.providerCtx().BlockTime(), firstBundle.Chain.ChainID, 7, types.ProviderConsAddress{})
 				providerKeeper.QueueGlobalSlashEntry(s.providerCtx(), globalEntry)
 				err := providerKeeper.QueueThrottledSlashPacketData(s.providerCtx(), firstBundle.Chain.ChainID, 1,
 					ccv.SlashPacketData{ValsetUpdateId: 1})
@@ -96,7 +96,7 @@ func (s *CCVTestSuite) TestStopConsumerChain() {
 
 				// Queue slash and vsc packet data for consumer 1, these queue entries will be not be removed
 				secondBundle := s.getBundleByIdx(1)
-				globalEntry = types.NewGlobalSlashEntry(s.providerCtx().BlockTime(), secondBundle.Chain.ChainID, 7, []byte{})
+				globalEntry = types.NewGlobalSlashEntry(s.providerCtx().BlockTime(), secondBundle.Chain.ChainID, 7, types.ProviderConsAddress{})
 				providerKeeper.QueueGlobalSlashEntry(s.providerCtx(), globalEntry)
 				err = providerKeeper.QueueThrottledSlashPacketData(s.providerCtx(), secondBundle.Chain.ChainID, 1,
 					ccv.SlashPacketData{ValsetUpdateId: 1})

tests/e2e/throttle.go

@@ -407,9 +407,11 @@ func (s *CCVTestSuite) TestDoubleSignDoesNotAffectThrottling() {
 
 		// 4th validator should have no slash log, all the others do
 		if val != s.providerChain.Vals.Validators[3] {
-			s.Require().True(providerKeeper.GetSlashLog(s.providerCtx(), sdk.ConsAddress(val.Address)))
+			s.Require().True(providerKeeper.GetSlashLog(s.providerCtx(),
+				providertypes.NewProviderConsAddress([]byte(val.Address))))
 		} else {
-			s.Require().False(providerKeeper.GetSlashLog(s.providerCtx(), sdk.ConsAddress(val.Address)))
+			s.Require().False(providerKeeper.GetSlashLog(s.providerCtx(),
+				providertypes.NewProviderConsAddress([]byte(val.Address))))
 		}
 	}
 }

testutil/crypto/crypto.go

@@ -11,6 +11,7 @@ import (
 	sdkcryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
 	sdktypes "github.com/cosmos/cosmos-sdk/types"
 	sdkstakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
+	providertypes "github.com/cosmos/interchain-security/x/ccv/provider/types"
 
 	tmcrypto "github.com/tendermint/tendermint/crypto"
 	tmprotocrypto "github.com/tendermint/tendermint/proto/tendermint/crypto"
@@ -98,3 +99,17 @@ func (v *CryptoIdentity) SDKValOpAddress() sdktypes.ValAddress {
 func (v *CryptoIdentity) SDKValConsAddress() sdktypes.ConsAddress {
 	return sdktypes.ConsAddress(v.ConsensusSDKPubKey().Address())
 }
+
+// Returns the cons address of the crypto identity as a ProviderConsAddress.
+// In most cases, one crypto identity should NOT be casted to both a ProviderConsAddress and ConsumerConsAddress.
+// However, test intention is left to the caller.
+func (v *CryptoIdentity) ProviderConsAddress() providertypes.ProviderConsAddress {
+	return providertypes.NewProviderConsAddress(v.SDKValConsAddress())
+}
+
+// Returns the cons address of the crypto identity as a ConsumerConsAddress.
+// In most cases, one crypto identity should NOT be casted to both a ProviderConsAddress and ConsumerConsAddress.
+// However, test intention is left to the caller.
+func (v *CryptoIdentity) ConsumerConsAddress() providertypes.ConsumerConsAddress {
+	return providertypes.NewConsumerConsAddress(v.SDKValConsAddress())
+}

testutil/keeper/expectations.go

@@ -10,6 +10,7 @@ import (
 	conntypes "github.com/cosmos/ibc-go/v4/modules/core/03-connection/types"
 	channeltypes "github.com/cosmos/ibc-go/v4/modules/core/04-channel/types"
 	ibctmtypes "github.com/cosmos/ibc-go/v4/modules/light-clients/07-tendermint/types"
+	providertypes "github.com/cosmos/interchain-security/x/ccv/provider/types"
 	"github.com/golang/mock/gomock"
 
 	host "github.com/cosmos/ibc-go/v4/modules/core/24-host"
@@ -92,31 +93,31 @@ func GetMocksForStopConsumerChain(ctx sdk.Context, mocks *MockedKeepers) []*gomo
 }
 
 func GetMocksForHandleSlashPacket(ctx sdk.Context, mocks MockedKeepers,
-	expectedProviderValConsAddr sdk.ConsAddress,
+	expectedProviderValConsAddr providertypes.ProviderConsAddress,
 	valToReturn stakingtypes.Validator, expectJailing bool) []*gomock.Call {
 
 	// These first two calls are always made.
 	calls := []*gomock.Call{
 
 		mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(
-			ctx, expectedProviderValConsAddr).Return(
+			ctx, expectedProviderValConsAddr.ToSdkConsAddr()).Return(
 			valToReturn, true,
 		).Times(1),
 
 		mocks.MockSlashingKeeper.EXPECT().IsTombstoned(ctx,
-			sdk.ConsAddress(expectedProviderValConsAddr)).Return(false).Times(1),
+			expectedProviderValConsAddr.ToSdkConsAddr()).Return(false).Times(1),
 	}
 
 	if expectJailing {
 		calls = append(calls, mocks.MockStakingKeeper.EXPECT().Jail(
 			gomock.Eq(ctx),
-			gomock.Eq(sdk.ConsAddress(expectedProviderValConsAddr)),
+			gomock.Eq(expectedProviderValConsAddr.ToSdkConsAddr()),
 		).Return())
 
 		// JailUntil is set in this code path.
 		calls = append(calls, mocks.MockSlashingKeeper.EXPECT().DowntimeJailDuration(ctx).Return(time.Hour).Times(1))
 		calls = append(calls, mocks.MockSlashingKeeper.EXPECT().JailUntil(ctx,
-			sdk.ConsAddress(expectedProviderValConsAddr), gomock.Any()).Times(1))
+			expectedProviderValConsAddr.ToSdkConsAddr(), gomock.Any()).Times(1))
 	}
 
 	return calls

x/ccv/consumer/keeper/keeper.go

@@ -381,7 +381,7 @@ func (k Keeper) SetOutstandingDowntime(ctx sdk.Context, address sdk.ConsAddress)
 func (k Keeper) DeleteOutstandingDowntime(ctx sdk.Context, consAddress string) {
 	consAddr, err := sdk.ConsAddressFromBech32(consAddress)
 	if err != nil {
-		return
+		return // TODO: this should panic with appropriate tests to validate the panic wont happen in normal cases.
 	}
 	store := ctx.KVStore(k.storeKey)
 	store.Delete(types.OutstandingDowntimeKey(consAddr))