Porting: bank module #5

Raneet10 · 2024-02-02T06:37:46Z

Description

This PR intends to port the bank module implementation from heimdall to cosmos-sdk v0.50.2.
Please look out for comments prefixed with // TODO HV2 or // HV2, as they indicate open points that need action or changes that impact heimdall within this PR.

github-actions · 2024-02-02T06:38:03Z

@Raneet10 your pull request is missing a changelog!

go.mod

glnro · 2024-02-02T09:00:36Z

x/bank/app_test.go

@@ -99,39 +102,29 @@ type suite struct {
 	BankKeeper         bankkeeper.Keeper
 	AccountKeeper      types.AccountKeeper
 	DistributionKeeper distrkeeper.Keeper
-	App                *runtime.App
+	App                *simapp.SimApp // use simapp instead for tests since depinject is not supported yet for heimdall app initialization


Fair enough, you might even consider just replacing with a minimal interface here for the purposes of testing. In future this test will be refactored out to an e2e directory

What minimal interface are you talking about ?

Instead of declaring that App must implement SimApp (which is the kitchen sink), you might consider creating a lighter weight test app interface. Just a suggestion but if the plan is to revert to runtime.App then nvm.

We haven't come to a decision whether we will be switching to depinject powered app. But I think we can keep simapp for now. If in future if the SDK testing package provides both ways of initializing an app (it should IMHO unless I missed it), then we can straight out swap simapp.

Just make sure that there's not interfaces in runtime.App unrelated to depinject that are missing from Simapp

I'm not sure I get that. What would be the consequence ?

glnro · 2024-02-09T16:53:53Z

x/bank/app_test.go

@@ -191,7 +229,7 @@ func TestMsgMultiSendWithAccounts(t *testing.T) {
 	baseApp := s.App.BaseApp
 	ctx := baseApp.NewContext(false)

-	require.NoError(t, testutil.FundAccount(ctx, s.BankKeeper, addr1, sdk.NewCoins(sdk.NewInt64Coin("foocoin", 67))))
+	require.NoError(t, testutil.FundAccount(ctx, s.BankKeeper, addr1, sdk.NewCoins(sdk.NewInt64Coin("matic", 68*feeAmount))))


This doesn't seem to make sense...why change this from 67 to 68? Additionally why is this an in subsequent tests the original amount multiplied by the fee amount instead of just incrementing?

This is because we deduct a certain amount of fee in MATIC (10^15 MATIC) in the antehandler while processing txs (see here). I tweaked the numbers to fit the tests.

I also had to tweak some number in auth module tests. What I don't get here tho is why 68 would work and 67 (only one unit less) would not, since feeAmount is granted

It is to match the numbers in expectedBalances in the make a valid tx test case. We could have also tweaked expectedBalances.

LGTM, will leave to @glnro for eventual resolution

I would make sure to put a note here as to why there's missing tokens, or at least in the test verify that those fee tokens went to the appropriate destination. Looks like 1matic is missing after the multisend, it would be clearer to check the feecollector balance has increased or something (as I assume its the fee collector this has been sent to)

That's a good point. I'll take a look.

Updated the tests 46dd21b.

However, as previously reported, there are some inconsistencies in fee_collector's account balance. This is primarily due to the following:

x/mint's begin blocker does some calculations on how many coins to mint (based on the supply, inflation, etc.) and sends it to the fee_collector .

x/distribution's begin blocker transfers all the accumulated fees to itself and then does some other processing related to rewards calculation etc.

I have tinkered the tests keeping these in mind. But the better alternative here it seems is to make simapp to resemble heimdall as closely as possible. We have created a JIRA ticket to track this internally.
Let me know what you think @glnro.

x/bank/keeper/keeper.go

x/auth/migrations/v2/store_test.go

x/bank/README.md

x/bank/keeper/keeper.go

x/bank/types/codec.go

x/bank/types/params.go

x/bank/types/querier.go

…,add MsgMultiSend to amino registry + rm unused commented code

x/bank/keeper/keeper.go

x/bank/app_test.go

x/bank/types/codec.go

x/bank/keeper/keeper.go

proto/cosmos/bank/v1beta1/tx.proto

go.mod

x/bank/app_test.go

x/bank/types/msgs_test.go

x/bank/types/params.go

x/bank/types/vesting.go

x/bank/module.go

marcello33 · 2024-03-06T20:33:59Z

Considering the merge of bank and supply modules, is there anything we need to import from here?

Raneet10 · 2024-03-07T10:37:55Z

Considering the merge of bank and supply modules, is there anything we need to import from here?

Nothing I felt was specific to heimdall as such; the code seemed to have been directly ported into heimdall v1.

marcello33 · 2024-03-07T10:42:40Z

I tried to run go mod tidy in the root folder on this branch and I get several changes which are not staged for commit

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   x/circuit/go.mod
        modified:   x/circuit/go.sum
        modified:   x/evidence/go.mod
        modified:   x/evidence/go.sum
        modified:   x/feegrant/go.mod
        modified:   x/feegrant/go.sum
        modified:   x/nft/go.mod
        modified:   x/nft/go.sum
        modified:   x/upgrade/go.mod
        modified:   x/upgrade/go.sum

Also, when running bash scripts/go-mod-tidy-all.sh, I get an unknown revision version on simapp

go: downloading cosmossdk.io/simapp v0.0.0-00010101000000-000000000000
go: cosmossdk.io/x/feegrant/simulation tested by
        cosmossdk.io/x/feegrant/simulation.test imports
        github.com/cosmos/cosmos-sdk/x/bank tested by
        github.com/cosmos/cosmos-sdk/x/bank.test imports
        cosmossdk.io/simapp: cosmossdk.io/simapp@v0.0.0-00010101000000-000000000000: invalid version: unknown revision 000000000000

Can we please make sure all the changes are committed (maybe you just need to pull auth branch once more) and that we get no errors while running such scripts?
Thanks

… validator set in tests

glnro · 2024-03-07T11:39:10Z

I'm also getting some compile errors when running tests...unfortunately still some remaining references to functions that were removed. Admittedly they are in irrelevant modules for heimdall but you still will need to address in order to get the entire project to compile.

Issues are in:
/x/circuit -> we call authtypes.NewModuleAddressOrBech32Address()
/x/evidence -> sdk.AccAddressFromBech32()
/x/upgrade -> authtypes.NewModuleAddressOrBech32Address()

marcello33 · 2024-03-07T11:51:53Z

I'm also getting some compile errors when running tests...unfortunately still some remaining references to functions that were removed. Admittedly they are in irrelevant modules for heimdall but you still will need to address in order to get the entire project to compile.

Issues are in: /x/circuit -> we call authtypes.NewModuleAddressOrBech32Address() /x/evidence -> sdk.AccAddressFromBech32() /x/upgrade -> authtypes.NewModuleAddressOrBech32Address()

@glnro I solved them in auth porting module branch, which is the target of this PR
I believe these errors will just get solved when @Raneet10 pulls the latest changes from mardizzone/POS-1956-auth branch

Raneet10 · 2024-03-08T04:11:53Z

I'm also getting some compile errors when running tests...unfortunately still some remaining references to functions that were removed. Admittedly they are in irrelevant modules for heimdall but you still will need to address in order to get the entire project to compile.
Issues are in: /x/circuit -> we call authtypes.NewModuleAddressOrBech32Address() /x/evidence -> sdk.AccAddressFromBech32() /x/upgrade -> authtypes.NewModuleAddressOrBech32Address()

@glnro I solved them in auth porting module branch, which is the target of this PR I believe these errors will just get solved when @Raneet10 pulls the latest changes from mardizzone/POS-1956-auth branch

This should fix it.

marcello33 · 2024-03-15T10:44:12Z

Since this PR is raised against mardizzone/POS-1956-auth I'd suggest you @Raneet10 to merge back that branch into yours before merging this PR. It will solve the conflicts. Also, you would be able to run the x/bank tests again (adjusting them in case or errors). I did the same for the gov PR

…d account balances

sergio-mena · 2024-03-27T16:39:25Z

x/bank/client/cli/tx.go

+			fromAddr := clientCtx.GetFromAddress()
+
+			msg := types.NewMsgSend(fromAddr, toAddr, coins)


Do we really need this hunk?

Updated 058e602.

sergio-mena · 2024-03-27T16:39:36Z

x/bank/client/cli/tx.go

@@ -75,6 +77,7 @@ When using '--dry-run' a key name cannot be used, only a bech32 address.
 	flags.AddTxFlagsToCmd(cmd)

 	return cmd
+


Suggested change

Done 058e602.

sergio-mena · 2024-03-27T16:50:20Z

x/bank/module.go

@@ -81,6 +81,7 @@ func (AppModuleBasic) RegisterGRPCGatewayRoutes(clientCtx client.Context, mux *g
 	if err := types.RegisterQueryHandlerClient(context.Background(), mux, types.NewQueryClient(clientCtx)); err != nil {
 		panic(err)
 	}
+


Suggested change

Done 058e602.

greg-szabo · 2024-04-03T21:57:33Z

proto/cosmos/bank/v1beta1/tx.proto

@@ -101,6 +101,7 @@ message MsgUpdateParamsResponse {}
 // message are left unchanged.
 //
 // Since: cosmos-sdk 0.47
+// HV2: Not used in Heimdall since multiple denoms are not supported


I wonder if deleting this message type and regenerating the protobuf types would make sense. It would reduce the code's footprint, but it would also introduce a bunch of deletions of unused code.

It depends on how important it is to keep the code as close to the vanilla SDK as possible.

It depends on how important it is to keep the code as close to the vanilla SDK as possible.

Exactly.
We want to keep the fork as close to upstream as possible to reduce the number of conflicts during merges. Besides, I faintly remember Lauren mentioning this msg type will be removed in an upcoming upstream SDK version but I could be wrong.

* POS-1956: solved conflicts * POS-1956: refactor ante decorators based on heimdall * POS-1956: remove comment * POS-1956: revert some minor changes * chg: rename todo comments * chg: comments * Addressed a few TODOs in auth (#2) * resolved todos in keeper.go (except 1) * updated x/auth/keeper/grpc_query.go with comments * added comments in auth/module.go * removed depinject from auth.ModuleInputs * resolved TODOs in auth/keeper/genesis (created NewBaseAccount and passed to processor) * removed some TODOs from x/auth/types/account.go * added few comments in x/auth/ante/ante.go * few final comments and modifications * add: generate proto files * chg: solve some TODOs * chg: solve some TODOs / restore multiSign as this won't be anyway used * chg: solve some TODOs / restore original AccAddress due to new strategy * chg: solve some TODOs / fix some tests / continue revert on AccAddress * chg: fix on signers loop * chg: fix on signers loop / 2 * chg: fix some tests and skip others * chg: regress to AccAddress for keyring record * chg: implement Addresses as hex entities / fix build and tests (for auth) * chg: fix build for other modules * chg: fix build for simapp / workaround for evidence module * chg: address more TODOs / use empty default prefix / fix build for prefix changes * chg: minor changes * chg: modify auth README * chg: remove bech32 comments / fix address codec * chg: improve comments * chg: rename hex codecs / add 0x prefix on address strings / fix tests accordingly * chg: add emptyness check on string method for address / restore original txBuilder tests * chg: more meaningful comments for the upcoming PR reviews * chg: remove bech22-related code / remove hex prefix / replace feeCollector with bankKeeper / address PR comments * chg: address PR comments / temp comment out scheduled CI jobs * chg: delete pulp related files / fix typo * chg: solve some TODOs / fix some tests and provide context for skipped tests * chg: remove SetGasMeter call and fix mempool test accordingly * chg: remove StdTxRaw * chg: address PR comments * chg: unskip other tests / distinguish TODO HV2 comments from HV2 comments * chg: fix VerifyAddressFormat / move fees reletad vars / remove ante handlers in simapp / adapt tests * chg: skip AccountProcessors for the time being in depInject / Re-enable tests accordingly * chg: increase test balances to pay for the DefaultMaxFee in heimdall / re-enable TestAccountRetriever test * chg: update a comment * chg: remove AccountProcessors until implemented at the end of migration * chg: add a TODO comment for HV2 sigVerify * fix typo in client/keys/show.go Co-authored-by: Sergio Mena <sergio@informal.systems> * chg: better comment for a TODO case * chg: better comment on TODO action * chg: add comment for credentials tests * chg: simplify txFeesSum for keeper_test * chg: fix comment on AddressBytesToString method * chg: fix comment on AddressBytesToString method /2 * chg: disable multisig * chg: implement cometBFT changes and adapt tests accordingly * chg: address PR comments * chg: better context for HV2 TODOs * chg: address comments to remove PublicKey_Secp256K1 cases * chg: updated cometBFT version to first 0xPolygon release * chg: address PR comments * chg: address PR comments * chg: go mod tidy all * chg: merge auth * chg: go mod tidy * chg: fix tests * chg: fix TestAminoUnmarshalJSON * chg: add DefaultFeeInMatic concept in auth README * chg: address comments * chg: better comment on panic * chg: remove vesting accounts from RandomGenesisAccounts during simulation * chg: update go deps for sdk and simapp * chg: removed comment for HFs * chg: remove heimdall types * chg: fix keyring tests * Porting: gov module (#7) * add: port gov module * chg: implement WeightedVoteOptions with constraints * chg: better TODOs descriptions * chg: POS-2135: fix some tests * chg: POS-2135: fix more tests * chg: POS-2135: update an address format * chg: fix few more tests * chg: fix few more tests * chg: fix some tests / temp revert some others to properly tune params later on * chg: fix TestHooks * chg: fix burn related methods / fix tests * chg: fix query for WeightedVoteOptions / better comments * chg: fix all tests in gov module * chg: fix a staking integration test * chg: POS-2142: edit gov readme * chg: use AccAddressFromHex in tests instead of addressCodec * chg: enable one test / provide better context for the only skipped test * chg: use hex acc addresses in gov tests * chg: return empty string on ProposalType normalization * chg: remove TextProposals / add comment for Msgs auto-execution * chg: re-enable textProposals / TBD with team * chg: remove comment * chg: better context for HV2 TODOs * chg: filter out non valid proposals msgs types and content * chg: fix typeUrls * chg: filter out not supported messages at time of proposals submit / fix tests accordingly * chg: go mod tidy * chg: address PR comments: filtering dedicated file / test msg types / edit comments * chg: register interfaces in gov test app * chg: better context for comments * chg: comment for future improvements * chg: consistent example of gov tx for submit proposal * chg: add msgServers in testApp to allow additional MsgUpdateParams types * chg: fix tests after merge * chg: update go deps for sdk and simapp * chg: comment * chg: comment in README for further actions --------- Co-authored-by: Raneet Debnath <raneetdebnath10@gmail.com> * Porting: bank module (#5) * proto,x/bank: initial port of heimdall related changes for bank module * x/bank: rm moduleName param from SubtractCoins + rm MsgSetSendEnabled,add MsgMultiSend to amino registry + rm unused commented code * simapp,x/auth,x/bank: address PR comments * x/bank: change feeAmount to defaultFeeAmount in test * x/bank: address PR comments * x/bank: use CreateRandomValidatorSet instead of manually initialising validator set in tests * x/bank,proto: change heimdall v2 comment format * all: fix broken simapp dep * x/bank: rm SetCoins * x/auth,x/bank: modify bank tests with assertions for fee_collector and account balances * x/bank: minor code refactors --------- Co-authored-by: Raneet Debnath <raneetdebnath10@gmail.com> Co-authored-by: Pratik Patil <pratikspatil024@gmail.com> Co-authored-by: Sergio Mena <sergio@informal.systems> Co-authored-by: Raneet Debnath <35629432+Raneet10@users.noreply.github.com>

sergio-mena · 2024-04-12T08:32:56Z

x/bank/keeper/keeper.go

@@ -378,6 +398,8 @@ func (k BaseKeeper) MintCoins(ctx context.Context, moduleName string, amounts sd
 	return nil
 }

+// HV2: not used in heimdall
+


Comment out and return error as done in the functions above?

sergio-mena · 2024-04-12T08:36:33Z

As requested, I did one last pass on this PR (after being merged). No concerns from my part, just one minor comment. Regarding previously open convos, I don't see anything left to do: you can resolve all of them AFAWAC.

Raneet10 requested review from glnro, sergio-mena, marcello33, pratikspatil024 and VAIBHAVJINDAL3012 February 2, 2024 07:02

glnro reviewed Feb 2, 2024

View reviewed changes

go.mod Show resolved Hide resolved

glnro reviewed Feb 2, 2024

View reviewed changes

proto,x/bank: initial port of heimdall related changes for bank module

6be925f

Raneet10 force-pushed the raneet10/pos-1970 branch from 3f15cce to 6be925f Compare February 5, 2024 06:37