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

Expose types from sc-service #5855

Merged
merged 20 commits into from
Nov 29, 2024

Conversation

RomarQ
Copy link
Contributor

@RomarQ RomarQ commented Sep 27, 2024

Description

At moonbeam we have worked on a lazy-loading feature which is a client mode that forks a live parachain and fetches its state on-demand, we have been able to do this by duplicating some code from sc_service::client. The objective of this PR is to simplify the implementation by making public some types in polkadot-sdk.

  • Modules:
    • sc_service::client I do not see a point to only expose this type when test-helpers feature is enabled

Integration

Not applicable, the PR just makes some types public.

Review Notes

The changes included in this PR give more flexibility for client developers by exposing important types.

@RomarQ RomarQ changed the title Expose client module from sc-service Expose types from sc-service Sep 27, 2024
@bkchr
Copy link
Member

bkchr commented Sep 27, 2024

  • sc_service::client I do not see a point to only expose this type when test-helpers feature is enabled

This feature should not exist at all...

Could you give some more details on how you implemented this? I mean why do you need to expose these types at all? I would say that you only need to write some Backend that fetches the values on demand (for sure it would not be async and whatever, but we don't support this any way right now).

@RomarQ
Copy link
Contributor Author

RomarQ commented Sep 30, 2024

  • sc_service::client I do not see a point to only expose this type when test-helpers feature is enabled

This feature should not exist at all...

Could you give some more details on how you implemented this? I mean why do you need to expose these types at all? I would say that you only need to write some Backend that fetches the values on demand (for sure it would not be async and whatever, but we don't support this any way right now).

We had to change the CallExecutor because of some validation in contextual_call, but there was no way to pass it to a Client instance without the use of test-helpers feature.

@bkchr
Copy link
Member

bkchr commented Oct 15, 2024

`` because of some validation in contextual_call

Which validation? Could you be more specific?

@RomarQ
Copy link
Contributor Author

RomarQ commented Nov 18, 2024

`` because of some validation in contextual_call

Which validation? Could you be more specific?

It was the as_trie_backend when recording is enabled, we cannot convert the lazy loading to a trie backend since we only have access to the state on-demand.

@bkchr
Copy link
Member

bkchr commented Nov 18, 2024

Dumb question, could you not just use a custom CallExecutor that wraps the LocalCallExecutor and always forwards recorder as None?

@RomarQ
Copy link
Contributor Author

RomarQ commented Nov 18, 2024

Dumb question, could you not just use a custom CallExecutor that wraps the LocalCallExecutor and always forwards recorder as None?

The Client struct is still not public without the use of test-helpers feature. About wrapping LocalCallExecutor, it is for sure possible, I have updated the PR.

@bkchr
Copy link
Member

bkchr commented Nov 18, 2024

Can you please remove the test-helpers feature completely?

@RomarQ
Copy link
Contributor Author

RomarQ commented Nov 18, 2024

Can you please remove the test-helpers feature completely?

Sure, will have a look at it 👍

@bkchr bkchr added the T0-node This PR/Issue is related to the topic “node”. label Nov 19, 2024
@bkchr bkchr requested a review from a team November 19, 2024 09:04
@bkchr
Copy link
Member

bkchr commented Nov 19, 2024

/cmd prdoc --bump major --audience node_dev

Copy link

Command "prdoc --bump major --audience node_dev" has failed ❌! See logs here

substrate/client/service/src/lib.rs Outdated Show resolved Hide resolved
@RomarQ
Copy link
Contributor Author

RomarQ commented Nov 19, 2024

/cmd prdoc --bump major --audience node_dev

added the PRdoc

@RomarQ
Copy link
Contributor Author

RomarQ commented Nov 25, 2024

@bkchr can this PR be merged?

@bkchr bkchr added this pull request to the merge queue Nov 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 25, 2024
@dmitry-markin dmitry-markin added this pull request to the merge queue Nov 29, 2024
Merged via the queue into paritytech:master with commit 72fb8bd Nov 29, 2024
191 of 198 checks passed
@RomarQ RomarQ deleted the rq/expose-client branch November 29, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants