-
Notifications
You must be signed in to change notification settings - Fork 633
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
Update Treasury to Support Relay Chain Block Number Provider #3970
base: master
Are you sure you want to change the base?
Update Treasury to Support Relay Chain Block Number Provider #3970
Conversation
substrate/frame/treasury/src/lib.rs
Outdated
// AUDIT REVIEW TODO! Needs to handle migration story for this better. | ||
LastSpendPeriod::<T, I>::get().unwrap_or(BlockNumberFor::<T>::zero()); |
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.
call out here
probably needs migration, but perhaps there is a way to avoid it with some clever logic?
else the first time this logic runs, it will execute a ton of burns at once.
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.
// AUDIT REVIEW TODO! Needs to handle migration story for this better. | |
LastSpendPeriod::<T, I>::get().unwrap_or(BlockNumberFor::<T>::zero()); | |
LastSpendPeriod::<T, I>::get().unwrap_or_else(|| calculate_last_spend_period()); |
in that way no migration is needed if the block number provider is the local block number
but we will need a migration when switch the block number provider
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.
but we will need a migration when switch the block number provider
Seems unavoidable
Otherwise good suggestion. Done.
fn on_initialize(_do_not_use_local_block_number: BlockNumberFor<T>) -> Weight { | ||
let block_number = T::BlockNumberProvider::current_block_number(); |
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 might be a common error when people write pallets depending on a BlockNumberProvider
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.
Yea, on_initialize
should be deprecated in favour of the new hooks from #1781.
substrate/frame/treasury/src/lib.rs
Outdated
// This unwrap should only occur one time on any blockchain. | ||
// `update_last_spend_period` will populate the `LastSpendPeriod` storage if it is | ||
// empty. | ||
.unwrap_or(Self::update_last_spend_period()); |
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.
It is possible to use the OnEmpty
syntax of the LastSpendPeriod
and then change it to ValueQuery
since it is not really optional.
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.
Feel like it is exactly the same thing, but more substrate magic.
or what am I missing?
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.
Yup. Depends on whether you like more or less magic 😆
@ggwpez i think it should be good now, but probably requires a more careful review as treasury changes leak into multiple other pallets |
@@ -114,7 +119,8 @@ fn activate_bounty<T: Config>( | |||
let approve_origin = | |||
T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; | |||
Bounties::<T>::approve_bounty(approve_origin, child_bounty_setup.bounty_id)?; | |||
Treasury::<T>::on_initialize(BlockNumberFor::<T>::zero()); |
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 shouldnt allow on_initialize(0)
: #4316
let update_due = frame_system::Pallet::<T>::block_number() + | ||
T::BountyUpdatePeriod::get(); | ||
let update_due = | ||
Self::treasury_block_number() + T::BountyUpdatePeriod::get(); |
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 imagine most of the logic breaks when a parachain migrates from System
to the Relay
BlockNumberProvider
...
Not sure what can be done about it without re-writing the pallet to use timepoints. Maybe we can use this as first step and advise to still use System
.
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.
a transition from system to relay will always require a migration due to the nature of the code.
you think we should be transitioning to use timestamps for all of these various pallet logic? I have always suggested against this, but perhaps due to the nature of Polkadot, it might be the right thing to do.
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 in the long term we need some kind of block-number-abstraction like #3268, but its a bit stale now as low-prio. That is not necessarily timestamps, but could be implemented with Relay blocknumber or something else.
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, just slightly worried that people will mess up their state by setting Relay
for the provider on a parachain.
Co-authored-by: Muharem <ismailov.m.h@gmail.com>
@shawntabrizi should we merge the PR? all required CIs are green |
Its no problem from me. Perhaps requires an audit, but I leave it to you all to get this merged :) I think the code makes sense, although adds yet another piece of complexity to pallet config. |
@shawntabrizi do we have a live or upcoming use case for this? |
Don't we need it for instances of Treasury on Collectives? |
I think we do not, Collectives chain does produce the blocks, and we do not need the spend period to be in sync with Relay Treasury spend period. |
There isn't a world ending pressure to get this in, but I think this is a natural improvement to the treasury system for the new models of accessing and using coretime. There was a project I was working on that would want this feature. Truthfully, with JAM coming up, it would be better to start making all of FRAME and the Polkadot SDK more "parachain" centric, whereas much of the pallets are designed with the relay chain in mind. |
This question is totally irrelevant for this pr here. With block times getting more dynamic, we need this fix. There are also other downstream users than "we" that may use or want to use it. |
This PR is still in a backlog for the audit. I assume we just wait. |
The goal of this PR is to have the treasury pallet work on a parachain which does not produce blocks on a regular schedule, thus can use the relay chain as a block provider.
Because blocks are not produced regularly, we cannot make the assumption that block number increases monotonically, and thus have new logic to handle multiple spend periods passing between blocks.