feat(x/bank): introduce msg burn to bank keeper #17569
Conversation
github-actions
bot
added
C:x/staking
C:x/gov
C:x/bank
C:x/auth
C:x/authz
C:x/group
labels
julienrbrt
changed the title
x/bank/keeper/msg_server.go
Outdated
| var coins sdk.Coins | ||
| for _, coin := range msg.Amount { | ||
| coins = append(coins, sdk.NewCoin(coin.Denom, coin.Amount)) | ||
| } |
There was a problem hiding this comment.
Why do we need to recreate the coins slice?
There was a problem hiding this comment.
Mainly tried to avoid using gogo annotations. The cast repeated is one of them
There was a problem hiding this comment.
Mainly tried to avoid using gogo annotations
Asking to learn, is this a thing we're generally striving towards? Or is this just preference?
There was a problem hiding this comment.
mainly in prep for protov2/pulsar but we dot really have a plan for how to get there today
x/bank/keeper/keeper.go
Outdated
| acc := k.ak.GetModuleAccount(ctx, moduleName) | ||
| func (k BaseKeeper) BurnCoins(ctx context.Context, address []byte, amounts sdk.Coins) error { | ||
| acc := k.ak.GetAccount(ctx, address) | ||
| fmt.Println("acc", acc) |
There was a problem hiding this comment.
let's make sure we remove this before merging
There was a problem hiding this comment.
Nice work and thank you @tac0turtle! Just a drive-by review, cheers!
x/bank/keeper/msg_server.go
Outdated
| for _, coin := range msg.Amount { | ||
| coins = append(coins, sdk.NewCoin(coin.Denom, coin.Amount)) | ||
| } |
There was a problem hiding this comment.
@tac0turtle I believe there is a method inside Coins.Add which handles deduplication https://pkg.go.dev/github.com/cosmos/cosmos-sdk/types#Coins.Add otherwise currently are we sure that msg.Amount will have unique coins?
| if base, ok := k.Keeper.(BaseKeeper); ok { | ||
| from, err = base.ak.AddressCodec().StringToBytes(msg.FromAddress) | ||
| if err != nil { | ||
| return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid from address: %s", err) | ||
| } | ||
| } else { | ||
| return nil, sdkerrors.ErrInvalidRequest.Wrapf("invalid keeper type: %T", k.Keeper) | ||
| } |
There was a problem hiding this comment.
Given that this code is independent of coins adding and validity, I'd recommend we move it upfront then bring the coins related code below it where it is closer to home.
Description
Closes: #11020
introduce msgBurn to bank module
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change