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

Adding try_state hook for Treasury pallet #1820

Merged
merged 18 commits into from
Oct 12, 2023

Conversation

wentelteefje
Copy link
Contributor

This PR adds a try_state hook for the Treasury pallet.
Part of #239.
The invariants checked are:

  1. ProposalCount must be $\geq$ number of elements in Proposals. The ProposalCount storage value is only increased, but never decreased, whereas individual proposals can be removed from the Proposals storage map.
  2. T::ProposalBondMinimum * Number of proposals with non-zero bond $\leq \sum_{p \in \textrm{Proposals}} \textrm{p.bond}$.

Since the introduction of the dispatchable spend, a bond is no longer required (bond is set to zero in these cases). However, the old way of proposing using a bond is still available and we can filter for these by looking at proposals with $\textrm{p.bond} \neq 0 $.

  1. (Optional) T::ProposalBondMaximum * Number of proposals with non-zero bond $\geq \sum_{p \in \textrm{Proposals}} \textrm{p.bond}$.

Analogous to 2., but only relevant when T::ProposalBondMaximum is not None.

  1. Each ProposalIndex contained in Approvals should exist in Proposals. Note, that this
    automatically implies Approvals.count() <= Proposals.count().
  2. SpendCount must be $\geq$ number of elements in Spends. Follows the same reasoning as invariant 1.
  3. For each spend entry contained in Spends we should have spend.expire_at > spend.valid_from.

@wentelteefje wentelteefje added the T10-tests This PR/Issue is related to tests. label Oct 8, 2023
@wentelteefje wentelteefje requested review from a team October 8, 2023 15:56
substrate/frame/treasury/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/treasury/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/treasury/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/treasury/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/treasury/src/tests.rs Outdated Show resolved Hide resolved
@paritytech-ci paritytech-ci requested a review from a team October 9, 2023 09:54
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Looks very clean, thanks!
Should probably try this before merging on Polkadot and Kusama states, so we know that it all works.

@ggwpez ggwpez added T1-FRAME This PR/Issue is related to core FRAME, the framework. and removed T1-FRAME This PR/Issue is related to core FRAME, the framework. labels Oct 10, 2023
@ggwpez
Copy link
Member

ggwpez commented Oct 10, 2023

bot fmt
It is cargo +nightly-2023-05-16 fmt if you want to run it locally.

@paritytech paritytech deleted a comment from command-bot bot Oct 10, 2023
@paritytech paritytech deleted a comment from command-bot bot Oct 10, 2023
@wentelteefje
Copy link
Contributor Author

cargo fmt seems to break markdown lists in comments. I guess I should fix these manually before merging too.

@ggwpez
Copy link
Member

ggwpez commented Oct 10, 2023

cargo fmt seems to break markdown lists in comments. I guess I should fix these manually before merging too.

Yea... lines should wrap at exactly 100 chars - also comments.

@wentelteefje
Copy link
Contributor Author

bot fmt

@paritytech paritytech deleted a comment from command-bot bot Oct 10, 2023
@paritytech paritytech deleted a comment from command-bot bot Oct 10, 2023
@wentelteefje
Copy link
Contributor Author

Looks very clean, thanks! Should probably try this before merging on Polkadot and Kusama states, so we know that it all works.

It works on rococo state (ProposalCount = 15), but because of the migration of Polkadot/Kusama runtimes into the fellowship's runtime repository I wasn't able to try it out on them.

Copy link
Contributor

@liamaharon liamaharon left a comment

Choose a reason for hiding this comment

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

looking good, thanks!

substrate/frame/treasury/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/treasury/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/treasury/src/tests.rs Outdated Show resolved Hide resolved
@wentelteefje
Copy link
Contributor Author

wentelteefje commented Oct 11, 2023

I have added tests for all try_state scenarios. During this I also noticed that there's no longer a way to check invariants 2 & 3 from above (my initial approach had a mistake). That's because faulty entries with a zero bond cannot be distinguished from valid entries made using the spend_local dispatchable. So it was good to think about that some more during test writing :) Also tested successfully against rococo state once more just in case.

As for the tests: I really don't like catching errors based on a string value. An enum would be much cleaner, but I guess there's no other way for now?

@liamaharon
Copy link
Contributor

Thanks, tests look great 👌

Other must be a String afaik. You might be able to avoid duplication be declaring them as consts, but not a big deal for the purposes of try-state.

@wentelteefje wentelteefje merged commit f0e6d2a into master Oct 12, 2023
108 of 109 checks passed
@wentelteefje wentelteefje deleted the wtf-pallet-treasury-add-try-state branch October 12, 2023 12:53
ordian added a commit that referenced this pull request Oct 16, 2023
* master: (54 commits)
  Publish `xcm-emulator` crate (#1881)
  Adding migrations to clean Rococo Gov 1 storage & reserved funds (#1849)
  Arkworks Elliptic Curve utils overhaul (#1870)
  Fix typos (#1878)
  fix: GoAhead signal only set when runtime upgrade is enacted from parachain side (#1176)
  Refactor staking ledger (#1484)
  Paired-key Crypto Scheme (#1705)
  Include polkadot version in artifact path (#1828)
  add link to rfc-0001 in broker README (#1862)
  Discard `Executor` (#1855)
  Macros to use path instead of ident (#1474)
  Remove clippy clone-double-ref lint noise (#1860)
  Refactor alliance benchmarks to v2 (#1868)
  Check executor params coherence (#1774)
  frame: use derive-impl for beefy and mmr pallets (#1867)
  sc-consensus-beefy: improve gossip logic (#1852)
  Adds instance support for composite enums (#1857)
  Fix links to implementers' guide (#1865)
  Disabled validators runtime API (#1257)
  Adding `try_state` hook for `Treasury` pallet (#1820)
  ...
ordian added a commit that referenced this pull request Oct 16, 2023
…ribution

* tsv-disabling-backing: (54 commits)
  Publish `xcm-emulator` crate (#1881)
  Adding migrations to clean Rococo Gov 1 storage & reserved funds (#1849)
  Arkworks Elliptic Curve utils overhaul (#1870)
  Fix typos (#1878)
  fix: GoAhead signal only set when runtime upgrade is enacted from parachain side (#1176)
  Refactor staking ledger (#1484)
  Paired-key Crypto Scheme (#1705)
  Include polkadot version in artifact path (#1828)
  add link to rfc-0001 in broker README (#1862)
  Discard `Executor` (#1855)
  Macros to use path instead of ident (#1474)
  Remove clippy clone-double-ref lint noise (#1860)
  Refactor alliance benchmarks to v2 (#1868)
  Check executor params coherence (#1774)
  frame: use derive-impl for beefy and mmr pallets (#1867)
  sc-consensus-beefy: improve gossip logic (#1852)
  Adds instance support for composite enums (#1857)
  Fix links to implementers' guide (#1865)
  Disabled validators runtime API (#1257)
  Adding `try_state` hook for `Treasury` pallet (#1820)
  ...
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
This PR adds a `try_state` hook for the `Treasury` pallet.
Part of paritytech#239.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T10-tests This PR/Issue is related to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants