-
Notifications
You must be signed in to change notification settings - Fork 15
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
upgrade: update to chainflip-monthly-2022-06 #2061
Conversation
@@ -474,11 +505,7 @@ impl<RpcClient: StateChainRpcApi> StateChainClient<RpcClient> { | |||
Err(rpc_err) => match rpc_err { | |||
// This occurs when a transaction with the same nonce is in the transaction pool (and the priority is | |||
// <= priority of that existing tx) | |||
RpcError::JsonRpcError(Error { | |||
// this is the error returned when the "priority is too low" i.e. nonce is too low |
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.
would like to keep something in the comment about "priority is too low" since this is the message that might be seen in the logs.
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.
Has this new code been tested? @dandanlen
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.
It has passed all unit tests...
About comment @kylezs, will sort it out now. It seemed to me that both comments were saying the same thing, ie. the one above and below were referring to the same error case. Will clarify that a bit.
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.
Actually, looking at it again, the comments to seem to be duplicates of each other, and the doc string of InvalidTransaction::Stale also contains the same information. Gonna leave as is, seems clear enough to me.
@@ -488,13 +515,13 @@ impl<RpcClient: StateChainRpcApi> StateChainClient<RpcClient> { | |||
} | |||
// This occurs when the nonce has already been *consumed* i.e a transaction with that nonce | |||
// is in a block | |||
RpcError::JsonRpcError(Error { | |||
// this is the error returned when the "transaction is outdated" i.e. nonce is too low |
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.
same as above
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.
Same situation - this comment just says the same as the one above, in a slightly different manner.
Just to check, I'm leaving the review until the polkadot.js issue is understood? As discussed. If you still want the review done now anyway, just let me know. |
Thanks - yes you can leave it for now, I want to dig into why the api is inaccessible when deployed on a testnet. FWIW it works fine when I run it locally. |
(And I need to rebase it again.) |
Mainly: - adapt remaining dependencies: jsonrpsee, libp2p On the CFE: - use jsonrpsee throughout - adapt error conversions In the runtime: - replace uses of generate_storage_alias! with #[storage_alias] - remove_prefix is deprecated
772516d
to
4dfad22
Compare
@@ -474,11 +505,7 @@ impl<RpcClient: StateChainRpcApi> StateChainClient<RpcClient> { | |||
Err(rpc_err) => match rpc_err { | |||
// This occurs when a transaction with the same nonce is in the transaction pool (and the priority is | |||
// <= priority of that existing tx) | |||
RpcError::JsonRpcError(Error { | |||
// this is the error returned when the "priority is too low" i.e. nonce is too low |
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.
Has this new code been tested? @dandanlen
); | ||
struct Pallet; | ||
|
||
impl StorageInstance for Pallet { |
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.
What is this?
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.
Good question - I thought I had to implement this trait manually but in fact it's taken care of by the storage_alias
macro. Will delete it.
utilities/Cargo.toml
Outdated
@@ -11,8 +11,7 @@ version = '0.1.0' | |||
[dependencies] | |||
hex = {version = "0.4.3", default-features = false} | |||
anyhow = {version = '1.0', optional = true} | |||
jsonrpc-core = {version = "18.0.0", optional = true} | |||
jsonrpc-core-client = {version = '18.0.0', optional = true} | |||
jsonrpsee = { version = "0.13.0", optional = true, features = ["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.
I don't believe this dependency is still needed
sparse_finalized_block_header_stream.next().await.unwrap()?; | ||
|
||
let chain_rpc_client = state_chain_rpc_client.chain_rpc_client.clone(); | ||
let chain_rpc_client = state_chain_rpc_client.rpc_client.clone(); |
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.
Prob should rename this variable to 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.
I've made a few more comments but looks good
@kylezs I think i've addressed all remaining comments, if there are no objections I'll merge this tomorrow. |
@dandanlen I pushed as you said. |
Overview:
On the CFE:
In the runtime:
clear(u32::MAX, None)
instead (see here).Most of the code changes are in commit 7b3a05a.