WIP: ADR 004 implementation #5539
Conversation
sunnya97
added
WIP
T: API Breaking
sunnya97
requested review from
alessio,
fedekunze,
jackzampolin and
rigelrozanski
as code owners
x/bank/internal/keeper/keeper.go
Outdated
| func (keeper BaseSendKeeper) SetCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) error { | ||
| for _, coin := range amt { |
There was a problem hiding this comment.
Note: this is possibly not equivalent to the old logic, in that it won't set denoms not in amt to zero.
There was a problem hiding this comment.
Ah, you're right, this is different. I think the functionality of this function should be like this (with note in CHANGELOG), and a second function should be added to the keeper ClearAllBalances(addr), which will set all balances to 0.
Then anyone who wants that original functionality would call clear all balances first, then setcoins
There was a problem hiding this comment.
As I understand this semantic differences, I believe requiring the upstream caller to ClearAllBalances(addr) is more error-prone and could cause bugs easily. I would rather see this function clear balances first (internally) before setting balances.
There was a problem hiding this comment.
Why not just call ClearAllBalances(addr) at the beginning of SetCoins?
There was a problem hiding this comment.
That's exactly what I'm saying, hence my TODO.
| @@ -31,9 +31,6 @@ type SimGenesisAccount struct { | |||
| // Validate checks for errors on the vesting and module account parameters | |||
| func (sga SimGenesisAccount) Validate() error { | |||
| if !sga.OriginalVesting.IsZero() { | |||
| if sga.OriginalVesting.IsAnyGT(sga.Coins) { | |||
There was a problem hiding this comment.
note to self/reviewer: This check needs to be handled in InitGenesis somewhere...
There was a problem hiding this comment.
Do you mean like in this function? https://github.com/cosmos/cosmos-sdk/pull/5539/files#diff-12a2c7e5c552684069ea8dd5104cb0e8R11
| if !fees.IsValid() { | ||
| return sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "invalid fee amount: %s", fees) | ||
| } | ||
|
|
||
| // verify the account has enough funds to pay for fees |
There was a problem hiding this comment.
note to self/reviewer: We will probably benefit from the vesting sub-pkg to have it's own DeductFees ante decorator. For standard accounts, these changes are fine as SendCoinsFromAccountToModule will handle accounting errors.
| if (baseAccount.Coins.IsZero() && !originalVesting.IsZero()) || originalVesting.IsAnyGT(baseAccount.Coins) { | ||
| return &BaseVestingAccount{}, errors.New("vesting amount cannot be greater than total amount") | ||
| } | ||
| // TODO: Move this check to bank |
There was a problem hiding this comment.
note to self/review: Handle this upstream.
|
Closing this PR as the changes will be too much to review efficiently. I'll instead create two PRs, the first with the core changes (minimal files and most prudent to review) and a second PR with the tests and misc. updates. |
closes #5519
Description
For contributor use:
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerFor admin use:
WIP,R4R,docs, etc)