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.

Sign up for GitHub

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

Coretime Sale ID #6626

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

Coretime Sale ID #6626

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
67ee785
init
FereMouSiopi Nov 24, 2024
dd0f481
add storage item
FereMouSiopi Nov 25, 2024
4978cfe
add prdoc
FereMouSiopi Nov 25, 2024
f8468a6
Merge branch 'master' into coretime-sale-id
FereMouSiopi Nov 25, 2024
1f1a777
SaleInfo for sale_index(id)
FereMouSiopi Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions prdoc/pr_6626.prdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: coretime sale id

doc:
- audience: Runtime Dev
Copy link
Contributor

Choose a reason for hiding this comment

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

Also relevant for Runtime Users

description: |
This pull request adds a `sale_id` in the event emitted when coretime sale starts.

crates:
- name: pallet-broker
bump: minor
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
bump: minor
bump: major

Adding a new field to an enum variant is a breaking change

10 changes: 9 additions & 1 deletion substrate/frame/broker/src/dispatchable_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,15 @@ impl<T: Config> Pallet<T> {
cores_offered: 0,
cores_sold: 0,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could just add the sale_id to the SaleInfoRecord?

Also we will need a migration that sets sale_id to the correct value.

Copy link
Author

@FereMouSiopi FereMouSiopi Dec 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We would not need to include sale_index in the event emitted when sale starts, instead when SaleInitialized?

Runtime devs and users should query the SaleInfo for the current sale Id?

Copy link
Author

Choose a reason for hiding this comment

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

Is there a way to know how many sales have elapsed since? this is for migration purposes.

Copy link
Member

Choose a reason for hiding this comment

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

The migration would need to require to know when the first sale happened and then it can calculate the current sale number

Copy link
Contributor

Choose a reason for hiding this comment

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

On Kusama that might be fiddly because we re-started sales (we have a "sale" which is one week long) - raises the question: do we count that as sale 0 or just ignore it as the symptom of a bug? Would be interesting to see how others have labelled it. I guess you could just have an input sales_at_block along with the start block and current block or whatever. I'm sure you could simplify that too, but that's the fully general one for which the upgrade wouldn't need to be timed.

Copy link
Member

Choose a reason for hiding this comment

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

do we count that as sale 0 or just ignore it as the symptom of a bug?

We just ignore. By restarting we have voided any sales etc any way? (don't remember the exact details)

Copy link
Author

@FereMouSiopi FereMouSiopi Dec 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

do we count that as sale 0 or just ignore it as the symptom of a bug?

We just ignore. By restarting we have voided any sales etc any way? (don't remember the exact details)

The start block has to be specific to the relaychain(i.e Westend, Kusama and Polkadot)?

};
Self::deposit_event(Event::<T>::SalesStarted { price: end_price, core_count });

// Get current sale_id and increment it
let current_sale_id = SaleId::<T>::get();
let new_sale_id = current_sale_id.saturating_add(1);

// Store the new sale_id for the next sale
SaleId::<T>::put(new_sale_id);

Self::deposit_event(Event::<T>::SalesStarted { price: end_price, core_count, sale_id: new_sale_id });
Self::rotate_sale(old_sale, &config, &status);
Status::<T>::put(&status);
Ok(())
Expand Down
6 changes: 6 additions & 0 deletions substrate/frame/broker/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,10 @@ pub mod pallet {
#[pallet::storage]
pub type RevenueInbox<T> = StorageValue<_, OnDemandRevenueRecordOf<T>, OptionQuery>;

/// Id of the current sale.
#[pallet::storage]
pub type SaleId<T> = StorageValue<_, u32, ValueQuery>;

#[pallet::event]
#[pallet::generate_deposit(pub(super) fn deposit_event)]
pub enum Event<T: Config> {
Expand Down Expand Up @@ -346,6 +350,8 @@ pub mod pallet {
price: BalanceOf<T>,
/// The maximum number of cores which this pallet will attempt to assign.
core_count: CoreIndex,
/// Identifier for the current sale.
sale_id: u32,
Copy link
Contributor

Choose a reason for hiding this comment

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

sale_index might be a better name for this (and throughout the PR)

},
/// The act of claiming revenue has begun.
RevenueClaimBegun {
Expand Down