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

Luxor/reduce council budget #5062

Merged
merged 24 commits into from
Mar 25, 2024

Conversation

ignazio-bovo
Copy link
Collaborator

@ignazio-bovo ignazio-bovo commented Jan 26, 2024

addresses: #3494

Budget is simply reduced by the amount (there is no token minting until workers/councilors gets paid)
I have also added a proposal, with the same parameters as the freeze_pallet proposal

  • add extrinsic to council pallet
  • add tests to council pallet
  • add proposal to codex pallet
  • add tests to codex pallet
  • add runtime configuration
  • add integration tests

@kdembler
Copy link
Collaborator

@ignazio-bovo please change target branch to luxor

Copy link
Collaborator

@kdembler kdembler left a comment

Choose a reason for hiding this comment

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

Look good to me, will approve once we finalize the params for proposal, waiting for council feedback

runtime-modules/proposals/codex/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 364 to 371
voting_period: days!(3),
grace_period: 0,
approval_quorum_percentage: TWO_OUT_OF_THREE,
approval_threshold_percentage: TWO_OUT_OF_THREE,
slashing_quorum_percentage: ALL,
slashing_threshold_percentage: ALL,
required_stake: Some(dollars!(50)),
constitutionality: 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to adjust those, will get back to you

Copy link
Collaborator

Choose a reason for hiding this comment

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

Council has approved a signal to make this 3/3 approval, 1 constitutionality: https://pioneer.joyutils.org/#/proposals/preview/797

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me know if also the quorum percentage is correct

Copy link
Contributor

@freakstatic freakstatic left a comment

Choose a reason for hiding this comment

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

LGTM!

textureAtlas.png Outdated Show resolved Hide resolved
@kdembler kdembler changed the base branch from nara to luxor March 1, 2024 13:10
@ignazio-bovo ignazio-bovo force-pushed the luxor/reduce-council-budget branch from bf8cf0c to c2acd1e Compare March 1, 2024 15:41
Copy link
Member

@mnaamani mnaamani left a comment

Choose a reason for hiding this comment

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

Runtime implementation looks good.
I think you ran the query-node build before the runtime spec was updated, that is why the query-node/chain-metadata/2002.json file is modified. Please revert it to one from master.
The integration test fails because the mappings do not handle case of the new proposal, so the processor fails to handle the event. Do you want to add the mapping and schema update for QN in this PR or a separately?

@ignazio-bovo
Copy link
Collaborator Author

The QN part is handled separately

@ignazio-bovo ignazio-bovo requested a review from mnaamani March 21, 2024 10:43
Copy link
Member

@mnaamani mnaamani left a comment

Choose a reason for hiding this comment

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

Just need to add a weight function for the decrease_council_budget dispatch call.

/// - db:
/// - `O(1)` doesn't depend on the state or parameters
/// # </weight>
#[weight = 10_000_000] // TODO: adjust
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a weight function for this dispatch

Copy link
Collaborator Author

@ignazio-bovo ignazio-bovo Mar 25, 2024

Choose a reason for hiding this comment

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

This was part of a bigger issue that there are no benchmarks.
I have added that for the reduce_council_budget, thanks for pointing it out

@@ -105,6 +104,7 @@ frame_support::construct_runtime!(
OperationsWorkingGroupBeta: working_group::<Instance7>::{Pallet, Call, Storage, Event<T>},
OperationsWorkingGroupGamma: working_group::<Instance8>::{Pallet, Call, Storage, Event<T>},
DistributionWorkingGroup: working_group::<Instance9>::{Pallet, Call, Storage, Event<T>},
Council: council::{Pallet, Call, Storage, Event<T>},
Copy link
Member

Choose a reason for hiding this comment

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

Why did you find it necessary to move the Council pallet down here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

error[E0277]: the trait bound `RuntimeEvent: From<pallet_council::RawEvent<u64, u64, u64, u64>>` is not satisfied
   --> runtime-modules/proposals/codex/src/tests/mock.rs:696:25
    |
696 |     type RuntimeEvent = RuntimeEvent;
    |                         ^^^^^^^^^^^^ the trait `From<pallet_council::RawEvent<u64, u64, u64, u64>>` is not implemented for `RuntimeEvent`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

without that

Copy link
Member

Choose a reason for hiding this comment

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

Is is something about the way this particular mock is implemented with respect to types. We do not seem to need to do the same for the main runtime/lib.rs

@mnaamani mnaamani self-requested a review March 25, 2024 19:13
Copy link
Member

@mnaamani mnaamani left a comment

Choose a reason for hiding this comment

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

I think this is good to merge, just left some general comments.

@@ -109,7 +108,7 @@ async function getOrCreateRuntimeWasmBytecode(store: DatabaseManager, bytecode:
async function parseProposalDetails(
event: SubstrateEvent,
store: DatabaseManager,
proposalDetails: RuntimeProposalDetails_V1001 | RuntimeProposalDetails_V2002
proposalDetails: RuntimeProposalDetails_V2003
Copy link
Member

Choose a reason for hiding this comment

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

I think this change is okay, although we probably should implement this function in future to account for any removed proposals between runtimes. To date we have only been adding new proposals.

/// - `O(1)` doesn't depend on the state or parameters
/// # </weight>
#[weight = CouncilWeightInfo::<T>::decrease_council_budget()]
pub fn decrease_council_budget(origin, reduction_amount: Balance<T>) -> Result<(), Error<T>> {
Copy link
Member

Choose a reason for hiding this comment

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

when adding a new dispatch that is invoked by a proposal, it is best to try to add it at the end so that the method index of existing dispatches doesn't change, so we can have the option to not remove all pending proposals on runtime upgrades.

@@ -105,6 +104,7 @@ frame_support::construct_runtime!(
OperationsWorkingGroupBeta: working_group::<Instance7>::{Pallet, Call, Storage, Event<T>},
OperationsWorkingGroupGamma: working_group::<Instance8>::{Pallet, Call, Storage, Event<T>},
DistributionWorkingGroup: working_group::<Instance9>::{Pallet, Call, Storage, Event<T>},
Council: council::{Pallet, Call, Storage, Event<T>},
Copy link
Member

Choose a reason for hiding this comment

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

Is is something about the way this particular mock is implemented with respect to types. We do not seem to need to do the same for the main runtime/lib.rs

@mnaamani mnaamani merged commit fff9f89 into Joystream:luxor Mar 25, 2024
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants