-
Notifications
You must be signed in to change notification settings - Fork 378
Always pass port to jsonrpsee WebSocket client #2339
Conversation
@@ -64,6 +64,18 @@ pub struct ReconnectingWsClient { | |||
to_worker_channel: TokioSender<RpcDispatcherMessage>, | |||
} | |||
|
|||
/// Format url and force addition of a port | |||
fn url_to_string_with_port(url: Url) -> Option<String> { |
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.
Why don't you just use set_port
?
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.
set_port checks internally and drops the port if it is the default one. Basically the url standard says that the port should be null if it is the default one. jsonrpsee behaves out of line here.
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.
Can we not just fix jsonrpsee?
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 can, this is a quick fix to make it work for now. But yeah I will open 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.
Opened an issue about this, paritytech/jsonrpsee#1048.
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.
@bkchr can we go forward with this? If this gets changed in jsonrpsee I will adjust here again, but for now this is an easy fix and I would like to have this functionality working asap.
@@ -325,6 +338,8 @@ impl ReconnectingWebsocketWorker { | |||
async fn new( | |||
urls: Vec<Url>, | |||
) -> (ReconnectingWebsocketWorker, TokioSender<RpcDispatcherMessage>) { | |||
let urls = urls.into_iter().filter_map(url_to_string_with_port).collect(); |
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 silently drops urls without a port.
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 mean either the url has a user-provided port or we use the default. If the URL has no host, it fails at initial parsing already. So this should never filter something out. I will add a log message in that case however, since filtering out anything would be undesired (but passing it on is also undesired since jsonrpsee will also not take it without a port).
bot merge |
Waiting for commit status. |
* master: Companion for #13624 (#2354) Introduce Fellowship into Collectives (#2186) NFTs 2.0 on Statemine (#2314) Bump assert_cmd from 2.0.8 to 2.0.10 (#2341) Bump clap from 4.1.8 to 4.1.11 (#2352) Companion for substrate #13312: Rename `Deterministic` to `Enforce` (#2350) [Companion #13634] keystore overhaul (iter) (#2345) Revert #2304 (#2349) Deprecate Currency: Companion for #12951 (#2334) Bump ci-linux image for rust 1.68 Always pass port to jsonrpsee WebSocket client (#2339) bump zombienet to v1.3.40 (#2348) Improve build times by disabling wasm-builder in `no_std` (#2308) Bump toml from 0.7.2 to 0.7.3 (#2340) Bump serde from 1.0.152 to 1.0.156 (#2329) Parachains should charge for proof size weight (#2326) dmp-queue: Store messages if already processed more than the maximum (#2343) [Companion #13615] Keystore overhaul (#2336) Bump quote from 1.0.23 to 1.0.26 (#2331)
edf33a2c85 Backport fix (for wasm `std` env) (#2339) git-subtree-dir: bridges git-subtree-split: edf33a2c85399d366e008228f2d9e63e8a492d95
* Squashed 'bridges/' changes from 0417308a48..278119fec2 278119fec2 Grandpa: Store the authority set changes (#2336) (#2337) 19f9c8ffdb remove message sender origin (#2322) 3c7c271d2e GRANDPA module: store accepted justifications (#2298) (#2301) fb7d12e793 GRANDPA justifications: equivocation detection primitives (#2295) (#2297) d03a2ed450 More backports from Cumulus subtree to polkadot-staging (#2283) 3c4ada921b Update dependecies (#2277) (#2281) 3e195c9e76 GRANDPA: optimize votes_ancestries when needed (#2262) (#2264) 7065bbabc6 Implement RuntimeDebug for GrandpaJustification (#2254) 8c9e59bcbc Define generate_grandpa_key_ownership_proof() (#2247) (#2248) 0b46956df7 Deduplicate Grandpa consensus log reading logic (#2245) (#2246) 96c9701710 Fix deps from Cumulus (#2244) git-subtree-dir: bridges git-subtree-split: 278119fec2b45990cf1271999b0c21befe7003d9 * Subtree update * Squashed 'bridges/' changes from 278119fec2..edf33a2c85 edf33a2c85 Backport fix (for wasm `std` env) (#2339) git-subtree-dir: bridges git-subtree-split: edf33a2c85399d366e008228f2d9e63e8a492d95
closes #2337
The jsonrpsee websocket client always expects a port. However, the
url
crate always strips away the default port.Sadly I did not find a way to force the
url
crate always to include a port into the URL, so I have to resort to manual string formatting.