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

Fix for Issue 4762 #4803

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

gotnoshoeson
Copy link

Issue #4762

  • Creates an enum for passing context of service_queues being called from within on_initialize and on_idle hooks. Uses a match statement inside of service_queues to continue using the same code, but NOT throw a defensive if being called within on_idle hook.
  • The issue requested to not throw the defensive if being called from within on_idle hook.
  • Created the ServiceQueuesContext enum to pass as an argument of service_queues when called within the on_initialize and on_idle hooks. A match statement was added inside of service_queues to continue to throw the defensive if called from on_initialize but NOT throw the defensive if called from on_idle.

@gotnoshoeson gotnoshoeson requested a review from a team as a code owner June 15, 2024 18:35
@cla-bot-2021
Copy link

cla-bot-2021 bot commented Jun 15, 2024

User @gotnoshoeson, please sign the CLA here.

@paritytech-review-bot paritytech-review-bot bot requested a review from a team June 16, 2024 22:07
substrate/frame/message-queue/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/message-queue/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/message-queue/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/message-queue/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/message-queue/src/lib.rs Outdated Show resolved Hide resolved
@bkchr bkchr added the T2-pallets This PR/Issue is related to a particular pallet. label Jun 25, 2024
@github-actions github-actions bot requested a review from bkchr June 26, 2024 01:50
Copy link

Review required! Latest push from author must always be reviewed

Comment on lines 1572 to 1573
// throw defensive message when service_queues is called from on_initialize
// don't throw message when service_queues is called from on_idle
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add the reason here, that it doesn't matter if on_idle doesn't have enough weight

Copy link
Author

Choose a reason for hiding this comment

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

Okay, that makes sense. Thank you

@ggwpez
Copy link
Member

ggwpez commented Jun 26, 2024

Please give it a descriptive title in case that it appears in the CHANGELOG.

@github-actions github-actions bot requested a review from ggwpez June 30, 2024 21:40
@bkchr
Copy link
Member

bkchr commented Jul 1, 2024

@gotnoshoeson what @ggwpez meant above is that, could you please add a prdoc file?

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6599029

@@ -1554,12 +1564,17 @@ impl<T: Get<O>, O: Into<u32>> Get<u32> for IntoU32<T, O> {
impl<T: Config> ServiceQueues for Pallet<T> {
type OverweightMessageAddress = (MessageOriginOf<T>, PageIndex, T::Size);

fn service_queues(weight_limit: Weight) -> Weight {
fn service_queues(weight_limit: Weight, context: ServiceQueuesContext) -> Weight {
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that this is implementing a trait function. Can you maybe put the inner logic into a new function and call that from here?

Having the ServiceQueuesContext in the interface would be ugly otherwise.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I pushed a new commit last night (PST). Now that I'm re-reading this I might not have implemented the way you requested.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, the code does not compile? You are implementing a trait function. Please try to run the pallet tests locally.

@github-actions github-actions bot requested a review from ggwpez July 4, 2024 04:52
@gotnoshoeson
Copy link
Author

Hi. Sorry I fell off this issue. But I'm back to finish if it's still open. Can you provide guidance of what is needed to complete? Thx

substrate/frame/message-queue/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/message-queue/src/lib.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants