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
+34 −0
Open

Coretime Sale ID #6626

Show file tree
Hide file tree
Changes from all 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 & Runtime Users
description: |
This pull request adds a `sale_id` in the event emitted when coretime sale starts.

crates:
- name: pallet-broker
bump: major
3 changes: 3 additions & 0 deletions substrate/frame/broker/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ mod benches {
.saturating_sub(T::MaxLeasedCores::get())
.try_into()
.unwrap(),
sale_index: 0u32,
}
.into(),
);
Expand Down Expand Up @@ -803,6 +804,7 @@ mod benches {
ideal_cores_sold: 0,
cores_offered: 0,
cores_sold: 0,
sale_index: 0u32,
};

let status = StatusRecord {
Expand Down Expand Up @@ -862,6 +864,7 @@ mod benches {
.saturating_sub(T::MaxLeasedCores::get())
.try_into()
.unwrap(),
sale_index: 1u32,
}
.into(),
);
Expand Down
2 changes: 2 additions & 0 deletions substrate/frame/broker/src/dispatchable_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ impl<T: Config> Pallet<T> {
ideal_cores_sold: 0,
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)?

Copy link
Member

Choose a reason for hiding this comment

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

Yes the start block has to be an input to the migration that depends on the relay chain.

Copy link
Contributor

Choose a reason for hiding this comment

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

By restarting we have voided any sales etc any way?

Yea no sales were completed. I just wonder how indexers etc will see it, as I know that often people rely on events quite heavily and they'll count x SaleInitialized events, but then the current sale has index x-2 instead of x-1 as they expect based on events.

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

yes, each network started sales at a different block height

sale_index: 0,
};

Self::deposit_event(Event::<T>::SalesStarted { price: end_price, core_count });
Self::rotate_sale(old_sale, &config, &status);
Status::<T>::put(&status);
Expand Down
2 changes: 2 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 @@ -323,6 +323,8 @@ pub mod pallet {
ideal_cores_sold: CoreIndex,
/// Number of cores which are/have been offered for sale.
cores_offered: CoreIndex,
/// Identifier for the current sale
sale_index: u32,
},
/// A new lease has been created.
Leased {
Expand Down
3 changes: 3 additions & 0 deletions substrate/frame/broker/src/migration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,8 @@ mod v3 {
pub sellout_price: Option<Balance>,
/// Number of cores which have been sold; never more than cores_offered.
pub cores_sold: CoreIndex,
/// Identifier for the current sale.
pub sale_index: u32,
}
}

Expand Down Expand Up @@ -328,6 +330,7 @@ pub mod v4 {
first_core: sale_info.first_core,
sellout_price: sale_info.sellout_price,
cores_sold: sale_info.cores_sold,
sale_index: sale_info.sale_index,
};
SaleInfo::<T>::put(updated_sale_info);
}
Expand Down
2 changes: 2 additions & 0 deletions substrate/frame/broker/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1231,6 +1231,7 @@ fn purchase_requires_valid_status_and_sale_info() {
ideal_cores_sold: 0,
cores_offered: 1,
cores_sold: 2,
sale_index: 0,
};
SaleInfo::<Test>::put(&dummy_sale);
assert_noop!(Broker::do_purchase(1, 100), Error::<Test>::Unavailable);
Expand Down Expand Up @@ -1273,6 +1274,7 @@ fn renewal_requires_valid_status_and_sale_info() {
ideal_cores_sold: 0,
cores_offered: 1,
cores_sold: 2,
sale_index: 0,
};
SaleInfo::<Test>::put(&dummy_sale);
assert_noop!(Broker::do_renew(1, 1), Error::<Test>::Unavailable);
Expand Down
7 changes: 7 additions & 0 deletions substrate/frame/broker/src/tick_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,11 @@ impl<T: Config> Pallet<T> {
} else {
None
};
let sale_index = if let Some(sale) = SaleInfo::<T>::get() {
sale.sale_index.saturating_add(1)
} else {
old_sale.sale_index
};

// Update SaleInfo
let new_sale = SaleInfoRecord {
Expand All @@ -260,6 +265,7 @@ impl<T: Config> Pallet<T> {
ideal_cores_sold,
cores_offered,
cores_sold: 0,
sale_index,
};

SaleInfo::<T>::put(&new_sale);
Expand All @@ -275,6 +281,7 @@ impl<T: Config> Pallet<T> {
region_end,
ideal_cores_sold,
cores_offered,
sale_index,
});

Some(())
Expand Down
2 changes: 2 additions & 0 deletions substrate/frame/broker/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,8 @@ pub struct SaleInfoRecord<Balance, RelayBlockNumber> {
pub sellout_price: Option<Balance>,
/// Number of cores which have been sold; never more than cores_offered.
pub cores_sold: CoreIndex,
/// Identifier for the current sale.
pub sale_index: u32,
}
pub type SaleInfoRecordOf<T> = SaleInfoRecord<BalanceOf<T>, RelayBlockNumberOf<T>>;

Expand Down