Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: introduce MsgOptIn and MsgOptOut #1620

Merged
merged 8 commits into from
Feb 7, 2024
Merged

feat!: introduce MsgOptIn and MsgOptOut #1620

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4eac8bf
init commit
insumity Feb 5, 2024
8f2269f
cleaning up
insumity Feb 5, 2024
a32e882
changed cons to val address
insumity Feb 5, 2024
b0d2337
Revert "changed cons to val address"
insumity Feb 5, 2024
32fed21
Update x/ccv/provider/keeper/keeper.go
insumity Feb 6, 2024
45bf863
took into account comments
insumity Feb 6, 2024
78e12c8
added key assignment
insumity Feb 7, 2024
656adc2
add contraint such that opt out only works if the chain is running
insumity Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions x/ccv/provider/keeper/key_assignment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper

import (
"encoding/base64"
"fmt"

errorsmod "cosmossdk.io/errors"
Expand All @@ -15,6 +16,53 @@ import (
ccvtypes "github.com/cosmos/interchain-security/v4/x/ccv/types"
)

// ParseConsumerKey parses the ED25519 PubKey`consumerKey` from a JSON string
// and constructs its corresponding `tmprotocrypto.PublicKey`
func (k Keeper) ParseConsumerKey(consumerKey string) (tmprotocrypto.PublicKey, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took this method code as is from msg_server.go (see below). I chose not to reuse the AssignConsumerKey method in partial_set_security.go when opting in a validator with a consumer key because this would entail:

  • creating a MsgAssign message inside HandleOptIn which seems counterintuitive in the sense that a message creates another message internally to do something;
  • AssignConsumerKey would emit emit an assignment event and potentially this could confuse relayers becuse for one MsgOptIn message, relayers get events as if another message was executed as well.

// parse consumer key as long as it's in the right format
pkType, keyStr, err := types.ParseConsumerKeyFromJson(consumerKey)
if err != nil {
return tmprotocrypto.PublicKey{}, err
}

// Note: the correct way to decide if a key type is supported is to check the
// consensus params. However this functionality was disabled in https://github.com/cosmos/interchain-security/pull/916
// as a quick way to get ed25519 working, avoiding amino/proto-any marshalling issues.

// make sure the consumer key type is supported
// cp := ctx.ConsensusParams()
// if cp != nil && cp.Validator != nil {
// if !tmstrings.StringInSlice(pkType, cp.Validator.PubKeyTypes) {
// return nil, errorsmod.Wrapf(
// stakingtypes.ErrValidatorPubKeyTypeNotSupported,
// "got: %s, expected one of: %s", pkType, cp.Validator.PubKeyTypes,
// )
// }
// }

// For now, only accept ed25519.
// TODO: decide what types should be supported.
if pkType != "/cosmos.crypto.ed25519.PubKey" {
return tmprotocrypto.PublicKey{}, errorsmod.Wrapf(
stakingtypes.ErrValidatorPubKeyTypeNotSupported,
"got: %s, expected: %s", pkType, "/cosmos.crypto.ed25519.PubKey",
)
}

pubKeyBytes, err := base64.StdEncoding.DecodeString(keyStr)
if err != nil {
return tmprotocrypto.PublicKey{}, err
}

consumerTMPublicKey := tmprotocrypto.PublicKey{
Sum: &tmprotocrypto.PublicKey_Ed25519{
Ed25519: pubKeyBytes,
},
}

return consumerTMPublicKey, nil
}

// GetValidatorConsumerPubKey returns a validator's public key assigned for a consumer chain
func (k Keeper) GetValidatorConsumerPubKey(
ctx sdk.Context,
Expand Down
41 changes: 1 addition & 40 deletions x/ccv/provider/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,12 @@ package keeper

import (
"context"
"encoding/base64"

errorsmod "cosmossdk.io/errors"

cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"

tmprotocrypto "github.com/cometbft/cometbft/proto/tendermint/crypto"
tmtypes "github.com/cometbft/cometbft/types"

"github.com/cosmos/interchain-security/v4/x/ccv/provider/types"
Expand Down Expand Up @@ -45,47 +42,11 @@ func (k msgServer) AssignConsumerKey(goCtx context.Context, msg *types.MsgAssign
return nil, stakingtypes.ErrNoValidatorFound
}

// parse consumer key as long as it's in the right format
pkType, keyStr, err := types.ParseConsumerKeyFromJson(msg.ConsumerKey)
if err != nil {
return nil, err
}

// Note: the correct way to decide if a key type is supported is to check the
// consensus params. However this functionality was disabled in https://github.com/cosmos/interchain-security/pull/916
// as a quick way to get ed25519 working, avoiding amino/proto-any marshalling issues.

// make sure the consumer key type is supported
// cp := ctx.ConsensusParams()
// if cp != nil && cp.Validator != nil {
// if !tmstrings.StringInSlice(pkType, cp.Validator.PubKeyTypes) {
// return nil, errorsmod.Wrapf(
// stakingtypes.ErrValidatorPubKeyTypeNotSupported,
// "got: %s, expected one of: %s", pkType, cp.Validator.PubKeyTypes,
// )
// }
// }

// For now, only accept ed25519.
// TODO: decide what types should be supported.
if pkType != "/cosmos.crypto.ed25519.PubKey" {
return nil, errorsmod.Wrapf(
stakingtypes.ErrValidatorPubKeyTypeNotSupported,
"got: %s, expected: %s", pkType, "/cosmos.crypto.ed25519.PubKey",
)
}

pubKeyBytes, err := base64.StdEncoding.DecodeString(keyStr)
consumerTMPublicKey, err := k.ParseConsumerKey(msg.ConsumerKey)
if err != nil {
return nil, err
}

consumerTMPublicKey := tmprotocrypto.PublicKey{
Sum: &tmprotocrypto.PublicKey_Ed25519{
Ed25519: pubKeyBytes,
},
}

if err := k.Keeper.AssignConsumerKey(ctx, msg.ChainId, validator, consumerTMPublicKey); err != nil {
return nil, err
}
Expand Down
16 changes: 15 additions & 1 deletion x/ccv/provider/keeper/partial_set_security.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package keeper
import (
errorsmod "cosmossdk.io/errors"
sdk "github.com/cosmos/cosmos-sdk/types"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
"github.com/cosmos/interchain-security/v4/x/ccv/provider/types"
)

Expand All @@ -28,7 +29,20 @@ func (k Keeper) HandleOptIn(ctx sdk.Context, chainID string, providerAddr types.
}

if consumerKey != nil {
// TODO (PR 1586): assign consumer key in this case
consumerTMPublicKey, err := k.ParseConsumerKey(*consumerKey)
if err != nil {
return err
}

validator, found := k.stakingKeeper.GetValidatorByConsAddr(ctx, providerAddr.Address)
if !found {
return stakingtypes.ErrNoValidatorFound
}

err = k.AssignConsumerKey(ctx, chainID, validator, consumerTMPublicKey)
if err != nil {
return err
}
}

return nil
Expand Down
53 changes: 53 additions & 0 deletions x/ccv/provider/keeper/partial_set_security_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
package keeper_test

import (
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
sdk "github.com/cosmos/cosmos-sdk/types"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
testkeeper "github.com/cosmos/interchain-security/v4/testutil/keeper"
"github.com/cosmos/interchain-security/v4/x/ccv/provider/types"
ccvtypes "github.com/cosmos/interchain-security/v4/x/ccv/types"
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/require"
"testing"
)
Expand Down Expand Up @@ -31,6 +37,53 @@ func TestHandleOptIn(t *testing.T) {
require.False(t, providerKeeper.IsToBeOptedIn(ctx, "chainID", providerAddr))
}

func TestHandleOptInWithConsumerKey(t *testing.T) {
providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
defer ctrl.Finish()

// generate a consensus public key for the provider
providerConsPubKey := ed25519.GenPrivKeyFromSecret([]byte{1}).PubKey()
consAddr := sdk.ConsAddress(providerConsPubKey.Address())
providerAddr := types.NewProviderConsAddress(consAddr)

calls := []*gomock.Call{
mocks.MockStakingKeeper.EXPECT().
GetValidatorByConsAddr(gomock.Any(), gomock.Any()).
DoAndReturn(func(ctx sdk.Context, addr sdk.ConsAddress) (stakingtypes.Validator, bool) {
if addr.Equals(providerAddr.Address) {
// Given `providerAddr`, `GetValidatorByConsAddr` returns a validator with the
// exact same `ConsensusPubkey`
pkAny, _ := codectypes.NewAnyWithValue(providerConsPubKey)
return stakingtypes.Validator{ConsensusPubkey: pkAny}, true
} else {
// for any other consensus address, we cannot find a validator
return stakingtypes.Validator{}, false
}
}).Times(2),
}

gomock.InOrder(calls...)
providerKeeper.SetProposedConsumerChain(ctx, "chainID", 1)

// create a sample consumer key to assign to the `providerAddr` validator
// on the consumer chain with id `chainID`
consumerKey := "{\"@type\":\"/cosmos.crypto.ed25519.PubKey\",\"key\":\"Ui5Gf1+mtWUdH8u3xlmzdKID+F3PK0sfXZ73GZ6q6is=\"}"
expectedConsumerPubKey, _ := providerKeeper.ParseConsumerKey(consumerKey)

err := providerKeeper.HandleOptIn(ctx, "chainID", providerAddr, &consumerKey)
require.NoError(t, err)

// assert that the consumeKey was assigned to `providerAddr` validator on chain with id `chainID`
actualConsumerPubKey, found := providerKeeper.GetValidatorConsumerPubKey(ctx, "chainID", providerAddr)
require.True(t, found)
require.Equal(t, expectedConsumerPubKey, actualConsumerPubKey)

// assert that the `consumerAddr` to `providerAddr` association was set as well
consumerAddr, _ := ccvtypes.TMCryptoPublicKeyToConsAddr(actualConsumerPubKey)
actualProviderConsAddr, found := providerKeeper.GetValidatorByConsumerAddr(ctx, "chainID", types.NewConsumerConsAddress(consumerAddr))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should definitely test that it is possible to assign a key the "normal way" after doing it like this. not sure if here is the right place to do it, or if it is easier with higher-level tests, e.g. e2e

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conceptually, if we exclude opting in, a MsgOptIn (with a consumer key) followed by a MsgAssign message is the same as two MsgAssigns the one after the other. If we can call twice or more MsgAssign today, I don't see why MsgOptIn would cause issues with this.
In any case, we could probably test this in an e2e test.

require.Equal(t, providerAddr, actualProviderConsAddr)
}

func TestHandleOptOut(t *testing.T) {
providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
defer ctrl.Finish()
Expand Down
Loading