-
Notifications
You must be signed in to change notification settings - Fork 0
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
Porting: gov module #7
Conversation
@marcello33 your pull request is missing a changelog! |
@@ -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 |
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.
If not doing anything why not just get rid of this case?
@@ -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. |
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.
nit: should this be a TODO comment ?
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.
Right, pushed with commit 1d959cc, thanks
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. |
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.
Hmm I don't think non-validators can submit/deposit on proposals can they ?
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.
You would need to add an additional check then on gov proposal submission, the proposer would need to be in the active set
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.
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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Pratik Patil <[email protected]> Co-authored-by: Sergio Mena <[email protected]> Co-authored-by: Raneet Debnath <[email protected]>
@@ -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() |
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.
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.
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.
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
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.
Resolved
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 |
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.
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?
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.
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.
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.
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.
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.
👍 resolved
@@ -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) |
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.
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
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.
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
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.
👍 resolved
if err != nil { | ||
panic(err) | ||
} | ||
|
||
return []sdk.Msg{ | ||
banktypes.NewMsgSend(govAcct, addr, sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(1000)))), |
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.
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?
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.
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?
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.
Yes, makes sense to me. Resolved
@@ -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`. |
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.
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
.
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.
Good one. Added panic
s for such cases here
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.
👍 resolved
@@ -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 |
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.
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.
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.
Right, this could help us indirectly. Done here. Had to change it slightly as - in the meantime - configs for gov
changed quite a bit.
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.
Thanks a lot @marcello33, I took a look at the issue when you shared it via slack, and it's looking great.
Resolved
@@ -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? |
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.
ProposalTypeCancelSoftwareUpgrade
depends on ProposalTypeSoftwareUpgrade
semantically, so whatever we do with one, we should do with the other.
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.
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.
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.
The note added resolves my comment, thanks.
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. |
This PR attempts to port the
gov
module implementation fromheimdall
tocosmos-sdk
for theheimdall v2
implementation.The ported code is based on the original PR, which highlights the differences between
heimdall-v1
andcosmos-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 divergentheimdall
implementations, where probably our intervention won't be needed.There may be changes outside the
gov
module, with the only purpose of building the overallcosmos-sdk
project.All tests are fixed for
gov
module, except for one unit test which is skipped, as it depends on the customstaking
module that will be implemented atheimdall v2
app layer.The changes in
gov
modulemight have impacts on other failing tests outsidegov
package. Such tests will be fixed when such modules will be considered for implementation/porting, or at the end of the migration process.