-
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
refactor: replace reconnecting-jsonrpsee-ws-client
with subxt-reconnecting-rpc-client
#1705
Conversation
@@ -112,6 +112,10 @@ getrandom = { workspace = true, optional = true } | |||
# Included if "native" feature is enabled | |||
tokio-util = { workspace = true, features = ["compat"], optional = true } | |||
|
|||
# Included if the reconnecting rpc client feature is enabled | |||
tokio = { workspace = true, optional = true } |
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.
Clean up tokio, perhaps we can replace it with futures channels and stuff
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.
We'll prob want a tokio feature flag or similar anyway so that we can make it easy to start up the UnstableBackend
etc!
I see web
enabled tokio?/sync; would
weband
tokio/sync` work together though?
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.
yes, tokio/sync works for wasm/web but that could easily be replaced by futures channel
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 forgot that I used tokio::sync::Notify
so I prefer to keep tokio but not runtime agnostic but it's doesn't matter anyway because jsonrpsee is already using tokio channels.
We could change that though if becomes an issue.
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.
Yup I don't mind that it uses tokio since jsonrpsee does anyway, so we don't really gain anything by trying to remove it anyways from this I guess!
reconnecting-jsonrpsee-ws-client
with subxt-reconnecting-rpc-client
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.
This doesn't appear to be exposed anywhere, or am I missing something? :)
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 don't understand your comment, what do you mean?
The entire module?
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.
Oh no nevermind; I just didn't see any pub mod reconnecting_rpc_client
but just missed that, prob because this PR didn't need to add it and I forgot it was already there :) I thought I was being dumb when I added that comment!
This looks solid to me; just some small comments + it needs actually exposing by the looks of it :) |
…lient' into add-dedicated-subxt-rpc-reconn-client
//! | ||
//! # Example | ||
//! | ||
//! ```no_run |
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.
Nice!
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.
One teeny nit re the reconnect Future/Stream thing left but otherwise looks clean to me; nice work!
This PR removes the dependency
reconnecting-rpc-client
because it has some more complexity that subxt doesn't need and adds a simple reconnecting-wrapper-rpc-client however it's a some lines of code but I think it's quite simple and clean. Thus, it shouldn't that much technical debt