Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Always pass port to jsonrpsee WebSocket client #2339

Merged
merged 3 commits into from
Mar 20, 2023

Conversation

skunert
Copy link
Contributor

@skunert skunert commented Mar 16, 2023

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.

@skunert skunert added B0-silent Changes should not be mentioned in any release notes A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. T0-node This PR/Issue is related to the topic “node”. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Mar 16, 2023
@skunert skunert requested a review from a team March 16, 2023 20:58
@@ -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> {
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

@skunert skunert Mar 16, 2023

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 👍

Copy link
Contributor Author

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.

Copy link
Contributor Author

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();
Copy link
Member

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.

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 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).

@bkchr
Copy link
Member

bkchr commented Mar 20, 2023

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit 2a9066c into paritytech:master Mar 20, 2023
ordian added a commit that referenced this pull request Mar 21, 2023
* 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)
bkontur added a commit that referenced this pull request Aug 10, 2023
edf33a2c85 Backport fix (for wasm `std` env) (#2339)

git-subtree-dir: bridges
git-subtree-split: edf33a2c85399d366e008228f2d9e63e8a492d95
paritytech-processbot bot pushed a commit that referenced this pull request Aug 10, 2023
* 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'--relay-chain-rpc-urls' does not work via wss connection
3 participants