JoshOrndorff / recipes

A Hands-On Cookbook for Aspiring Blockchain Chefs
GNU General Public License v3.0
378 stars 188 forks source link

Upgrade Recipes to Substrate 3.0 #419

Closed JoshOrndorff closed 3 years ago

JoshOrndorff commented 3 years ago

This Pull Request updates the Substrate Recipes to use the published Substrate 3.0 crates along with other ecosystem updates such as parity-scale-codec, substrate-wasm-builder, and substrate-fixed.

All code is compiling and passing tests locally. We just have to get CI sorted out. Thanks @jimmychu0807 and @sacha-l for the help.

Checklist:

JoshOrndorff commented 3 years ago

@jimmychu0807 Basically this is what I need your help with.

https://github.com/JoshOrndorff/recipes/blob/joshy-3.0/pallets/ocw-demo/Cargo.toml#L13-L14

alt_serde = { version = "1", default-features = false, features = ["derive"] }
serde_json = { version = "1", default-features = false, git = "https://github.com/Xanewok/json", branch = "no-std", features = ["alloc"] }

Ohh! Maybe this will help https://github.com/serde-rs/json/pull/606

JoshOrndorff commented 3 years ago

@thiolliere can you help me out? This PR updates a lot of code, but I only need your help on one specific test mock. I've tried to update it from Substrate 2.0 to 3.0 in commit 152dedb . As best I can tell, it looks the same as all the other test mocks in this repo. But it fails to build with:

$ cargo test -p randomness
   Compiling randomness v3.0.0 (/home/joshy/ProgrammingProjects/recipes/pallets/randomness)
error[E0277]: the trait bound `pallet_randomness_collective_flip::Module<TestRuntime>: IntegrityTest` is not satisfied
  --> pallets/randomness/src/tests.rs:13:1
   |
13 | / construct_runtime!(
14 | |     pub enum TestRuntime where
15 | |         Block = Block,
16 | |         NodeBlock = Block,
...  |
22 | |     }
23 | | );
   | |__^ the trait `IntegrityTest` is not implemented for `pallet_randomness_collective_flip::Module<TestRuntime>`
   |
   = note: required because of the requirements on the impl of `IntegrityTest` for `(pallet_randomness_collective_flip::Module<TestRuntime>,)`
   = note: 1 redundant requirements hidden
   = note: required because of the requirements on the impl of `IntegrityTest` for `(Module<TestRuntime>, (pallet_randomness_collective_flip::Module<TestRuntime>,))`
   = note: required by `sp_api_hidden_includes_decl_storage::hidden_include::traits::IntegrityTest::integrity_test`
   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
error: could not compile `randomness`

To learn more, run the command again with --verbose.
gui1117 commented 3 years ago

@JoshOrndorff it is only implemented on std, you need to import the pallet with std:

diff --git a/pallets/randomness/Cargo.toml b/pallets/randomness/Cargo.toml
index 5d050c1..12b7115 100644
--- a/pallets/randomness/Cargo.toml
+++ b/pallets/randomness/Cargo.toml
@@ -18,7 +18,7 @@ sp-runtime = { version = '3.0', default-features = false }
 sp-std = { version = '3.0', default-features = false }

 [dev-dependencies]
-pallet-randomness-collective-flip = { version = '3.0', default-features = false }
+pallet-randomness-collective-flip = { version = '3.0', default-features = false, features = ['std'] }
 sp-io = { version = '3.0', default-features = false }
 serde = "1.0"
jimmychu0807 commented 3 years ago

@JoshOrndorff Joshy thank you for the work. Will work on this.

jimmychu0807 commented 3 years ago

@JoshOrndorff, I pushed your branch to recipes here, so that I can work on another branch on top of what you have done.

jimmychu0807 commented 3 years ago

Please check PR #424

jimmychu0807 commented 3 years ago

@JoshOrndorff could you elaborate what the last point I'm wondering if all of it should be moved into the rustdocs means?

JoshOrndorff commented 3 years ago

@jimmychu0807 regarding I'm wondering if all of it should be moved into the rustdocs, I was basically referring to this long-standing issue. https://github.com/substrate-developer-hub/recipes/issues/138 But having thought about it, it is out of scope for this update. This is about getting to 3.0.

I've edited the description to reflect the almost-done state we've reached. Thanks for your help with the std issue!

jimmychu0807 commented 3 years ago

@JoshOrndorff could merge in the latest commit of joshy-3.0 in recipe to get the fix on adding key to keystore in service.rs.

sacha-l commented 3 years ago
  • [ ] Switch to FRAME V2 syntax? - @DevHubTeam would you prefer that in this PR or a followup?

I think this should be a separate followup PR 👍🏻

JoshOrndorff commented 3 years ago

Looks like the CI job is getting cancelled after it hangs for 6 hours. @TriplEight Do you know anything about this?

TriplEight commented 3 years ago

Looking at the other PRs in this repo I see it's possible to compile Substrate on those Gihub's subtle virtual machines, so I think the reason is in how do you test. Consider testing the similar way as in substrate

jimmychu0807 commented 3 years ago

I believe this:

TriplEight commented 3 years ago

I've fixed CI improving the caching. Previous setup was conflicting with "run in a container" setup. However, the testing itself takes a whopping 25 min in GHA (and on my testing machine it took under 4 minutes), this might be a sign to use a serious CI system.

jimmychu0807 commented 3 years ago

thanks @TriplEight for fixing the CI

TriplEight commented 3 years ago

I wonder why since after 8fc5203 the CI jobs became duplicated. Before: image after: image Ran an experimental branch from a fork and it was not the case. https://github.com/substrate-developer-hub/recipes/pull/431

Maybe it happened due to I committed to this PR directly (and Pr was from a @JoshOrndorff 's fork)

jimmychu0807 commented 3 years ago

@TriplEight Let see in future PRs if these CI workflow resume to be normal or run in a duplicated fashion. Don't worry about it too much for now.