refactor(x/auth): audit x/auth changes (backport #21146) (#21302)

Co-authored-by: son trinh <[email protected]>

CHANGELOG.md

@@ -210,7 +210,6 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i
 
 * (simapp) [#19146](https://github.com/cosmos/cosmos-sdk/pull/19146) Replace `--v` CLI option with `--validator-count`/`-n`.
 * (module) [#19370](https://github.com/cosmos/cosmos-sdk/pull/19370) Deprecate `module.Configurator`, use `appmodule.HasMigrations` and `appmodule.HasServices` instead from Core API.
-* (x/auth) [#20531](https://github.com/cosmos/cosmos-sdk/pull/20531) Deprecate auth keeper `NextAccountNumber`, use `keeper.AccountsModKeeper.NextAccountNumber` instead.
 
 ## [v0.50.8](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.8) - 2024-07-15
 

x/auth/CHANGELOG.md

@@ -37,10 +37,11 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * [#18780](https://github.com/cosmos/cosmos-sdk/pull/18780) Move sig verification out of the for loop, into the authenticate method.
 * [#19188](https://github.com/cosmos/cosmos-sdk/pull/19188) Remove creation of `BaseAccount` when sending a message to an account that does not exist. 
     * When signing a transaction with an account that has not been created accountnumber 0 must be used
+* [#19363](https://github.com/cosmos/cosmos-sdk/pull/19363), [#19370](https://github.com/cosmos/cosmos-sdk/pull/19370) RegisterMigrations, InitGenesis and ExportGenesis return error instead of panic.
 
 ### CLI Breaking Changes
 
-* (vesting) [#18100](https://github.com/cosmos/cosmos-sdk/pull/18100) `appd tx vesting create-vesting-account` takes an amount of coin as last argument instead of second. Coins are space separated.
+* (vesting) [#19539](https://github.com/cosmos/cosmos-sdk/pull/19539) Remove vesting CLI.
 
 ### API Breaking Changes
 
@@ -49,9 +50,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * [#19161](https://github.com/cosmos/cosmos-sdk/pull/19161) Remove `simulate` from `SetGasMeter`
 * [#19363](https://github.com/cosmos/cosmos-sdk/pull/19363) Remove `IterateAccounts` and `GetAllAccounts` methods from the AccountKeeper interface and Keeper.
 * [#19290](https://github.com/cosmos/cosmos-sdk/issues/19290) Pass `appmodule.Environment` to NewKeeper instead of passing individual services. 
-* [#19535](https://github.com/cosmos/cosmos-sdk/pull/19535) Remove vesting account creation when the chain is running. The accounts module is required for creating vesting accounts on a running chain. 
+* [#19535](https://github.com/cosmos/cosmos-sdk/pull/19535) Remove vesting account creation when the chain is running. The accounts module is required for creating [#vesting accounts](../accounts/defaults/lockup/README.md) on a running chain. 
 * [#19600](https://github.com/cosmos/cosmos-sdk/pull/19600) add a consensus query method to the consensus module in order for modules to query consensus for the consensus params. 
-<!-- TODO add a link to lockup accounts docs -->
 
 ### Consensus Breaking Changes
 
@@ -64,4 +64,8 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * [#19239](https://github.com/cosmos/cosmos-sdk/pull/19239) Sets from flag in multi-sign command to avoid no key name provided error.
 * [#19099](https://github.com/cosmos/cosmos-sdk/pull/19099) `verifyIsOnCurve` now checks if we are simulating to avoid malformed public key error.
 * [#20323](https://github.com/cosmos/cosmos-sdk/pull/20323) Ignore undecodable txs in GetBlocksWithTxs.
-* [#20963](https://github.com/cosmos/cosmos-sdk/pull/20963) UseGrantedFees used to return error with raw addresses. Now it uses addresses in string format.
+* [#20963](https://github.com/cosmos/cosmos-sdk/pull/20963) UseGrantedFees used to return error with raw addresses. Now it uses addresses in string format.
+
+### Deprecated
+
+* (x/auth) [#20531](https://github.com/cosmos/cosmos-sdk/pull/20531) Deprecate auth keeper `NextAccountNumber`, use `keeper.AccountsModKeeper.NextAccountNumber` instead.

x/auth/ante/unorderedtx/manager.go

@@ -32,7 +32,7 @@ type TxHash [32]byte
 // Manager contains the tx hash dictionary for duplicates checking, and expire
 // them when block production progresses.
 type Manager struct {
-	// blockCh defines a channel to receive newly committed block heights
+	// blockCh defines a channel to receive newly committed block time
 	blockCh chan time.Time
 	// doneCh allows us to ensure the purgeLoop has gracefully terminated prior to closing
 	doneCh chan struct{}
@@ -152,7 +152,7 @@ func (m *Manager) OnInit() error {
 	return nil
 }
 
-// OnNewBlock sends the latest block number to the background purge loop, which
+// OnNewBlock sends the latest block time to the background purge loop, which
 // should be called in ABCI Commit event.
 func (m *Manager) OnNewBlock(blockTime time.Time) {
 	m.blockCh <- blockTime
@@ -213,7 +213,7 @@ func (m *Manager) flushToFile() error {
 	return nil
 }
 
-// expiredTxs returns expired tx hashes based on the provided block height.
+// expiredTxs returns expired tx hashes based on the provided block time.
 func (m *Manager) expiredTxs(blockTime time.Time) []TxHash {
 	m.mu.RLock()
 	defer m.mu.RUnlock()

x/auth/ante/unorderedtx/manager_test.go

@@ -103,11 +103,11 @@ func TestUnorderedTxManager_Flow(t *testing.T) {
 	currentTime := time.Now()
 
 	// Seed the manager with a txs, some of which should eventually be purged and
-	// the others will remain. Txs with TTL less than or equal to 50 should be purged.
-	for i := 1; i <= 100; i++ {
+	// the others will remain. First 25 Txs should be purged.
+	for i := 1; i <= 50; i++ {
 		txHash := [32]byte{byte(i)}
 
-		if i <= 50 {
+		if i <= 25 {
 			txm.Add(txHash, currentTime.Add(time.Millisecond*500*time.Duration(i)))
 		} else {
 			txm.Add(txHash, currentTime.Add(time.Hour))
@@ -123,19 +123,19 @@ func TestUnorderedTxManager_Flow(t *testing.T) {
 		for t := range ticker.C {
 			txm.OnNewBlock(t)
 
-			if t.After(currentTime.Add(time.Millisecond * 500 * time.Duration(50))) {
+			if t.After(currentTime.Add(time.Millisecond * 500 * time.Duration(25))) {
 				doneBlockCh <- true
 				return
 			}
 		}
 	}()
 
-	// Eventually all the txs that should be expired by block 50 should be purged.
+	// Eventually all the txs that are expired should be purged.
 	// The remaining txs should remain.
 	require.Eventually(
 		t,
 		func() bool {
-			return txm.Size() == 50
+			return txm.Size() == 25
 		},
 		2*time.Minute,
 		5*time.Second,

x/auth/ante/unorderedtx/snapshotter.go

@@ -80,10 +80,8 @@ func (s *Snapshotter) restore(height uint64, payloadReader snapshot.ExtensionPay
 		copy(txHash[:], payload[i:i+txHashSize])
 
 		timestamp := binary.BigEndian.Uint64(payload[i+txHashSize : i+chunkSize])
-		// need to come up with a way to fetch blocktime to filter out expired txs
-		//
-		// right now we dont have access block time at this flow, so we would just include the expired txs
-		// and let it be purge during purge loop
+
+		// purge any expired txs
 		if timestamp != 0 && timestamp > height {
 			s.m.Add(txHash, time.Unix(int64(timestamp), 0))
 		}

x/auth/keeper/keeper_test.go

@@ -2,6 +2,7 @@ package keeper_test
 
 import (
 	"context"
+	"encoding/binary"
 	"testing"
 
 	"github.com/golang/mock/gomock"
@@ -250,3 +251,32 @@ func (suite *KeeperTestSuite) TestInitGenesis() {
 	// we expect nextNum to be 2 because we initialize fee_collector as account number 1
 	suite.Require().Equal(2, int(nextNum))
 }
+
+func (suite *KeeperTestSuite) TestMigrateAccountNumberUnsafe() {
+	suite.SetupTest() // reset
+
+	legacyAccNum := uint64(10)
+	val := make([]byte, 8)
+	binary.LittleEndian.PutUint64(val, legacyAccNum)
+
+	// Set value for legacy account number
+	store := suite.accountKeeper.KVStoreService.OpenKVStore(suite.ctx)
+	err := store.Set(types.GlobalAccountNumberKey.Bytes(), val)
+	require.NoError(suite.T(), err)
+
+	// check if value is set
+	val, err = store.Get(types.GlobalAccountNumberKey.Bytes())
+	require.NoError(suite.T(), err)
+	require.NotEmpty(suite.T(), val)
+
+	suite.acctsModKeeper.EXPECT().InitAccountNumberSeqUnsafe(gomock.Any(), gomock.Any()).AnyTimes().DoAndReturn(func(ctx context.Context, accNum uint64) (uint64, error) {
+		return legacyAccNum, nil
+	})
+
+	err = keeper.MigrateAccountNumberUnsafe(suite.ctx, &suite.accountKeeper)
+	require.NoError(suite.T(), err)
+
+	val, err = store.Get(types.GlobalAccountNumberKey.Bytes())
+	require.NoError(suite.T(), err)
+	require.Empty(suite.T(), val)
+}

x/auth/keeper/msg_server.go

@@ -23,7 +23,7 @@ func NewMsgServerImpl(ak AccountKeeper) types.MsgServer {
 	}
 }
 
-func (ms msgServer) NonAtomicExec(goCtx context.Context, msg *types.MsgNonAtomicExec) (*types.MsgNonAtomicExecResponse, error) {
+func (ms msgServer) NonAtomicExec(ctx context.Context, msg *types.MsgNonAtomicExec) (*types.MsgNonAtomicExecResponse, error) {
 	if msg.Signer == "" {
 		return nil, errors.New("empty signer address string is not allowed")
 	}
@@ -42,7 +42,7 @@ func (ms msgServer) NonAtomicExec(goCtx context.Context, msg *types.MsgNonAtomic
 		return nil, err
 	}
 
-	results, err := ms.ak.NonAtomicMsgsExec(goCtx, signer, msgs)
+	results, err := ms.ak.NonAtomicMsgsExec(ctx, signer, msgs)
 	if err != nil {
 		return nil, err
 	}

x/auth/keeper/msg_server_test.go

@@ -1,7 +1,17 @@
 package keeper_test
 
 import (
+	"context"
+
+	"github.com/cosmos/gogoproto/proto"
+	any "github.com/cosmos/gogoproto/types/any"
+	"github.com/golang/mock/gomock"
+	"google.golang.org/protobuf/runtime/protoiface"
+
 	"cosmossdk.io/x/auth/types"
+
+	codectypes "github.com/cosmos/cosmos-sdk/codec/types"
+	"github.com/cosmos/cosmos-sdk/testutil/testdata"
 )
 
 func (s *KeeperTestSuite) TestUpdateParams() {
@@ -94,6 +104,20 @@ func (s *KeeperTestSuite) TestUpdateParams() {
 			expectErr: true,
 			expErrMsg: "invalid SECK256k1 signature verification cost",
 		},
+		{
+			name: "valid transaction",
+			req: &types.MsgUpdateParams{
+				Authority: s.accountKeeper.GetAuthority(),
+				Params: types.Params{
+					MaxMemoCharacters:      140,
+					TxSigLimit:             9,
+					TxSizeCostPerByte:      5,
+					SigVerifyCostED25519:   694,
+					SigVerifyCostSecp256k1: 511,
+				},
+			},
+			expectErr: false,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -109,3 +133,73 @@ func (s *KeeperTestSuite) TestUpdateParams() {
 		})
 	}
 }
+
+func (s *KeeperTestSuite) TestNonAtomicExec() {
+	_, _, addr := testdata.KeyTestPubAddr()
+
+	msgUpdateParams := &types.MsgUpdateParams{}
+
+	msgAny, err := codectypes.NewAnyWithValue(msgUpdateParams)
+	s.Require().NoError(err)
+
+	testCases := []struct {
+		name      string
+		req       *types.MsgNonAtomicExec
+		expectErr bool
+		expErrMsg string
+	}{
+		{
+			name: "error: empty signer",
+			req: &types.MsgNonAtomicExec{
+				Signer: "",
+				Msgs:   []*any.Any{},
+			},
+			expectErr: true,
+			expErrMsg: "empty signer address string is not allowed",
+		},
+		{
+			name: "error: invalid signer",
+			req: &types.MsgNonAtomicExec{
+				Signer: "invalid_signer",
+				Msgs:   []*any.Any{},
+			},
+			expectErr: true,
+			expErrMsg: "invalid signer address",
+		},
+		{
+			name: "error: empty messages",
+			req: &types.MsgNonAtomicExec{
+				Signer: addr.String(),
+				Msgs:   []*any.Any{},
+			},
+			expectErr: true,
+			expErrMsg: "messages cannot be empty",
+		},
+		{
+			name: "valid transaction",
+			req: &types.MsgNonAtomicExec{
+				Signer: addr.String(),
+				Msgs:   []*any.Any{msgAny},
+			},
+			expectErr: false,
+		},
+	}
+
+	s.acctsModKeeper.EXPECT().SendModuleMessageUntyped(gomock.Any(), gomock.Any(), gomock.Any()).
+		DoAndReturn(func(_ context.Context, sender []byte, msg proto.Message) (protoiface.MessageV1, error) {
+			return msg, nil
+		}).AnyTimes()
+
+	for _, tc := range testCases {
+		tc := tc
+		s.Run(tc.name, func() {
+			_, err := s.msgServer.NonAtomicExec(s.ctx, tc.req)
+			if tc.expectErr {
+				s.Require().Error(err)
+				s.Require().Contains(err.Error(), tc.expErrMsg)
+			} else {
+				s.Require().NoError(err)
+			}
+		})
+	}
+}

x/auth/vesting/README.md

@@ -582,42 +582,3 @@ according to a custom vesting schedule.
 Coins in this account can still be used for delegating and for governance votes even while locked.
 
 
-## CLI
-
-A user can query and interact with the `vesting` module using the CLI.
-
-### Transactions
-
-The `tx` commands allow users to interact with the `vesting` module.
-
-```bash
-simd tx vesting --help
-```
-
-#### create-periodic-vesting-account
-
-The `create-periodic-vesting-account` command creates a new vesting account funded with an allocation of tokens, where a sequence of coins and period length in seconds. Periods are sequential, in that the duration of a period only starts at the end of the previous period. The duration of the first period starts upon account creation.
-
-```bash
-simd tx vesting create-periodic-vesting-account [to_address] [periods_json_file] [flags]
-```
-
-Example:
-
-```bash
-simd tx vesting create-periodic-vesting-account cosmos1.. periods.json
-```
-
-#### create-vesting-account
-
-The `create-vesting-account` command creates a new vesting account funded with an allocation of tokens. The account can either be a delayed or continuous vesting account, which is determined by the '--delayed' flag. All vesting accounts created will have their start time set by the committed block's time. The end_time must be provided as a UNIX epoch timestamp.
-
-```bash
-simd tx vesting create-vesting-account [to_address] [amount] [end_time] [flags]
-```
-
-Example:
-
-```bash
-simd tx vesting create-vesting-account cosmos1.. 100stake 2592000
-```

x/auth/vesting/types/codec.go

@@ -3,13 +3,10 @@ package types
 import (
 	corelegacy "cosmossdk.io/core/legacy"
 	"cosmossdk.io/core/registry"
-	coretransaction "cosmossdk.io/core/transaction"
 	authtypes "cosmossdk.io/x/auth/types"
 	"cosmossdk.io/x/auth/vesting/exported"
 
-	"github.com/cosmos/cosmos-sdk/codec/legacy"
 	sdk "github.com/cosmos/cosmos-sdk/types"
-	"github.com/cosmos/cosmos-sdk/types/msgservice"
 )
 
 // RegisterLegacyAminoCodec registers the vesting interfaces and concrete types on the
@@ -21,9 +18,6 @@ func RegisterLegacyAminoCodec(cdc corelegacy.Amino) {
 	cdc.RegisterConcrete(&DelayedVestingAccount{}, "cosmos-sdk/DelayedVestingAccount")
 	cdc.RegisterConcrete(&PeriodicVestingAccount{}, "cosmos-sdk/PeriodicVestingAccount")
 	cdc.RegisterConcrete(&PermanentLockedAccount{}, "cosmos-sdk/PermanentLockedAccount")
-	legacy.RegisterAminoMsg(cdc, &MsgCreateVestingAccount{}, "cosmos-sdk/MsgCreateVestingAccount")
-	legacy.RegisterAminoMsg(cdc, &MsgCreatePermanentLockedAccount{}, "cosmos-sdk/MsgCreatePermLockedAccount")
-	legacy.RegisterAminoMsg(cdc, &MsgCreatePeriodicVestingAccount{}, "cosmos-sdk/MsgCreatePeriodVestAccount")
 }
 
 // RegisterInterfaces associates protoName with AccountI and VestingAccount
@@ -55,12 +49,4 @@ func RegisterInterfaces(registrar registry.InterfaceRegistrar) {
 		&PeriodicVestingAccount{},
 		&PermanentLockedAccount{},
 	)
-
-	registrar.RegisterImplementations(
-		(*coretransaction.Msg)(nil),
-		&MsgCreateVestingAccount{},
-		&MsgCreatePermanentLockedAccount{},
-	)
-
-	msgservice.RegisterMsgServiceDesc(registrar, &_Msg_serviceDesc)
 }