-
Notifications
You must be signed in to change notification settings - Fork 254
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
Support embedded full/light node clients. #91
Conversation
4abcbeb
to
bf7b839
Compare
10d4b69
to
9b475a2
Compare
any ideas how to write tests for this? |
4a6c28f
to
a1f1202
Compare
1668bf7
to
678a805
Compare
@ascjones can you please review? While the light client still doesn't work, it now also supports spawning full nodes, which means we can start working on removing the |
db612aa
to
c9b942d
Compare
Enabled all non subxt tests to use the node-template from git. They are no longer |
I'll have a look tomorrow |
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.
Looks good, though I'd like a bit more documentation describing what is going on i.e. embedding a full/light node into the client itself. And whether it is just intended for testing etc. Perhaps a short section in the readme would be good.
Also in the future we could define our own local test runtime (as you have previously suggested) which includes contracts
.
One downside here is that running the tests will now take considerably longer (CI time has more than doubled to 25 minutes), but at least the tests are running now on CI.
@ascjones added a README section |
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.
I'm hesitant to merge this at the moment because of the massive increase in compile time, although it is massively useful because it allows tests to be run on CI (and I like the idea of the embedded light client).
I can be persuaded of the benefits, but it does conflict with the idea of keeping this library (relatively) lightweight.
|
||
[patch.crates-io] | ||
sc-network = { git = "https://github.com/paritytech/substrate" } | ||
sc-service = { git = "https://github.com/paritytech/substrate" } |
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.
What are these patches for? Something not yet included in rc3
? It might prevent publishing the main crate
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 problem is that node template isn't published, but it's required for the tests. So this makes sure that when it's checked out locally it uses the same versions as the template. I was hoping this trick would allow publish to crates.io
The library still is light weight, only compiling the tests takes longer, if you add it as a dep, without the light client enabled it shouldn't increase compile times |
I'd love this to be the defacto standard way of building substrate based applications. We're using it heavily and once your metadata work is finished, I think it'll be on track to be that. We're building tools like dart bindgen that will allow us to rapidly create flutter ui's using subxt. |
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.
LGTM
This let's the
ClientBuilder
specify ajsonrpsee::Client
and implements ajsonrpsee::ClientTransport
that wraps a substrate light client.TODO:
use node template from official repo, depends on [0]
in a follow up remove support for the subxt_test macro. Due to usability improvements, it's no longer useful, and migrate the remaining tests to using the node-template where possible.
[0] Expose chain_spec and service in node-template substrate#6313
Closes #49