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

feat(governance): 🧊 test that community pool spend proposals can be disabled #4222

Merged
merged 12 commits into from
Apr 19, 2024

Conversation

cratelyn
Copy link
Contributor

@cratelyn cratelyn commented Apr 16, 2024

fixes #4079. related to, but does not finish #2105.

this introduces a new governance parameter that can be used to freeze or unfreeze the community pool. this branch additionally introduces a few mock consensus tests to exercise the governance component, to show that it works when unfrozen, and is properly disabled when frozen.

this introduces test coverage to show that community pool spend proposals can be disabled via a chain parameter.

"the point"

the core mechanical change of this branch is made in these commits: see #4222 (comment) below.

  • f88a981 feat(app): ✅ no spend proposals when pool is frozen
  • 5129012 feat(app): 🧊 community pool actions not accepted when frozen
  • 51e7063 feat(proto): 🏂 add community_pool_is_frozen to governance params

NB: this is a consensus-breaking change, as a parameter will now block community pool transactions when this new parameter is active.

related changes

some changes made, related to writing tests for the governance component:

  • 6b3be58 refactor(governance): 💶 Value accessors for proposal actions
  • de16dac feat(tx): 🌮 ActionPlan: From for community pool actions
  • 4e6b874 feat(amount): ➖ Amount: std::ops::SubAssign

tests

this addresses part of #2105. we should add tests that exercise the other
tallying logic before that issue is complete. tests are added in the following
commits:

  • fedfa8d tests(app): 💭 test a community pool spend
  • 201fe48 tests(app): 🏊 test a community pool deposit
  • 8543869 tests(app): 🥶 test that community pool can be frozen

@cratelyn cratelyn added A-governance Area: Governance _P-V1 Priority: slated for V1 release labels Apr 16, 2024
@cratelyn cratelyn added this to the Sprint 4 milestone Apr 16, 2024
@cratelyn cratelyn self-assigned this Apr 16, 2024
@cratelyn cratelyn force-pushed the kate/governance-can-toggle-community-pool branch from a988b3a to 7836575 Compare April 18, 2024 00:47
this echoes a previous change to some of the staking actions. this is
helpful for e.g. manually forming governance proposal submissions with a
`TransactionPlan`.

this only changes the submission and withdrawal action, deposit claims
are left alone for now.
this adds a `community_pool_is_frozen` field to the governance
parameters. it is not yet wired into any application logic, this commit
solely adjusts protobuffer definitions.
@cratelyn cratelyn force-pushed the kate/governance-can-toggle-community-pool branch from 7836575 to 073e56d Compare April 18, 2024 00:48
@cratelyn cratelyn added the protobuf-changes Makes changes to the protobuf definitions. label Apr 18, 2024
@cratelyn
Copy link
Contributor Author

💬 a tertiary discussion point

i see the following parameter: https://github.com/penumbra-zone/penumbra/blob/main/crates/core/component/community-pool/src/params.rs#L10-L13

my understanding is that this is similar, but not completely the same as, community_pool_is_frozen. i'd like to briefly gesture at it though, in case this sounds incorrect.

@cratelyn cratelyn force-pushed the kate/governance-can-toggle-community-pool branch from 073e56d to 94ea042 Compare April 18, 2024 14:51
@cratelyn cratelyn marked this pull request as ready for review April 18, 2024 14:53
@cratelyn cratelyn force-pushed the kate/governance-can-toggle-community-pool branch from 94ea042 to 1c9f124 Compare April 18, 2024 14:57
@cratelyn cratelyn added the consensus-breaking breaking change to execution of on-chain data label Apr 18, 2024
@cratelyn cratelyn marked this pull request as draft April 18, 2024 15:05
this adds an invariant to the application, blocking community pool spend
proposals when `community_pool_is_frozen` is true.
this adds a complimentary test to the previous test case, showing that
we cannot make a community pool deposit if the pool is currently frozen.
first, we write a test that can be used to make a deposit into the
community pool. show that the balance is updated accordingly.
@cratelyn cratelyn force-pushed the kate/governance-can-toggle-community-pool branch from 1c9f124 to fedfa8d Compare April 18, 2024 15:12
@cratelyn cratelyn marked this pull request as ready for review April 18, 2024 15:16
@hdevalence
Copy link
Member

💬 a tertiary discussion point

i see the following parameter: https://github.com/penumbra-zone/penumbra/blob/main/crates/core/component/community-pool/src/params.rs#L10-L13

my understanding is that this is similar, but not completely the same as, community_pool_is_frozen. i'd like to briefly gesture at it though, in case this sounds incorrect.

Hmm, it looks pretty much the same to me

@hdevalence
Copy link
Member

maybe we don't need both?

@cratelyn
Copy link
Contributor Author

💬 a tertiary discussion point

i see the following parameter: https://github.com/penumbra-zone/penumbra/blob/main/crates/core/component/community-pool/src/params.rs#L10-L13
my understanding is that this is similar, but not completely the same as, community_pool_is_frozen. i'd like to briefly gesture at it though, in case this sounds incorrect.

Hmm, it looks pretty much the same to me

cool, i am glad i asked! to state the difference explicitly, this new parameter also prevents community pool deposits when the pool is frozen. whether that is a useful property, i'm unsure.

we probably don't need both though, i think that is a fair point nonetheless. in that case @hdevalence, do you think that we should keep the existing option, or remove/deprecate it in favor of this option?

@hdevalence
Copy link
Member

I think we should just have one parameter, it was an oversight on my part to not check that the work was already completed. The important thing is making sure that there's a chain parameter that prevents any community pool transactions from occurring.

@cratelyn
Copy link
Contributor Author

cratelyn commented Apr 18, 2024

I think we should just have one parameter, it was an oversight on my part to not check that the work was already completed. The important thing is making sure that there's a chain parameter that prevents any community pool transactions from occurring.

sounds good. i'll suggest the following course:

  • 1. revert the commits here introducing a new parameter.
  • 2. rephrase the tests to work with the existing parameter.

at risk of being pedantic again, the existing community_pool_spend_proposals_enabled parameter only blocks specifically the proposal of community pool spend proposals, and transactions depositing into the pool are still allowed when that flag is on. that is, community_pool_spend_proposals_enabled lets funds go into the pool, but not out.

i think that is fine, but i want to be sure that it serves our needs, re: "prevents any community pool transactions from occurring." if so, this pr doesn't need to be consensus-breaking, and we can just treat this as the addition of more tests.

this reverts three commits:

* fedfa8d tests(app): 💭 test a community pool spend
* 201fe48 tests(app): 🏊 test a community pool deposit
* 8543869 tests(app): 🥶 test that community pool can be frozen
@cratelyn cratelyn force-pushed the kate/governance-can-toggle-community-pool branch from 9d44cbf to e249626 Compare April 18, 2024 19:08
@cratelyn cratelyn added A-testing Area: Relates to testing of Penumbra and removed consensus-breaking breaking change to execution of on-chain data protobuf-changes Makes changes to the protobuf definitions. labels Apr 18, 2024
@cratelyn cratelyn changed the title feat(governance): 🧊 community pool can be frozen via governance parameter feat(governance): 🧊 test that community pool spend proposals can be disabled Apr 18, 2024
@cratelyn cratelyn merged commit 3852420 into main Apr 19, 2024
8 checks passed
@cratelyn cratelyn deleted the kate/governance-can-toggle-community-pool branch April 19, 2024 14:08
@cratelyn cratelyn mentioned this pull request Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-governance Area: Governance A-testing Area: Relates to testing of Penumbra _P-V1 Priority: slated for V1 release
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add governance parameter for freezing/unfreezing the community pool
3 participants