feat(x/gov): add governors #73

giunatale · 2025-01-14T11:03:43Z

iterating over the work started in #16, this PR adds governors to the x/gov module.

More details can be also read in #16

Only sketching out how the tally would work for now. Lots of stuff is left undefined but the strategy is the one outlined in https://gist.github.com/giunatale/95e9b43f6e265ba32b29e2769f7b8a37?permalink_comment_id=5067400#gistcomment-5067400 but with the restriction of being able to delegate a percentage of its bonded tokens to at most one governor (https://gist.github.com/giunatale/95e9b43f6e265ba32b29e2769f7b8a37?permalink_comment_id=5187246#gistcomment-5187246) It assumes that every time someone redelegates, undelegates or adds to its delegations a hook is called to check if there is a governor delegated to in x/gov, and the governor's total shares updated accordingly. The governor's total shares are a collection of share amounts for different validators.

can only delegate all or nothing to governors now

change also the governors election logic, now only voting governors are counted removes a lot of logic that could cause performance issues

no need to limit governors number, all voting governors are counted

* test: keeper governor * test: ValidateGovernorMinSelfDelegation * test: governance delegation * perf: check IsActive first * chore: remove deprecated maxGovernors

giunatale · 2025-01-14T11:09:18Z

since this work was left untouched for a while, we need to give it a final pass to identify potentially missing tests and check test coverage, and add documentation. Ideally we would also want to do some more in depth profiling/benchmarking to assess the performance impact of this addition.

x/gov/keeper/tally.go

+	for _, govInfo := range allGovernors {
+		governor, _ := k.GetGovernor(ctx, govInfo.Address)
+
+		if k.ValidateGovernorMinSelfDelegation(ctx, governor) && len(govInfo.Vote) > 0 {
+			governorsInfos = append(governorsInfos, govInfo)
+		}
+	}


x/gov/keeper/tally.go

+	for valAddrStr, shares := range governor.ValShares {
+		if val, ok := currValidators[valAddrStr]; ok {
+			sharesAfterDeductions := shares.Sub(governor.ValSharesDeductions[valAddrStr])
+			votingPower = votingPower.Add(sharesAfterDeductions.MulInt(val.GetBondedTokens()).Quo(val.GetDelegatorShares()))
+		}
+	}


tbruyelle · 2025-01-16T08:24:39Z

proto/atomone/gov/v1/query.proto

+
+  // GovernanceDelegation queries a delegation
+  rpc GovernanceDelegation(QueryGovernanceDelegationRequest) returns (QueryGovernanceDelegationResponse) {
+    option (google.api.http).get = "/atomone/gov/v1/delegations/{delegator_address}";


I would rather see singular delegation here, as it will always return a single.

This is for consistency, the plural is used in other places too (e.g. proposals)

Understandable, then the first one /atomone/gov/v1/governor/{governor_address} should be /atomone/gov/v1/governors/{governor_address} ?

but also e.g. validator info for a single validator in x/staking is "validators/{validator-address}"

Understandable, then the first one /atomone/gov/v1/governor/{governor_address} should be /atomone/gov/v1/governors/{governor_address} ?

yes

* add governors adr * Update docs/architecture/adr-004-governors.md Co-authored-by: Thomas Bruyelle <thomas.bruyelle@gmail.com> --------- Co-authored-by: Thomas Bruyelle <thomas.bruyelle@gmail.com>

* add governors documentation * add txs in CLI section

The gov module InitGenesis must happen after staking because governors requires to access the validator delegations.

* perf(x/gov): remove useless bech32 conversion Accessing `delegation.ValidatorAddress` is similar to calling `delegation.GetValidatorAddress().String()` but w/o useless conversions. * perf(x/gov): remove useless address conversion * perf(x/gov): remove useless bech32 conversion * perf(x/gov): remove unused active field from GovernorGovInfo * perf(x/gov): assume vote weight is 1 most of the time

Spotted by `make vulncheck` Last seen in https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:14 ``` Vulnerability atomone-hub#1: GO-2024-3279 ASA-2024-010: cosmossdk.io/math: Mismatched bit-length validation in sdk.Int and sdk.Dec can lead to panic More info: https://pkg.go.dev/vuln/GO-2024-3279 Module: cosmossdk.io/math Found in: cosmossdk.io/math@v1.3.0 Fixed in: cosmossdk.io/math@v1.4.0 Example traces found: Error: atomone-hub#1: app/export.go:209:63: app.AtomOneApp.prepForZeroHeightGenesis calls keeper.Keeper.ApplyAndReturnValidatorSetUpdates, which calls math.Int.Add Error: atomone-hub#2: app/genesis_account.go:31:33: app.SimGenesisAccount.Validate calls types.Coins.IsAnyNil, which eventually calls math.Int.BigInt Error: atomone-hub#3: x/gov/types/v1beta1/gov.pb.go:732:20: v1beta1.TallyResult.Equal calls math.Int.Equal Error: atomone-hub#4: x/gov/keeper/deposit.go:180:97: keeper.Keeper.AddDeposit calls types.Coins.IsAllGTE, which calls math.Int.GT Error: atomone-hub#5: ante/gov_vote_ante.go:46:[14](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:15): ante.GovVoteDecorator.AnteHandle calls types.ChainAnteDecorators, which eventually calls math.Int.GTE Error: atomone-hub#6: app/app.go:254:26: app.AtomOneApp.BeginBlocker calls module.Manager.BeginBlock, which eventually calls math.Int.Int64 Error: atomone-hub#7: app/app.go:254:26: app.AtomOneApp.BeginBlocker calls module.Manager.BeginBlock, which eventually calls math.Int.IsInt64 Error: atomone-hub#8: x/gov/keeper/deposit.go:145:28: keeper.Keeper.AddDeposit calls types.NewCoin, which eventually calls math.Int.IsNegative Error: atomone-hub#9: x/gov/types/v1/msgs.go:101:30: types.MsgSubmitProposal.ValidateBasic calls types.MsgCancelUnbondingDelegation.ValidateBasic, which calls math.Int.IsPositive Error: atomone-hub#10: x/gov/keeper/tally.go:58:23: keeper.Keeper.HasReachedQuorum calls math.Int.IsZero Error: atomone-hub#11: x/gov/keeper/deposit.go:[15](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:16)5:20: keeper.Keeper.AddDeposit calls types.Coin.IsGTE, which calls math.Int.LT Error: atomone-hub#12: x/gov/keeper/deposit.go:63:37: keeper.DeleteAndBurnDeposits calls keeper.BaseKeeper.BurnCoins, which eventually calls math.Int.Marshal Error: atomone-hub#13: x/gov/types/v1beta1/gov.pb.go:994:38: v1beta1.TallyResult.MarshalToSizedBuffer calls math.Int.MarshalTo Error: atomone-hub#14: app/export.go:11:2: app.init calls staking.init, which eventually calls math.Int.Mul Error: atomone-hub#15: x/gov/keeper/deposit.go:[16](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:17)8:54: keeper.Keeper.AddDeposit calls keeper.BaseKeeper.SendCoinsFromAccountToModule, which eventually calls math.Int.Neg Error: atomone-hub#16: app/export.go:209:63: app.AtomOneApp.prepForZeroHeightGenesis calls keeper.Keeper.ApplyAndReturnValidatorSetUpdates, which eventually calls math.Int.Quo Error: #[17](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:18): ante/gov_vote_ante.go:46:14: ante.GovVoteDecorator.AnteHandle calls types.ChainAnteDecorators, which eventually calls math.Int.QuoRaw Error: #[18](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:19): x/gov/types/v1beta1/msgs.go:167:29: v1beta1.MsgDeposit.ValidateBasic calls types.Coins.IsAnyNegative, which eventually calls math.Int.Sign Error: #[19](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:20): x/gov/types/v1beta1/gov.pb.go:1322:16: v1beta1.TallyResult.Size calls math.Int.Size Error: #[20](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:21): x/gov/types/v1/tally.go:12:27: types.NewTallyResult calls math.Int.String Error: #[21](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:22): app/export.go:209:63: app.AtomOneApp.prepForZeroHeightGenesis calls keeper.Keeper.ApplyAndReturnValidatorSetUpdates, which calls math.Int.Sub Error: #[22](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:23): x/gov/keeper/deposit.go:145:73: keeper.Keeper.AddDeposit calls math.Int.ToLegacyDec Error: atomone-hub#23: x/gov/types/v1beta1/gov.pb.go:2141:29: v1beta1.TallyResult.Unmarshal calls math.Int.Unmarshal Error: atomone-hub#24: x/gov/types/v1beta1/msgs.go:263:32: v1beta1.MsgVoteWeighted.ValidateBasic calls math.LegacyDec.Add Error: atomone-hub#25: app/sim/sim_state.go:203:34: sim.AppStateRandomizedFn calls module.SimulationManager.GenerateGenesisStates, which eventually calls math.LegacyDec.BigInt Error: atomone-hub#26: ante/gov_vote_ante.go:46:14: ante.GovVoteDecorator.AnteHandle calls types.ChainAnteDecorators, which eventually calls math.LegacyDec.Ceil Error: atomone-hub#27: x/gov/types/v1beta1/params.go:68:[24](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:25): v1beta1.TallyParams.Equal calls math.LegacyDec.Equal Error: atomone-hub#28: x/gov/client/cli/query.go:624:15: cli.GetCmdQueryProposer calls fmt.Sprintf, which eventually calls math.LegacyDec.Format Error: atomone-hub#29: x/gov/types/v1beta1/msgs.go:270:19: v1beta1.MsgVoteWeighted.ValidateBasic calls math.LegacyDec.GT Error: atomone-hub#30: x/gov/keeper/tally.go:93:26: keeper.Keeper.HasReachedQuorum calls math.LegacyDec.GTE Error: atomone-hub#31: x/gov/types/v1/params.go:129:22: types.Params.ValidateBasic calls math.LegacyDec.IsNegative Error: atomone-hub#32: x/gov/types/v1/msgs.go:101:30: types.MsgSubmitProposal.ValidateBasic calls types.MsgUpdateParams.ValidateBasic, which calls math.LegacyDec.IsNil Error: atomone-hub#33: x/gov/types/v1/params.go:140:26: types.Params.ValidateBasic calls math.LegacyDec.IsPositive Error: atomone-hub#34: x/gov/keeper/deposit.go:138:28: keeper.Keeper.AddDeposit calls math.LegacyDec.IsZero Error: atomone-hub#35: x/gov/types/v1beta1/msgs.go:274:19: v1beta1.MsgVoteWeighted.ValidateBasic calls math.LegacyDec.LT Error: atomone-hub#36: app/export.go:96:53: app.AtomOneApp.prepForZeroHeightGenesis calls keeper.Keeper.WithdrawDelegationRewards, which eventually calls math.LegacyDec.LTE Error: atomone-hub#37: app/app.go:[25](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:26)4:26: app.AtomOneApp.BeginBlocker calls module.Manager.BeginBlock, which eventually calls math.LegacyDec.Marshal Error: atomone-hub#38: x/gov/types/v1beta1/gov.pb.go:1187:41: v1beta1.TallyParams.MarshalToSizedBuffer calls math.LegacyDec.MarshalTo Error: atomone-hub#39: x/gov/keeper/deposit.go:145:79: keeper.Keeper.AddDeposit calls math.LegacyDec.Mul Error: atomone-hub#40: x/gov/keeper/tally.go:1[26](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:27):31: keeper.tallyVotes calls keeper.Keeper.IterateDelegations, which eventually calls math.LegacyDec.MulInt Error: atomone-hub#41: app/export.go:96:53: app.AtomOneApp.prepForZeroHeightGenesis calls keeper.Keeper.WithdrawDelegationRewards, which eventually calls math.LegacyDec.MulInt64 Error: atomone-hub#42: app/app.go:254:26: app.AtomOneApp.BeginBlocker calls module.Manager.BeginBlock, which eventually calls math.LegacyDec.MulTruncate Error: atomone-hub#43: app/export.go:151:60: app.AtomOneApp.prepForZeroHeightGenesis calls keeper.Hooks.BeforeDelegationCreated, which eventually calls math.LegacyDec.Neg Error: atomone-hub#44: x/gov/keeper/tally.go:88:39: keeper.Keeper.HasReachedQuorum calls math.LegacyDec.Quo Error: atomone-hub#45: app/app.go:254:26: app.AtomOneApp.BeginBlocker calls module.Manager.BeginBlock, which eventually calls math.LegacyDec.QuoInt Error: atomone-hub#46: app/app.go:254:26: app.AtomOneApp.BeginBlocker calls module.Manager.BeginBlock, which eventually calls math.LegacyDec.QuoRoundUp Error: atomone-hub#47: app/export.go:96:53: app.AtomOneApp.prepForZeroHeightGenesis calls keeper.Keeper.WithdrawDelegationRewards, which eventually calls math.LegacyDec.QuoTruncate Error: atomone-hub#48: x/gov/keeper/deposit.go:246:112: keeper.Keeper.validateInitialDeposit calls math.LegacyDec.RoundInt Error: atomone-hub#49: x/gov/simulation/genesis.go:84:52: simulation.GenTallyParamsConstitutionalThreshold calls math.LegacyDec.RoundInt64 Error: atomone-hub#50: x/gov/types/v1beta1/gov.pb.go:1392:19: v1beta1.TallyParams.Size calls math.LegacyDec.Size Error: atomone-hub#51: x/gov/keeper/msg_server.go:[27](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:28)4:29: keeper.legacyMsgServer.VoteWeighted calls math.LegacyDec.String Error: atomone-hub#52: x/gov/keeper/tally.go:41:25: keeper.Keeper.Tally calls math.LegacyDec.Sub Error: atomone-hub#53: x/gov/keeper/deposit.go:145:108: keeper.Keeper.AddDeposit calls math.LegacyDec.TruncateInt Error: atomone-hub#54: cmd/atomoned/main.go:16:26: atomoned.main calls cmd.Execute, which eventually calls math.LegacyDec.TruncateInt64 Error: atomone-hub#55: x/gov/types/v1beta1/gov.pb.go:2680:32: v1beta1.TallyParams.Unmarshal calls math.LegacyDec.Unmarshal Error: atomone-hub#56: app/export.go:96:53: app.AtomOneApp.prepForZeroHeightGenesis calls keeper.Keeper.WithdrawDelegationRewards, which eventually calls math.LegacyMinDec Error: atomone-hub#57: x/gov/simulation/genesis.go:73:37: simulation.GenMinDepositRatio calls math.LegacyMustNewDecFromStr Error: atomone-hub#58: x/gov/types/v1beta1/msgs.go:257:34: v1beta1.MsgVoteWeighted.ValidateBasic calls math.LegacyNewDec Error: atomone-hub#59: app/sim/sim_state.go:203:34: sim.AppStateRandomizedFn calls module.SimulationManager.GenerateGenesisStates, which eventually calls math.LegacyNewDecFromBigIntWithPrec Error: atomone-hub#60: x/gov/keeper/tally.go:88:64: keeper.Keeper.HasReachedQuorum calls math.LegacyNewDecFromInt Error: atomone-hub#61: x/gov/keeper/grpc_query.go:406:35: keeper.legacyQueryServer.Params calls math.LegacyNewDecFromStr Error: atomone-hub#62: x/gov/simulation/genesis.go:85:27: simulation.GenTallyParamsConstitutionalThreshold calls math.LegacyNewDecWithPrec Error: atomone-hub#63: x/gov/types/v1/params.go:132:32: types.Params.ValidateBasic calls math.LegacyOneDec Error: atomone-hub#64: app/export.go:96:53: app.AtomOneApp.prepForZeroHeightGenesis calls keeper.Keeper.WithdrawDelegationRewards, which eventually calls math.LegacySmallestDec Error: atomone-hub#65: x/gov/keeper/tally.go:41:77: keeper.Keeper.Tally calls math.LegacyZeroDec Error: atomone-hub#66: app/app.go:254:26: app.AtomOneApp.BeginBlocker calls module.Manager.BeginBlock, which eventually calls math.MaxInt Error: atomone-hub#67: app/app.go:254:26: app.AtomOneApp.BeginBlocker calls module.Manager.BeginBlock, which eventually calls math.MinInt Error: atomone-hub#68: app/helpers/test_helpers.go:79:69: helpers.Setup calls math.NewInt Error: atomone-hub#69: x/gov/keeper/proposal.go:72:24: keeper.Keeper.SubmitProposal calls baseapp.RegisterService, which eventually calls math.NewIntFromBigInt Error: atomone-hub#70: x/gov/migrations/v3/convert.go:73:35: migrations.ConvertToLegacyTallyResult calls math.NewIntFromString Error: atomone-hub#71: x/gov/types/v1beta1/codec.go:7:2: v1beta1.init calls types.init, which calls math.NewIntFromUint64 Error: atomone-hub#72: app/sim/sim_state.go:203:34: sim.AppStateRandomizedFn calls module.SimulationManager.GenerateGenesisStates, which eventually calls math.OneInt Error: atomone-hub#73: x/gov/types/v1beta1/tally.go:55:36: v1beta1.EmptyTallyResult calls math.ZeroInt Error: atomone-hub#74: app/sim/sim_state.go:205:[31](https://github.com/atomone-hub/atomone/actions/runs/12228478419/job/34106805715#step:4:32): sim.AppStateRandomizedFn calls json.Marshal, which eventually calls math.init Error: atomone-hub#75: x/gov/types/v1beta1/genesis.go:6:2: v1beta1.init calls math.init ```

giunatale added 30 commits September 13, 2024 14:07

add protos

8cb99c2

governor object started (copy from Validator)

41b042f

wip

d4a8c03

wip

96abd4c

...

f9ad9d8

...

700703d

missing governor election

f6c3392

not right, cannot iterate on all governors

6951d84

remove percentage and try sorting governors by VP

1a8495c

can only delegate all or nothing to governors now

add msgs

a769118

update test mock keepers

ca19aaf

...

be1db00

compiles

e1a2bbb

add query and cli

851c005

allow governor status updates only once a month

372bca2

genesis

4ea0dad

fix e2e

fedba97

add governors vp invariant

68645f2

iterate over correct values

201b112

format pass

e5c1284

fix missing return

6f37870

cleanup protos

c0cdec9

Merge branch 'main' into governors

80f44c7

fix merge mistake

afa8d07

fix merge mistakes

58d6a83

approx VP computation for quorum with governors

44d71e6

fix error in keeper.GetGovernor

5767cca

protogen

25d017a

add min bonding requirements for governors

671755c

giunatale and others added 18 commits September 25, 2024 13:31

Merge remote-tracking branch 'origin/main' into governors

e0d3865

fix error codes

48f45be

add alternative for tallying governors VP

0659b9d

remove aggregate VP for governors

8418fed

change also the governors election logic, now only voting governors are counted removes a lot of logic that could cause performance issues

don't use nil

7bd0a30

remove maxGovernors param

abe5fc2

no need to limit governors number, all voting governors are counted

tally results updated only once only per voter

8b79c43

...

1f8e5a9

Merge branch 'governors' into governors-no-aggregate-vp

9560c40

Merge remote-tracking branch 'origin/main' into governors

374c315

Merge branch 'governors' into governors-no-aggregate-vp

de5f46a

Merge branch 'main' into governors

90f540c

fmt

c5e5ce4

Merge branch 'governors' into governors-no-aggregate-vp

a0b0650

fix/refac: tally with governors (#2)

7a792a8

test: keeper governor funcs (#3)

8f1448d

* test: keeper governor * test: ValidateGovernorMinSelfDelegation * test: governance delegation * perf: check IsActive first * chore: remove deprecated maxGovernors

Merge branch 'main' into governors-no-aggregate-vp

3d4fc3f

fix merge conflicts

2713498

giunatale requested review from tbruyelle and jaekwon as code owners January 14, 2025 11:03

github-advanced-security bot found potential problems Jan 14, 2025

View reviewed changes

tbruyelle reviewed Jan 16, 2025

View reviewed changes

tbruyelle and others added 6 commits January 16, 2025 09:57

test(x/gov): add e2e tests for governors (#4)

8dee330

fix(x/gov): REST query governor URL

f25a8a4

docs(x/gov): add governors ADR (#5)

539ee0e

* add governors adr * Update docs/architecture/adr-004-governors.md Co-authored-by: Thomas Bruyelle <thomas.bruyelle@gmail.com> --------- Co-authored-by: Thomas Bruyelle <thomas.bruyelle@gmail.com>

docs(x/gov): add governors documentation (#6)

1b03975

* add governors documentation * add txs in CLI section

fix: gov init genesis order (#7)

e48e30b

The gov module InitGenesis must happen after staking because governors requires to access the validator delegations.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(x/gov): add governors #73

feat(x/gov): add governors #73

giunatale commented Jan 14, 2025 •

edited

Loading

giunatale commented Jan 14, 2025

tbruyelle Jan 16, 2025

giunatale Jan 16, 2025

tbruyelle Jan 16, 2025

giunatale Jan 16, 2025

giunatale Jan 16, 2025

feat(x/gov): add governors #73

Are you sure you want to change the base?

feat(x/gov): add governors #73

Conversation

giunatale commented Jan 14, 2025 • edited Loading

giunatale commented Jan 14, 2025

tbruyelle Jan 16, 2025

Choose a reason for hiding this comment

giunatale Jan 16, 2025

Choose a reason for hiding this comment

tbruyelle Jan 16, 2025

Choose a reason for hiding this comment

giunatale Jan 16, 2025

Choose a reason for hiding this comment

giunatale Jan 16, 2025

Choose a reason for hiding this comment

giunatale commented Jan 14, 2025 •

edited

Loading