From 5367c93e1c146c1bc72a629b2350f2b94d46b3bb Mon Sep 17 00:00:00 2001
From: Luke <luchenqun@qq.com>
Date: Sun, 17 Dec 2023 21:26:47 +0800
Subject: [PATCH] imp(feegrant): ensure we only execute revokeAllowance if
 there is no error is the grant is to be removed (#18767)

---
 x/feegrant/CHANGELOG.md          | 4 ++++
 x/feegrant/keeper/keeper.go      | 5 +----
 x/feegrant/keeper/keeper_test.go | 8 +++++---
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/x/feegrant/CHANGELOG.md b/x/feegrant/CHANGELOG.md
index 215bf157a1..e48e88ab28 100644
--- a/x/feegrant/CHANGELOG.md
+++ b/x/feegrant/CHANGELOG.md
@@ -35,6 +35,10 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 * [#18047](https://github.com/cosmos/cosmos-sdk/pull/18047) Added a limit of 200 grants pruned per EndBlock and the method PruneAllowances that prunes 75 expired grants on every run.
 
+### Improvements
+
+* [#18767](https://github.com/cosmos/cosmos-sdk/pull/18767) Ensure we only execute revokeAllowance if there is no error is the grant is to be removed.
+
 ### API Breaking Changes
 
 * [#15606](https://github.com/cosmos/cosmos-sdk/pull/15606) `NewKeeper` now takes a `KVStoreService` instead of a `StoreKey` and methods in the `Keeper` now take a `context.Context` instead of a `sdk.Context`.
diff --git a/x/feegrant/keeper/keeper.go b/x/feegrant/keeper/keeper.go
index 6b67205c97..1a2366a4e6 100644
--- a/x/feegrant/keeper/keeper.go
+++ b/x/feegrant/keeper/keeper.go
@@ -250,12 +250,9 @@ func (k Keeper) UseGrantedFees(ctx context.Context, granter, grantee sdk.AccAddr
 	}
 
 	remove, err := grant.Accept(ctx, fee, msgs)
-	if remove {
+	if remove && err == nil {
 		// Ignoring the `revokeFeeAllowance` error, because the user has enough grants to perform this transaction.
 		_ = k.revokeAllowance(ctx, granter, grantee)
-		if err != nil {
-			return err
-		}
 		emitUseGrantEvent(ctx, granterStr, granteeStr)
 
 		return nil
diff --git a/x/feegrant/keeper/keeper_test.go b/x/feegrant/keeper/keeper_test.go
index dc92fe2985..911bd007a7 100644
--- a/x/feegrant/keeper/keeper_test.go
+++ b/x/feegrant/keeper/keeper_test.go
@@ -295,10 +295,12 @@ func (suite *KeeperTestSuite) TestUseGrantedFee() {
 	suite.Error(err)
 	suite.Contains(err.Error(), "fee allowance expired")
 
-	// verify: feegrant is revoked
+	// verify: feegrant is not revoked
+	// Because using the past feegrant will return err, data will be rolled back in actual scenarios.
+	// Only when the feegrant allowance used up in a certain transaction feegrant will revoked success due to err is nil
+	// abci's EndBlocker will remove the expired feegrant.
 	_, err = suite.feegrantKeeper.GetAllowance(ctx, suite.addrs[0], suite.addrs[2])
-	suite.Error(err)
-	suite.Contains(err.Error(), "not found")
+	suite.Require().NoError(err)
 }
 
 func (suite *KeeperTestSuite) TestIterateGrants() {