-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Rococo: ability to programatically assign slots to teams #3943
Conversation
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 to me
Still need to write tests, but requesting early feedback. |
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.
I'm probably being too harsh for a pallet that will only end up on a test chain, but I don't like that atomicity isn't kept and that a weakly bounded storage map is iterated.
Apart from that 👍 for making this happen.
ensure!( | ||
!T::Leaser::already_leased( | ||
id, | ||
current_lease_period, | ||
// Check current lease & next one | ||
current_lease_period.saturating_add( | ||
T::BlockNumber::from(2u32) | ||
.saturating_mul(T::PermanentSlotLeasePeriodLength::get().into()) | ||
) | ||
), | ||
Error::<T>::OngoingLeaseExists | ||
); |
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.
You are already placing a hard dependency on the slots
pallet, so probably you can do a more efficient check here directly with the storage struct.
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.
Using contains-key
on the leases storage map would not be enough .. then I'd have to re-implement some of the logic that's nicely encapsulated (and tested) in T::Leaser::already-leased
function. Also this call shouldn't be called that often, so I'd go for correctness over further optimisation, what you think ?
Where is the programatic part of this? You will run some off-chain bot that can call these functions? Also, might be worth just implementing a "permanent" lease into the main |
Thx @shawntabrizi, gonna revise the code as per your comments.
That was the phrasing of the initial issue, I realise now it is confusing, thank you for pointing it out.
Fair enough. I took the approach of doing it in a separate pallet, which is only active on Rococo / Westend, so as to avoid borking the slots pallet in this first iteration. I'd rather get this out in this form, and then move perm slots to the slots pallet in a separare PR, what you think ? |
Is the end result much different than https://github.com/paritytech/polkadot/blob/master/runtime/common/src/slots.rs#L164 Could just change |
Not much indeed. Then I'd remove the permanent -related stuff in favour of just using |
In general i don't see that much issue with this PR, especially if it is only going on Rococo, but the "automation" part of this PR is quite questionable :) Feels like all of this can be automated with more flexibility using a permissioned origin and some off-chain application. |
One could argue anything can be automated with more flexibility using a permissioned origin and some off-chain app 😉 |
@shawntabrizi Anything else you'd like to see revised ? |
* master: (23 commits) Fix path of the polkadot_injected docker image (#4463) update docs on `validation_upgrade_frequency` (#4460) Fix path of the polkadot sha256 (#4458) Fix cumulus companion CI job (#4451) Add doc about runtime version bump (#4418) Bump trybuild from 1.0.52 to 1.0.53 (#4455) companion for #10231 (#4306) trivial fix (#4441) Rococo: ability to programatically assign slots to teams (#3943) Add parent header hash to log (#4421) bump tx versions (#4447) Companion for substrate/10347 (#4413) Improve paras runtime `BenchBuilder` api (#4318) Add CI team to `CODEOWNERS` file (#4350) XCM Benchmarks for Generic Instructions (#3940) Announce only on releases (#4417) Companion for 10379 (EnsureOneOf) (#4405) add pallet-babe/std (#4438) update Cargo.lock Squashed 'bridges/' changes from 407bf44a8a..1602249f0a ...
@stiiifff please remember to update the Substrate docs on the new process you and your team has established: https://docs.substrate.io/tutorials/v3/cumulus/rococo/ |
Thx ! I had not seen that issue, I will update the docs. |
Resolves #3942
Added new
assigned_slots
pallet with calls to assign permanent or temporary parachain slots on Rococo:Flow
parasSudoWrapper.sudoScheduleParaInitialize
) and onboarded as a parathread.assignedSlots.assignTempParachainSlot
, specifying the para Id and lease period start (Current lease period or next one).assigned_slots
pallet keeps track of temporary slots and distributes turns fairly between temp slots (best effort).assignedSlots.assignPermParachainSlot
or unassign a slot (permanent or temporary) withassignedSlots.unassignParachainSlot
(this clears any lease which downgrades the para back to a parathread).assigned_slots
pallet) relies on the existing slot lease mechanism in theslots
pallet.