-
Notifications
You must be signed in to change notification settings - Fork 11
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 subxt onclineclient / rpc #125
Conversation
1 similar comment
Coverage after merging pg/node_client into main
Coverage Report
|
Hey @pgherveou thanks for your contribution 🙌, something like this is definitely useful for teams but we need to brainstorming about the |
Sure feel free to close, just wanted to try it out |
Coverage after merging pg/node_client into main
Coverage Report
|
Coverage after merging pg/node_client into main
Coverage Report
|
No, I don't think we should close it. I think is in the right path and worth to explore it and see the feedback from other teams. Then we can rethink some of the abstractions. @wirednkod / @l0r1s . Just FYI @pgherveou we want to add also support to run |
crates/orchestrator/src/spawner.rs
Outdated
loop { | ||
match connect().await { | ||
Err(_) if retries >= 0 => { | ||
println!("Error connecting, retrying ..."); |
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.
should be added to the crates/configuration/src/shared/constants.rs
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.
Are you moving all &str into this file? This might make logging a bit more tedious, curious what you gain from this?
Should be nice, but not really useful for our usecase. We mainly want to interact with the node through subxt. That's what we do today, but with a solochain |
Sounds good! and I think this is a great step forward :) |
This is the code I could replace with zsdk. So pretty much what's missing are these 2 apis I added here. Other methods like kill are I believe already available. Then I would also need to specify a custom path for the collator, but I believe it can already be specified as well. |
Yes, looks like an awesome use-case. We were thinking on make the client/rpc call to create the client and not auto-connect the nodes at spawn phase. Something like from end-user perspective let client = network.get_node("alice")?.client().await?; |
that would work too, as long as the sdk can handle the retry logic |
One thing that would be nice as well is to use log and a custom target instead of println, so that these logs can easily be stripped from our tests. |
@pgherveou, I'm not familiarized with the subxt internals, did you know if they handle reconnections? I wonder if make sense to |
under the hood, looks like it's a tokio TcpStream. if the Node is stopped you will have to recreate the client. You will receive this kind of error if you try to read from it.
Maybe creating the client on demand like you are suggesting and letting the consumer deal with re-connection is good enough. |
updated the PR to do it lazily, this remove the need for the custom retry function as well |
RpcClient::from_url(&self.ws_uri).await | ||
} | ||
|
||
pub async fn client(&self) -> Result<OnlineClient<PolkadotConfig>, subxt::Error> { |
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.
Does make sense to allow users to also specify the concrete Config
type?
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.
mmm good point. Using moonbeam for example you would need to specify a custom config.
A bit of a shame that Rust does not let you specify a default value for the generic trait here.
I mean you could but then rustc complains with
defaults for type parameters are only allowed in `struct`, `enum`, `type`, or `trait` definitions this was previously accepted by the compiler but is being phased out;
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.
Just an small question and the is good to merge 🙌
Looks good @pgherveou! thanks for your contribution 🙌 🙌 I will fix the fmt/clippy and merge this one. |
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.
Besides the clippy issues this make sense. Thank you @pgherveou
🙏 looks like there are quite a lot of clippy issues not related to the changes here. Should you ship the fixes in a different PR? |
Coverage after merging pg/node_client into main
Coverage Report
|
merged, thanks @pgherveou!! |
just wanted to throw a quick implementation that exposes the OnclineClient on the node.
This would make the sdk super useful to enable cross chain e2e tests in ink!