Skip to content
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

Merged
merged 24 commits into from
Sep 1, 2022
Merged

Conversation

dandanlen
Copy link
Collaborator

@dandanlen dandanlen commented Aug 18, 2022

Overview:

  • bump a load of dependencies, the main one being jsonrpsee
  • update the rust toolchain version

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, use clear(u32::MAX, None) instead (see here).

Most of the code changes are in commit 7b3a05a.

@dandanlen dandanlen requested a review from kylezs August 18, 2022 15:51
engine/src/multisig_p2p.rs Show resolved Hide resolved
@@ -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
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Copy link
Collaborator Author

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.

engine/src/state_chain_observer/client.rs Outdated Show resolved Hide resolved
state-chain/custom-rpc/src/lib.rs Show resolved Hide resolved
state-chain/pallets/cf-vaults/src/lib.rs Outdated Show resolved Hide resolved
@AlastairHolmes
Copy link
Contributor

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.

@dandanlen
Copy link
Collaborator Author

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.

@dandanlen
Copy link
Collaborator Author

(And I need to rebase it again.)

engine/src/eth/event.rs Show resolved Hide resolved
engine/src/multisig/db/persistent.rs Show resolved Hide resolved
engine/src/multisig_p2p.rs Show resolved Hide resolved
engine/src/state_chain_observer/client.rs Outdated Show resolved Hide resolved
@@ -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
Copy link
Contributor

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

state-chain/pallets/cf-governance/Cargo.toml Show resolved Hide resolved
state-chain/pallets/cf-vaults/src/lib.rs Show resolved Hide resolved
);
struct Pallet;

impl StorageInstance for Pallet {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this?

Copy link
Collaborator Author

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.

state-chain/runtime/src/lib.rs Show resolved Hide resolved
@@ -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"] }
Copy link
Contributor

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

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.

Copy link
Contributor

@AlastairHolmes AlastairHolmes left a 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

@dandanlen
Copy link
Collaborator Author

@kylezs I think i've addressed all remaining comments, if there are no objections I'll merge this tomorrow.

@AlastairHolmes
Copy link
Contributor

@dandanlen I pushed as you said.

@dandanlen dandanlen changed the title chore: update to chainflip-monthly-2022-06 upgrade: update to chainflip-monthly-2022-06 Sep 1, 2022
@dandanlen dandanlen enabled auto-merge (squash) September 1, 2022 08:46
@dandanlen dandanlen merged commit 938e3b1 into develop Sep 1, 2022
@dandanlen dandanlen deleted the update/monthly-2022-06 branch September 1, 2022 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants