chore: x/circuit audit changes (backport #16901)

proto/cosmos/circuit/v1/query.proto

@@ -8,7 +8,7 @@ import "cosmos/circuit/v1/types.proto";
 import "google/api/annotations.proto";
 import "cosmos/query/v1/query.proto";
 
-// Query defines the circuit Msg service.
+// Query defines the circuit gRPC querier service.
 service Query {
   // Account returns account permissions.
   rpc Account(QueryAccountRequest) returns (AccountResponse) {

proto/cosmos/circuit/v1/tx.proto

@@ -39,7 +39,7 @@ message MsgAuthorizeCircuitBreaker {
   Permissions permissions = 3;
 }
 
-// MsgAuthorizeCircuitBreaker defines the Msg/AuthorizeCircuitBreaker response type.
+// MsgAuthorizeCircuitBreakerResponse defines the Msg/AuthorizeCircuitBreaker response type.
 message MsgAuthorizeCircuitBreakerResponse {
   bool success = 1;
 }
@@ -59,7 +59,7 @@ message MsgTripCircuitBreaker {
   repeated string msg_type_urls = 2;
 }
 
-// MsgTripCircuitBreaker defines the Msg/TripCircuitBreaker response type.
+// MsgTripCircuitBreakerResponse defines the Msg/TripCircuitBreaker response type.
 message MsgTripCircuitBreakerResponse {
   bool success = 1;
 }

x/circuit/README.md

@@ -59,7 +59,7 @@ Authorize, is called by the module authority (default governance module account)
 
 ### Trip
 
-Trip, is called by an account to disable message execution for a specific msgURL. 
+Trip, is called by an authorized account to disable message execution for a specific msgURL. If empty, all the msgs will be disabled.
 
 ```protobuf
   // TripCircuitBreaker pauses processing of Msg's in the state machine.
@@ -68,7 +68,7 @@ Trip, is called by an account to disable message execution for a specific msgURL
 
 ### Reset
 
-Reset is called to enable execution of a previously disabled message. 
+Reset is called by an authorized account to enable execution for a specific msgURL of previously disabled message. If empty, all the disabled messages will be enabled.
 
 ```protobuf
   // ResetCircuitBreaker resumes processing of Msg's in the state machine that
@@ -118,8 +118,8 @@ The circuit module emits the following events:
 
 | Type    | Attribute Key | Attribute Value           |
 |---------|---------------|---------------------------|
-| string  | granter       | {granteeAddress}          |
-| string  | grantee       | {granterAddress}          |
+| string  | granter       | {granterAddress}          |
+| string  | grantee       | {granteeAddress}          |
 | string  | permission    | {granteePermissions}      |
 | message | module        | circuit                   |
 | message | action        | authorize_circuit_breaker |

x/circuit/client/cli/tx.go

@@ -44,7 +44,7 @@ func AuthorizeCircuitBreakerCmd() *cobra.Command {
 		"ALL_MSGS" =      2,
 		"SUPER_ADMIN" =   3,`,
 		Example: fmt.Sprintf(`%s circuit authorize [address] 0 "cosmos.bank.v1beta1.MsgSend,cosmos.bank.v1beta1.MsgMultiSend"`, version.AppName),
-		Args:    cobra.RangeArgs(3, 4),
+		Args:    cobra.RangeArgs(2, 3),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			clientCtx, err := client.GetClientTxContext(cmd)
 			if err != nil {
@@ -62,7 +62,7 @@ func AuthorizeCircuitBreakerCmd() *cobra.Command {
 			}
 
 			var typeUrls []string
-			if len(args) == 4 {
+			if len(args) == 3 {
 				typeUrls = strings.Split(args[2], ",")
 			}
 

x/circuit/keeper/genesis_test.go

@@ -0,0 +1,88 @@
+package keeper_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/suite"
+
+	storetypes "cosmossdk.io/store/types"
+	"cosmossdk.io/x/circuit"
+	"cosmossdk.io/x/circuit/keeper"
+	"cosmossdk.io/x/circuit/types"
+
+	"github.com/cosmos/cosmos-sdk/codec"
+	addresscodec "github.com/cosmos/cosmos-sdk/codec/address"
+	"github.com/cosmos/cosmos-sdk/runtime"
+	"github.com/cosmos/cosmos-sdk/testutil"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil"
+	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
+)
+
+type GenesisTestSuite struct {
+	suite.Suite
+
+	ctx       context.Context
+	keeper    keeper.Keeper
+	cdc       *codec.ProtoCodec
+	addrBytes []byte
+}
+
+func TestGenesisTestSuite(t *testing.T) {
+	suite.Run(t, new(GenesisTestSuite))
+}
+
+func (s *GenesisTestSuite) SetupTest() {
+	key := storetypes.NewKVStoreKey(types.StoreKey)
+	testCtx := testutil.DefaultContextWithDB(s.T(), key, storetypes.NewTransientStoreKey("transient_test"))
+	encCfg := moduletestutil.MakeTestEncodingConfig(circuit.AppModuleBasic{})
+
+	sdkCtx := testCtx.Ctx
+	s.ctx = sdkCtx
+	s.cdc = codec.NewProtoCodec(encCfg.InterfaceRegistry)
+	authority := authtypes.NewModuleAddress("gov")
+	ac := addresscodec.NewBech32Codec("cosmos")
+
+	bz, err := ac.StringToBytes(authority.String())
+	s.Require().NoError(err)
+	s.addrBytes = bz
+
+	s.keeper = keeper.NewKeeper(s.cdc, runtime.NewKVStoreService(key), authority.String(), ac)
+}
+
+func (s *GenesisTestSuite) TestInitExportGenesis() {
+	perms := types.Permissions{
+		Level:         3,
+		LimitTypeUrls: []string{"test"},
+	}
+	err := s.keeper.Permissions.Set(s.ctx, s.addrBytes, perms)
+	s.Require().NoError(err)
+
+	var accounts []*types.GenesisAccountPermissions
+	genAccsPerms := types.GenesisAccountPermissions{
+		Address:     sdk.AccAddress(s.addrBytes).String(),
+		Permissions: &perms,
+	}
+	accounts = append(accounts, &genAccsPerms)
+
+	url := "test_url"
+
+	genesisState := &types.GenesisState{
+		AccountPermissions: accounts,
+		DisabledTypeUrls:   []string{url},
+	}
+
+	s.keeper.InitGenesis(s.ctx, genesisState)
+
+	exported := s.keeper.ExportGenesis(s.ctx)
+	bz, err := s.cdc.MarshalJSON(exported)
+	s.Require().NoError(err)
+
+	var exportedGenesisState types.GenesisState
+	err = s.cdc.UnmarshalJSON(bz, &exportedGenesisState)
+	s.Require().NoError(err)
+
+	s.Require().Equal(genesisState.AccountPermissions, exportedGenesisState.AccountPermissions)
+	s.Require().Equal(genesisState.DisabledTypeUrls, exportedGenesisState.DisabledTypeUrls)
+}

x/circuit/keeper/keeper.go

@@ -69,6 +69,7 @@ func (k *Keeper) GetAuthority() []byte {
 	return k.authority
 }
 
+// IsAllowed returns true when msg URL is not found in the DisableList for given context, else false.
 func (k *Keeper) IsAllowed(ctx context.Context, msgURL string) (bool, error) {
 	has, err := k.DisableList.Has(ctx, msgURL)
 	return !has, err

x/circuit/keeper/msg_server_test.go

@@ -12,7 +12,7 @@ import (
 
 const msgSend = "cosmos.bank.v1beta1.MsgSend"
 
-func Test_AuthorizeCircuitBreaker(t *testing.T) {
+func TestAuthorizeCircuitBreaker(t *testing.T) {
 	ft := initFixture(t)
 
 	srv := keeper.NewMsgServerImpl(ft.keeper)
@@ -31,7 +31,7 @@ func Test_AuthorizeCircuitBreaker(t *testing.T) {
 	perms, err := ft.keeper.Permissions.Get(ft.ctx, add1)
 	require.NoError(t, err)
 
-	require.Equal(t, adminPerms, perms, "admin perms are not the same")
+	require.Equal(t, adminPerms, perms, "admin perms are the same")
 
 	// add a super user
 	allmsgs := types.Permissions{Level: types.Permissions_LEVEL_ALL_MSGS, LimitTypeUrls: []string{""}}
@@ -45,22 +45,21 @@ func Test_AuthorizeCircuitBreaker(t *testing.T) {
 	perms, err = ft.keeper.Permissions.Get(ft.ctx, add2)
 	require.NoError(t, err)
 
-	require.Equal(t, allmsgs, perms, "admin perms are not the same")
+	require.Equal(t, allmsgs, perms)
 
 	// unauthorized user who does not have perms trying to authorize
 	superPerms := &types.Permissions{Level: types.Permissions_LEVEL_SUPER_ADMIN, LimitTypeUrls: []string{}}
 	msg = &types.MsgAuthorizeCircuitBreaker{Granter: addresses[3], Grantee: addresses[2], Permissions: superPerms}
 	_, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg)
-	require.Error(t, err, "user with no permission should fail in authorizing others")
+	require.Error(t, err, "user with no permission fails in authorizing others")
 
 	// user with permission level all_msgs tries to grant another user perms
 	somePerms := &types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{}}
 	msg = &types.MsgAuthorizeCircuitBreaker{Granter: addresses[2], Grantee: addresses[3], Permissions: somePerms}
 	_, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg)
-	require.Error(t, err, "user[2] does not have permission to grant others permission")
-
-	// user successfully grants another user perms to a specific permission
+	require.Error(t, err, "super user[2] does not have permission to grant others permission")
 
+	// admin successfully grants another user perms to a specific permission
 	somemsgs := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{msgSend}}
 	msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[3], Permissions: &somemsgs}
 	_, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg)
@@ -72,24 +71,24 @@ func Test_AuthorizeCircuitBreaker(t *testing.T) {
 	perms, err = ft.keeper.Permissions.Get(ft.ctx, add3)
 	require.NoError(t, err)
 
-	require.Equal(t, somemsgs, perms, "admin perms are not the same")
+	require.Equal(t, somemsgs, perms)
 
 	add4, err := ft.ac.StringToBytes(addresses[4])
 	require.NoError(t, err)
 
 	perms, err = ft.keeper.Permissions.Get(ft.ctx, add4)
-	require.ErrorIs(t, err, collections.ErrNotFound, "user should have no perms by default")
+	require.ErrorIs(t, err, collections.ErrNotFound, "users have no perms by default")
 
-	require.Equal(t, types.Permissions{Level: types.Permissions_LEVEL_NONE_UNSPECIFIED, LimitTypeUrls: nil}, perms, "user should have no perms by default")
+	require.Equal(t, types.Permissions{Level: types.Permissions_LEVEL_NONE_UNSPECIFIED, LimitTypeUrls: nil}, perms, "users have no perms by default")
 
-	// admin tries grants another user permission ALL_MSGS with limited urls populated
-	invalidmsgs := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{msgSend}}
-	msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[4], Permissions: &invalidmsgs}
+	// admin tries grants another user permission SOME_MSGS with limited urls populated
+	permis := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{msgSend}}
+	msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[4], Permissions: &permis}
 	_, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg)
 	require.NoError(t, err)
 }
 
-func Test_TripCircuitBreaker(t *testing.T) {
+func TestTripCircuitBreaker(t *testing.T) {
 	ft := initFixture(t)
 
 	srv := keeper.NewMsgServerImpl(ft.keeper)
@@ -124,7 +123,7 @@ func Test_TripCircuitBreaker(t *testing.T) {
 	require.NoError(t, err)
 	require.False(t, allowed, "circuit breaker should be tripped")
 
-	// user with no permission attempts to trips circuit breaker
+	// user with no permission attempts to trip circuit breaker
 	unknownTrip := &types.MsgTripCircuitBreaker{Authority: addresses[4], MsgTypeUrls: []string{url}}
 	_, err = srv.TripCircuitBreaker(ft.ctx, unknownTrip)
 	require.Error(t, err)
@@ -139,16 +138,16 @@ func Test_TripCircuitBreaker(t *testing.T) {
 	// try to trip two messages but user only has permission for one
 	someTrip := &types.MsgTripCircuitBreaker{Authority: addresses[2], MsgTypeUrls: []string{url, url2}}
 	_, err = srv.TripCircuitBreaker(ft.ctx, someTrip)
-	require.ErrorContains(t, err, "MsgEditValidator")
+	require.ErrorContains(t, err, "MsgEditValidator: unauthorized")
 
 	// user tries to trip an already tripped circuit breaker
 	alreadyTripped := "cosmos.bank.v1beta1.MsgSend"
 	twoTrip := &types.MsgTripCircuitBreaker{Authority: addresses[1], MsgTypeUrls: []string{alreadyTripped}}
 	_, err = srv.TripCircuitBreaker(ft.ctx, twoTrip)
-	require.Error(t, err)
+	require.ErrorContains(t, err, "already disabled")
 }
 
-func Test_ResetCircuitBreaker(t *testing.T) {
+func TestResetCircuitBreaker(t *testing.T) {
 	ft := initFixture(t)
 	authority, err := ft.ac.BytesToString(ft.mockAddr)
 	require.NoError(t, err)
@@ -174,7 +173,6 @@ func Test_ResetCircuitBreaker(t *testing.T) {
 	require.NoError(t, err)
 	require.True(t, allowed, "circuit breaker should be reset")
 
-	// user has no  permission to reset circuit breaker
 	// admin trips circuit breaker
 	_, err = srv.TripCircuitBreaker(ft.ctx, admintrip)
 	require.NoError(t, err)
@@ -183,6 +181,7 @@ func Test_ResetCircuitBreaker(t *testing.T) {
 	require.NoError(t, err)
 	require.False(t, allowed, "circuit breaker should be tripped")
 
+	// user has no permission to reset circuit breaker
 	unknownUserReset := &types.MsgResetCircuitBreaker{Authority: addresses[1], MsgTypeUrls: []string{url}}
 	_, err = srv.ResetCircuitBreaker(ft.ctx, unknownUserReset)
 	require.Error(t, err)
@@ -208,8 +207,7 @@ func Test_ResetCircuitBreaker(t *testing.T) {
 	_, err = srv.ResetCircuitBreaker(ft.ctx, allMsgsReset)
 	require.NoError(t, err)
 
-	// user tries to reset an message they dont have permission to reset
-
+	// user tries to reset a message they dont have permission to reset
 	url = "cosmos.staking.v1beta1.MsgCreateValidator"
 	// give restricted perms to a user
 	someMsgs := &types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{url2}}
@@ -221,7 +219,7 @@ func Test_ResetCircuitBreaker(t *testing.T) {
 	_, err = srv.TripCircuitBreaker(ft.ctx, admintrip)
 	require.NoError(t, err)
 
-	// user with all messages resets circuit breaker
+	// user with some messages resets circuit breaker
 	someMsgsReset := &types.MsgResetCircuitBreaker{Authority: addresses[2], MsgTypeUrls: []string{url}}
 	_, err = srv.ResetCircuitBreaker(ft.ctx, someMsgsReset)
 	require.NoError(t, err)

x/circuit/keeper/query.go

@@ -17,7 +17,7 @@ type QueryServer struct {
 	keeper Keeper
 }
 
-// NewMsgServerImpl returns an implementation of the circuit MsgServer interface
+// NewQueryServer returns an implementation of the circuit QueryServer interface
 // for the provided Keeper.
 func NewQueryServer(keeper Keeper) types.QueryServer {
 	return &QueryServer{keeper: keeper}