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

feat: add embedded client back in #318

Closed
wants to merge 2 commits into from

Conversation

sander2
Copy link
Contributor

@sander2 sander2 commented Nov 10, 2021

The integration test requires a slight modification on the substrate repo, which is why this PR is currently using a fork. I will make a PR on there soon.

One unfortunate thing about this PR is that while the client is feature-gated, it is enabled by default in order to make the integration test work. The number of transitive dependencies being pulled in is quite significant. The alternative is to not have it enabled by default, but then integration tests will fail to compile unless called with e.g. --all-features.

@sander2 sander2 force-pushed the feat/embedded-client branch 3 times, most recently from 96b8f57 to 643d8e6 Compare November 11, 2021 19:08
@@ -9,7 +9,6 @@ format_code_in_doc_comments = false
comment_width = 80
normalize_comments = true # changed
normalize_doc_attributes = false
license_template_path = "FILE_TEMPLATE" # changed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to disable this, because this feature is kind of broken. I kept getting license check failed from rustfmt even after doing cat FILE_TEMPLATE src/client.rs > /tmp/f && mv /tmp/f src/client.rs

@sander2 sander2 force-pushed the feat/embedded-client branch from 643d8e6 to e064a1a Compare November 12, 2021 07:24
@sander2
Copy link
Contributor Author

sander2 commented Nov 26, 2021

@ascjones @dvdplm Could you guys have a look at this pr please?

@ascjones
Copy link
Contributor

The reasoning for removing this were:

  1. Big increase in CI build times because of the substrate dependencies, as you allude to above.
  2. Tightly coupled to substrate and jsonrpsee APIs.
  3. All the tests for it had been removed, replaced by spawning a test node (though I do see you have added one integration test back)

And generally to keep the library as lightweight as possible, in terms of amount of code to maintain, dependencies and build times etc. And also as much flexibility of supported substrate versions as possible.

I understand the benefit of being able to run an embedded light-client node for decentralization etc - so we do need a story for it, I'm just not convinced that it should necessarily exist inside this crate.

Can you explain to me how you this crate fits into your infrastructure?

@sander2
Copy link
Contributor Author

sander2 commented Nov 30, 2021

I'm just not convinced that it should necessarily exist inside this crate.

Hmm maybe if we introduce some configuration trait we can actually create it as a separate crate outside of this repo.

Can you explain to me how you this crate fits into your infrastructure?

We use it for testing, which has the advantage of not having to host the binaries for the parachain, and it helps in debugging because you can set breakpoints inside of parachain code. It's also convenient that cargo test just work without having to manually download a binary.

Having said all that, the current setup we use also has its problems. Specifically, the versions of substrate crates used in the parachain need to align with the ones used in subxt, otherwise we get compilation errors because the correct trait versions are not implemented, which is a pain to maintain.

@ascjones
Copy link
Contributor

ascjones commented Nov 30, 2021

Hmm maybe if we introduce some configuration trait we can actually create it as a separate crate outside of this repo.

By all means if we can have some abstraction here that would allow plugging in of an embedded client then I'm all for that.

Also @niklasad1 mentioned some changes that were coming to jsonrpsee that could make the glue code we had for the embedded client redundant. Hope he can provide more info.

@ascjones
Copy link
Contributor

ascjones commented Dec 2, 2021

Linking #341

@jsdw
Copy link
Collaborator

jsdw commented Feb 3, 2022

We've decided that an embedded client shouldn't form part of the subxt library at the moment owing to the maintenance burden and complexity of keeping dependencies aligned. See #341 for a little more detail.

So I'm going to close this for now.

@jsdw jsdw closed this Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants