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

Fellowship Treasury #109

Conversation

muharem
Copy link
Contributor

@muharem muharem commented Nov 30, 2023

Treasury Pallet Instance for the Fellowship in Polkadot Collectives.

In this update, we present a Treasury Pallet Instance that is under the control of the Fellowship body, with oversight from the Root and Treasurer origins. Here's how it is governed:

  • the Root origin have the authority to reject or approve spend proposals, with no amount limit for approvals.
  • the Treasurer origin have the authority to reject or approve spend proposals, with approval limits of up to 10,000,000 DOT.
  • Voice of all Fellows ranked at 3 or above can reject or approve spend proposals, with a maximum approval limit of 10,000 DOT.
  • Voice of Fellows ranked at 4 or above can also reject or approve spend proposals, with a maximum approval limit of 10,000,000 DOT.

Additionally, we introduce the Asset Rate Pallet Instance to establish conversion rates from asset A to B. This is used to determine if a proposed spend amount involving a non-native asset is permissible by the commanding origin. The rates can be set up by the Root, Treasurer origins, or Voice of all Fellows.

test with xcm-emulator for the same setup in Westend - paritytech/polkadot-sdk#2532
more details on new treasury features and asset rate pallet - paritytech/polkadot-sdk#1333

@muharem muharem mentioned this pull request Nov 30, 2023
pub const MaxBalance: Balance = Balance::max_value();
// The asset's interior location for the paying account. This is the Fellowship Treasury
// pallet instance (which sits at index 65).
pub FellowshipTreasuryInteriorLocation: InteriorMultiLocation = PalletInstance(65).into();
Copy link
Contributor

Choose a reason for hiding this comment

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

I won't block on this but it will be great if there is a way to not having hardcoded number here

Copy link
Contributor

Choose a reason for hiding this comment

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

Without pulling in the entire runtime enum, it will have to be declared somewhere, right? Of course if more parachains need this value then it could be pulled up to some common area that they import from.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we could have a test somewhere to assert the value matches

Copy link
Contributor

Choose a reason for hiding this comment

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

This location is not actually the problem, can just read it directly (which I have changed it to do). The problem is more here. I had added two tests and a link to an issue to pull these important pallet indices into a common area.

type OnSlash = ();
type ProposalBond = HundredPercent;
type ProposalBondMinimum = MaxBalance;
type ProposalBondMaximum = MaxBalance;
Copy link
Contributor

Choose a reason for hiding this comment

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

again, out of scope of this PR, but this means the treasury pallet needs some serious refactoring

Copy link
Contributor Author

@muharem muharem Feb 14, 2024

Choose a reason for hiding this comment

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

related calls were deprecated half year ago, with a note that they will be removed in Feb

type RuntimeEvent = RuntimeEvent;
type CreateOrigin = EitherOfDiverse<
EnsureRoot<AccountId>,
EitherOfDiverse<EnsureXcm<IsVoiceOfBody<GovernanceLocation, TreasurerBodyId>>, Fellows>,
Copy link
Contributor

Choose a reason for hiding this comment

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

just to confirm, we are happy with this origin configuration?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems reasonable to me.

@@ -621,6 +639,7 @@ construct_runtime!(
Proxy: pallet_proxy::{Pallet, Call, Storage, Event<T>} = 42,
Preimage: pallet_preimage::{Pallet, Call, Storage, Event<T>, HoldReason} = 43,
Scheduler: pallet_scheduler::{Pallet, Call, Storage, Event<T>} = 44,
AssetRate: pallet_asset_rate::{Pallet, Call, Storage, Event<T>} = 45,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I will prefer to use the default syntax to not explicitly listing things here but it maybe good to keep consistent in this PR and do the change later in one go

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. We really need to switch this.

relay/polkadot/constants/src/lib.rs Show resolved Hide resolved
// pallet
// --chain=collectives-westend-dev
// --steps=2
// --repeat=2
Copy link
Member

Choose a reason for hiding this comment

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

Sure need to be re-generated before the next release.

Co-authored-by: Oliver Tale-Yazdi <[email protected]>
@bkchr
Copy link
Contributor

bkchr commented Dec 8, 2023

Ser @xlc, maybe another pass?

@bkchr
Copy link
Contributor

bkchr commented Dec 8, 2023

/merge

@fellowship-merge-bot fellowship-merge-bot bot merged commit 49a04e5 into polkadot-fellows:main Dec 8, 2023
15 checks passed
@fellowship-merge-bot
Copy link
Contributor

Enabled auto-merge in Pull Request

Available commands
  • /merge: Enables auto-merge for Pull Request
  • /merge cancel: Cancels auto-merge for Pull Request
  • /merge help: Shows this menu

For more information see the documentation

@joepetrowski joepetrowski deleted the muharem-fellowship-collecitves-treasury branch December 8, 2023 21:50
fellowship-merge-bot bot pushed a commit that referenced this pull request Mar 11, 2024
…oded numbers usage for indexes (#182)

This PR solves older TODO + addresses
#109 (comment).

<!-- Remember that you can run `/merge` to enable auto-merge in the PR
-->

<!-- Remember to modify the changelog. If you don't need to modify it,
you can check the following box.
Instead, if you have already modified it, simply delete the following
line. -->

- [X] Does not require a CHANGELOG entry

---------

Co-authored-by: Bastian Köcher <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants