Skip to content

Commit

Permalink
took into account comments (using protos for marshalling string slice…
Browse files Browse the repository at this point in the history
…, fixed issues in the UpdateConsumer logic, added extra check to abort spurious proposals)
  • Loading branch information
insumity committed Aug 23, 2024
1 parent 8beffdc commit ec5ccec
Show file tree
Hide file tree
Showing 9 changed files with 502 additions and 257 deletions.
4 changes: 4 additions & 0 deletions proto/interchain_security/ccv/provider/v1/provider.proto
Original file line number Diff line number Diff line change
Expand Up @@ -460,3 +460,7 @@ message PowerShapingParameters {
// Corresponds to whether inactive validators are allowed to validate the consumer chain.
bool allow_inactive_vals = 7;
}

// ConsumerIds contains consumer ids of chains
// Used so we can easily (de)serialize slices of strings
message ConsumerIds { repeated string consumer_ids = 1; }
10 changes: 10 additions & 0 deletions tests/integration/provider_gov_hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@ func (s *CCVTestSuite) TestAfterPropSubmissionAndVotingPeriodEnded() {
err = govKeeper.SetProposal(ctx, proposal)
s.Require().NoError(err)

// the proposal can only be submitted if the owner of the chain is the gov module
providerKeeper.SetConsumerOwnerAddress(ctx, msgUpdateConsumer.ConsumerId, "some bogus address")

err = providerKeeper.Hooks().AfterProposalSubmission(ctx, proposal.Id)
s.Require().Error(err)

// the proposal can only be submitted if the owner of the chain is the gov module
govModuleAddress := "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn"
providerKeeper.SetConsumerOwnerAddress(ctx, msgUpdateConsumer.ConsumerId, govModuleAddress)

err = providerKeeper.Hooks().AfterProposalSubmission(ctx, proposal.Id)
s.Require().NoError(err)

Expand Down
13 changes: 13 additions & 0 deletions x/ccv/provider/keeper/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,19 @@ func (h Hooks) AfterProposalSubmission(goCtx context.Context, proposalId uint64)
for _, msg := range p.GetMessages() {
sdkMsg, isMsgUpdateConsumer := msg.GetCachedValue().(*providertypes.MsgUpdateConsumer)
if isMsgUpdateConsumer {
// A `MsgUpdateConsumer` can only succeed if the owner of the consumer chain is the gov module.
// If that's not the case, we immediately fail the proposal.
// Note that someone could potentially change the owner of a chain to be that of the gov module
// while a proposal is active and before the proposal is executed. Even then, we still do not allow
// `MsgUpdateConsumer` proposals if the owner of the chain is not the gov module to avoid someone forgetting
// to change the owner address while the proposal is active.
ownerAddress, err := h.k.GetConsumerOwnerAddress(ctx, sdkMsg.ConsumerId)
if err != nil {
return fmt.Errorf("cannot find owner address for consumer with consumer id (%s): %s", sdkMsg.ConsumerId, err.Error())
} else if ownerAddress != h.k.GetAuthority() {
return fmt.Errorf("owner address (%s) is not the gov module (%s)", ownerAddress, h.k.GetAuthority())
}

if hasUpdateConsumerMsg {
return fmt.Errorf("proposal can contain at most one `MsgUpdateConsumer` message")
}
Expand Down
64 changes: 54 additions & 10 deletions x/ccv/provider/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
"github.com/cosmos/interchain-security/v5/x/ccv/provider/types"
ccvtypes "github.com/cosmos/interchain-security/v5/x/ccv/types"
"strings"
)

type msgServer struct {
Expand Down Expand Up @@ -313,20 +314,27 @@ func (k msgServer) CreateConsumer(goCtx context.Context, msg *types.MsgCreateCon
k.SetConsumerChainId(ctx, consumerId, msg.ChainId)

if err := k.SetConsumerMetadata(ctx, consumerId, msg.Metadata); err != nil {
return &types.MsgCreateConsumerResponse{}, err
return &types.MsgCreateConsumerResponse{}, errorsmod.Wrapf(types.ErrInvalidConsumerMetadata,
"cannot set consumer metadata: %s", err.Error())
}

// initialization parameters are optional and hence could be nil
if msg.InitializationParameters != nil {
if err := k.SetConsumerInitializationParameters(ctx, consumerId, *msg.InitializationParameters); err != nil {
return &types.MsgCreateConsumerResponse{}, err
return &types.MsgCreateConsumerResponse{}, errorsmod.Wrapf(types.ErrInvalidConsumerInitializationParameters,
"cannot set consumer initialization parameters: %s", err.Error())
}
}

// power-shaping parameters are optional and hence could be null
if msg.PowerShapingParameters != nil {
if msg.PowerShapingParameters.Top_N != 0 {
return &types.MsgCreateConsumerResponse{}, errorsmod.Wrap(types.ErrCannotCreateTopNChain,
"cannot create a Top N chain using the `MsgCreateConsumer` message; use `MsgUpdateConsumer` instead")
}
if err := k.SetConsumerPowerShapingParameters(ctx, consumerId, *msg.PowerShapingParameters); err != nil {
return &types.MsgCreateConsumerResponse{}, err
return &types.MsgCreateConsumerResponse{}, errorsmod.Wrapf(types.ErrInvalidPowerShapingParameters,
"cannot set power shaping parameters")
}
}

Expand Down Expand Up @@ -354,27 +362,63 @@ func (k msgServer) UpdateConsumer(goCtx context.Context, msg *types.MsgUpdateCon
return &types.MsgUpdateConsumerResponse{}, errorsmod.Wrapf(types.ErrUnauthorized, "expected owner address %s, got %s", ownerAddress, msg.Signer)
}

// A consumer chain can only become a Top N chain if the owner is the gov module. Because of this, to create a
// Top N chain, we need two `MsgUpdateConsumer` messages: i) one that would set the `ownerAddress` to the gov module
// and ii) one that would set the `Top_N` to something greater than 0.
if msg.PowerShapingParameters != nil && msg.PowerShapingParameters.Top_N > 0 && ownerAddress != k.GetAuthority() {
return &types.MsgUpdateConsumerResponse{}, errorsmod.Wrapf(types.ErrInvalidTransformToTopN,
"an update to a Top N chain can only be done if chain is owner is the gov module")
}

k.Keeper.SetConsumerOwnerAddress(ctx, consumerId, msg.NewOwnerAddress)
if strings.TrimSpace(msg.NewOwnerAddress) != "" {
k.Keeper.SetConsumerOwnerAddress(ctx, consumerId, msg.NewOwnerAddress)
}

if msg.Metadata != nil {
k.Keeper.SetConsumerMetadata(ctx, msg.ConsumerId, *msg.Metadata)
if err := k.SetConsumerMetadata(ctx, consumerId, *msg.Metadata); err != nil {
return &types.MsgUpdateConsumerResponse{}, errorsmod.Wrapf(types.ErrInvalidConsumerMetadata,
"cannot set consumer metadata: %s", err.Error())
}
}

if msg.InitializationParameters != nil {
k.Keeper.SetConsumerInitializationParameters(ctx, msg.ConsumerId, *msg.InitializationParameters)
if err = k.Keeper.SetConsumerInitializationParameters(ctx, msg.ConsumerId, *msg.InitializationParameters); err != nil {
return &types.MsgUpdateConsumerResponse{}, errorsmod.Wrapf(types.ErrInvalidConsumerInitializationParameters,
"cannot set consumer initialization parameters: %s", err.Error())
}
}

if msg.PowerShapingParameters != nil {
oldTopN := k.Keeper.GetTopN(ctx, consumerId)
k.Keeper.SetConsumerPowerShapingParameters(ctx, msg.ConsumerId, *msg.PowerShapingParameters)
k.Keeper.PopulateAllowlist(ctx, consumerId)
k.Keeper.PopulateDenylist(ctx, consumerId)
k.Keeper.PopulateMinimumPowerInTopN(ctx, consumerId, oldTopN)
if err = k.SetConsumerPowerShapingParameters(ctx, consumerId, *msg.PowerShapingParameters); err != nil {
return &types.MsgUpdateConsumerResponse{}, errorsmod.Wrapf(types.ErrInvalidPowerShapingParameters,
"cannot set power shaping parameters")
}

k.Keeper.UpdateAllowlist(ctx, consumerId, msg.PowerShapingParameters.Allowlist)
k.Keeper.UpdateDenylist(ctx, consumerId, msg.PowerShapingParameters.Denylist)
err = k.Keeper.UpdateMinimumPowerInTopN(ctx, consumerId, oldTopN, msg.PowerShapingParameters.Top_N)
if err != nil {
return &types.MsgUpdateConsumerResponse{}, errorsmod.Wrapf(types.ErrCannotUpdateMinimumPowerInTopN,
"could not update minimum power in top N, oldTopN: %d, newTopN: %d, error: %s", oldTopN, msg.PowerShapingParameters.Top_N, err.Error())
}
}

// Last check: a Top N cannot change its owner address to something different from the gov module if the chain
// remains a Top N chain.
currentOwnerAddress, err := k.GetConsumerOwnerAddress(ctx, consumerId)
if err != nil {
return &types.MsgUpdateConsumerResponse{}, errorsmod.Wrapf(types.ErrNoOwnerAddress, "cannot retrieve owner address %s: %s", ownerAddress, err.Error())
}

currentPowerShapingParameters, err := k.GetConsumerPowerShapingParameters(ctx, consumerId)
if err != nil {
return &types.MsgUpdateConsumerResponse{}, errorsmod.Wrapf(types.ErrInvalidPowerShapingParameters, "cannot retrieve power shaping parameters: %s", err.Error())
}

if currentPowerShapingParameters.Top_N != 0 && currentOwnerAddress != k.GetAuthority() {
return &types.MsgUpdateConsumerResponse{}, errorsmod.Wrapf(types.ErrInvalidTransformToOptIn,
"a move to a new owner address that is not the gov module can only be done if `Top N` is set to 0")
}

if k.CanLaunch(ctx, consumerId) {
Expand Down
Loading

0 comments on commit ec5ccec

Please sign in to comment.