Frame: Agile Coretime Broker pallet (RFC-1) #14568
Conversation
gavofyork
added
A0-please_review
| fn leadin_factor_at(_: FixedU64) -> FixedU64 { | ||
| FixedU64::one() |
There was a problem hiding this comment.
The argument could be a Perbill to prevent underflows.
There was a problem hiding this comment.
Bit ugly to pass in a Perbill and require a FixedU64 out, but sure I guess.
| pub fn complete() -> Self { | ||
| Self([255u8; 10]) | ||
| } | ||
| pub fn is_void(&self) -> bool { |
There was a problem hiding this comment.
We have a Clear trait with docs "Trait for things that can be clear (have no bits set). For numeric types, essentially the same as Zero". Not sure if you want to use that, but then the clear function here would need to be renamed to unset or so.
| /// operation, the Relay-chain SHOULD send a `notify_core_count(count)` message back. | ||
| fn request_core_count(count: CoreIndex); | ||
|
|
||
| /// Requests that the Relay-chain send a `notify_revenue` message back at or soon after |
There was a problem hiding this comment.
Does this make assumptions about an ordered communication channel?
Otherwise it could be useful to also include the when into the notify_revenue message.
There was a problem hiding this comment.
Does this make assumptions about an ordered communication channel?
Yes.
when is included in the notify_revenue message (it gets returned as the first param of the check_notify_revenue_info result).
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
There was a problem hiding this comment.
| #![deny(missing_docs)] | |
Looks like a good candidate for this.
| fn check_notify_core_count() -> Option<u16>; | ||
|
|
||
| /// Provide the amount of revenue accumulated from Instantaneous Coretime Sales from Relay-chain | ||
| /// block number `last_until` to `until`, not including `until` itself. `last_until` is defined |
There was a problem hiding this comment.
last_until is explained, but what is until?
There was a problem hiding this comment.
until is the first item of the result.
| ensure!(check_owner == region.owner, Error::<T>::NotOwner); | ||
| } | ||
| let pivot = region_id.begin.saturating_add(pivot_offset); | ||
| ensure!(pivot < region.end, Error::<T>::PivotTooLate); |
There was a problem hiding this comment.
Maybe also checking that pivot is not zero?
There was a problem hiding this comment.
I think with pivot_offset==0 it is possible to clone the region.
Will adda check against it.
| region.owner = new_owner; | ||
| Regions::<T>::insert(®ion_id, ®ion); | ||
| let duration = region.end.saturating_sub(region_id.begin); | ||
| Self::deposit_event(Event::Transferred { |
There was a problem hiding this comment.
I think the edge-case where the owner sends to themselves should not emit a Transferred event.
There was a problem hiding this comment.
That case could be an early-exit, I guess.
| let new_region_ids = (region_id, RegionId { begin: pivot, ..region_id.clone() }); | ||
|
|
||
| Regions::<T>::insert(&new_region_ids.0, &RegionRecord { end: pivot, ..region.clone() }); |
There was a problem hiding this comment.
Would this not cause an overlap of the regions for pivot since its the end of region 0 and the begin or region 1?
There was a problem hiding this comment.
regions are begin..end, not begin..=end, so they don't include the end block.
| T::PalletId::get().into_account_truncating() | ||
| } | ||
|
|
||
| pub fn sale_price(sale: &SaleInfoRecordOf<T>, now: BlockNumberFor<T>) -> BalanceOf<T> { |
There was a problem hiding this comment.
It has the invariant that leadin_length != 0, dont know if that is always the case.
There was a problem hiding this comment.
Yes it's expected to be. But perhaps should be rewritten to be safe in case not.
| _ => return Weight::zero(), | ||
| }; | ||
|
|
||
| let mut meter = WeightMeter::max_limit(); |
There was a problem hiding this comment.
I think it will need a proper weight limit eventually, or?
There was a problem hiding this comment.
Yes - I guess this should be zero. @gupnik ?
There was a problem hiding this comment.
Indeed, thanks for catching this. Will fix this in the monorepo.
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
zepter format features Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
|
bot merge |
The implementation of polkadot-fellows/RFCs#1.
TODO
do_tickstuffFor consideration: