Skip to content
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

Add hooks to governance actions #9133

Merged
merged 10 commits into from
Apr 21, 2021
Merged

Add hooks to governance actions #9133

merged 10 commits into from
Apr 21, 2021

Conversation

sunnya97
Copy link
Member

@sunnya97 sunnya97 commented Apr 16, 2021

Description

closes: #9105


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@codecov
Copy link

codecov bot commented Apr 17, 2021

Codecov Report

Merging #9133 (24e5109) into master (603e895) will increase coverage by 0.06%.
The diff coverage is 73.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9133      +/-   ##
==========================================
+ Coverage   58.82%   58.89%   +0.06%     
==========================================
  Files         583      585       +2     
  Lines       32769    32798      +29     
==========================================
+ Hits        19275    19315      +40     
+ Misses      11216    11199      -17     
- Partials     2278     2284       +6     
Impacted Files Coverage Δ
x/gov/types/hooks.go 0.00% <0.00%> (ø)
x/gov/keeper/keeper.go 73.46% <50.00%> (+22.35%) ⬆️
simapp/app.go 83.09% <100.00%> (+0.33%) ⬆️
x/gov/abci.go 89.23% <100.00%> (+0.34%) ⬆️
x/gov/keeper/deposit.go 70.58% <100.00%> (+1.54%) ⬆️
x/gov/keeper/hooks.go 100.00% <100.00%> (ø)
x/gov/keeper/proposal.go 78.64% <100.00%> (+5.11%) ⬆️
x/gov/keeper/vote.go 89.28% <100.00%> (+0.19%) ⬆️
... and 4 more

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, could we get changes to the spec as well?

Comment on lines 58 to 62
AfterProposalSubmission(ctx sdk.Context, proposalID uint64) // Must be called after proposal is submitted
AfterProposalDeposit(ctx sdk.Context, proposalID uint64, depositorAddr sdk.AccAddress) // Must be called after a deposit is made
AfterProposalVote(ctx sdk.Context, proposalID uint64, voterAddr sdk.AccAddress) // Must be called after a vote on a proposal is cast
AfterProposalInactive(ctx sdk.Context, proposalID uint64) // Must be called when proposal become inactive
AfterProposalActive(ctx sdk.Context, proposalID uint64) // Must be called when proposal become active
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would propose to put the struct as 2nd argument everywhere:

AfterProposalVote(ctx sdk.Context, vote Vote)
AfterProposalActive(ctx sdk.Context, proposal Proposal)

etc, so that users can access all fields if needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the hooks in the staking module don't pass around the full structs, and only pass the minimal information needed such that the recipient can query the staking module for more information if desired.

For example, in the new delegation hook, it just passes the validator address and delegator address. The recipient can then use stakingKeeper.GetDelegation(valAddr, delAddr) to get the delegation struct.

I was trying to keep the same standard for these as well, but I'm not sure the pros/cons of doing this vs the entire struct (in which case, we should switch it for staking hooks as well imo).

@aaronc
Copy link
Member

aaronc commented Apr 20, 2021

This generally seems reasonable. I am a bit concerned about merging another feature into master when we are this closing to cutting a 0.43 beta. But maybe it is okay... It seems like this isn't really state breaking unless someone actually registers hooks, correct?

In future releases, I am wanting to think towards a more standardized way of specifying hooks as I outlined here: #7459 (comment). I am thinking this will also be relevant for #9066.

One alternate approach I would like to consider for the functionality in this PR is adding event listeners for typed events to BaseApp. In reviewing this PR I notice that there is an event emitted every time one of these hooks is called basically containing the same info as the event. What would it look like to migrate x/gov to typed events (#7474) and then have event listeners in BaseApp that get run during DeliverTx but after Msgs are processed?

@sunnya97
Copy link
Member Author

sunnya97 commented Apr 20, 2021

@aaronc Yeah, this shouldn't be state breaking unless someone uses the available hooks.

Ideally, getting this into v0.43 if possible would be great, because we need this feature for Osmosis, and would like to just use x/gov from the cosmos-sdk instead of needing to fork it if possible.

It seems that ADR 33 left what to do about hooks as a future TODO.

For now, I'd prefer to get this PR in just asap to expose the functionality, following the same hooks model as x/staking. And then we can figure out a new model of hooks for a future version as that will require a much longer process with a proper ADR, and upgrade these hooks along with the staking ones at that time.

@aaronc
Copy link
Member

aaronc commented Apr 20, 2021

For now, I'd prefer to get this PR in just asap to expose the functionality, following the same hooks model as x/staking. And then we can figure out a new model of hooks for a future version as that will require a much longer process with a proper ADR, and upgrade these hooks along with the staking ones at that time.

That's fine. Shouldn't we add some tests here?

@sunnya97
Copy link
Member Author

@aaronc, Added some tests! 👍

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

x/gov/keeper/keeper.go Show resolved Hide resolved
h.AfterProposalVotingPeriodEndedValid = true
}

func TestHooks(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we have a testing suite for this instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wdym?


import "github.com/cosmos/cosmos-sdk/x/gov/types"

func UpdateHooks(k *Keeper, h types.GovHooks) *Keeper {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you define this on the other test file instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, so the reason it's like this is that the hooks field in the keeper is unexposed, so it can only be accessed by the keeper package, not the keeper_test package.

And we don't want to add this function to the keeper.go file because that then it would be available to all other modules, which isn't what we want. The trick is to put it in a file with the _test suffix but still in the keeper package, so it's only built during tests and thus accessible to other tests, but not other modules.

See: https://stackoverflow.com/questions/24622388/how-to-test-a-unexported-private-function-in-go-golang

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense! thanks for the clarification

@sunnya97 sunnya97 merged commit bffcae5 into master Apr 21, 2021
@sunnya97 sunnya97 deleted the sikka/governance-hooks branch April 21, 2021 16:59
@barriebyron
Copy link
Contributor

LGTM, could we get changes to the spec as well?

I was just looking for the doc updates today!

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
* add governance hooks

* fix lint

* fix lint

* CHANGELOG

* sh -> gh

* improve comments

* add test

* add more tests

* rename two of the hooks

Co-authored-by: ahmedaly113 <ahmedaly113@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add hooks to x/gov OnVote
7 participants