-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Conversation
Codecov Report
@@ 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
|
40ddefd
to
d99861a
Compare
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.
LGTM, could we get changes to the spec as well?
x/gov/types/expected_keepers.go
Outdated
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 |
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 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.
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.
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).
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 |
@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. |
That's fine. Shouldn't we add some tests here? |
5b8eb0b
to
1d1530d
Compare
@aaronc, Added some tests! 👍 |
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.
LGTM
h.AfterProposalVotingPeriodEndedValid = true | ||
} | ||
|
||
func TestHooks(t *testing.T) { |
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 wonder if we have a testing suite for this instead?
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.
Wdym?
|
||
import "github.com/cosmos/cosmos-sdk/x/gov/types" | ||
|
||
func UpdateHooks(k *Keeper, h types.GovHooks) *Keeper { |
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.
can you define this on the other test file instead?
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.
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
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.
makes sense! thanks for the clarification
I was just looking for the doc updates today! |
* 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>
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.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes