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

fix(bank): fix unhandled error for vesting #13690

Merged
merged 14 commits into from
Nov 3, 2022
Merged

fix(bank): fix unhandled error for vesting #13690

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0d7c57e
fix(bank): fix unhanlded error for vesting
fedekunze Oct 29, 2022
f5f8a40
fix conflicts
fedekunze Oct 29, 2022
2926c1e
changelog
fedekunze Oct 29, 2022
d8f3d85
better error msg
fedekunze Oct 29, 2022
48d622a
Merge branch 'main' into fedekunze/handle-locked-amt
alexanderbez Oct 31, 2022
0a696b0
Apply suggestions from code review
fedekunze Oct 31, 2022
3af9fd6
Merge branch 'main' into fedekunze/handle-locked-amt
julienrbrt Oct 31, 2022
2d910ec
Merge branch 'main' into fedekunze/handle-locked-amt
atheeshp Nov 2, 2022
f944e06
tests: Add test to verify trying to send locked tokens raises an err…
MalteHerrmann Nov 2, 2022
8314b0a
Merge branch 'main' into fedekunze/handle-locked-amt
julienrbrt Nov 2, 2022
f445888
Merge branch 'main' into fedekunze/handle-locked-amt
likhita-809 Nov 3, 2022
9a93321
re-add denom validation
julienrbrt Nov 3, 2022
e0dae11
Merge branch 'main' into fedekunze/handle-locked-amt
julienrbrt Nov 3, 2022
3c075a9
Merge branch 'main' into fedekunze/handle-locked-amt
julienrbrt Nov 3, 2022
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Bug Fixes

* (bank) [#13691](https://github.com/cosmos/cosmos-sdk/issues/13691) Fix unhandled error for vesting account transfers, when total vesting amount exceeds total balance.
* [#13553](https://github.com/cosmos/cosmos-sdk/pull/13553) Ensure all parameter validation for decimal types handles nil decimal values.
* [#13145](https://github.com/cosmos/cosmos-sdk/pull/13145) Fix panic when calling `String()` to a Record struct type.
* [#13116](https://github.com/cosmos/cosmos-sdk/pull/13116) Fix a dead-lock in the `Group-TotalWeight` `x/group` invariant.
Expand Down
2 changes: 1 addition & 1 deletion types/coin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ func (coin Coin) SafeSub(coinB Coin) (Coin, error) {

res := Coin{coin.Denom, coin.Amount.Sub(coinB.Amount)}
if res.IsNegative() {
return Coin{}, fmt.Errorf("negative coin amount")
return Coin{}, fmt.Errorf("negative coin amount: %s", res)
}

return res, nil
Expand Down
24 changes: 18 additions & 6 deletions x/bank/keeper/send.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,18 +232,30 @@ func (k BaseSendKeeper) subUnlockedCoins(ctx sdk.Context, addr sdk.AccAddress, a

for _, coin := range amt {
balance := k.GetBalance(ctx, addr, coin.Denom)
locked := sdk.NewCoin(coin.Denom, lockedCoins.AmountOf(coin.Denom))
spendable := balance.Sub(locked)

_, hasNeg := sdk.Coins{spendable}.SafeSub(coin)
// NOTE: denom and amount validation should occur before transfer
locked := sdk.Coin{
Denom: coin.Denom,
Amount: lockedCoins.AmountOfNoDenomValidation(coin.Denom),
Copy link
Contributor

Choose a reason for hiding this comment

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

What benefit do we have to calling AmountOfNoDenomValidation here over AmountOf? Since validation should occur prior as you pointed out, AmountOf seems practical, no?

Copy link
Collaborator Author

@fedekunze fedekunze Oct 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

NewCoin and AmountOf validate the denom. Here we don't need to because we are already passing a Coins type as an argument. By using AmountOfNoDenomValidation and sdk.Coin{} we're saving two unnecessary denom validations just in this operation

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit against this. The idea is that these APIs exist for a reason, and unless explicitly not needed, e.g. denom validation is explicitly not required, then New* constructors should always be called. This also keeps consistency with the rest of the codebase and makes the PR diff smaller.

Copy link
Member

Choose a reason for hiding this comment

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

I tend to agree on this one, I really like consistency. Unless there's a noticeable performance difference I would go with AmountOf.

Copy link
Member

Choose a reason for hiding this comment

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

Shall we just push to this branch and fix it then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yah!

Copy link
Member

Choose a reason for hiding this comment

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

Reverted!

}

spendable, hasNeg := sdk.Coins{balance}.SafeSub(locked)
if hasNeg {
return sdkerrors.Wrapf(sdkerrors.ErrInsufficientFunds, "%s is smaller than %s", spendable, coin)
return sdkerrors.Wrapf(sdkerrors.ErrInsufficientFunds,
"locked amount exceeds account balance funds: %s > %s", locked, balance)
}

if _, hasNeg := spendable.SafeSub(coin); hasNeg {
return sdkerrors.Wrapf(
sdkerrors.ErrInsufficientFunds,
"spendable balance %s is smaller than %s",
spendable, coin,
)
}

newBalance := balance.Sub(coin)

err := k.setBalance(ctx, addr, newBalance)
if err != nil {
if err := k.setBalance(ctx, addr, newBalance); err != nil {
return err
}
}
Expand Down