Upgrade jsonrpc to 0.18.0#9547
Conversation
I think this says all :P
bkchr
added
A0-please_review
|
Should we revert this then: #9518 ? |
andresilva
left a comment
andresilva
left a comment
There was a problem hiding this comment.
LGTM, only question is about changes from bounded to unbounded channels in a couple of places.
| #[wasm_bindgen(js_name = "rpcSend")] | ||
| pub fn rpc_send(&mut self, rpc: &str) -> js_sys::Promise { | ||
| let rpc_session = RpcSession::new(mpsc01::channel(1).0); | ||
| let rpc_session = RpcSession::new(mpsc::unbounded().0); |
There was a problem hiding this comment.
Any particular reason why this and below was changed to unbounded?
| impl Metadata { | ||
| /// Create new `Metadata` with session (Pub/Sub) support. | ||
| pub fn new(transport: mpsc::Sender<String>) -> Self { | ||
| pub fn new(transport: mpsc::UnboundedSender<String>) -> Self { |
There was a problem hiding this comment.
Why change to unbounded?
There was a problem hiding this comment.
This comes from jsonrpc 🤷 I only having adapted to the requirements :D
There was a problem hiding this comment.
It was unbounded before too, it's just the name that is more explicit now.
There was a problem hiding this comment.
I think this change causes the related BEEFY Update jsonrpc PR to fail.
We run into the old chicken-and-egg problem. For the BEEFY PR to compile it needs an updated version of sc_rpc::Metadata
There was a problem hiding this comment.
The PR is failling because it is using master of Substrate that does not yet have this pr.
There was a problem hiding this comment.
The PR is failling because it is using master of Substrate that does not yet have this pr.
Yep, that is what I wrote ...
| let mut runtime = tokio::runtime::current_thread::Runtime::new().unwrap(); | ||
| runtime.block_on(rx).unwrap() | ||
| } | ||
| #[cfg(test)] |
There was a problem hiding this comment.
This whole file is already only compiled when config is test.
| false => None, | ||
| } | ||
|
|
||
| version_differs.then(|| new_version.clone()) |
| warn!("Failed to submit extrinsic: {}", err); | ||
| // reject the subscriber (ignore errors - we don't care if subscriber is no longer there). | ||
| let _ = subscriber.reject(err.into()); | ||
| return |
There was a problem hiding this comment.
This is a bit shittier as we're duplicating the same error handling as below.
bkchr
added
the
D3-trivial 🧸
Good question |
I think at least we should change the version there (in this PR) to |
| impl Metadata { | ||
| /// Create new `Metadata` with session (Pub/Sub) support. | ||
| pub fn new(transport: mpsc::Sender<String>) -> Self { | ||
| pub fn new(transport: mpsc::UnboundedSender<String>) -> Self { |
There was a problem hiding this comment.
It was unbounded before too, it's just the name that is more explicit now.
I have reverted it |
|
bot merge |
|
Trying merge. |
ghost
merged commit 
5 participants
I think this says all :P
polkadot companion: paritytech/polkadot#3633