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

Task(bump) Pallet Salary #5163

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

Conversation

Doordashcon
Copy link
Contributor

@Doordashcon Doordashcon commented Jul 27, 2024

Resolves #4545

Blocked on #5194

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable-int
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7086050

@Doordashcon Doordashcon marked this pull request as ready for review November 7, 2024 21:54
@Doordashcon Doordashcon requested a review from a team as a code owner November 7, 2024 21:54
Copy link
Contributor

@gupnik gupnik left a comment

Choose a reason for hiding this comment

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

Thank you for taking this up!

@@ -146,6 +165,7 @@ parameter_types! {
}

impl Config for Test {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use derive_impl here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if we need to


#[pallet::hooks]
impl<T: Config<I>, I: 'static> Hooks<BlockNumberFor<T>> for Pallet<T, I> {
#[cfg(feature = "experimental")]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove experimental now and get this audited. Thoughts @kianenigma @ggwpez ?

@Doordashcon Doordashcon requested a review from a team as a code owner November 8, 2024 17:10
@paritytech-review-bot paritytech-review-bot bot requested a review from a team November 8, 2024 17:11
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

Looks good, some comments

substrate/frame/salary/src/lib.rs Outdated Show resolved Hide resolved
match res {
Ok(_) => log::info!(target: LOG_TARGET, "Submitted the task."),
Err(e) => log::error!(target: LOG_TARGET, "Error submitting task: {:?}", e),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So every block we try to submit the task?

Is submitting a transaction so cheap? Maybe we should at least verify the task condition before trying to submit it.

maybe we can use the task list here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gupnik what is the recommended way to submit tasks? every pallet do its own offchain worker that calls Task::iter()? or is there some system offchain worker responsible for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't know about this. I think system should have an offchain worker that calls Task::iter(), otherwise what is the point of the iter method.

substrate/frame/salary/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/salary/src/lib.rs Outdated Show resolved Hide resolved
prdoc/pr_5163.prdoc Outdated Show resolved Hide resolved
polkadot/xcm/xcm-builder/Cargo.toml Show resolved Hide resolved
@paritytech-review-bot paritytech-review-bot bot requested a review from a team November 20, 2024 15:22
@Doordashcon
Copy link
Contributor Author

Ready for review @gui1117 @gupnik

@paritytech-review-bot paritytech-review-bot bot requested a review from a team November 22, 2024 02:33
- audience: Runtime Dev
description: |
This PR places `pallet_salary::bump` on an offchain task, this acts as an alternative to
munaul execution (i.e tasks are executed in a random order).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
munaul execution (i.e tasks are executed in a random order).
manual execution (i.e tasks are executed in a random order).

@@ -89,13 +95,16 @@ pub struct ClaimantStatus<CycleIndex, Balance, Id> {
pub mod pallet {
use super::*;
use frame_support::{dispatch::Pays, pallet_prelude::*};
use frame_system::pallet_prelude::*;
use frame_system::pallet_prelude::{BlockNumberFor, *};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
use frame_system::pallet_prelude::{BlockNumberFor, *};
use frame_system::pallet_prelude::*;

Comment on lines +420 to +428
let mut status = Status::<T, I>::get().ok_or(Error::<T, I>::NotStarted)?;
status.cycle_index.saturating_inc();
status.cycle_start = frame_system::Pallet::<T>::block_number();
status.budget = T::Budget::get();
status.total_registrations = Zero::zero();
status.total_unregistered_paid = Zero::zero();
Status::<T, I>::put(&status);

Pallet::deposit_event(Event::<T, I>::CycleStarted { index: status.cycle_index });
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can factorize with the bump call into one function: do_bump_unchecked.

@gui1117 gui1117 added the T2-pallets This PR/Issue is related to a particular pallet. label Dec 2, 2024
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.

why do we need to do a manual bump in pallet-salary?
4 participants