-
Notifications
You must be signed in to change notification settings - Fork 4.1k
feat!: public setbalance #25771
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.
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
base: feat/krakatoa
Are you sure you want to change the base?
feat!: public setbalance #25771
Conversation
|
@Eric-Warehime your pull request is missing a changelog! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/krakatoa #25771 +/- ##
=================================================
- Coverage 70.40% 70.18% -0.23%
=================================================
Files 835 835
Lines 54513 54513
=================================================
- Hits 38381 38258 -123
- Misses 16132 16255 +123
🚀 New features to boost your workflow:
|
| CreditVirtualAccounts(ctx context.Context) error | ||
| SendCoinsFromVirtual(ctx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) error | ||
| SendCoinsToVirtual(ctx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) error | ||
| SetBalance(ctx context.Context, addr sdk.AccAddress, balance sdk.Coin) error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels like a dangerous method to include in the base interface
i'm (fairly) certain all of these methods here are fool proof, but SetBalance is not. for example, Mint/BurnCoins handles updating the supply for you. SetBalance can be misused and put the state out of whack.
would it make more sense to have an UnsafeBank or SudoBank-esque interface?
What we did before is use |
| // setBalance sets the coin balance for an account by address. | ||
| func (k BaseSendKeeper) setBalance(ctx context.Context, addr sdk.AccAddress, balance sdk.Coin) error { | ||
| // SetBalance sets the coin balance for an account by address. | ||
| func (k BaseSendKeeper) SetBalance(ctx context.Context, addr sdk.AccAddress, balance sdk.Coin) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compare SetBalance with SendCoins or Mint/Burn:
SetBalancedon't check theLockedCoinsof vesting accounts.SetBalancedon't update supply, it assumes the caller will keep the supply unchanged.SetBalancedon't emit events.
Especially the first one, it seems can become a security vulnerability easily if the caller is not careful.
Makes SetBalance public.
This is required in order to be able to properly parallelize gas token sends in cosmos EVM. The existing implementation uses
MintandBurnin order to update the balances of EVM accounts in the corresponding bank token. However, this requires us to hit the module account on ever gas txn.Directly setting the balance instead removes the mint/burn in the bank wrapper and therefore removes the store access to the same module account value on every txn.