Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions runtime/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ cumulus-pallet-parachain-system = { git = "https://github.com/paritytech/cumulus
cumulus-primitives-core = { git = "https://github.com/paritytech/cumulus", default-features = false, branch = "rococo-v1" }
parachain-info = { git = "https://github.com/paritytech/cumulus", default-features = false, branch = "rococo-v1" }

[dev-dependencies]
cumulus-test-relay-sproof-builder = { git = "https://github.com/paritytech/cumulus", default-features = false, branch = "rococo-v1" }
cumulus-primitives-parachain-inherent = { git = "https://github.com/paritytech/cumulus", default-features = false, branch = "rococo-v1" }

[build-dependencies]
substrate-wasm-builder = { version = "4.0.0", git = "https://github.com/paritytech/substrate", branch = "rococo-v1" }

Expand Down
71 changes: 70 additions & 1 deletion runtime/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

#![cfg(test)]

use cumulus_primitives_parachain_inherent::ParachainInherentData;
use frame_support::{
assert_noop, assert_ok,
dispatch::Dispatchable,
Expand Down Expand Up @@ -116,6 +117,10 @@ fn origin_of(account_id: AccountId) -> <Runtime as frame_system::Config>::Origin
<Runtime as frame_system::Config>::Origin::signed(account_id)
}

fn inherent_origin() -> <Runtime as frame_system::Config>::Origin {
<Runtime as frame_system::Config>::Origin::none()
}

#[test]
fn join_collator_candidates() {
ExtBuilder::default()
Expand Down Expand Up @@ -228,5 +233,69 @@ fn transfer_through_evm_to_stake() {

#[test]
fn reward_block_authors() {
assert!(true);
ExtBuilder::default()
.balances(vec![
(AccountId::from(ALICE), 2_000 * GLMR),
(AccountId::from(BOB), 1_000 * GLMR),
])
.staking(vec![
// collator
(AccountId::from(ALICE), None, 1_000 * GLMR),
// nominators
(
AccountId::from(BOB),
Some(AccountId::from(ALICE)),
500 * GLMR,
),
])
.build()
.execute_with(|| {
// set parachain inherent data
use cumulus_primitives_core::PersistedValidationData;
use cumulus_test_relay_sproof_builder::RelayStateSproofBuilder;
let sproof_builder = RelayStateSproofBuilder::default();
let (relay_parent_storage_root, relay_chain_state) =
sproof_builder.into_state_root_and_proof();
let vfp = PersistedValidationData {
relay_parent_number: 1u32,
relay_parent_storage_root,
..Default::default()
};
let parachain_inherent_data = ParachainInherentData {
validation_data: vfp,
relay_chain_state: relay_chain_state,
downward_messages: Default::default(),
horizontal_messages: Default::default(),
};
assert_ok!(
Call::ParachainSystem(ParachainSystemCall::set_validation_data(
parachain_inherent_data
))
.dispatch(inherent_origin())
);
fn set_alice_as_author() {
assert_ok!(
Call::AuthorInherent(AuthorInherentCall::set_author(AccountId::from(ALICE)))
.dispatch(inherent_origin())
);
}
for x in 2..1201 {
set_alice_as_author();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need to set alice as author if she is the only collator? Maybe set up a test with more than one collator and somehow count how many time each collator was selected as author and test that the balance reward is correct in a dynamic way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An author must be set every block or on_finalize in author-inherent panics.

Having multiple collators to test correct reward distribution is already done in parachain-staking unit tests. I think it's reasonable to add the same test here as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joelamouche This is a good chance to learn about Substrate inherents. When a collator authors a block, they have the opportunity to include inherent extrinsics. In moonbeam there are three of them (timestamp, validation_data, and author) and all three are required. It is behind-the-scenes from a user perspective, but every time a collator authors a moonbeam block, they insert these three inherents. Since Amar is mocking here, and there is no collator process to do any actual block authoring, we have to call these inherents manually. So setting the author to Alice here is mocking Alice authoring the block.

Copy link
Contributor Author

@4meta5 4meta5 Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe set up a test with more than one collator and somehow count how many time each collator was selected as author and test that the balance reward is correct in a dynamic way

I'd rather do this at the pallet level (and have done it there) where I can mock the set_author command more easily.

I added Charlie, Dave, and Eve to the set of collators in this test and none of them were included in the set of valid block authors so it kept returning the following error:

---- reward_block_authors stdout ----
thread 'reward_block_authors' panicked at 'Expected Ok(_). Got Err(
    DispatchErrorWithPostInfo {
        post_info: PostDispatchInfo {
            actual_weight: None,
            pays_fee: Pays::Yes,
        },
        error: DispatchError::Module {
            index: 17,
            error: 1,
            message: Some(
                "CannotBeAuthor",
            ),
        },
    },
)', runtime/tests/integration_test.rs:337:17

Testing multiple block authors in a runtime integration test is worthwhile, but I'd prefer for it to be pursued in a follow up...

run_to_block(x);
}
// no rewards doled out yet
assert_eq!(Balances::free_balance(AccountId::from(ALICE)), 1_000 * GLMR,);
assert_eq!(Balances::free_balance(AccountId::from(BOB)), 500 * GLMR,);
set_alice_as_author();
run_to_block(1201);
// rewards minted and distributed
assert_eq!(
Balances::free_balance(AccountId::from(ALICE)),
1000010038599992699200,
);
assert_eq!(
Balances::free_balance(AccountId::from(BOB)),
500003650399996349600,
);
});
}