From 840340fa4f03ad8541ad0bc6e3cdbeb27ecf8e23 Mon Sep 17 00:00:00 2001 From: "jaeseung.bae" Date: Wed, 14 Feb 2024 09:25:19 +0900 Subject: [PATCH] chore: fix side-effect from original bank.send --- x/bankplus/keeper/inactive.go | 24 +++++----- x/bankplus/keeper/inactive_test.go | 32 ++++++------- x/bankplus/keeper/keeper.go | 72 +++++++++++++++--------------- x/bankplus/module/module.go | 6 ++- 4 files changed, 68 insertions(+), 66 deletions(-) diff --git a/x/bankplus/keeper/inactive.go b/x/bankplus/keeper/inactive.go index a173658809..664d4ea9d2 100644 --- a/x/bankplus/keeper/inactive.go +++ b/x/bankplus/keeper/inactive.go @@ -20,30 +20,30 @@ func inactiveAddrKey(addr sdk.AccAddress) []byte { } // isStoredInactiveAddr checks if the address is stored or not as blocked address -func (keeper BaseKeeper) isStoredInactiveAddr(ctx context.Context, address sdk.AccAddress) bool { - store := keeper.storeService.OpenKVStore(ctx) +func (k BaseKeeper) isStoredInactiveAddr(ctx context.Context, address sdk.AccAddress) bool { + store := k.storeService.OpenKVStore(ctx) bz, _ := store.Get(inactiveAddrKey(address)) return bz != nil } // addToInactiveAddr adds a blocked address to the store. -func (keeper BaseKeeper) addToInactiveAddr(ctx context.Context, address sdk.AccAddress) { - store := keeper.storeService.OpenKVStore(ctx) - addrString, err := keeper.addrCdc.BytesToString(address) +func (k BaseKeeper) addToInactiveAddr(ctx context.Context, address sdk.AccAddress) { + store := k.storeService.OpenKVStore(ctx) + addrString, err := k.addrCdc.BytesToString(address) if err != nil { panic(err) } blockedCAddr := types.InactiveAddr{Address: addrString} - bz := keeper.cdc.MustMarshal(&blockedCAddr) + bz := k.cdc.MustMarshal(&blockedCAddr) if err := store.Set(inactiveAddrKey(address), bz); err != nil { panic(err) } } // deleteFromInactiveAddr deletes blocked address from store -func (keeper BaseKeeper) deleteFromInactiveAddr(ctx context.Context, address sdk.AccAddress) { - store := keeper.storeService.OpenKVStore(ctx) +func (k BaseKeeper) deleteFromInactiveAddr(ctx context.Context, address sdk.AccAddress) { + store := k.storeService.OpenKVStore(ctx) err := store.Delete(inactiveAddrKey(address)) if err != nil { panic(err) @@ -53,16 +53,16 @@ func (keeper BaseKeeper) deleteFromInactiveAddr(ctx context.Context, address sdk // loadAllInactiveAddrs loads all blocked address and set to `inactiveAddr`. // This function is executed when the app is initiated and save all inactive address in caches // in order to prevent to query to store in every time to send -func (keeper BaseKeeper) loadAllInactiveAddrs(ctx context.Context) { - store := keeper.storeService.OpenKVStore(ctx) +func (k BaseKeeper) loadAllInactiveAddrs(ctx context.Context) { + store := k.storeService.OpenKVStore(ctx) adapter := runtime.KVStoreAdapter(store) iterator := storetypes.KVStorePrefixIterator(adapter, inactiveAddrsKeyPrefix) defer iterator.Close() for ; iterator.Valid(); iterator.Next() { var bAddr types.InactiveAddr - keeper.cdc.MustUnmarshal(iterator.Value(), &bAddr) + k.cdc.MustUnmarshal(iterator.Value(), &bAddr) - keeper.inactiveAddrs[bAddr.Address] = true + k.inactiveAddrs[bAddr.Address] = true } } diff --git a/x/bankplus/keeper/inactive_test.go b/x/bankplus/keeper/inactive_test.go index 8cde6b5f98..7574051ee7 100644 --- a/x/bankplus/keeper/inactive_test.go +++ b/x/bankplus/keeper/inactive_test.go @@ -30,7 +30,7 @@ func TestBankPlus(t *testing.T) { type BankPlusTestSuite struct { suite.Suite mockCtrl *gomock.Controller - keeper BaseKeeper + cut BaseKeeper ctx context.Context } @@ -55,7 +55,7 @@ func (s *BankPlusTestSuite) SetupTest() { BytesToString(authtypes.NewModuleAddress(govtypes.ModuleName)) s.Require().NoError(err) - s.keeper = NewBaseKeeper( + s.cut = NewBaseKeeper( codec, kvStoreService, mockAccKeeper, @@ -70,7 +70,7 @@ func (s *BankPlusTestSuite) TearDownTest() { } func (s *BankPlusTestSuite) TestInactiveAddress() { - require.Equal(s.T(), 0, len(s.keeper.inactiveAddrs)) + require.Equal(s.T(), 0, len(s.cut.inactiveAddrs)) addr := genAddr() anotherAddr := genAddr() s.addAddrOk(addr) @@ -82,36 +82,36 @@ func (s *BankPlusTestSuite) TestInactiveAddress() { } func (s *BankPlusTestSuite) addAddrOk(addr sdk.AccAddress) { - s.keeper.addToInactiveAddr(s.ctx, addr) - require.True(s.T(), s.keeper.isStoredInactiveAddr(s.ctx, addr)) + s.cut.addToInactiveAddr(s.ctx, addr) + require.True(s.T(), s.cut.isStoredInactiveAddr(s.ctx, addr)) } func (s *BankPlusTestSuite) duplicateAddOk(addr sdk.AccAddress) { - s.keeper.addToInactiveAddr(s.ctx, addr) - require.True(s.T(), s.keeper.isStoredInactiveAddr(s.ctx, addr)) + s.cut.addToInactiveAddr(s.ctx, addr) + require.True(s.T(), s.cut.isStoredInactiveAddr(s.ctx, addr)) } func (s *BankPlusTestSuite) deleteAddrOk(addr sdk.AccAddress) { - s.keeper.deleteFromInactiveAddr(s.ctx, addr) - require.False(s.T(), s.keeper.isStoredInactiveAddr(s.ctx, addr)) + s.cut.deleteFromInactiveAddr(s.ctx, addr) + require.False(s.T(), s.cut.isStoredInactiveAddr(s.ctx, addr)) } func (s *BankPlusTestSuite) falseForUnknownAddr(anotherAddr sdk.AccAddress) { - require.False(s.T(), s.keeper.isStoredInactiveAddr(s.ctx, anotherAddr)) + require.False(s.T(), s.cut.isStoredInactiveAddr(s.ctx, anotherAddr)) } func (s *BankPlusTestSuite) noErrorWhenDeletionOfUnknownAddr(anotherAddr sdk.AccAddress) { require.NotPanicsf(s.T(), func() { - s.keeper.deleteFromInactiveAddr(s.ctx, anotherAddr) + s.cut.deleteFromInactiveAddr(s.ctx, anotherAddr) }, "no panic expected") } func (s *BankPlusTestSuite) testLoadAllInactiveAddrs(addr, anotherAddr sdk.AccAddress) { - s.keeper.addToInactiveAddr(s.ctx, addr) - s.keeper.addToInactiveAddr(s.ctx, anotherAddr) - require.Equal(s.T(), 0, len(s.keeper.inactiveAddrs)) - s.keeper.loadAllInactiveAddrs(s.ctx) - require.Equal(s.T(), 2, len(s.keeper.inactiveAddrs)) + s.cut.addToInactiveAddr(s.ctx, addr) + s.cut.addToInactiveAddr(s.ctx, anotherAddr) + require.Equal(s.T(), 0, len(s.cut.inactiveAddrs)) + s.cut.loadAllInactiveAddrs(s.ctx) + require.Equal(s.T(), 2, len(s.cut.inactiveAddrs)) } func genAddr() sdk.AccAddress { diff --git a/x/bankplus/keeper/keeper.go b/x/bankplus/keeper/keeper.go index efa739f919..33476408ec 100644 --- a/x/bankplus/keeper/keeper.go +++ b/x/bankplus/keeper/keeper.go @@ -54,123 +54,123 @@ func NewBaseKeeper( } } -func (keeper BaseKeeper) InitializeBankPlus(ctx context.Context) { - keeper.loadAllInactiveAddrs(ctx) +func (k BaseKeeper) InitializeBankPlus(ctx context.Context) { + k.loadAllInactiveAddrs(ctx) } // SendCoinsFromModuleToAccount transfers coins from a ModuleAccount to an AccAddress. // It will panic if the module account does not exist. -func (keeper BaseKeeper) SendCoinsFromModuleToAccount( +func (k BaseKeeper) SendCoinsFromModuleToAccount( ctx context.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins, ) error { - senderAddr := keeper.ak.GetModuleAddress(senderModule) + senderAddr := k.ak.GetModuleAddress(senderModule) if senderAddr.Empty() { panic(errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", senderModule)) } - if keeper.BlockedAddr(recipientAddr) { + if k.BlockedAddr(recipientAddr) { return errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive funds", recipientAddr) } - return keeper.SendCoins(ctx, senderAddr, recipientAddr, amt) + return k.SendCoins(ctx, senderAddr, recipientAddr, amt) } // SendCoinsFromModuleToModule transfers coins from a ModuleAccount to another. // It will panic if either module account does not exist. -func (keeper BaseKeeper) SendCoinsFromModuleToModule( +func (k BaseKeeper) SendCoinsFromModuleToModule( ctx context.Context, senderModule, recipientModule string, amt sdk.Coins, ) error { - senderAddr := keeper.ak.GetModuleAddress(senderModule) + senderAddr := k.ak.GetModuleAddress(senderModule) if senderAddr.Empty() { panic(errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", senderModule)) } - recipientAcc := keeper.ak.GetModuleAccount(ctx, recipientModule) + recipientAcc := k.ak.GetModuleAccount(ctx, recipientModule) if recipientAcc == nil { panic(errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", recipientModule)) } - return keeper.SendCoins(ctx, senderAddr, recipientAcc.GetAddress(), amt) + return k.SendCoins(ctx, senderAddr, recipientAcc.GetAddress(), amt) } // SendCoinsFromAccountToModule transfers coins from an AccAddress to a ModuleAccount. // It will panic if the module account does not exist. -func (keeper BaseKeeper) SendCoinsFromAccountToModule( +func (k BaseKeeper) SendCoinsFromAccountToModule( ctx context.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins, ) error { - recipientAcc := keeper.ak.GetModuleAccount(ctx, recipientModule) + recipientAcc := k.ak.GetModuleAccount(ctx, recipientModule) if recipientAcc == nil { panic(errorsmod.Wrapf(sdkerrors.ErrUnknownAddress, "module account %s does not exist", recipientModule)) } - return keeper.SendCoins(ctx, senderAddr, recipientAcc.GetAddress(), amt) + return k.SendCoins(ctx, senderAddr, recipientAcc.GetAddress(), amt) } -func (keeper BaseKeeper) isInactiveAddr(addr sdk.AccAddress) bool { - addrString, err := keeper.addrCdc.BytesToString(addr) +func (k BaseKeeper) isInactiveAddr(addr sdk.AccAddress) bool { + addrString, err := k.addrCdc.BytesToString(addr) if err != nil { panic(err) } - return keeper.inactiveAddrs[addrString] + return k.inactiveAddrs[addrString] } // SendCoins transfers amt coins from a sending account to a receiving account. // This is wrapped bank the `SendKeeper` interface of `bank` module, // and checks if `toAddr` is a inactiveAddr managed by the module. -func (keeper BaseKeeper) SendCoins(ctx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) error { +func (k BaseKeeper) SendCoins(ctx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) error { // if toAddr is smart contract, check the status of contract. - if keeper.isInactiveAddr(toAddr) { + if k.isInactiveAddr(toAddr) { return errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive funds", toAddr) } - return keeper.BaseSendKeeper.SendCoins(ctx, fromAddr, toAddr, amt) + return k.BaseSendKeeper.SendCoins(ctx, fromAddr, toAddr, amt) } // AddToInactiveAddr adds the address to `inactiveAddr`. -func (keeper BaseKeeper) AddToInactiveAddr(ctx context.Context, addr sdk.AccAddress) { - addrString, err := keeper.addrCdc.BytesToString(addr) +func (k BaseKeeper) AddToInactiveAddr(ctx context.Context, addr sdk.AccAddress) { + addrString, err := k.addrCdc.BytesToString(addr) if err != nil { panic(err) } - if !keeper.inactiveAddrs[addrString] { - keeper.inactiveAddrs[addrString] = true + if !k.inactiveAddrs[addrString] { + k.inactiveAddrs[addrString] = true - keeper.addToInactiveAddr(ctx, addr) + k.addToInactiveAddr(ctx, addr) } } // DeleteFromInactiveAddr removes the address from `inactiveAddr`. -func (keeper BaseKeeper) DeleteFromInactiveAddr(ctx context.Context, addr sdk.AccAddress) { - addrString, err := keeper.addrCdc.BytesToString(addr) +func (k BaseKeeper) DeleteFromInactiveAddr(ctx context.Context, addr sdk.AccAddress) { + addrString, err := k.addrCdc.BytesToString(addr) if err != nil { panic(err) } - if keeper.inactiveAddrs[addrString] { - delete(keeper.inactiveAddrs, addrString) + if k.inactiveAddrs[addrString] { + delete(k.inactiveAddrs, addrString) - keeper.deleteFromInactiveAddr(ctx, addr) + k.deleteFromInactiveAddr(ctx, addr) } } // IsInactiveAddr returns if the address is added in inactiveAddr. -func (keeper BaseKeeper) IsInactiveAddr(addr sdk.AccAddress) bool { - addrString, err := keeper.addrCdc.BytesToString(addr) +func (k BaseKeeper) IsInactiveAddr(addr sdk.AccAddress) bool { + addrString, err := k.addrCdc.BytesToString(addr) if err != nil { panic(err) } - return keeper.inactiveAddrs[addrString] + return k.inactiveAddrs[addrString] } -func (keeper BaseKeeper) InputOutputCoins(ctx context.Context, input types.Input, outputs []types.Output) error { - if keeper.deactMultiSend { +func (k BaseKeeper) InputOutputCoins(ctx context.Context, input types.Input, outputs []types.Output) error { + if k.deactMultiSend { return sdkerrors.ErrNotSupported.Wrap("MultiSend was deactivated") } for _, out := range outputs { - if keeper.inactiveAddrs[out.Address] { + if k.inactiveAddrs[out.Address] { return errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "%s is not allowed to receive funds", out.Address) } } - return keeper.BaseSendKeeper.InputOutputCoins(ctx, input, outputs) + return k.BaseSendKeeper.InputOutputCoins(ctx, input, outputs) } diff --git a/x/bankplus/module/module.go b/x/bankplus/module/module.go index 3a8d7b34f6..0e8d77469b 100644 --- a/x/bankplus/module/module.go +++ b/x/bankplus/module/module.go @@ -73,7 +73,7 @@ func (a AppModule) RegisterInvariants(ir sdk.InvariantRegistry) { func (a AppModule) RegisterServices(cfg module.Configurator) { banktypes.RegisterMsgServer(cfg.MsgServer(), bankkeeper.NewMsgServerImpl(a.bankKeeper)) banktypes.RegisterQueryServer(cfg.QueryServer(), a.bankKeeper) - m := bankkeeper.NewMigrator(a.bankKeeper.(keeper.BaseKeeper).BaseKeeper, a.legacySubspace) + m := bankkeeper.NewMigrator(a.bankKeeper.(bankkeeper.BaseKeeper), a.legacySubspace) if err := cfg.RegisterMigration(banktypes.ModuleName, 1, m.Migrate1to2); err != nil { panic(fmt.Sprintf("failed to migrate x/bank from version 1 to 2: %v", err)) } @@ -163,7 +163,9 @@ func ProvideModule(in ModuleInputs) ModuleOutputs { in.Logger, ) - m := NewAppModule(in.Cdc, bankKeeper, in.AccountKeeper, in.LegacySubspace) + originalBankKeeper := bankkeeper.NewBaseKeeper(in.Cdc, in.StoreService, in.AccountKeeper, blockedAddresses, authorityString, in.Logger) + m := NewAppModule(in.Cdc, originalBankKeeper, in.AccountKeeper, in.LegacySubspace) + return ModuleOutputs{ BankKeeper: bankKeeper, Module: m,