-
Notifications
You must be signed in to change notification settings - Fork 781
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
Add "run to block" tools #7109
Add "run to block" tools #7109
Conversation
Signed-off-by: Xavier Lau <[email protected]>
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.
Nice work! Left some minor things to fix up :)
substrate/frame/system/src/lib.rs
Outdated
after_initialize: Box<dyn 'a + FnMut(BlockNumberFor<T>)>, | ||
before_finalize: Box<dyn 'a + FnMut(BlockNumberFor<T>)>, | ||
after_finalize: Box<dyn 'a + FnMut(BlockNumberFor<T>)>, | ||
_marker: PhantomData<&'a 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.
As we are using T
, this should not be required.
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.
Yep, did a refactor and forgot about this.
/// provided closures at each block. | ||
/// | ||
/// These hooks allows custom logic to be executed at each block at specific location. | ||
/// For example, you might use one of them to set a timestamp for each block. |
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.
Maybe mentioning that finalize_block
is called before each on_initialize
.
substrate/frame/staking/src/mock.rs
Outdated
// having `timestamp::on_initialize` called before `staking::on_initialize`. Also, if | ||
// session length is 1, then it is already triggered. |
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.
The comment doesn't really hold? :D
substrate/frame/staking/src/mock.rs
Outdated
// Revert the `Authorship::OnInitialize`'s `note_author` reward for easy testing. | ||
for i in start_session_idx..end_session_idx { | ||
let blocks_per_session = if i == 0 { | ||
// Skip block 0. | ||
period - 1 | ||
} else { | ||
period | ||
}; | ||
let extra_reward_points = blocks_per_session as RewardPoint * 20; | ||
|
||
ErasRewardPoints::<Test>::mutate(i / sessions_per_era, |p| { | ||
if let Some(author_11) = p.individual.get_mut(&11) { | ||
p.total = p.total.saturating_sub(extra_reward_points); | ||
*author_11 = (*author_11).saturating_sub(extra_reward_points); | ||
} | ||
}); | ||
} |
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.
Can we not just set the EventHandler
to ()
to prevent this from happening?
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.
Aha, missed this one.
substrate/frame/staking/src/mock.rs
Outdated
System::run_to_block_with::<AllPalletsWithSystem>( | ||
1, | ||
frame_system::RunToBlockHooks::default().after_initialize(|_| { | ||
Timestamp::set_timestamp(INIT_TIMESTAMP); | ||
}), | ||
); |
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.
Can we not just call the run_to_block
function here? (I know that the timestamp would be different, but the current setting here is wrong any way)
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 the origin code appears to be equivalent to running to block 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.
What I mean is to use run_to_block
from this file and not call System::run_to_block
manually.
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.
Okay, then reset the timestamp.
Make sense, to keep logic in a single place.
substrate/frame/staking/src/mock.rs
Outdated
System::run_to_block_with::<AllPalletsWithSystem>( | ||
1, | ||
frame_system::RunToBlockHooks::default().after_initialize(|_| { | ||
Timestamp::set_timestamp(INIT_TIMESTAMP); | ||
}), | ||
); |
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.
What I mean is to use run_to_block
from this file and not call System::run_to_block
manually.
/tip medium |
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.
Looks good to me, thank you
/cmd prdoc --audience runtime_dev --bump patch |
b72e76f
* master: (33 commits) Implement `pallet-asset-rewards` (#3926) [pallet-revive] Add host function `to_account_id` (#7091) [pallet-revive] Remove revive events (#7164) [pallet-revive] Remove debug buffer (#7163) litep2p: Provide partial results to speedup GetRecord queries (#7099) [pallet-revive] Bump asset-hub westend spec version (#7176) Remove 0 as a special case in gas/storage meters (#6890) [pallet-revive] Fix `caller_is_root` return value (#7086) req-resp/litep2p: Reject inbound requests from banned peers (#7158) Add "run to block" tools (#7109) Fix reversed error message in DispatchInfo (#7170) approval-voting: Make importing of duplicate assignment idempotent (#6971) Parachains: Use relay chain slot for velocity measurement (#6825) PRDOC: Document `validate: false` (#7117) xcm: convert properly assets in xcmpayment apis (#7134) CI: Only format umbrella crate during umbrella check (#7139) approval-voting: Fix sending of assignments after restart (#6973) Retry approval on availability failure if the check is still needed (#6807) [pallet-revive-eth-rpc] persist eth transaction hash (#6836) litep2p: Sufix litep2p to the identify agent version for visibility (#7133) ...
Introduce
frame_system::Pallet::run_to_block
,frame_system::Pallet::run_to_block_with
, andframe_system::RunToBlockHooks
to establish a genericrun_to_block
mechanism for mock tests, minimizing redundant implementations across various pallets.Closes #299.
Polkadot address: 156HGo9setPcU2qhFMVWLkcmtCEGySLwNqa3DaEiYSWtte4Y