Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(x/bank): swap sendrestriction check in InputOutputCoins #21976

Merged
merged 5 commits into from
Oct 3, 2024
Merged

refactor(x/bank): swap sendrestriction check in InputOutputCoins #21976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
323cd78
refactor(x/bank): swap sendrestriction check in InputOutputCoins
julienrbrt Sep 30, 2024
88dbd51
cl
julienrbrt Sep 30, 2024
1ea05fe
Merge branch 'main' into julien/bank
julienrbrt Oct 1, 2024
a0518d0
updates
julienrbrt Oct 2, 2024
a95e85c
Merge branch 'main' into julien/bank
julienrbrt Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions x/bank/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,14 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#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`.
* [#20354](https://github.com/cosmos/cosmos-sdk/pull/20354) Reduce the number of `ValidateDenom` calls in `bank.SendCoins`.
* [#21976](https://github.com/cosmos/cosmos-sdk/pull/21976) Resolve a footgun by swapping send restrictions check in `InputOutputCoins` before coin deduction.

### Bug Fixes

* [#21407](https://github.com/cosmos/cosmos-sdk/pull/21407) Fix handling of negative spendable balances.
* The `SpendableBalances` query now correctly reports spendable balances when one or more denoms are negative (used to report all zeros). Also, this query now looks up only the balances for the requested page.
* The `SpendableCoins` keeper method now returns the positive spendable balances even when one or more denoms have more locked than available (used to return an empty `Coins`).
* The `SpendableCoin` keeper method now returns a zero coin if there's more locked than available (used to return a negative coin).
* The `SpendableBalances` query now correctly reports spendable balances when one or more denoms are negative (used to report all zeros). Also, this query now looks up only the balances for the requested page.
* The `SpendableCoins` keeper method now returns the positive spendable balances even when one or more denoms have more locked than available (used to return an empty `Coins`).
* The `SpendableCoin` keeper method now returns a zero coin if there's more locked than available (used to return a negative coin).

### API Breaking Changes

Expand Down
14 changes: 7 additions & 7 deletions x/bank/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,7 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() {
suite.Require().NoError(err)
fromAcc := authtypes.NewBaseAccountWithAddress(fromAddr)
inputAccs := []sdk.AccountI{fromAcc}
suite.authKeeper.EXPECT().GetAccount(suite.ctx, inputAccs[0].GetAddress()).Return(inputAccs[0]).AnyTimes()
toAddr1 := accAddrs[1]
toAddr1Str, err := suite.authKeeper.AddressCodec().BytesToString(toAddr1)
suite.Require().NoError(err)
Expand Down Expand Up @@ -878,7 +879,7 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() {
},
expErr: "restriction test error",
expBals: expBals{
from: sdk.NewCoins(newFooCoin(959), newBarCoin(412)),
from: sdk.NewCoins(newFooCoin(959), newBarCoin(500)),
to1: sdk.NewCoins(newFooCoin(15)),
to2: sdk.NewCoins(newFooCoin(26)),
},
Expand Down Expand Up @@ -907,7 +908,7 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() {
},
},
expBals: expBals{
from: sdk.NewCoins(newFooCoin(948), newBarCoin(400)),
from: sdk.NewCoins(newFooCoin(948), newBarCoin(488)),
to1: sdk.NewCoins(newFooCoin(26)),
to2: sdk.NewCoins(newFooCoin(26), newBarCoin(12)),
},
Expand Down Expand Up @@ -937,8 +938,8 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() {
},
expErr: "second restriction error",
expBals: expBals{
from: sdk.NewCoins(newFooCoin(904), newBarCoin(400)),
to1: sdk.NewCoins(newFooCoin(38)),
from: sdk.NewCoins(newFooCoin(948), newBarCoin(488)),
to1: sdk.NewCoins(newFooCoin(26)),
to2: sdk.NewCoins(newFooCoin(26), newBarCoin(12)),
},
},
Expand Down Expand Up @@ -966,8 +967,8 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() {
},
},
expBals: expBals{
from: sdk.NewCoins(newFooCoin(904), newBarCoin(365)),
to1: sdk.NewCoins(newFooCoin(38), newBarCoin(25)),
from: sdk.NewCoins(newFooCoin(948), newBarCoin(453)),
to1: sdk.NewCoins(newFooCoin(26), newBarCoin(25)),
to2: sdk.NewCoins(newFooCoin(26), newBarCoin(22)),
},
},
Expand All @@ -980,7 +981,6 @@ func (suite *KeeperTestSuite) TestInputOutputCoinsWithRestrictions() {
actualRestrictionArgs = nil
suite.bankKeeper.SetSendRestriction(tc.fn)
ctx := suite.ctx
suite.mockInputOutputCoins(inputAccs, tc.outputAddrs)
input := banktypes.Input{
Address: fromStrAddr,
Coins: tc.inputCoins,
Expand Down
29 changes: 21 additions & 8 deletions x/bank/keeper/send.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,14 +148,15 @@ func (k BaseSendKeeper) InputOutputCoins(ctx context.Context, input types.Input,
return err
}

err = k.subUnlockedCoins(ctx, inAddress, input.Coins)
if err != nil {
return err
// ensure all coins can be sent
type toSend struct {
AddressStr string
Address []byte
Coins sdk.Coins
}

var outAddress sdk.AccAddress
sending := make([]toSend, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimize 'sending' slice allocation

To improve performance by reducing memory allocations, consider preallocating the sending slice with the capacity of len(outputs).

You can update the code as follows:

-sending := make([]toSend, 0)
+sending := make([]toSend, 0, len(outputs))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
sending := make([]toSend, 0)
sending := make([]toSend, 0, len(outputs))

for _, out := range outputs {
outAddress, err = k.ak.AddressCodec().StringToBytes(out.Address)
outAddress, err := k.ak.AddressCodec().StringToBytes(out.Address)
if err != nil {
return err
}
Expand All @@ -165,13 +166,25 @@ func (k BaseSendKeeper) InputOutputCoins(ctx context.Context, input types.Input,
return err
}

if err := k.addCoins(ctx, outAddress, out.Coins); err != nil {
sending = append(sending, toSend{
Address: outAddress,
AddressStr: out.Address,
Coins: out.Coins,
})
}

if err := k.subUnlockedCoins(ctx, inAddress, input.Coins); err != nil {
return err
}
Comment on lines +176 to +178
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure transactional integrity when subtracting coins

By moving subUnlockedCoins after processing outputs, there is a potential risk that if an error occurs during addCoins, the sender's coins have already been subtracted, but not all recipients have received their coins. Ensure that the entire operation is atomic and that any errors during addCoins will result in a full rollback to maintain balance consistency.


for _, out := range sending {
if err := k.addCoins(ctx, out.Address, out.Coins); err != nil {
return err
}

if err := k.EventService.EventManager(ctx).EmitKV(
types.EventTypeTransfer,
event.NewAttribute(types.AttributeKeyRecipient, out.Address),
event.NewAttribute(types.AttributeKeyRecipient, out.AddressStr),
event.NewAttribute(types.AttributeKeySender, input.Address),
event.NewAttribute(sdk.AttributeKeyAmount, out.Coins.String()),
); err != nil {
Expand Down
4 changes: 2 additions & 2 deletions x/group/keeper/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package keeper
import (
"context"

"cosmossdk.io/x/gov/types"
"cosmossdk.io/x/group"

"github.com/cosmos/cosmos-sdk/telemetry"
)
Expand All @@ -12,7 +12,7 @@ import (
// prunes expired proposals.
func (k Keeper) EndBlocker(ctx context.Context) error {
start := telemetry.Now()
defer telemetry.ModuleMeasureSince(types.ModuleName, start, telemetry.MetricKeyEndBlocker)
defer telemetry.ModuleMeasureSince(group.ModuleName, start, telemetry.MetricKeyEndBlocker)

if err := k.TallyProposalsAtVPEnd(ctx); err != nil {
return err
Expand Down
Loading