-
Notifications
You must be signed in to change notification settings - Fork 115
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
Luxor/reduce council budget #5062
Conversation
@ignazio-bovo please change target branch to |
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.
Look good to me, will approve once we finalize the params for proposal, waiting for council feedback
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, |
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.
We need to adjust those, will get back to you
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.
Council has approved a signal to make this 3/3 approval, 1 constitutionality: https://pioneer.joyutils.org/#/proposals/preview/797
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.
Let me know if also the quorum percentage is correct
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!
… metadatat" This reverts commit 4041f80.
bf8cf0c
to
c2acd1e
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.
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?
The QN part is handled separately |
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.
Just need to add a weight function for the decrease_council_budget
dispatch call.
runtime-modules/council/src/lib.rs
Outdated
/// - db: | ||
/// - `O(1)` doesn't depend on the state or parameters | ||
/// # </weight> | ||
#[weight = 10_000_000] // TODO: adjust |
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.
Should we add a weight function for this dispatch
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.
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>}, |
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.
Why did you find it necessary to move the Council pallet down here.
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.
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`
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.
without that
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.
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
[QN] Luxor: Reduce council budget mappings
Fix decrease council budget proposal amount value
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 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 |
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 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>> { |
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.
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>}, |
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.
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
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