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

Jump to bottom

Broker pallet: RegionDropped event fix & additional tests #1609

Merged
merged 5 commits into from
Sep 18, 2023
Merged

Broker pallet: RegionDropped event fix & additional tests #1609

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
12a1dc8
minor fixes
Szegoo Sep 17, 2023
d653f39
add interlacing test
Szegoo Sep 17, 2023
9dd2ac6
partitioning test
Szegoo Sep 17, 2023
397f17f
add insta pool payout test
Szegoo Sep 18, 2023
dd05a5e
revert accidental change
Szegoo Sep 18, 2023
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
4 changes: 2 additions & 2 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 @@ -76,7 +76,7 @@ impl<T: Config> Pallet<T> {
last_timeslice: Self::current_timeslice(),
};
let now = frame_system::Pallet::<T>::block_number();
let dummy_sale = SaleInfoRecord {
let new_sale = SaleInfoRecord {
sale_start: now,
leadin_length: Zero::zero(),
price,
Expand All @@ -89,7 +89,7 @@ impl<T: Config> Pallet<T> {
cores_sold: 0,
};
Self::deposit_event(Event::<T>::SalesStarted { price, core_count });
Self::rotate_sale(dummy_sale, &config, &status);
Self::rotate_sale(new_sale, &config, &status);
Status::<T>::put(&status);
Ok(())
}
Expand Down
119 changes: 116 additions & 3 deletions substrate/frame/broker/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,9 @@ fn drop_renewal_works() {
let e = Error::<Test>::StillValid;
assert_noop!(Broker::do_drop_renewal(region.core, region.begin + 3), e);
advance_to(12);
assert_eq!(AllowedRenewals::<Test>::iter().count(), 1);
assert_ok!(Broker::do_drop_renewal(region.core, region.begin + 3));
assert_eq!(AllowedRenewals::<Test>::iter().count(), 0);
let e = Error::<Test>::UnknownRenewal;
assert_noop!(Broker::do_drop_renewal(region.core, region.begin + 3), e);
});
Expand All @@ -90,7 +92,10 @@ fn drop_contribution_works() {
advance_to(2);
let region = Broker::do_purchase(1, u64::max_value()).unwrap();
// Place region in pool. Active in pool timeslices 4, 5, 6 = rcblocks 8, 10, 12; we
// expect the contribution record to timeout 3 timeslices following 7 = 10
// expect the contribution record to timeout 3 timeslices following 7 = 14
//
// Due to the contribution_timeout being configured for 3 timeslices, the contribution
// can only be discarded at timeslice 10, i.e. rcblock 20.
assert_ok!(Broker::do_pool(region, Some(1), 1, Final));
assert_eq!(InstaPoolContribution::<Test>::iter().count(), 1);
advance_to(19);
Expand Down Expand Up @@ -378,6 +383,41 @@ fn instapool_partial_core_payouts_work() {
});
}

#[test]
fn instapool_core_payouts_work_with_partitioned_region() {
TestExt::new().endow(1, 1000).execute_with(|| {
assert_ok!(Broker::do_start_sales(100, 1));
advance_to(2);
let region = Broker::do_purchase(1, u64::max_value()).unwrap();
let (region1, region2) = Broker::do_partition(region, None, 2).unwrap();
// `region1` duration is from rcblock 8 to rcblock 12. This means that the
// coretime purchased during this time period will be purchased from `region1`
//
// `region2` duration is from rcblock 12 to rcblock 14 and during this period
// coretime will be purchased from `region2`.
assert_ok!(Broker::do_pool(region1, None, 2, Final));
assert_ok!(Broker::do_pool(region2, None, 3, Final));
assert_ok!(Broker::do_purchase_credit(1, 20, 1));
advance_to(8);
assert_ok!(TestCoretimeProvider::spend_instantaneous(1, 10));
advance_to(11);
assert_eq!(pot(), 20);
assert_eq!(revenue(), 100);
assert_ok!(Broker::do_claim_revenue(region1, 100));
assert_eq!(pot(), 10);
assert_eq!(balance(2), 10);
advance_to(12);
assert_ok!(TestCoretimeProvider::spend_instantaneous(1, 10));
advance_to(15);
assert_eq!(pot(), 10);
assert_ok!(Broker::do_claim_revenue(region2, 100));
assert_eq!(pot(), 0);
// The balance of account `2` remains unchanged.
assert_eq!(balance(2), 10);
assert_eq!(balance(3), 10);
});
}

#[test]
fn initialize_with_system_paras_works() {
TestExt::new().execute_with(|| {
Expand Down Expand Up @@ -654,6 +694,79 @@ fn partition_then_interlace_works() {
});
}

#[test]
fn partitioning_after_assignment_works() {
TestExt::new().endow(1, 1000).execute_with(|| {
assert_ok!(Broker::do_start_sales(100, 1));
advance_to(2);
// We will initially allocate a task to a purchased region, and after that
// we will proceed to partition the region.
let region = Broker::do_purchase(1, u64::max_value()).unwrap();
assert_ok!(Broker::do_assign(region, None, 1001, Provisional));
let (_region, region1) = Broker::do_partition(region, None, 2).unwrap();
// After the partitioning if we assign a new task to `region` the other region
// will still be assigned to `Task(1001)`.
assert_ok!(Broker::do_assign(region1, None, 1002, Provisional));
advance_to(10);
assert_eq!(
CoretimeTrace::get(),
vec![
(
6,
AssignCore {
core: 0,
begin: 8,
assignment: vec![(Task(1001), 57600),],
end_hint: None
}
),
(
10,
AssignCore {
core: 0,
begin: 12,
assignment: vec![(Task(1002), 57600),],
end_hint: None
}
),
]
);
});
}

#[test]
fn interlacing_after_assignment_works() {
TestExt::new().endow(1, 1000).execute_with(|| {
assert_ok!(Broker::do_start_sales(100, 1));
advance_to(2);
// We will initially allocate a task to a purchased region, and after that
// we will proceed to interlace the region.
let region = Broker::do_purchase(1, u64::max_value()).unwrap();
assert_ok!(Broker::do_assign(region, None, 1001, Provisional));
let (region1, _region) =
Broker::do_interlace(region, None, CoreMask::from_chunk(0, 40)).unwrap();
// Interlacing the region won't affect the assignment. The entire region will still
// be assigned to `Task(1001)`.
//
// However, after we assign a task to `region1` the `_region` won't be assigned
// to `Task(1001)` anymore. It will become idle.
assert_ok!(Broker::do_assign(region1, None, 1002, Provisional));
advance_to(10);
assert_eq!(
CoretimeTrace::get(),
vec![(
6,
AssignCore {
core: 0,
begin: 8,
assignment: vec![(Idle, 28800), (Task(1002), 28800)],
end_hint: None
}
),]
);
});
}

#[test]
fn reservations_are_limited() {
TestExt::new().execute_with(|| {
Expand Down Expand Up @@ -866,7 +979,7 @@ fn assign_should_drop_invalid_region() {
advance_to(10);
assert_ok!(Broker::do_assign(region, Some(1), 1001, Provisional));
region.begin = 7;
System::assert_last_event(Event::RegionDropped { region_id: region, duration: 0 }.into());
System::assert_last_event(Event::RegionDropped { region_id: region, duration: 3 }.into());
});
}

Expand All @@ -879,7 +992,7 @@ fn pool_should_drop_invalid_region() {
advance_to(10);
assert_ok!(Broker::do_pool(region, Some(1), 1001, Provisional));
region.begin = 7;
System::assert_last_event(Event::RegionDropped { region_id: region, duration: 0 }.into());
System::assert_last_event(Event::RegionDropped { region_id: region, duration: 3 }.into());
});
}

Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/broker/src/utility_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,9 @@ impl<T: Config> Pallet<T> {

let last_committed_timeslice = status.last_committed_timeslice;
if region_id.begin <= last_committed_timeslice {
let duration = region.end.saturating_sub(region_id.begin);
region_id.begin = last_committed_timeslice + 1;
if region_id.begin >= region.end {
let duration = region.end.saturating_sub(region_id.begin);
Self::deposit_event(Event::RegionDropped { region_id, duration });
return Ok(None)
}
Expand Down
Loading