Porting: gov module #7

marcello33 · 2024-02-22T09:55:16Z

This PR attempts to port the gov module implementation from heimdall to cosmos-sdk for the heimdall v2 implementation.

The ported code is based on the original PR, which highlights the differences between heimdall-v1 and cosmos-sdk v0.37.4.

Please, pay particular attention to the comments starting with prefix // TODO HV2, as they define open points that need action within this PR, or later on (once other modules are implemented).
Comments starting with // HV2 represent actions taken in this PR due to divergent heimdall implementations, where probably our intervention won't be needed.

There may be changes outside the gov module, with the only purpose of building the overall cosmos-sdk project.
All tests are fixed for gov module, except for one unit test which is skipped, as it depends on the custom staking module that will be implemented at heimdall v2 app layer.
The changes in gov modulemight have impacts on other failing tests outside gov package. Such tests will be fixed when such modules will be considered for implementation/porting, or at the end of the migration process.

… later on

github-actions · 2024-02-22T09:55:35Z

@marcello33 your pull request is missing a changelog!

… edit comments

x/gov/README.md

x/gov/abci.go

x/gov/client/cli/tx.go

x/gov/keeper/msg_validator.go

glnro · 2024-03-13T21:32:37Z

x/gov/keeper/deposit.go

@@ -238,11 +240,13 @@ func (keeper Keeper) ChargeDeposit(ctx context.Context, proposalID uint64, destA
 		distributionAddress := keeper.authKeeper.GetModuleAddress(disttypes.ModuleName)
 		switch {
 		case destAddress == "":
+			/* HV2: in heimdall we have no BurnCoins func


If not doing anything why not just get rid of this case?

Raneet10 · 2024-04-02T11:02:28Z

proto/cosmos/upgrade/v1beta1/upgrade.proto

@@ -43,6 +43,10 @@ message Plan {
  google.protobuf.Any upgraded_client_state = 5 [deprecated = true];
 }

+// TODO HV2: this was deleted from heimdall's gov/types/proposal.go.


nit: should this be a TODO comment ?

Right, pushed with commit 1d959cc, thanks

Raneet10 · 2024-04-02T11:37:10Z

x/gov/README.md

-other users do not get the right to participate in governance. However, they
-can submit and deposit on proposals.
+Polygon PoS network, participants are validators. Other holders and users do not get the right to participate in governance. 
+However, they can submit and deposit on proposals.


Hmm I don't think non-validators can submit/deposit on proposals can they ?

You would need to add an additional check then on gov proposal submission, the proposer would need to be in the active set

Ok addressed this with a TODO, will clarify later what to do (based also on discussion with gov team

* 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-11T17:20:15Z

tests/e2e/gov/tx.go

@@ -545,7 +545,8 @@ func (s *E2ETestSuite) TestNewCmdWeightedVote() {
 	for _, tc := range testCases {
 		tc := tc
 		s.Run(tc.name, func() {
-			cmd := cli.NewCmdWeightedVote()
+			// TODO HV2: changed from NewCmdWeightedVote to NewCmdVote. Fix tests accordingly.
+			cmd := cli.NewCmdVote()


Even if weighted voting is not supported in Heimdall, would it nevertheless be simpler to leave this as NewCmdWeightedVote so that we spend less time re-doing the tests? (at the end of the day, this is test code)
Or are you planning on testing something else than what is tested in vanilla SDK v0.50.x?

This would also apply to file x/feegrant/client/cli/tx_test.go below.

Right, I agree. Changed it in the very beginning with the purpose of implementing some tests, but both are actually covered in the vanilla SDK. Reverted here

sergio-mena · 2024-04-11T17:35:35Z

x/gov/README.md

+Polygon PoS network, participants are validators. Other holders and users do not get the right to participate in governance. 
+However, they can submit and deposit on proposals.
+
+// TODO HV2: if we don't want non-validators to submit/deposit on proposals, we would need to add an additional check then on gov proposal submission, to validate the proposer is in the active set


Wasn't that the role of the ValidatorID field that was added in HeimdallV1 to many of the gov messages? (see e.g., definition of struct MsgSubmitProposal here).
Was it a conscious decision not to forward-port those added fields?

Never mind, I see below it was conscious as ValidatorID is redundant with address. And now understand your comment about the "extra check". I totally agree.

Yes, the only impact I could see is in L1 contracts where we use validator IDs iirc. Such contracts should anyway provide an utility function to convert from ID to address and vice-versa. I already took a note to discuss this with smart contracts team.

👍 resolved

sergio-mena · 2024-04-11T18:11:49Z

x/gov/keeper/deposit.go

@@ -45,7 +45,9 @@ func (keeper Keeper) DeleteAndBurnDeposits(ctx context.Context, proposalID uint6
 		return err
 	}

-	return keeper.bankKeeper.BurnCoins(ctx, types.ModuleName, coinsToBurn)
+	// HV2: in heimdall we have no BurnCoins func, returning nil
+	// return keeper.bankKeeper.BurnCoins(ctx, types.ModuleName, coinsToBurn)


Hmmm. Is it correct to delete the deposits but neither burn not refund the coins? Will it cause some general invariant regarding total supply to no longer hold?

I have the impression it would be safer to revert this hunk, and make sure no one ever calls this method:

either panic at the beginning of this method

or comment it out and fix the build errors by commenting its callsites

Yeah logically I agree. However, this function is not invoked anywhere (already removed all the callsites, compared to - for example RefundAndDeleteDeposits which we leverage and it's still used in abci.go
Also, can't re-enable the BurnCoins method as it's been removed from the expected BankKeeper (see here)
However, adding an extra panic at the beginning of the function sounds safe. Done here

👍 resolved

sergio-mena · 2024-04-11T18:21:51Z

x/gov/keeper/common_test.go

 	if err != nil {
 		panic(err)
 	}

 	return []sdk.Msg{
-		banktypes.NewMsgSend(govAcct, addr, sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(1000)))),


As we'll be accepting "update param" messages, wouldn't it make the test more "interesting" if we changed this banktypes.NewMsgSend by an example of "update param" message?

I decided to leave only legacy proposals there to reduce the impact on tests using getTestProposal()
Instead, I tested all the update params types (also some unsupported) here. WDYT?

Yes, makes sense to me. Resolved

sergio-mena · 2024-04-12T07:46:31Z

x/gov/keeper/vote.go

@@ -29,6 +29,15 @@ func (keeper Keeper) AddVote(ctx context.Context, proposalID uint64, voterAddr s
 		return err
 	}

+	// HV2: check on the length of the options. This should never fail as it is only used by `MsgServer.Vote`.


If it should never fail because we control all callsites and none should provide inputs that make this fail, then it's maybe worth panicking instead of returning an error. This comment also applies to other sanity checks you added in tally.go.

Good one. Added panics for such cases here

👍 resolved

sergio-mena · 2024-04-12T07:48:50Z

x/gov/types/config.go

@@ -4,11 +4,14 @@ package types
 type Config struct {
 	// MaxMetadataLen defines the maximum proposal metadata length.
 	MaxMetadataLen uint64
+	// MaxOptionsLen defines the maximum number of options a proposal can have.
+	MaxOptionsLen uint64


Would you be up for opening an issue on Cosmos Hub asking for this?
This seems to me can be implemented in vanilla SDK as more people may want to use it.
If they agree to upstream it, you can get rid of it in your fork in a future version.

Right, this could help us indirectly. Done here. Had to change it slightly as - in the meantime - configs for gov changed quite a bit.

Thanks a lot @marcello33, I took a look at the issue when you shared it via slack, and it's looking great.
Resolved

sergio-mena · 2024-04-12T07:56:09Z

x/upgrade/types/proposal.go

@@ -5,6 +5,9 @@ import (
 )

 const (
+	// TODO HV2: ProposalTypeSoftwareUpgrade was removed. What to do with it?
+	//  ProposalTypeCancelSoftwareUpgrade was not even there in heimdall's gov/types/proposal.go. What to do with it?


ProposalTypeCancelSoftwareUpgrade depends on ProposalTypeSoftwareUpgrade semantically, so whatever we do with one, we should do with the other.

Right, added a note here
This is already filtered out at proposals submission time, but maybe we could even de-register the types going forward.

The note added resolves my comment, thanks.

sergio-mena · 2024-04-12T07:57:34Z

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

Raneet10 and others added 22 commits February 14, 2024 16:02

add: port gov module

20c7bab

Merge branch 'mardizzone/POS-1956-auth' into mardizzone/POS-2129-gov

5fabe14

Merge branch 'mardizzone/POS-1956-auth' into mardizzone/POS-2129-gov

6ae10dc

chg: implement WeightedVoteOptions with constraints

2529319

chg: better TODOs descriptions

e1a6c2f

Merge branch 'mardizzone/POS-1956-auth' into mardizzone/POS-2129-gov

246fce5

Merge branch 'mardizzone/POS-1956-auth' into mardizzone/POS-2129-gov

f159c6d

chg: POS-2135: fix some tests

ae5a857

chg: POS-2135: fix more tests

8d85eb1

chg: POS-2135: update an address format

0dc2578

chg: fix few more tests

f53183e

Merge branch 'mardizzone/POS-1956-auth' into mardizzone/POS-2129-gov

705b5e8

chg: fix few more tests

4010477

chg: fix some tests / temp revert some others to properly tune params…

1b93e32

… later on

chg: fix TestHooks

ae2226d

chg: fix burn related methods / fix tests

ae826fb

chg: fix query for WeightedVoteOptions / better comments

36d79e4

chg: fix all tests in gov module

8ea74f0

chg: fix a staking integration test

89cac52

Merge branch 'mardizzone/POS-1956-auth' into mardizzone/POS-2129-gov

c92a5e0

Merge branch 'mardizzone/POS-1956-auth' into mardizzone/POS-2129-gov

e85fe55

chg: POS-2142: edit gov readme

c793367

marcello33 changed the title ~~Mardizzone/pos 2129 gov~~ Porting: gov module Feb 22, 2024

marcello33 added 6 commits February 22, 2024 11:15

chg: use AccAddressFromHex in tests instead of addressCodec

d4fc81d

Merge branch 'mardizzone/POS-1956-auth' into mardizzone/POS-2129-gov

28f9940

chg: enable one test / provide better context for the only skipped test

756fcf2

chg: use hex acc addresses in gov tests

9b9609a

Merge branch 'mardizzone/POS-1956-auth' into mardizzone/POS-2129-gov

ba0eac3

Merge branch 'mardizzone/POS-1956-auth' into mardizzone/POS-2129-gov

2031f34

marcello33 added 6 commits March 7, 2024 12:12

Merge branch 'mardizzone/POS-1956-auth' into mardizzone/POS-2129-gov

72ea82b

chg: go mod tidy

9f8955c

chg: address PR comments: filtering dedicated file / test msg types /…

39fd8a9

… edit comments

chg: register interfaces in gov test app

0223595

Merge branch 'mardizzone/POS-1956-auth' into mardizzone/POS-2129-gov

21f52a1

chg: better context for comments

a3097b1

VAIBHAVJINDAL3012 reviewed Mar 12, 2024

View reviewed changes

x/gov/README.md Show resolved Hide resolved

x/gov/abci.go Show resolved Hide resolved

x/gov/abci.go Show resolved Hide resolved

x/gov/client/cli/tx.go Outdated Show resolved Hide resolved

x/gov/keeper/msg_validator.go Show resolved Hide resolved

marcello33 added 3 commits March 12, 2024 13:40

chg: comment for future improvements

efc6cf5

chg: consistent example of gov tx for submit proposal

3f551ab

chg: add msgServers in testApp to allow additional MsgUpdateParams types

ec29c6e

glnro reviewed Mar 13, 2024

View reviewed changes

marcello33 added 2 commits March 15, 2024 11:39

Merge branch 'mardizzone/POS-1956-auth' into mardizzone/POS-2129-gov

f72216e

chg: fix tests after merge

268611f

marcello33 mentioned this pull request Mar 15, 2024

Porting: bank module #5

Merged

marcello33 added 2 commits March 15, 2024 11:46

chg: update go deps for sdk and simapp

942d844

Merge branch 'mardizzone/POS-1956-auth' into mardizzone/POS-2129-gov

39dd6b9

Raneet10 reviewed Apr 2, 2024

View reviewed changes

marcello33 added 2 commits April 8, 2024 13:48

chg: comment

1d959cc

chg: comment in README for further actions

e2147a2

marcello33 merged commit 4e252ce into mardizzone/POS-1956-auth Apr 8, 2024
9 of 39 checks passed

sergio-mena reviewed Apr 11, 2024

View reviewed changes

sergio-mena reviewed Apr 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Porting: gov module #7

Porting: gov module #7

marcello33 commented Feb 22, 2024 •

edited

Loading

github-actions bot commented Feb 22, 2024

glnro Mar 13, 2024

Raneet10 Apr 2, 2024

marcello33 Apr 8, 2024 •

edited

Loading

Raneet10 Apr 2, 2024

glnro Apr 2, 2024

marcello33 Apr 8, 2024

sergio-mena Apr 11, 2024

marcello33 Apr 17, 2024

sergio-mena Apr 18, 2024

sergio-mena Apr 11, 2024

sergio-mena Apr 11, 2024

marcello33 Apr 17, 2024

sergio-mena Apr 18, 2024

sergio-mena Apr 11, 2024

marcello33 Apr 17, 2024

sergio-mena Apr 18, 2024

sergio-mena Apr 11, 2024

marcello33 Apr 17, 2024

sergio-mena Apr 18, 2024

sergio-mena Apr 12, 2024

marcello33 Apr 17, 2024

sergio-mena Apr 18, 2024

sergio-mena Apr 12, 2024

marcello33 Apr 17, 2024

sergio-mena Apr 18, 2024

sergio-mena Apr 12, 2024

marcello33 Apr 17, 2024

sergio-mena Apr 18, 2024

sergio-mena commented Apr 12, 2024 •

edited

Loading

Porting: gov module #7

Porting: gov module #7

Conversation

marcello33 commented Feb 22, 2024 • edited Loading

github-actions bot commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcello33 Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergio-mena commented Apr 12, 2024 • edited Loading

marcello33 commented Feb 22, 2024 •

edited

Loading

marcello33 Apr 8, 2024 •

edited

Loading

sergio-mena commented Apr 12, 2024 •

edited

Loading