-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Improve sc-service API
#5364
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
Improve sc-service API
#5364
Conversation
7d0fef5 to
a8b6767
Compare
|
Would
This is probably the last thing that I'd like to stop using |
…ons, introduce `sc_service::config::RpcConfiguration` to replace a bunch of flat fields
63606c8 to
9c1534b
Compare
|
The CI pipeline was cancelled due to failure one of the required jobs. |
…e-api # Conflicts: # cumulus/polkadot-parachain/src/command.rs
…e-api # Conflicts: # cumulus/polkadot-parachain/polkadot-parachain-lib/src/service.rs # cumulus/test/service/src/lib.rs # substrate/client/cli/src/config.rs # substrate/client/service/src/builder.rs # substrate/client/service/src/config.rs # substrate/client/service/src/lib.rs
|
Resolved merge conflicts after #4792 and would really appreciate if someone takes a closer look here |
dmitry-markin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes seem justified to me. Thanks again for streamlined per-commit review process!
bkchr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two nitpicks.
|
Applied suggestions |
I suppose you want to start your service without any rpc server then? Can you open an issue for it please? I don't remember why we do it but that can changed if desired also with the latest refactoring it should be possible do |
This improves `sc-service` API by not requiring the whole `&Configuration`, using specific configuration options instead. `RpcConfiguration` was also extracted from `Configuration` to group all RPC options together. We don't use Substrate's CLI and would rather not use `Configuration` either, but some key public functions require it even though they ignored most of the fields anyway. `RpcConfiguration` is very helpful not just for consolidation of the fields, but also to finally make RPC optional for our use case, while Substrate still runs RPC server on localhost even if listening address is explicitly set to `None`, which is annoying (and I suspect there is a reason for it, so didn't want to change the default just yet). While this is a breaking change, most developers will not notice it if they use higher-level APIs. Fixes #2897 --------- Co-authored-by: Niklas Adolfsson <[email protected]>
* Initialize * Companion for paritytech/polkadot-sdk#4977 * Companion for paritytech/polkadot-sdk#5078 * Companion for paritytech/polkadot-sdk#5214 * Companion for paritytech/polkadot-sdk#5431 * Companion for paritytech/polkadot-sdk#4792 * Companion for paritytech/polkadot-sdk#5613 * Companion for paritytech/polkadot-sdk#7688 * Fixes * Companion for paritytech/polkadot-sdk#5364 * Fixes * Disable storage limit & fix format issue * Benchmark only * Patch evm * Update patch info * Fixes --------- Co-authored-by: bear <[email protected]>
* Initialize * Companion for paritytech/polkadot-sdk#4977 * Companion for paritytech/polkadot-sdk#5078 * Companion for paritytech/polkadot-sdk#5214 * Companion for paritytech/polkadot-sdk#5431 * Companion for paritytech/polkadot-sdk#4792 * Companion for paritytech/polkadot-sdk#5613 * Companion for paritytech/polkadot-sdk#7688 * Fixes * Companion for paritytech/polkadot-sdk#5364 * Fixes * Disable storage limit & fix format issue * Benchmark only * Patch evm * Update patch info * Fixes --------- Co-authored-by: bear <[email protected]>
* Initialize * Companion for paritytech/polkadot-sdk#4977 * Companion for paritytech/polkadot-sdk#5078 * Companion for paritytech/polkadot-sdk#5214 * Companion for paritytech/polkadot-sdk#5431 * Companion for paritytech/polkadot-sdk#4792 * Companion for paritytech/polkadot-sdk#5613 * Companion for paritytech/polkadot-sdk#7688 * Fixes * Companion for paritytech/polkadot-sdk#5364 * Fixes * Disable storage limit & fix format issue * Benchmark only * Patch evm * Update patch info * Fixes --------- Co-authored-by: bear <[email protected]>
* Initialize * Companion for paritytech/polkadot-sdk#4977 * Companion for paritytech/polkadot-sdk#5078 * Companion for paritytech/polkadot-sdk#5214 * Companion for paritytech/polkadot-sdk#5431 * Companion for paritytech/polkadot-sdk#4792 * Companion for paritytech/polkadot-sdk#5613 * Companion for paritytech/polkadot-sdk#7688 * Fixes * Companion for paritytech/polkadot-sdk#5364 * Fixes * Disable storage limit & fix format issue * Benchmark only * Patch evm * Update patch info * Fixes --------- Co-authored-by: bear <[email protected]>
This improves
sc-serviceAPI by not requiring the whole&Configuration, using specific configuration options instead.RpcConfigurationwas also extracted fromConfigurationto group all RPC options together.We don't use Substrate's CLI and would rather not use
Configurationeither, but some key public functions require it even though they ignored most of the fields anyway.RpcConfigurationis very helpful not just for consolidation of the fields, but also to finally make RPC optional for our use case, while Substrate still runs RPC server on localhost even if listening address is explicitly set toNone, which is annoying (and I suspect there is a reason for it, so didn't want to change the default just yet).While this is a breaking change, most developers will not notice it if they use higher-level APIs.
Fixes #2897