-
Notifications
You must be signed in to change notification settings - Fork 746
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
Coretime Sale ID #6626
base: master
Are you sure you want to change the base?
Coretime Sale ID #6626
Conversation
User @FereMouSiopi, please sign the CLA here. |
@@ -105,7 +105,15 @@ impl<T: Config> Pallet<T> { | |||
cores_offered: 0, | |||
cores_sold: 0, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far, I agree that it would be better as part of SaleInfo instead of in its own storage value
substrate/frame/broker/src/lib.rs
Outdated
@@ -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, |
There was a problem hiding this comment.
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)
prdoc/pr_6626.prdoc
Outdated
|
||
crates: | ||
- name: pallet-broker | ||
bump: minor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump: minor | |
bump: major |
Adding a new field to an enum variant is a breaking change
prdoc/pr_6626.prdoc
Outdated
title: coretime sale id | ||
|
||
doc: | ||
- audience: Runtime Dev |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FereMouSiopi still misses a migration.
Resolves #6354