Modular gov tally #64
Conversation
…os-sdk into modular-gov-tally
x/gov/stakingtally/stakingtally.go
Outdated
| stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper" | ||
| stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" | ||
| ) | ||
|
|
There was a problem hiding this comment.
stakingtally shouldn't be in staking module?
There was a problem hiding this comment.
Yeah, it probably should be actually
There was a problem hiding this comment.
Do we also need to provide additional param to tally router? In case module could customize tally based on that param.
There was a problem hiding this comment.
Gov would have no dependency on staking at all then I think
There was a problem hiding this comment.
It would be nice to reduce dependencies, currently some modules are too related each other.
x/gov/tokenbalance/tokenbalance.go
Outdated
|
|
||
| // Tally iterates over the votes and updates the tally of a proposal based on the voting power of the | ||
| // voters | ||
| func handleTokenBalanceTally(ctx sdk.Context, proposal govtypes.Proposal, gk govkeeper.Keeper, bk govtypes.BankKeeper, denom string) (passes bool, burnDeposits bool, tallyResults govtypes.TallyResult) { |
There was a problem hiding this comment.
Where should we put this if we move staking tally to staking module ?
Maybe bank module? :)
There was a problem hiding this comment.
Hmm, I guess. Is it going to be odd to have a reference to the gov module within the bank module, even if its just in a submodule?
Should we maybe have a separate folder of tallystrategies, and have that filled with submodules?
| // AddRoute adds a governance handler for a given path. It returns the Router | ||
| // so AddRoute calls can be linked. It will panic if the router is sealed. | ||
| func (rtr *tallyrouter) AddRoute(path string, t TallyStrategy) TallyRouter { | ||
| if rtr.sealed { |
There was a problem hiding this comment.
Ah, I actually meant to get rid of that. I got rid of the .Seal() function on this router, because the point is we want other modules to add Routes dynamically.
940d2cf#diff-240cbd96be80c2c1a5bf3dce2b5aa335deabe0e7d737fca0b85032fd015c01c4L33
There was a problem hiding this comment.
You can remove this check and the sealed variable from tally_router
| string description = 2; | ||
| string title = 1; | ||
| string description = 2; | ||
| string tallystrategy = 3; |
There was a problem hiding this comment.
Do we need to make all text proposals to have dynamic tally strategy? Or add some configuration for tally strategy for a specific type of proposal to be restricted to few listings?
TODOs:
TallyParamsto insidestakingtallystakingtallystrategy to insidestakingtally