Frame: Agile Coretime Broker pallet (RFC-1) by gavofyork · Pull Request #14568 · paritytech/substrate

gavofyork · 2023-07-12T19:16:57Z

The implementation of polkadot-fellows/RFCs#1.

TODO

For consideration:

Consolidation of InstaPoolHistory records as long as contributors don't change

ggwpez

Some comments... not through yet

ggwpez · 2023-08-22T11:38:41Z

+	fn leadin_factor_at(_: FixedU64) -> FixedU64 {
+		FixedU64::one()


The argument could be a Perbill to prevent underflows.

Bit ugly to pass in a Perbill and require a FixedU64 out, but sure I guess.

ggwpez · 2023-08-22T11:42:27Z

+	pub fn complete() -> Self {
+		Self([255u8; 10])
+	}
+	pub fn is_void(&self) -> bool {


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.

No opinion.

ggwpez · 2023-08-22T12:30:19Z

+	/// 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


Does this make assumptions about an ordered communication channel?
Otherwise it could be useful to also include the when into the notify_revenue message.

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).

ggwpez · 2023-08-22T12:32:36Z

+// 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.
+


Suggested change

#![deny(missing_docs)]

Looks like a good candidate for this.

ggwpez · 2023-08-22T12:40:04Z

+	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


last_until is explained, but what is until?

until is the first item of the result.

ggwpez

Only two non-nit comments about adapt_price and do_configure.

ggwpez · 2023-08-22T18:03:28Z

+			ensure!(check_owner == region.owner, Error::<T>::NotOwner);
+		}
+		let pivot = region_id.begin.saturating_add(pivot_offset);
+		ensure!(pivot < region.end, Error::<T>::PivotTooLate);


Maybe also checking that pivot is not zero?

I think with pivot_offset==0 it is possible to clone the region.
Will adda check against it.

ggwpez · 2023-08-22T18:04:49Z

+		region.owner = new_owner;
+		Regions::<T>::insert(&region_id, &region);
+		let duration = region.end.saturating_sub(region_id.begin);
+		Self::deposit_event(Event::Transferred {


I think the edge-case where the owner sends to themselves should not emit a Transferred event.

That case could be an early-exit, I guess.

ggwpez · 2023-08-22T18:46:46Z

+		let new_region_ids = (region_id, RegionId { begin: pivot, ..region_id.clone() });
+
+		Regions::<T>::insert(&new_region_ids.0, &RegionRecord { end: pivot, ..region.clone() });


Would this not cause an overlap of the regions for pivot since its the end of region 0 and the begin or region 1?

regions are begin..end, not begin..=end, so they don't include the end block.

ggwpez · 2023-08-24T16:45:38Z

+		T::PalletId::get().into_account_truncating()
+	}
+
+	pub fn sale_price(sale: &SaleInfoRecordOf<T>, now: BlockNumberFor<T>) -> BalanceOf<T> {


It has the invariant that leadin_length != 0, dont know if that is always the case.

Yes it's expected to be. But perhaps should be rewritten to be safe in case not.

ggwpez · 2023-08-24T17:03:24Z

+			_ => return Weight::zero(),
+		};
+
+		let mut meter = WeightMeter::max_limit();


I think it will need a proper weight limit eventually, or?

Yes - I guess this should be zero. @gupnik ?

Indeed, thanks for catching this. Will fix this in the monorepo.

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>

ggwpez · 2023-08-24T20:37:12Z

bot merge

gavofyork added 30 commits July 8, 2023 18:27

Add Broker pallet

bec7b68

Flesh out CorePart

c73ce75

Repotting and fleshing out

fd58ded

more drafting

4d085c1

process timeslice

7a42089

Test Fungibles completed

c17ef17

Auctions

797f70d

Price morphing

055ac7e

First tests

1949774

Tidying up config/status

afa138d

Docs

3caa44b

Timeslice todying

60fb792

More Timeslice tidying

036dd96

Tests]

8d299ca

Repotting.

99de31f

Tests

08e9eb1

Tests

6f13d32

System InstaPool cores and payout

68df2d1

Better Relay Test framework

ed57518

Tests and instapool fixes

f09cde2

Support NFT interface

bd090a1

Proper renewals

ad48ade

Better events, results

065b019

Test transfer

46852d2

Renewal test

e309594

Repot some impls and make dispatchables.

955356f

Better weight

2f20e0a

Test migration

3638f55

Document events

66b8082

Introduce durations

f9a3665

gupnik added 3 commits August 3, 2023 17:43

Fixes feature propagation

8e6072c

Fixes feature propagation

8eb3187

Adds doc fix

b863d5d

gavofyork added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Aug 22, 2023

gavofyork closed this Aug 22, 2023

gavofyork reopened this Aug 22, 2023

ggwpez reviewed Aug 22, 2023

View reviewed changes

ggwpez reviewed Aug 24, 2023

View reviewed changes

ggwpez added 13 commits August 24, 2023 19:06

Syntax nits

fbb570f

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Reuse Bit assign functions

0478e09

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Bitwise tests

7cc753f

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

adapt_price: Edge case for sold == target

6e2dd79

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Add sanity checking to ConfigRecord

94105d3

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Add deny(missing_docs) where possible

6103d1d

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

partition: forbid pivot_offset == 0

a71813d

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Merge remote-tracking branch 'origin/master' into gav-broker

2622540

Sort features

0e4b434

zepter format features Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Import Zero from new location

3df2580

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Clippy: remove redundant clone

b6bdc4f

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

try to fix build

08bb271

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Fix CI

7bced00

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

ggwpez approved these changes Aug 24, 2023

View reviewed changes

paritytech-processbot Bot merged commit 46c78d5 into master Aug 24, 2023

paritytech-processbot Bot deleted the gav-broker branch August 24, 2023 20:37

joepetrowski mentioned this pull request Sep 9, 2023

Coretime Chain paritytech/polkadot-sdk#1479

Merged

3 tasks

crystalin mentioned this pull request Oct 20, 2023

Update substrate/polkadot/cumulus from v0.9.43 to v1.1.0 moonbeam-foundation/moonbeam#2535

Closed

ggwpez mentioned this pull request Jan 2, 2024

Audit for pallet-broker paritytech/polkadot-sdk#2837

Closed

		fn leadin_factor_at(_: FixedU64) -> FixedU64 {
		FixedU64::one()

		let new_region_ids = (region_id, RegionId { begin: pivot, ..region_id.clone() });

		Regions::<T>::insert(&new_region_ids.0, &RegionRecord { end: pivot, ..region.clone() });

Conversation

gavofyork commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

ggwpez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggwpez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ggwpez Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggwpez commented Aug 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gavofyork commented Jul 12, 2023 •

edited

Loading

ggwpez Aug 24, 2023 •

edited

Loading