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

Subscription execution hook #8

Closed
wants to merge 9 commits into from
Closed

Conversation

gdnathan
Copy link
Collaborator

closes #6

@gdnathan gdnathan added the enhancement New feature or request label Jun 16, 2022
@gdnathan gdnathan requested a review from tdelabro June 16, 2022 16:31
@gdnathan gdnathan marked this pull request as draft June 17, 2022 07:30
@gdnathan gdnathan marked this pull request as ready for review June 17, 2022 07:31
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@gdnathan gdnathan requested a review from tdelabro June 17, 2022 14:20
use sp_std::marker::PhantomData;

pub trait WeightInfo {
fn do_execute_subscriptions(itts: u32) -> Weight;
Copy link
Contributor

Choose a reason for hiding this comment

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

itts what an horrible abreviation haha
use the full work as much as you can

#[pallet::event]
// #[pallet::generate_deposit(pub(super) fn deposit_event)]
pub enum Event<T: Config> {
SomethingStored(u32, T::AccountId),
Copy link
Contributor

Choose a reason for hiding this comment

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

add an event to signal execution of a subscription

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated
pub fn do_execute_subscriptions(n: T::BlockNumber) -> Weight {
let mut itterations = 0;

let mut subs = unwrap_or_return!(Subscriptions::<T>::get(), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

subs -> subscriptions


impl<T: Config> Pallet<T> {
pub fn do_execute_subscriptions(n: T::BlockNumber) -> Weight {
let mut itterations = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

only one t in iterations

}

impl<T: Config> Pallet<T> {
pub fn do_execute_subscriptions(n: T::BlockNumber) -> Weight {
Copy link
Contributor

Choose a reason for hiding this comment

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

n is a terrible variable name.
use current_block_number

src/lib.rs Outdated
return 0
}
loop {
if let Some((block, (mut sub, account))) = subs.pop() {
Copy link
Contributor

Choose a reason for hiding this comment

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

sub should be subscription
account subscriber
block block_number

pub fn do_execute_subscriptions(n: T::BlockNumber) -> Weight {
let mut itterations = 0;

let subs = unwrap_or_return!(Subscriptions::<T>::get(n), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

You just did a read, so don't return 0.
Return weight of a read

Comment on lines +193 to +194
}
T::WeightInfo::do_execute_subscriptions(itterations)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove the key we used (current_block) from the storage

Comment on lines +188 to +192
let new_subs = Subscriptions::<T>::take(new_block).map(|mut subs| {
subs.push((sub.clone(), account.clone()));
subs
}).unwrap_or_else(|| vec!((sub, account)));
Subscriptions::<T>::insert(new_block, new_subs);
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to take it. You can just read it. You are going to replace the value with an insert just after anyhow.

Comment on lines +188 to +192
let new_subs = Subscriptions::<T>::take(new_block).map(|mut subs| {
subs.push((sub.clone(), account.clone()));
subs
}).unwrap_or_else(|| vec!((sub, account)));
Subscriptions::<T>::insert(new_block, new_subs);
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing this you will do a read and a write for each subscription.
You should rather store the value you updated in a on-memory cache, update this cache as many time as you want and and only write them back to storage at the end of the whole loop

@gdnathan gdnathan closed this Aug 3, 2022
@gdnathan gdnathan deleted the subscription_execution_hook branch August 3, 2022 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pallet on_initialize hook
2 participants