-
Notifications
You must be signed in to change notification settings - Fork 261
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
refactor(rpc): support default port in URL #1122
Conversation
Update jsonrpsee to v0.20 to support the default port number in URLs.
version: MetadataVersion, | ||
) -> Result<Vec<u8>, FetchMetadataError> { | ||
let (sender, receiver) = WsTransportClientBuilder::default() | ||
.build(url.to_string().parse::<Uri>().unwrap()) |
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 removed this, it doesn't make sense to parse to URL twice instead better to just clone the URL?! :)
Thus, all APIs are changed to accept the URL by value instead of reference...
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.
Hehe yeah good catch, no idea why we were doing that :D
@@ -54,7 +54,7 @@ getrandom = { version = "0.2", default-features = false } | |||
hex = "0.4.3" | |||
heck = "0.4.1" | |||
impl-serde = { version = "0.4.0" } | |||
jsonrpsee = { version = "0.16" } | |||
jsonrpsee = { version = "0.20" } |
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.
Wierd; I updated jsonrpsee to 0.19 not long ago; I wonder where those changes went!
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.
(Maybe I got distracted and never actually merged those changes, whoops!)
url: &Uri, | ||
url: Url, | ||
version: MetadataVersion, | ||
) -> Result<Vec<u8>, FetchMetadataError> { | ||
tokio_block_on(fetch_metadata_bytes(url, version)) | ||
} | ||
|
||
/// Returns the raw, 0x prefixed metadata hex from the provided URL, blocking the current thread. | ||
pub fn fetch_metadata_hex_blocking( | ||
url: &Uri, | ||
url: Url, |
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 wonder why passing the reference no longer works with Url
but did with Uri
? (this trickled up to meaning ectra clones and stuff in other places; no big deal but curious :))
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.
Yeah, if you look at by comment above you can see that we are parsing an "already parsed URL" again using its String representation :D
Such that it was possible to use as a borrow before...
version: MetadataVersion, | ||
) -> Result<Vec<u8>, FetchMetadataError> { | ||
let client = HttpClientBuilder::default() | ||
.request_timeout(Duration::from_secs(180)) | ||
.build(url.to_string())?; | ||
.build(url)?; |
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.
Aah this is why the cloning was needed I guess!
return Err(Error::LightClient(LightClientError::InvalidScheme)); | ||
} | ||
|
||
let (sender, receiver) = ws_transport(url).await?; | ||
|
||
Ok(ClientBuilder::default() | ||
.max_notifs_per_subscription(4096) | ||
Ok(Client::builder() |
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.
Awesome!
Updates jsonrpsee to v0.20 to support the default port number in URLs.