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

Migrate Runtime to the Substrate version 2.0.0 rc4 #1075

Conversation

shamil-gadelshin
Copy link
Contributor

@shamil-gadelshin shamil-gadelshin commented Jul 29, 2020

Converted the runtime crate and all pallets.

# Conflicts:
#	Cargo.lock
#	runtime-modules/hiring/src/mock.rs
#	runtime-modules/recurring-reward/src/mock/mod.rs
#	runtime-modules/token-minting/src/mock.rs
Copy link
Member

@mnaamani mnaamani left a comment

Choose a reason for hiding this comment

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

So tremendous job doing this conversion.
I left a few questions really. No major change requests everything seems to be in order.
Build and tests all going well.
Out of curiosity I did try to do a cargo update and see if a build with latest dependencies works, but I did run into the libp2p build error.
substrate rc5 is out and it does bump version of libp2p, perhaps it will help.

pub fn run_to_block(n: u64) {
while System::block_number() < n {
<System as OnFinalize<u64>>::on_finalize(System::block_number());
<ContentWorkingGroup as OnFinalize<u64>>::on_finalize(System::block_number());
Copy link
Member

Choose a reason for hiding this comment

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

should we also be doing this for other modules that are part of the this test runtime, like Balances and Minting?


//TODO: Convert errors to the Substrate decl_error! macro.
/// Result with string error message. This exists for backward compatibility purpose.
pub type DispatchResult = Result<(), &'static str>;
Copy link
Member

Choose a reason for hiding this comment

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

This type alias is defined in each pallet lib.rs
Is there a particular reason you didn't just put it once in the common crate ?

Copy link
Member

Choose a reason for hiding this comment

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

I think I understand now why you are keeping it local.

runtime-modules/proposals/codex/src/lib.rs Show resolved Hide resolved
}

pub type Extrinsic = TestXt<Call, ()>;

/*
Copy link
Member

Choose a reason for hiding this comment

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

Can this commented code be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure (conversion artifact).

runtime/Cargo.toml Show resolved Hide resolved
pub const IndexDeposit: Balance = 0; // no minimum deposit
}

impl pallet_indices::Trait for Runtime {
Copy link
Member

Choose a reason for hiding this comment

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

I've always chosen not to include the account indices module because of its behavior when the minimum existential deposit was configured to be zero. Basically it would allow re-use of an index for new accounts which can be undesirable and lead to some confusion on the part of users.

use working_group::{Instance2, Worker};

#[test]
fn storage_provider_helper_succeeds() {
initial_test_ext().execute_with(|| {
// Bug in random module requires move the initial block number.
Copy link
Member

Choose a reason for hiding this comment

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

does the random module seed itself with some initial state from genesis or simply the block number? I imagine that is part of how it can be deterministic?

runtime/src/migration.rs Show resolved Hide resolved
runtime/src/lib.rs Show resolved Hide resolved
@shamil-gadelshin
Copy link
Contributor Author

shamil-gadelshin commented Aug 5, 2020

Part of this PR

@mnaamani
Copy link
Member

mnaamani commented Aug 5, 2020

Thanks for taking time to answer my questions.
Merging despite travis being unhappy, but that is okay because the node hasn't been updated in this PR.

@mnaamani mnaamani merged commit d7f8e51 into Joystream:substrate_version_upgrade Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants