Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Move sc-client into sc-service #5502

Merged
32 commits merged into from
Apr 28, 2020
Merged

Move sc-client into sc-service #5502

32 commits merged into from
Apr 28, 2020

Conversation

seunlanlege
Copy link
Contributor

see #4452

@parity-cla-bot
Copy link

It looks like @seunlanlege signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@gavofyork gavofyork added the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 3, 2020
@seunlanlege seunlanlege marked this pull request as ready for review April 8, 2020 13:55
@seunlanlege seunlanlege requested a review from cecton as a code owner April 8, 2020 15:12
@seunlanlege
Copy link
Contributor Author

EXPECTED:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error: Error type can not change between runtime apis
  --> $DIR/mock_only_one_error_type.rs:23:3
   |
23 |         type Error = u64;
   |         ^^^^
error[E0277]: the trait bound `u32: std::convert::From<std::string::String>` is not satisfied
  --> $DIR/mock_only_one_error_type.rs:15:1
   |
15 | / sp_api::mock_impl_runtime_apis! {
16 | |     impl Api<Block> for MockApi {
17 | |         type Error = u32;
18 | |
...  |
26 | |     }
27 | | }
   | | ^
   | | |
   | |_the trait `std::convert::From<std::string::String>` is not implemented for `u32`
   |   in this macro invocation
   |
   = help: the following implementations were found:
             <u32 as std::convert::From<bool>>
             <u32 as std::convert::From<char>>
             <u32 as std::convert::From<h2::frame::reason::Reason>>
             <u32 as std::convert::From<h2::frame::stream_id::StreamId>>
           and 16 others
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
ACTUAL OUTPUT:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error: Error type can not change between runtime apis
  --> $DIR/mock_only_one_error_type.rs:23:3
   |
23 |         type Error = u64;
   |         ^^^^
error[E0277]: the trait bound `u32: std::convert::From<std::string::String>` is not satisfied
  --> $DIR/mock_only_one_error_type.rs:15:1
   |
15 | / sp_api::mock_impl_runtime_apis! {
16 | |     impl Api<Block> for MockApi {
17 | |         type Error = u32;
18 | |
...  |
26 | |     }
27 | | }
   | | ^
   | | |
   | |_the trait `std::convert::From<std::string::String>` is not implemented for `u32`
   |   in this macro invocation
   |
   = help: the following implementations were found:
             <u32 as std::convert::From<bool>>
             <u32 as std::convert::From<char>>
             <u32 as std::convert::From<h2::frame::reason::Reason>>
             <u32 as std::convert::From<h2::frame::reason::Reason>>
           and 18 others
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
note: If the actual output is the correct output you can bless it by rerunning
      your test with the environment variable TRYBUILD=overwrite

note: If the actual output is the correct output you can bless it by rerunning
your test with the environment variable TRYBUILD=overwrite

So the library was updated and the stack frame has changed as a result, not sure how to go about this though.

@seunlanlege
Copy link
Contributor Author

Also this:

thread 'main' panicked at 'unable to generate rocksdb bindings: ()', /ci-cache/substrate/cargo/check-web-wasm/registry/src/git.luolix.top-1ecc6299db9ec823/librocksdb-sys-6.6.4/build.rs:30:20

@seunlanlege seunlanlege added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Apr 9, 2020
@tomusdrw tomusdrw removed their request for review April 15, 2020 07:35
@arkpar
Copy link
Member

arkpar commented Apr 19, 2020

Impressive work. @seunlanlege Are there still any pending issues with this PR? If not, let's rebase and merge.

@svyatonik Could you please check if there are any potential issues with the light client changes?

bin/node/cli/src/service.rs Show resolved Hide resolved
Comment on lines -268 to +246
-> Result<
Service<
ConcreteBlock,
ConcreteClient,
LongestChain<ConcreteBackend, ConcreteBlock>,
NetworkStatus<ConcreteBlock>,
NetworkService<ConcreteBlock, <ConcreteBlock as BlockT>::Hash>,
ConcreteTransactionPool,
OffchainWorkers<
ConcreteClient,
<ConcreteBackend as sc_client_api::backend::Backend<Block>>::OffchainStorage,
ConcreteBlock,
>
>,
ServiceError,
>
-> Result<impl AbstractService, ServiceError>
Copy link
Member

Choose a reason for hiding this comment

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

Same. Please bring it back.

@seunlanlege
Copy link
Contributor Author

Polkadot companion PR is green 🎊 🥳

Screenshot from 2020-04-26 17-56-15

@bkchr
Copy link
Member

bkchr commented Apr 26, 2020

Please make sure your polkadot changes work with cumulus.

Copy link
Contributor

@gnunicorn gnunicorn left a comment

Choose a reason for hiding this comment

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

Thanks, this is a big step forwards. Let's please restrict the actual requirements a bit and make sure that we have a patch pending for cumulus if necessary.

client/api/src/longest_chain.rs Outdated Show resolved Hide resolved
client/service/src/client/genesis.rs Show resolved Hide resolved
client/service/src/lib.rs Show resolved Hide resolved
client/service/src/lib.rs Outdated Show resolved Hide resolved
@seunlanlege
Copy link
Contributor Author

@gnunicorn is this eligible for a mergeoncegreen tag?

Copy link
Contributor

@gnunicorn gnunicorn left a comment

Choose a reason for hiding this comment

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

lgtm

@gnunicorn gnunicorn added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Apr 28, 2020
@gnunicorn
Copy link
Contributor

bot merge

@bkchr
Copy link
Member

bkchr commented Apr 28, 2020

So this works with cumulus?

@seunlanlege
Copy link
Contributor Author

So this works with cumulus?

Will have a PR for cumulus ready soon.

@ghost ghost merged commit fc6d55c into master Apr 28, 2020
@ghost ghost deleted the seun-the-great-client-refactor branch April 28, 2020 11:59
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants