From f258c5d8498a0398432f34a3e75a7d25e25b0521 Mon Sep 17 00:00:00 2001
From: wr1159 <56021409+wr1159@users.noreply.github.com>
Date: Mon, 3 Jun 2024 09:32:36 +0800
Subject: [PATCH] feat(x/bank): Placing SendRestriction before Deduction of
 Coins in SendCoins (#20517)

---
 x/bank/CHANGELOG.md          | 1 +
 x/bank/README.md             | 2 +-
 x/bank/keeper/keeper_test.go | 7 ++-----
 x/bank/keeper/send.go        | 4 ++--
 4 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/x/bank/CHANGELOG.md b/x/bank/CHANGELOG.md
index 9d5a917185..22f1637fbf 100644
--- a/x/bank/CHANGELOG.md
+++ b/x/bank/CHANGELOG.md
@@ -33,6 +33,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 ### Improvements
 
 * [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `SendCoinsFromModuleToAccount`, `SendCoinsFromModuleToModule`, `SendCoinsFromAccountToModule`, `DelegateCoinsFromAccountToModule`, `UndelegateCoinsFromModuleToAccount`, `MintCoins` and `BurnCoins` methods now returns an error instead of panicking if any module accounts does not exist or unauthorized.
+* [#20517](https://github.com/cosmos/cosmos-sdk/pull/20517) `SendCoins` now checks for `SendRestrictions` before instead of after deducting coins using `subUnlockedCoins`.
 
 ### API Breaking Changes
 
diff --git a/x/bank/README.md b/x/bank/README.md
index 3bee1e5022..5a660c019d 100644
--- a/x/bank/README.md
+++ b/x/bank/README.md
@@ -275,7 +275,7 @@ Both functions compose the provided restriction with any previously provided res
 `PrependSendRestriction` adds the restriction to be run before any previously provided send restrictions.
 The composition will short-circuit when an error is encountered. I.e. if the first one returns an error, the second is not run.
 
-During `SendCoins`, the send restriction is applied after coins are removed from the from address, but before adding them to the to address.
+During `SendCoins`, the send restriction is applied before coins are removed from the from address and adding them to the to address.
 During `InputOutputCoins`, the send restriction is applied after the input coins are removed and once for each output before the funds are added.
 
 A send restriction function should make use of a custom value in the context to allow bypassing that specific restriction.
diff --git a/x/bank/keeper/keeper_test.go b/x/bank/keeper/keeper_test.go
index 47b75f0291..5820d95422 100644
--- a/x/bank/keeper/keeper_test.go
+++ b/x/bank/keeper/keeper_test.go
@@ -1180,7 +1180,7 @@ func (suite *KeeperTestSuite) TestSendCoinsWithRestrictions() {
 			},
 			expErr: "test restriction error",
 			expBals: expBals{
-				from: sdk.NewCoins(newFooCoin(885), newBarCoin(273)),
+				from: sdk.NewCoins(newFooCoin(985), newBarCoin(473)),
 				to1:  sdk.NewCoins(newFooCoin(15)),
 				to2:  sdk.NewCoins(newBarCoin(27)),
 			},
@@ -1194,12 +1194,9 @@ func (suite *KeeperTestSuite) TestSendCoinsWithRestrictions() {
 			actualRestrictionArgs = nil
 			suite.bankKeeper.SetSendRestriction(tc.fn)
 			ctx := suite.ctx
-			if len(tc.expErr) > 0 {
-				suite.authKeeper.EXPECT().GetAccount(ctx, fromAddr).Return(fromAcc)
-			} else {
+			if len(tc.expErr) == 0 {
 				suite.mockSendCoins(ctx, fromAcc, tc.finalAddr)
 			}
-
 			var err error
 			testFunc := func() {
 				err = suite.bankKeeper.SendCoins(ctx, fromAddr, tc.toAddr, tc.amt)
diff --git a/x/bank/keeper/send.go b/x/bank/keeper/send.go
index 47321da51b..31354c1b85 100644
--- a/x/bank/keeper/send.go
+++ b/x/bank/keeper/send.go
@@ -189,12 +189,12 @@ func (k BaseSendKeeper) SendCoins(ctx context.Context, fromAddr, toAddr sdk.AccA
 	}
 
 	var err error
-	err = k.subUnlockedCoins(ctx, fromAddr, amt)
+	toAddr, err = k.sendRestriction.apply(ctx, fromAddr, toAddr, amt)
 	if err != nil {
 		return err
 	}
 
-	toAddr, err = k.sendRestriction.apply(ctx, fromAddr, toAddr, amt)
+	err = k.subUnlockedCoins(ctx, fromAddr, amt)
 	if err != nil {
 		return err
 	}