[Bank] Remove the unsafe balance changing API #8473

jgimeno · 2021-01-29T10:09:05Z

Description

This PR removes all the usage of unsafe balance changing methods from the bank keeper exposed API and all the sdk tests relying on it.

A new function was added in simapp, called FundAccount which can be used to fund an account which requires balance for tests.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2021-01-29T10:27:28Z

Codecov Report

Merging #8473 (f4bff7c) into master (d56de85) will decrease coverage by 0.05%.
The diff coverage is 32.46%.

@@            Coverage Diff             @@
##           master    #8473      +/-   ##
==========================================
- Coverage   61.47%   61.42%   -0.06%     
==========================================
  Files         658      658              
  Lines       37588    37627      +39     
==========================================
+ Hits        23109    23111       +2     
- Misses      12067    12104      +37     
  Partials     2412     2412

Impacted Files	Coverage Δ
simapp/state.go	`0.00% <0.00%> (ø)`
simapp/test_helpers.go	`0.49% <0.00%> (-0.01%)`	⬇️
x/distribution/keeper/genesis.go	`9.32% <0.00%> (ø)`
x/bank/keeper/genesis.go	`68.42% <50.00%> (ø)`
x/gov/genesis.go	`87.80% <50.00%> (+0.30%)`	⬆️
x/bank/keeper/send.go	`73.63% <85.71%> (-10.91%)`	⬇️
x/bank/keeper/keeper.go	`72.50% <100.00%> (-2.50%)`	⬇️
x/staking/genesis.go	`60.62% <100.00%> (+5.42%)`	⬆️
... and 2 more

* init supply in a different way * remove external usage of set supply * change(staking): replace SetSupply with MintCoins in tests * change(evidence): replace SetSupply with MintCoins in tests * change(crisis): remove SetSupply in tests * change(bank): remove set supply from genesis tests * change(bank): remove set supply from keeper tests * change(bank): remove remaining set supply usage from keeper tests * change(bank): remove set supply usage from grpc query and querier tests * change(bank): remove SetSupply from keeper interface Co-authored-by: Frojdi Dymylja <[email protected]>

…mos-sdk into jonathan-frojdi/refactor-bank

) * change(distribution): remove SetBalances usage from keeper tests * add(simapp): FundAccount utility function * chore(staking): use FundAccount in keeper tests * change(staking): remove usage of SetBalance in allocation tests * change(staking): remove usage of SetBalance in delegation tests * change(staking): remove usage of SetBalance in proposal handler tests * change(staking): remove usage of SetBalances in grpc query tests * change(staking): remove usage of SetBalances in operations tests * change(distribution): remove usage of SetBalances in genesis * change(authz): remove usage of SetBalances keeper and operations test * fix(authz): TestKeeperFees failing test * change(slashing): remove SetBalances from expected BankKeeper * change(slashing): remove usage of SetBalances in tests * change(distribution): remove SetBalances from expected BankKeeper * change(genutil): remove usage of SetBalances from tests * change(gov): remove SetBalances from expected BankKeeper * change(gov): remove usage of SetBalances from tests * change(staking): remove usage of SetBalances from slash tests * change(staking): remove SetBalances from expected BankKeeper * change(staking): remove usage of SetBalances from delegation tests * change(staking): remove usage of SetBalances from operations tests * change(staking): remove usage of SetBalances from validator tests * change(bank): remove usage of SetBalances from app tests * change(bank): remove usage of SetBalances from bench tests * change(bank): remove usage of SetBalances from querier tests * change(bank): remove usage of SetBalances from grpc query tests * change(bank): remove usage of SetBalances from operations tests * change(bank): partially remove usage of SetBalances from keeper tests * change(bank): finalize removal of usage of SetBalances from keeper tests * change(auth): remove usage of SetBalances from verify tests * change(auth): partially remove usage of SetBalances from tests * [Bank refactor]: finalize removal of setbalances from auth (#8527) * add tests with is check tx * temp commit * fix test * fix other test and remove setbalances * change(auth): remove usage of SetBalances is vesting tests Co-authored-by: Jonathan Gimeno <[email protected]> * change(types): remove usage of SetBalances in queries * fix(types): pagination tests * [Bank refactor] fix pagination tests (#8550) * fix tests * lint * change(bank): remove SetBalances from keeper public API Co-authored-by: Jonathan Gimeno <[email protected]> Co-authored-by: SaReN <[email protected]>

…rojdi/refactor-bank

fdymylja · 2021-02-16T13:07:30Z

I prefer if you feel, that we can open then an issue with explicit description on what is the problem with simapp dependencies splitted around and solve it completely, but leave this branch as it was, a branch just to remove the unsafe methods just to avoid that it ends just being a half made task.

I do agree on this point, I think this PR has showed some problems in the structure of simapp and its usage, I think the correct approach would be to open an issue and highlight those problems and eventually solve them, instead of creating a new package which might or might not be used or ignored in the future. I'm not even sure creating a new package is the correct solution of this problem.

I think it would probably be worth to open a discussion on the structure and role of simapp in the sdk.

But I don't think this PR is the correct place to start tackling this problem. I think it's better if we introduce this bad change which keeps thing consistent, rather than introducing a possible solution which adds confusion and might get discarded in the future.

My philosophy is that "it's better to be consistent in doing thins in a bad way than being inconsistent in doing them in a good way"

alessio · 2021-02-16T13:15:07Z

Please @robert-zaremba unblock this PR. More work will ensue in separate PRs.

robert-zaremba · 2021-02-16T14:27:59Z

But I don't think this PR is the correct place to start tackling this problem. I think it's better if we introduce this bad change which keeps thing consistent,

I don't agree on that. Creating a new package is simple thing, and I'm only concerned here for the new code. I'm worried that this excuse will continue and and we will pollute more and more this and other similar packages.
That being said, I don't want to block this PR. I want that we will really limit test related add ons in production packages (and I consider simpapp production because people are just copying it in their production code).

robert-zaremba · 2021-02-16T14:34:17Z

I created an issue for refactoring simapp. I have a feeling it won't be done any time soon and that's why I want to urge to stop polluting simapp and increasing future breaking changes -> ref x/auth ;)

#8597

robert-zaremba

Unblocking. At minimum I would like to change FundAccount arguments - the function doesn't need to depend on the App. It only needs BankKeeper as an argument.

My other general comments adding stuff to simapp package are here: https://github.com/cosmos/cosmos-sdk/pull/8473/files#issuecomment-779871480

PaddyMc

👍

PaddyMc · 2021-02-16T15:44:20Z

x/genutil/gentx_test.go

-		suite.ctx, addr, sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 25)},
-	)
-	suite.Require().NoError(err)
+	suite.Require().NoError(simapp.FundAccount(suite.app, suite.ctx, addr, sdk.Coins{sdk.NewInt64Coin(sdk.DefaultBondDenom, 25)}))


I feel this should be added to linting rules, developers shouldn't need to think about these things 🤔

fdymylja · 2021-02-16T15:50:36Z

I don't agree on that. Creating a new package is simple thing, and I'm only concerned here for the new code. I'm worried that this excuse will continue and and we will pollute more and more this and other similar packages.
That being said, I don't want to block this PR. I want that we will really limit test related add ons in production packages (and I consider simpapp production because people are just copying it in their production code).

I totally agree with what you're saying, the decision was made in order not to add more confusion, there are a lot of test helpers and packages in the sdk and adding one more or migrating the code somewhere else does not help. We should have a clear idea on the role of each package and the utilities that go inside it, this is why we felt this PR was not the correct place for this dicussion.

albertchon · 2021-05-15T22:44:50Z

At minimum I would like to change FundAccount arguments - the function doesn't need to depend on the App. It only needs BankKeeper as an argument

I highly agree with what @robert-zaremba said. Would definitely make testing easier on my end at least.

fedekunze · 2021-05-18T10:53:26Z

This PR breaks Ethermint as there is no way to set an updated balance to the store, which is required by the vm.StateDB interface. Is there any chance we could introduce AddBalance(ctx sdk.Context, address sdk.AccAddress, coins sdk.Coins) and SubBalance(ctx sdk.Context, address sdk.AccAddress, coins sdk.Coins) that can call these internal functions?

fdymylja · 2021-05-18T11:01:09Z

@fedekunze the AddBalance and SubBalance methods were removed because they bypassed supply checks.

If I may ask, why can't your use case be covered via BurnCoins/MintCoins and MoveCoins (this is at least how we work-around set balance in a safe way)?

fedekunze · 2021-05-18T12:42:26Z

If I may ask, why can't your use case be covered via BurnCoins/MintCoins and MoveCoins (this is at least how we work-around set balance in a safe way)?

Yeah that's possible, but ideally, it should be exposed through the bank keeper instead of having other modules create their own accounting methods

fdymylja · 2021-05-18T13:17:39Z

If I may ask, why can't your use case be covered via BurnCoins/MintCoins and MoveCoins (this is at least how we work-around set balance in a safe way)?

Yeah that's possible, but ideally, it should be exposed through the bank keeper instead of having other modules create their own accounting methods

I'm not sure if I understood what you're suggesting, but if it means to bring back AddBalance/SetBalance/SubBalance then it would mean going back to how it was before. Which would also break rosetta as balance and supply tracking are not guaranteed to work anymore.

If you mean creating methods which make "account funding" easier, then we could bring in to bank the simapp.FundAccount function.

robert-zaremba · 2021-05-18T20:36:43Z

We want to move FundAccount away from simapp: #9346

* temp commit * setbalance now is internal * remove set balances in genesis * feedback test commit * update tests * fix: genesis panic message * fix not bonded pool * fix(staking): genesis test * fix(simapp): rollback state fix change * fix(staking): genesis large val set test * [Bank Refactor] Frojdi jonathan/remove setsupply (cosmos#8491) * init supply in a different way * remove external usage of set supply * change(staking): replace SetSupply with MintCoins in tests * change(evidence): replace SetSupply with MintCoins in tests * change(crisis): remove SetSupply in tests * change(bank): remove set supply from genesis tests * change(bank): remove set supply from keeper tests * change(bank): remove remaining set supply usage from keeper tests * change(bank): remove set supply usage from grpc query and querier tests * change(bank): remove SetSupply from keeper interface Co-authored-by: Frojdi Dymylja <[email protected]> * remove setbalances from genesis in gov * remove keyring * add init genesis state * change(staking): make genesis checks coherent and add tests * remove setbalances on distribution * fix(staking): genesis tests * [Bank Refactor]: Remove SetBalances usage from the code and tests (cosmos#8509) * change(distribution): remove SetBalances usage from keeper tests * add(simapp): FundAccount utility function * chore(staking): use FundAccount in keeper tests * change(staking): remove usage of SetBalance in allocation tests * change(staking): remove usage of SetBalance in delegation tests * change(staking): remove usage of SetBalance in proposal handler tests * change(staking): remove usage of SetBalances in grpc query tests * change(staking): remove usage of SetBalances in operations tests * change(distribution): remove usage of SetBalances in genesis * change(authz): remove usage of SetBalances keeper and operations test * fix(authz): TestKeeperFees failing test * change(slashing): remove SetBalances from expected BankKeeper * change(slashing): remove usage of SetBalances in tests * change(distribution): remove SetBalances from expected BankKeeper * change(genutil): remove usage of SetBalances from tests * change(gov): remove SetBalances from expected BankKeeper * change(gov): remove usage of SetBalances from tests * change(staking): remove usage of SetBalances from slash tests * change(staking): remove SetBalances from expected BankKeeper * change(staking): remove usage of SetBalances from delegation tests * change(staking): remove usage of SetBalances from operations tests * change(staking): remove usage of SetBalances from validator tests * change(bank): remove usage of SetBalances from app tests * change(bank): remove usage of SetBalances from bench tests * change(bank): remove usage of SetBalances from querier tests * change(bank): remove usage of SetBalances from grpc query tests * change(bank): remove usage of SetBalances from operations tests * change(bank): partially remove usage of SetBalances from keeper tests * change(bank): finalize removal of usage of SetBalances from keeper tests * change(auth): remove usage of SetBalances from verify tests * change(auth): partially remove usage of SetBalances from tests * [Bank refactor]: finalize removal of setbalances from auth (cosmos#8527) * add tests with is check tx * temp commit * fix test * fix other test and remove setbalances * change(auth): remove usage of SetBalances is vesting tests Co-authored-by: Jonathan Gimeno <[email protected]> * change(types): remove usage of SetBalances in queries * fix(types): pagination tests * [Bank refactor] fix pagination tests (cosmos#8550) * fix tests * lint * change(bank): remove SetBalances from keeper public API Co-authored-by: Jonathan Gimeno <[email protected]> Co-authored-by: SaReN <[email protected]> * change(bank): remove SubtractCoins from keeper public API * change(ibc/transfer): remove AddCoins from relay tests * change(bank): remove AddCoins from public keeper API * fix imports * remove set balances * fix fee test * remove set balances * fix(staking): remove dependency on minter authorization for staking pools * chore: update CHANGELOG.md * update: x/distribution/keeper/keeper_test.go Co-authored-by: Robert Zaremba <[email protected]> * Update simapp/test_helpers.go Co-authored-by: Robert Zaremba <[email protected]> * Update x/staking/genesis_test.go Co-authored-by: Robert Zaremba <[email protected]> * fix(simapp): FundAccount amount variable name * fix some PR issues Co-authored-by: Frojdi Dymylja <[email protected]> Co-authored-by: Frojdi Dymylja <[email protected]> Co-authored-by: SaReN <[email protected]> Co-authored-by: Alessio Treglia <[email protected]> Co-authored-by: Robert Zaremba <[email protected]>

jgimeno added 3 commits January 28, 2021 17:01

temp commit

af3df53

setbalance now is internal

0763fea

remove set balances in genesis

8216752

jgimeno and others added 25 commits January 29, 2021 11:56

feedback test commit

7fe6aeb

update tests

6a2a36d

fix: genesis panic message

63f4a93

fix not bonded pool

bbc457f

fix(staking): genesis test

3b91703

fix(simapp): rollback state fix change

3a9e784

fix(staking): genesis large val set test

d15ff9e

remove setbalances from genesis in gov

466279a

remove keyring

29f5660

add init genesis state

6ad4503

change(staking): make genesis checks coherent and add tests

1fa3c1e

remove setbalances on distribution

37d7884

fix(staking): genesis tests

612472c

Merge branch 'jonathan-frojdi/refactor-bank' of github.com:cosmos/cos…

c4bd11c

…mos-sdk into jonathan-frojdi/refactor-bank

change(bank): remove SubtractCoins from keeper public API

5e7b1ad

change(ibc/transfer): remove AddCoins from relay tests

58572ad

change(bank): remove AddCoins from public keeper API

c3409ee

Merge branch 'master' into jonathan-frojdi/refactor-bank

d2d930a

fix imports

b0fab44

remove set balances

030bfbe

fix fee test

5b236c6

remove set balances

5fdc895

Merge branch 'master' of github.com:cosmos/cosmos-sdk into jonathan-f…

a7d83b2

…rojdi/refactor-bank

fdymylja changed the title ~~Jonathan frojdi/refactor bank~~ [Bank] Make all balance changes trackable via events Feb 11, 2021

robert-zaremba mentioned this pull request Feb 16, 2021

Remove testing related functions from simapp #8597

Closed

4 tasks

robert-zaremba approved these changes Feb 16, 2021

View reviewed changes

PaddyMc approved these changes Feb 16, 2021

View reviewed changes

jgimeno added 3 commits February 17, 2021 12:30

Merge branch 'master' into jonathan-frojdi/refactor-bank

ece597e

fix some PR issues

1f0e2de

Merge branch 'master' into jonathan-frojdi/refactor-bank

f4bff7c

sahith-narahari approved these changes Feb 17, 2021

View reviewed changes

alessio added the A:automerge Automatically merge PR once all prerequisites pass. label Feb 17, 2021

mergify bot merged commit abb3dfe into master Feb 17, 2021

mergify bot deleted the jonathan-frojdi/refactor-bank branch February 17, 2021 18:20

alessio added the C:x/bank label Feb 18, 2021

zmanian mentioned this pull request Feb 21, 2021

Inspect and fix a slow pattern in InitGenesis of certain modules #7766

Closed

4 tasks

jgimeno linked an issue Feb 22, 2021 that may be closed by this pull request

Support Rosetta API #6831

Closed

7 tasks

blushi mentioned this pull request May 12, 2021

SDK Core Audit - simapp updates #9315

Merged

9 tasks

atheeshp mentioned this pull request May 17, 2021

Fix genesis supply handling (backport #8930) #9252

Closed

blushi mentioned this pull request May 17, 2021

Move and change arguments of simapp FundAccount and FundModuleAccount #9346

Closed

2 tasks

dudong2 mentioned this pull request Mar 28, 2022

remove SetBalance, SetSupply func Finschia/finschia-sdk#477

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bank] Remove the unsafe balance changing API #8473

[Bank] Remove the unsafe balance changing API #8473

jgimeno commented Jan 29, 2021 •

edited by fdymylja

Loading

codecov bot commented Jan 29, 2021 •

edited

Loading

fdymylja commented Feb 16, 2021

alessio commented Feb 16, 2021

robert-zaremba commented Feb 16, 2021

robert-zaremba commented Feb 16, 2021

robert-zaremba left a comment

PaddyMc left a comment

PaddyMc Feb 16, 2021

fdymylja commented Feb 16, 2021 •

edited

Loading

albertchon commented May 15, 2021

fedekunze commented May 18, 2021

fdymylja commented May 18, 2021 •

edited

Loading

fedekunze commented May 18, 2021

fdymylja commented May 18, 2021 •

edited

Loading

robert-zaremba commented May 18, 2021

[Bank] Remove the unsafe balance changing API #8473

[Bank] Remove the unsafe balance changing API #8473

Conversation

jgimeno commented Jan 29, 2021 • edited by fdymylja Loading

Description

codecov bot commented Jan 29, 2021 • edited Loading

Codecov Report

fdymylja commented Feb 16, 2021

alessio commented Feb 16, 2021

robert-zaremba commented Feb 16, 2021

robert-zaremba commented Feb 16, 2021

robert-zaremba left a comment

Choose a reason for hiding this comment

PaddyMc left a comment

Choose a reason for hiding this comment

PaddyMc Feb 16, 2021

Choose a reason for hiding this comment

fdymylja commented Feb 16, 2021 • edited Loading

albertchon commented May 15, 2021

fedekunze commented May 18, 2021

fdymylja commented May 18, 2021 • edited Loading

fedekunze commented May 18, 2021

fdymylja commented May 18, 2021 • edited Loading

robert-zaremba commented May 18, 2021

jgimeno commented Jan 29, 2021 •

edited by fdymylja

Loading

codecov bot commented Jan 29, 2021 •

edited

Loading

fdymylja commented Feb 16, 2021 •

edited

Loading

fdymylja commented May 18, 2021 •

edited

Loading

fdymylja commented May 18, 2021 •

edited

Loading