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.

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

Make it actually work #2

Closed
wants to merge 17 commits into from
Closed

Make it actually work #2

wants to merge 17 commits into from

Conversation

seunlanlege
Copy link
Contributor

Adds manual seal and a test runtime, will make it generic over the runtime once we confirm that it works in tests.

Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm!

src/node.rs Outdated
}

let _ = builder.is_test(true).try_init();
pub fn build_logger() -> Logger {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it pub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah my mistake

src/node.rs Outdated
pub fn build_node(config: Configuration) -> Result<(TaskManager, RpcHandlers), sc_service::Error> {
// Channel for the rpc handler to communicate with the authorship task.
let (command_sink, commands_stream) = futures::channel::mpsc::channel(10);
pub fn start_node<Block, RuntimeApi, Executor, F>(cli_args: &[&str], spec_factory: F)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not InternalNode::new?

pub fn new(node: InternalNode<Runtime>) -> Self {
Self { node }
impl<Runtime: frame_system::Trait> Deterministic<Runtime> {
pub fn new(node: InternalNode) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of preferred when InternalNode had Runtime generic type as well so it was type checked. Now it seems you can create any kind of InternalNode and then attempt to use it as any Runtime when wrapped in Deterministic.

As discussed privately, we might want to introduce some trait that will bound Runtime and the types required to construct InternalNode: Block, RuntimeApi, Executor, ChainSpecFactory so that both are used together.

Fine with this approach for now though.


test.produce_blocks(15);
test.assert_log_line("db", "best = true");
// test.assert_log_line("db", "best = true");
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this work again? This is a useful way to test very abstract stuff that might be hard to test otherwise.

type Balances = pallet_balances::Module<Runtime>;

test.produce_blocks(1);

let alice = Sr25519Keyring::Alice.pair();
let alice = Sr25519Keyring::Charlie.pair();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let alice = Sr25519Keyring::Charlie.pair();
let charlie = Sr25519Keyring::Charlie.pair();

let events = frame_system::Module::<Runtime>::events();
log::info!("{:#?}", events);
(
Balances::free_balance(MultiSigner::from(bob.public()).into_account()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems we always only use bob.public() or alice.public() can't we get it at the declaration level to simplify the code?

)
});

// FIXME: alice' balance doesnt change lmao
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that's fixed now after swtiching to Charlie?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nahhh, the problem still persists, I suspect that the balance is cached in the overlay but I have no way of testing this hypothesis.

Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

looks good, merge at will after fixing the grumbles.

Seems that the offchain test is failing:

thread 'should_run_off_chain_worker' panicked at 'No logs from db module.', /rustc/d3fb005a39e62501b8b0b356166e515ae24e2e54/src/libstd/macros.rs:16:9


[workspace]
members = [
"runtime"
]

[build-dependencies]
substrate-build-script-utils = { git = "https://github.com/paritytech/substrate.git", branch = "seun-substrate-test-runner" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind updating to master?

@@ -0,0 +1,8 @@
use substrate_build_script_utils::{generate_cargo_keys, rerun_if_git_head_changed};
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this thing doing exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nothing now that cli has been removed

@@ -133,7 +136,7 @@ impl system::Trait for Runtime {
/// The aggregated dispatch type that is available for extrinsics.
type Call = Call;
/// The lookup mechanism to get account ID from whatever is passed in dispatchers.
type Lookup = IdentityLookup<AccountId>;
type Lookup = Indices;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are Indices strictly necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably not

})
.ok()?;
let signature = raw_payload.using_encoded(|payload| C::sign(payload, public))?;
let address = Indices::unlookup(account);
Copy link
Contributor

Choose a reason for hiding this comment

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

Without Indices it should be just let address = account;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh, didn't know that

let (client, backend, keystore, mut task_manager) =
new_full_parts::<Node::OpaqueBlock, Node::RuntimeApi, Node::Executor>(&config)?;
let client = Arc::new(client);
let import_queue = manual_seal::import_queue(Box::new(client.clone()), &task_manager.spawn_handle(), None);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to avoid this initialization code? Can we plug into some existing bootstrap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, we're setting up a node with manual seal, there's, unfortunately, no bootstrap available yet for setting up a node.

@seunlanlege seunlanlege closed this Sep 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants