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

Migrate the node project. #1126

Merged
merged 6 commits into from
Aug 12, 2020
Merged

Migrate the node project. #1126

merged 6 commits into from
Aug 12, 2020

Conversation

shamil-gadelshin
Copy link
Contributor

Node project was converted to the Substrate version 2.0.0rc4

Comments:

  • build scripts in both runtime and node will be changed during the work on the node_spec_builder.
  • tracing, telemetry, weights, benchmarking are out of the scope of the current release
  • runtime API, constants, primitives were moved to separate modules
  • node executor, service, RPC modules were merged in the single node crate unlike the substrate base implementation

@shamil-gadelshin shamil-gadelshin self-assigned this Aug 5, 2020
@shamil-gadelshin shamil-gadelshin mentioned this pull request Aug 5, 2020
Merged
@shamil-gadelshin shamil-gadelshin changed the base branch from substrate_version_upgrade to master August 6, 2020 07:06
@shamil-gadelshin shamil-gadelshin changed the base branch from master to substrate_version_upgrade August 6, 2020 07:06
mnaamani
mnaamani reviewed Aug 8, 2020
View reviewed changes
node/src/command.rs Show resolved Hide resolved
runtime/src/lib.rs
@@ -724,8 +609,8 @@ construct_runtime!(
Offences: pallet_offences::{Module, Call, Storage, Event},
RandomnessCollectiveFlip: pallet_randomness_collective_flip::{Module, Call, Storage},
Sudo: pallet_sudo::{Module, Call, Config<T>, Storage, Event<T>},
Contracts: pallet_contracts::{Module, Call, Config, Storage, Event<T>},
Copy link
Member

Choose a reason for hiding this comment

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

Unless we are able to control who can deploy contracts, we should we keep the Contracts module out for now until we decide we need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I'll remove all traces of the contracts pallet.

runtime/src/runtime_api.rs
};

/// The SignedExtension to the basic transaction logic.
pub type SignedExtra = (
Copy link
Member

Choose a reason for hiding this comment

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

If the contracts module is in the runtime, I think there needs to be a signed extension, something about checks on Gas?

Copy link
Contributor Author

@shamil-gadelshin shamil-gadelshin Aug 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'll remove all traces of the contracts pallet.

@mnaamani
Copy link
Member

mnaamani commented Aug 8, 2020

The node is building, runs and with a dev chain blocks are being produced. I'm testing with some scripts here: https://github.com/mnaamani/joystream/tree/iznik-api-examples/api-examples

Based on the latest types library and updated polkadot-js/api as per #1124

Having issues with submitting transactions:

2020-08-08 17:49:46        RPC-CORE: submitExtrinsic(extrinsic: Extrinsic): Hash:: 1002: Verification Error: Execution: Could not convert parameter `tx` between node and runtime: No such variant in enum MultiSignature: RuntimeApi("Execution: Could not convert parameter `tx` between node and runtime: No such variant in
Error: 1002: Verification Error: Execution: Could not convert parameter `tx` between node and runtime: No such variant in enum MultiSignature: RuntimeApi("Execution: Could not convert parameter `tx` between node and runtime: No such variant in

We may not be using the correct version of polkadot-js/api or there is a misconfiguration in the node/runtime

@mnaamani
Copy link
Member

mnaamani commented Aug 8, 2020

The travis build is taking very long time to build, and is not completing, so we may need to tweak the .travis.yml file

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received

The build has been terminated

@mnaamani
Copy link
Member

mnaamani commented Aug 8, 2020

Having issues with submitting transactions:

Perhaps I'm not properly constructing types ? @Lezek123

@Lezek123
Copy link
Contributor

Lezek123 commented Aug 8, 2020

Tried to reproduce. I used https://github.com/shamil-gadelshin/substrate-runtime-joystream/tree/migrate_runtime_module4 to start a development node and https://github.com/mnaamani/joystream/tree/iznik-api-examples/api-examples to run the scripts.

I ran:
yarn workspace api-examples script injectDataObjects
and then:
yarn workspace api-examples script listDataDirectory
But didn't run into any issues.

The output of the second one also confirmed that:
Data Directory contains 132 objects

@mnaamani
Copy link
Member

Tried to reproduce. I used https://github.com/shamil-gadelshin/substrate-runtime-joystream/tree/migrate_runtime_module4 to start a development node and https://github.com/mnaamani/joystream/tree/iznik-api-examples/api-examples to run the scripts.

I ran:
yarn workspace api-examples script injectDataObjects
and then:
yarn workspace api-examples script listDataDirectory
But didn't run into any issues.

The output of the second one also confirmed that:
Data Directory contains 132 objects

Those were the steps I followed. I tried rebuilding the node a second time but I'm still getting the same issue.

@shamil-gadelshin
Copy link
Contributor Author

Part of this PR

@Lezek123 Lezek123 mentioned this pull request Aug 11, 2020
Merged
@Lezek123
Copy link
Contributor

I was able to reproduice @mnaamani's issue by fetching the last commit, which I'm not sure I fetched before (fb9845b)

Then I re-built the runtime using 10fa8dd and the issue didn't occur anymore.

That leads me to think that fb9845b introduced some changes that are causing the mentioned error.

mnaamani
mnaamani reviewed Aug 12, 2020
View reviewed changes
runtime/src/lib.rs
@@ -110,7 +106,7 @@ impl system::Trait for Runtime {
type Hash = Hash;
type Hashing = BlakeTwo256;
type AccountId = AccountId;
type Lookup = Indices;
type Lookup = IdentityLookup<AccountId>;
Copy link
Member

@mnaamani mnaamani Aug 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

With this change, as identified by @shamil-gadelshin we were required to add "Address": "AccountId","LookupSource": "AccountId" to the types registration in polkadot-js api, which isn't done automatically by the api library unless it detects the node-template specName in the runtime version.

@mnaamani
Copy link
Member

mnaamani commented Aug 12, 2020 •
edited
Loading

That leads me to think that fb9845b introduced some changes that are causing the mentioned error.

You were correct. The change was specifically how the address lookup is done and requires a change in the types registration: https://github.com/Joystream/joystream/pull/1126/files#r469018972

@mnaamani mnaamani merged commit 46cd91c into Joystream:substrate_version_upgrade Aug 12, 2020
@Lezek123 Lezek123 mentioned this pull request Aug 12, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnaamani mnaamani mnaamani left review comments

Assignees

@shamil-gadelshin shamil-gadelshin

Labels
runtime
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shamil-gadelshin @mnaamani @Lezek123