Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Commit

Permalink
Cater for edge case of heavier being head
Browse files Browse the repository at this point in the history
  • Loading branch information
@emostov
emostov committed Sep 23, 2021
1 parent aa0aaeb commit fa1a33d
Show file tree
Hide file tree
Showing 3 changed files with 169 additions and 130 deletions.
85 changes: 51 additions & 34 deletions frame/bags-list/src/list/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -395,58 +395,75 @@ impl<T: Config> List<T> {
use frame_support::ensure;

let lighter_node = Node::<T>::get(&lighter_id).ok_or(pallet::Error::IdNotFound)?;
let mut heavier_node = Node::<T>::get(&heavier_id).ok_or(pallet::Error::IdNotFound)?;
let heavier_node = Node::<T>::get(&heavier_id).ok_or(pallet::Error::IdNotFound)?;

ensure!(lighter_node.bag_upper == heavier_node.bag_upper, pallet::Error::NotInSameBag);

// this is the most expensive check, so we do it last.
ensure!(weight_of(&heavier_id) > weight_of(&lighter_id), pallet::Error::NotHeavier);

// we need to write the bag to storage if its head and/or tail is updated.
if lighter_node.is_terminal() || heavier_node.is_terminal() {
let mut bag = Bag::<T>::get(lighter_node.bag_upper).ok_or_else(|| {
debug_assert!(false, "bag that should exist cannot be found");
crate::log!(warn, "bag that should exist cannot be found");
pallet::Error::BagNotFound
})?;
debug_assert!(bag.iter().count() > 1);
// remove the heavier node from this list. Note that this removes the node from storage and
// decrements the node counter.
Self::remove(&heavier_id);

if bag.head.as_ref() == Some(lighter_id) {
debug_assert!(lighter_node.next.is_some(), "lighter node must have next if head");
bag.head = Some(heavier_id.clone());
}
// re-fetch `lighter_node` from storage since it may have been updated when `heavier_node`
// was removed.
let lighter_node = Node::<T>::get(&lighter_id).ok_or(pallet::Error::IdNotFound)?;

// re-assign bag tail if lighter is the tail
if bag.tail.as_ref() == Some(heavier_id) {
debug_assert!(heavier_node.prev.is_some(), "heavier node must have prev if tail");
bag.tail = heavier_node.prev.clone();
}
Self::insert_at(lighter_node, heavier_node)?;

// within this block we know the bag must have been updated, so we update storage.
bag.put()
}
// since `insert_at` was successful, increment the counter, which got decremented by
// `Self::remove`.
crate::CounterForListNodes::<T>::mutate(|prev_count| {
*prev_count = prev_count.saturating_add(1)
});

// cut heavier out of the list, updating its neighbors.
heavier_node.excise();
Ok(())
}

// re-fetch `lighter_node` from storage since it may have been updated when `heavier_node` was
// excised.
let mut lighter_node = Node::<T>::get(&lighter_id).ok_or(pallet::Error::IdNotFound)?;
/// Insert `node` directly in front of `at`.
fn insert_at(mut at: Node<T>, mut node: Node<T>) -> Result<(), crate::pallet::Error<T>> {
use crate::pallet;

// connect `heavier_node` to its new `prev`.
if let Some(mut prev) = lighter_node.prev() {
prev.next = Some(heavier_id.clone());
node.prev = at.prev.clone();
if let Some(mut prev) = at.prev() {
prev.next = Some(node.id().clone());
prev.put()
}
heavier_node.prev = lighter_node.prev;

// connect `heavier_node` and `lighter`.
heavier_node.next = Some(lighter_id.clone());
lighter_node.prev = Some(heavier_id.clone());
// connect `node` and `at`.
node.next = Some(at.id().clone());
at.prev = Some(node.id().clone());

if at.is_terminal() || node.is_terminal() {
// `at` is the tail and/or `node` is the head, so we make sure the bag is updated. Note,
// since `node` is always in front of `at` we know that 1) there is always at least 2
// nodes in the bag, and 2) only `node` could be the head and only `at` could be the
// tail.
let mut bag = Bag::<T>::get(at.bag_upper).ok_or_else(|| {
debug_assert!(false, "bag that should exist cannot be found");
crate::log!(warn, "bag that should exist cannot be found");
pallet::Error::BagNotFound
})?;

if node.prev == None {
bag.head = Some(node.id().clone())
}

if at.next == None {
bag.tail = Some(at.id().clone())
}

debug_assert!(at.prev.is_some(), "`at` cannot be the head");
debug_assert!(node.next.is_some(), "`node` cannot be the tail");

bag.put()
};

// write the updated nodes to storage.
lighter_node.put();
heavier_node.put();
at.put();
node.put();

Ok(())
}
Expand Down
5 changes: 1 addition & 4 deletions frame/bags-list/src/list/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -376,10 +376,7 @@ mod list {
}

#[test]
#[cfg_attr(
debug_assertions,
should_panic = "bag that should exist cannot be found"
)]
#[cfg_attr(debug_assertions, should_panic = "bag that should exist cannot be found")]
fn put_in_front_of_exits_early_if_bag_not_found() {
ExtBuilder::default().build_and_execute_no_post_check(|| {
let node_710_no_bag =
Expand Down
209 changes: 117 additions & 92 deletions frame/bags-list/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ mod pallet {
}

#[test]
fn put_in_front_of_with_two_node_bag() {
fn put_in_front_of_two_node_bag() {
ExtBuilder::default().add_ids(vec![(710, 15), (711, 16)]).build_and_execute(|| {
// given
assert_eq!(
Expand All @@ -221,122 +221,147 @@ mod pallet {
}

#[test]
fn put_in_front_of_with_non_terminal_nodes() {
ExtBuilder::default()
.add_ids(vec![(11, 20), (710, 15), (711, 16), (12, 20)])
.build_and_execute(|| {
// given
assert_eq!(
List::<Runtime>::get_bags(),
vec![(10, vec![1]), (20, vec![11, 710, 711, 12]), (1_000, vec![2, 3, 4])]
);

assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);

// when
assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711));

// then
assert_eq!(
List::<Runtime>::get_bags(),
vec![(10, vec![1]), (20, vec![11, 711, 710, 12]), (1_000, vec![2, 3, 4])]
);
});
fn put_in_front_of_two_node_bag_heavier_is_head() {
ExtBuilder::default().add_ids(vec![(711, 16), (710, 15)]).build_and_execute(|| {
// given
assert_eq!(
List::<Runtime>::get_bags(),
vec![(10, vec![1]), (20, vec![711, 710]), (1_000, vec![2, 3, 4])]
);

assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);

// when
assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711));

// then
assert_eq!(
List::<Runtime>::get_bags(),
vec![(10, vec![1]), (20, vec![711, 710]), (1_000, vec![2, 3, 4])]
);
});
}

#[test]
fn put_in_front_of_with_lighter_as_head() {
ExtBuilder::default()
.add_ids(vec![(710, 15), (711, 16), (12, 20)])
.build_and_execute(|| {
// given
assert_eq!(
List::<Runtime>::get_bags(),
vec![(10, vec![1]), (20, vec![710, 711, 12]), (1_000, vec![2, 3, 4])]
);

assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);

// when
assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711));

// then
assert_eq!(
List::<Runtime>::get_bags(),
vec![(10, vec![1]), (20, vec![711, 710, 12]), (1_000, vec![2, 3, 4])]
);
});
fn put_in_front_of_head_and_tail_non_terminal_nodes() {
ExtBuilder::default().add_ids(vec![(5, 1_000)]).build_and_execute(|| {
// given
assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]);

StakingMock::set_vote_weight_of(&4, 1_000);
StakingMock::set_vote_weight_of(&3, 999);

// when
assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 3, 4));

// then
assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 4, 3, 5])]);
});
}

#[test]
fn put_in_front_of_with_heavier_as_tail() {
ExtBuilder::default()
.add_ids(vec![(11, 20), (710, 15), (711, 16)])
.build_and_execute(|| {
// given
assert_eq!(
List::<Runtime>::get_bags(),
vec![(10, vec![1]), (20, vec![11, 710, 711]), (1_000, vec![2, 3, 4])]
);

assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);

// when
assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711));

// then
assert_eq!(
List::<Runtime>::get_bags(),
vec![(10, vec![1]), (20, vec![11, 711, 710]), (1_000, vec![2, 3, 4])]
);
});
fn put_in_front_of_lighter_is_head_heavier_is_non_terminal() {
ExtBuilder::default().build_and_execute(|| {
// given
assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);

StakingMock::set_vote_weight_of(&3, 1_000);
StakingMock::set_vote_weight_of(&2, 999);

// when
assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 2, 3));

// then
assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![3, 2, 4])]);
});
}

#[test]
fn put_in_front_of_with_heavier_and_lighter_terminal_multiple_middle_nodes() {
ExtBuilder::default()
.add_ids(vec![(710, 15), (11, 20), (12, 20), (711, 15)])
.build_and_execute(|| {
// given
assert_eq!(
List::<Runtime>::get_bags(),
vec![(10, vec![1]), (20, vec![710, 11, 12, 711]), (1_000, vec![2, 3, 4])]
);

assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&710), 15);
assert_eq!(<Runtime as Config>::VoteWeightProvider::vote_weight(&711), 16);

// when
assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 710, 711));

// then
assert_eq!(
List::<Runtime>::get_bags(),
vec![(10, vec![1]), (20, vec![711, 710, 11, 12]), (1_000, vec![2, 3, 4])]
);
});
fn put_in_front_of_heavier_is_tail_lighter_is_non_terminal() {
ExtBuilder::default().build_and_execute(|| {
// given
assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);

StakingMock::set_vote_weight_of(&4, 1_000);
StakingMock::set_vote_weight_of(&3, 999);

// when
assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 3, 4));

// then
assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 4, 3])]);
});
}

#[test]
fn heavier_is_head_lighter_is_not_terminal() {
fn put_in_front_of_heavier_is_tail_lighter_is_head() {
ExtBuilder::default().add_ids(vec![(5, 1_000)]).build_and_execute(|| {
// given
assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]);

StakingMock::set_vote_weight_of(&4, 1_000);
StakingMock::set_vote_weight_of(&5, 1_000);
StakingMock::set_vote_weight_of(&2, 999);

// when
BagsList::put_in_front_of(Origin::signed(0), 2, 4).unwrap();
assert_ok!(BagsList::put_in_front_of(Origin::signed(0), 2, 5));

// then
assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![5, 2, 3, 4])]);
});
}

#[test]
fn put_in_front_of_heavier_is_head_lighter_is_not_terminal() {
ExtBuilder::default().add_ids(vec![(5, 1_000)]).build_and_execute(|| {
// given
assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]);

StakingMock::set_vote_weight_of(&2, 1_000);
StakingMock::set_vote_weight_of(&4, 999);

// when
BagsList::put_in_front_of(Origin::signed(0), 4, 2).unwrap();

// then
assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![3, 2, 4, 5])]);
});
}

#[test]
fn put_in_front_of_lighter_is_tail_heavier_is_not_terminal() {
ExtBuilder::default().add_ids(vec![(5, 900)]).build_and_execute(|| {
// given
assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]);

StakingMock::set_vote_weight_of(&5, 900);
StakingMock::set_vote_weight_of(&3, 1_000);

// when
BagsList::put_in_front_of(Origin::signed(0), 5, 3).unwrap();

// then
assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 4, 3, 5])]);
});
}

#[test]
fn put_in_front_of_lighter_is_tail_heavier_is_head() {
ExtBuilder::default().build_and_execute(|| {
// given
assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);

StakingMock::set_vote_weight_of(&2, 1_000);
StakingMock::set_vote_weight_of(&4, 999);

// when
BagsList::put_in_front_of(Origin::signed(0), 4, 2).unwrap();

// then
assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![3, 2, 4])]);
});
}

#[test]
fn put_in_front_of_errors_if_heavier_is_less_than_lighter() {
ExtBuilder::default().add_ids(vec![(711, 16), (710, 15)]).build_and_execute(|| {
Expand Down

0 comments on commit fa1a33d

Please sign in to comment.