From 426144d35c5baeeb8b3d895d16c8f0e77c15152f Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Fri, 25 Oct 2024 17:30:54 +0200 Subject: [PATCH 01/24] add workaround to support json rpc 2.0 with `author_submitAndWatch` trusted call --- core-primitives/rpc/src/lib.rs | 21 +++++++++++ core/direct-rpc-server/src/rpc_responder.rs | 36 ++++++++++++------- .../src/rpc_watch_extractor.rs | 16 +++++++-- .../rpc-handler/src/direct_top_pool_api.rs | 9 +---- 4 files changed, 59 insertions(+), 23 deletions(-) diff --git a/core-primitives/rpc/src/lib.rs b/core-primitives/rpc/src/lib.rs index 06cd8e6737..aab1fed564 100644 --- a/core-primitives/rpc/src/lib.rs +++ b/core-primitives/rpc/src/lib.rs @@ -62,6 +62,27 @@ pub struct RpcResponse { pub id: Id, } +#[derive(Clone, Encode, Decode, Debug, Serialize, Deserialize)] +pub struct RpcSubscriptionUpdate { + pub jsonrpc: String, + pub id: Id, + pub method: String, + pub params: SubscriptionParams, +} + +#[derive(Clone, Encode, Decode, Debug, Serialize, Deserialize)] +pub struct SubscriptionParams { + pub error: Option, + pub result: String, + pub subscription: String, +} + +impl RpcSubscriptionUpdate { + pub fn new(method: String, params: SubscriptionParams) -> Self { + Self { jsonrpc: "2.0".to_owned(), id: Id::Number(1), method, params } + } +} + #[derive(Clone, Encode, Decode, Serialize, Deserialize)] pub struct RpcRequest { pub jsonrpc: String, diff --git a/core/direct-rpc-server/src/rpc_responder.rs b/core/direct-rpc-server/src/rpc_responder.rs index bc288abf65..5baf21a7a7 100644 --- a/core/direct-rpc-server/src/rpc_responder.rs +++ b/core/direct-rpc-server/src/rpc_responder.rs @@ -19,8 +19,8 @@ use crate::{ response_channel::ResponseChannel, DirectRpcError, DirectRpcResult, RpcConnectionRegistry, RpcHash, SendRpcResponse, }; -use alloc::format; -use itp_rpc::{RpcResponse, RpcReturnValue}; +use alloc::{format, string::ToString}; +use itp_rpc::{RpcResponse, RpcReturnValue, RpcSubscriptionUpdate, SubscriptionParams}; use itp_types::{DirectRequestStatus, TrustedOperationStatus}; use itp_utils::{FromHexPrefixed, ToHexPrefixed}; use log::*; @@ -59,6 +59,17 @@ where self.response_channel.respond(connection, string_response).map_err(|e| e.into()) } + + fn encode_and_send_subscription_update( + &self, + connection: Registry::Connection, + rpc_response: &RpcSubscriptionUpdate, + ) -> DirectRpcResult<()> { + let string_response = + serde_json::to_string(&rpc_response).map_err(DirectRpcError::SerializationError)?; + + self.response_channel.respond(connection, string_response).map_err(|e| e.into()) + } } impl SendRpcResponse @@ -83,22 +94,21 @@ where .withdraw(&hash) .ok_or(DirectRpcError::InvalidConnectionHash)?; - let mut new_response = rpc_response.clone(); - - let mut result = RpcReturnValue::from_hex(&rpc_response.result) - .map_err(|e| DirectRpcError::Other(format!("{:?}", e).into()))?; - let do_watch = continue_watching(&status_update); - // update response - result.do_watch = do_watch; - result.status = DirectRequestStatus::TrustedOperationStatus(status_update); - new_response.result = result.to_hex(); + let sub = RpcSubscriptionUpdate::new( + "author_submitAndWatchExtrinsic".to_string(), + SubscriptionParams { + error: None, + result: DirectRequestStatus::TrustedOperationStatus(status_update).to_hex(), + subscription: hash.to_hex(), + }, + ); - self.encode_and_send_response(connection_token, &new_response)?; + self.encode_and_send_subscription_update(connection_token, &sub)?; if do_watch { - self.connection_registry.store(hash, connection_token, new_response); + self.connection_registry.store(hash, connection_token, rpc_response); } debug!("updating status event successful"); diff --git a/core/direct-rpc-server/src/rpc_watch_extractor.rs b/core/direct-rpc-server/src/rpc_watch_extractor.rs index a117a34b4a..bcc514a521 100644 --- a/core/direct-rpc-server/src/rpc_watch_extractor.rs +++ b/core/direct-rpc-server/src/rpc_watch_extractor.rs @@ -21,6 +21,7 @@ use codec::Decode; use itp_rpc::{RpcResponse, RpcReturnValue}; use itp_types::DirectRequestStatus; use itp_utils::FromHexPrefixed; +use log::info; use std::marker::PhantomData; pub struct RpcWatchExtractor @@ -55,8 +56,19 @@ where type Hash = Hash; fn must_be_watched(&self, rpc_response: &RpcResponse) -> DirectRpcResult> { - let rpc_return_value = RpcReturnValue::from_hex(&rpc_response.result) - .map_err(|e| DirectRpcError::Other(format!("{:?}", e).into()))?; + let rpc_return_value = match RpcReturnValue::from_hex(&rpc_response.result) { + Ok(return_value) => return_value, + Err(e) => { + // if the return value is a hash + if let Ok(hash) = Self::Hash::from_hex(&rpc_response.result) { + // fixme: hack to support json rpc 2.0 spec without rpc-server refactor. + info!("Returning hash as connection token"); + return Ok(Some(hash)) + } + + return Err(DirectRpcError::Other(format!("{:?}", e).into())).into() + }, + }; if !rpc_return_value.do_watch { return Ok(None) diff --git a/sidechain/rpc-handler/src/direct_top_pool_api.rs b/sidechain/rpc-handler/src/direct_top_pool_api.rs index bf4ce02f0d..c1291d53b9 100644 --- a/sidechain/rpc-handler/src/direct_top_pool_api.rs +++ b/sidechain/rpc-handler/src/direct_top_pool_api.rs @@ -60,14 +60,7 @@ pub fn add_top_pool_direct_rpc_methods( ]) .unwrap_or_else(|e| error!("failed to update prometheus metric: {:?}", e)); let json_value = match author_submit_extrinsic_inner(local_author.clone(), params) { - Ok(hash_value) => RpcReturnValue { - do_watch: true, - value: hash_value.encode(), - status: DirectRequestStatus::TrustedOperationStatus( - TrustedOperationStatus::Submitted, - ), - } - .to_hex(), + Ok(hash_value) => hash_value.to_hex(), Err(error) => compute_hex_encoded_return_error(error.as_str()), }; Ok(json!(json_value)) From 8d70fb87d00d56202da9a5cdd49240669d9ed3a8 Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sat, 26 Oct 2024 19:56:28 +0200 Subject: [PATCH 02/24] update trusted_op_direct_request --- cli/src/trusted_operation.rs | 51 ++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/cli/src/trusted_operation.rs b/cli/src/trusted_operation.rs index 0ee160741c..3cda720cc7 100644 --- a/cli/src/trusted_operation.rs +++ b/cli/src/trusted_operation.rs @@ -26,7 +26,7 @@ use enclave_bridge_primitives::Request; use ita_stf::{Getter, TrustedCallSigned}; use itc_rpc_client::direct_client::{DirectApi, DirectClient}; use itp_node_api::api_client::{ApiClientError, ENCLAVE_BRIDGE}; -use itp_rpc::{RpcRequest, RpcResponse, RpcReturnValue}; +use itp_rpc::{RpcRequest, RpcResponse, RpcReturnValue, RpcSubscriptionUpdate}; use itp_sgx_crypto::ShieldingCryptoEncrypt; use itp_stf_primitives::types::{ShardIdentifier, TrustedOperation}; use itp_types::{ @@ -268,7 +268,7 @@ pub fn read_shard(trusted_args: &TrustedCli) -> StdResult( +pub fn send_direct_request( cli: &Cli, trusted_args: &TrustedCli, operation_call: &TrustedOperation, @@ -287,50 +287,55 @@ fn send_direct_request( let (sender, receiver) = channel(); direct_api.watch(jsonrpc_call, sender); - debug!("waiting for rpc response"); + // the first response of `submitAndWatch` is just the plain top hash + let top_hash = receiver.recv().map_err(|e| { + error!("failed to receive rpc response: {:?}", e); + direct_api.close().unwrap(); + TrustedOperationError::Default { msg: "failed to receive rpc response".to_string() } + })?; + + debug!("subscribing to updates for top with hash: {top_hash}"); + loop { + debug!("waiting for update"); + match receiver.recv() { Ok(response) => { debug!("received response"); - let response: RpcResponse = serde_json::from_str(&response).unwrap(); - if let Ok(return_value) = RpcReturnValue::from_hex(&response.result) { - debug!("successfully decoded rpc response: {:?}", return_value); - match return_value.status { + let subscription_update: RpcSubscriptionUpdate = + serde_json::from_str(&response).unwrap(); + trace!("successfully decoded rpc response: {:?}", subscription_update); + if let Ok(direct_request_status) = + DirectRequestStatus::from_hex(&subscription_update.params.result) + { + debug!("successfully decoded request status: {:?}", direct_request_status); + match direct_request_status { DirectRequestStatus::Error => { + let err = subscription_update.params.error.unwrap_or("{}".into()); debug!("request status is error"); - if let Ok(value) = String::decode(&mut return_value.value.as_slice()) { - error!("{}", value); - } direct_api.close().unwrap(); return Err(TrustedOperationError::Default { - msg: "[Error] DirectRequestStatus::Error".to_string(), + msg: format!("[Error] DirectRequestStatus::Error: {err}"), }) }, DirectRequestStatus::TrustedOperationStatus(status) => { debug!("request status is: {:?}", status); - if let Ok(value) = Hash::decode(&mut return_value.value.as_slice()) { + if let Ok(value) = + Hash::from_hex(&subscription_update.params.subscription) + { println!("Trusted call {:?} is {:?}", value, status); } if connection_can_be_closed(status) { direct_api.close().unwrap(); - let value = - decode_response_value(&mut return_value.value.as_slice())?; - return Ok(value) + return Ok(Default::default()) } }, DirectRequestStatus::Ok => { debug!("request status is ignored"); direct_api.close().unwrap(); - let value = decode_response_value(&mut return_value.value.as_slice())?; - return Ok(value) + return Ok(Default::default()) }, } - if !return_value.do_watch { - debug!("do watch is false, closing connection"); - direct_api.close().unwrap(); - let value = decode_response_value(&mut return_value.value.as_slice())?; - return Ok(value) - } }; }, Err(e) => { From 02466c870f1ea0ce855c76082adc03c10219e6e4 Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sat, 26 Oct 2024 19:57:03 +0200 Subject: [PATCH 03/24] fix obsolete imports in rpc_responder --- core/direct-rpc-server/src/rpc_responder.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/direct-rpc-server/src/rpc_responder.rs b/core/direct-rpc-server/src/rpc_responder.rs index 5baf21a7a7..9ecde0c2db 100644 --- a/core/direct-rpc-server/src/rpc_responder.rs +++ b/core/direct-rpc-server/src/rpc_responder.rs @@ -19,10 +19,10 @@ use crate::{ response_channel::ResponseChannel, DirectRpcError, DirectRpcResult, RpcConnectionRegistry, RpcHash, SendRpcResponse, }; -use alloc::{format, string::ToString}; +use alloc::string::ToString; use itp_rpc::{RpcResponse, RpcReturnValue, RpcSubscriptionUpdate, SubscriptionParams}; use itp_types::{DirectRequestStatus, TrustedOperationStatus}; -use itp_utils::{FromHexPrefixed, ToHexPrefixed}; +use itp_utils::ToHexPrefixed; use log::*; use std::{sync::Arc, vec::Vec}; From 11547419e80abf49fb589cec64d6e5060d69a9b5 Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sat, 26 Oct 2024 20:18:12 +0200 Subject: [PATCH 04/24] make direct request no longer generic, it can only return the hash anyhow, we can easily introduce it later. --- cli/src/benchmark/mod.rs | 16 ++++++---- cli/src/evm/commands/evm_call.rs | 6 ++-- cli/src/evm/commands/evm_create.rs | 5 ++-- cli/src/guess_the_number/commands/guess.rs | 3 +- .../commands/push_by_one_day.rs | 3 +- .../guess_the_number/commands/set_winnings.rs | 4 +-- .../trusted_base_cli/commands/set_balance.rs | 4 +-- cli/src/trusted_base_cli/commands/transfer.rs | 5 ++-- .../commands/unshield_funds.rs | 5 ++-- cli/src/trusted_operation.rs | 29 ++++++++++++++----- 10 files changed, 50 insertions(+), 30 deletions(-) diff --git a/cli/src/benchmark/mod.rs b/cli/src/benchmark/mod.rs index 550bc18cc3..b08e7b87c8 100644 --- a/cli/src/benchmark/mod.rs +++ b/cli/src/benchmark/mod.rs @@ -33,8 +33,9 @@ use itp_stf_primitives::{ }; use itp_types::{ AccountInfo, Balance, ShardIdentifier, TrustedOperationStatus, - TrustedOperationStatus::{InSidechainBlock, Submitted}, + TrustedOperationStatus::InSidechainBlock, H256, }; +use itp_utils::FromHexPrefixed; use log::*; use rand::Rng; use rayon::prelude::*; @@ -334,7 +335,14 @@ fn wait_for_top_confirmation( ) -> BenchmarkTransaction { let started = Instant::now(); - let submitted = wait_until(&client.receiver, is_submitted); + // the first response of `submitAndWatch` is just the plain top hash + let submitted = match client.receiver.recv() { + Ok(hash) => Some((H256::from_hex(&hash).unwrap(), Instant::now())), + Err(e) => { + error!("recv error: {e:?}"); + None + }, + }; let confirmed = if wait_for_sidechain_block { // We wait for the transaction hash that actually matches the submitted hash @@ -361,10 +369,6 @@ fn wait_for_top_confirmation( } } -fn is_submitted(s: TrustedOperationStatus) -> bool { - matches!(s, Submitted) -} - fn is_sidechain_block(s: TrustedOperationStatus) -> bool { matches!(s, InSidechainBlock(_)) } diff --git a/cli/src/evm/commands/evm_call.rs b/cli/src/evm/commands/evm_call.rs index 294b6a0331..690d3720ed 100644 --- a/cli/src/evm/commands/evm_call.rs +++ b/cli/src/evm/commands/evm_call.rs @@ -19,7 +19,7 @@ use crate::{ get_layer_two_evm_nonce, get_layer_two_nonce, trusted_cli::TrustedCli, trusted_command_utils::{get_identifiers, get_pair_from_str}, - trusted_operation::perform_trusted_operation, + trusted_operation::{perform_trusted_operation, send_direct_request}, Cli, CliResult, CliResultOk, }; use ita_stf::{Index, TrustedCall, TrustedGetter}; @@ -31,6 +31,7 @@ use itp_types::AccountId; use log::*; use sp_core::{crypto::Ss58Codec, Pair, H160, U256}; use std::{boxed::Box, vec::Vec}; + #[derive(Parser)] pub struct EvmCallCommands { /// Sender's incognito AccountId in ss58check format, mnemonic or hex seed @@ -81,7 +82,6 @@ impl EvmCallCommands { ) .sign(&KeyPair::Sr25519(Box::new(sender)), nonce, &mrenclave, &shard) .into_trusted_operation(trusted_args.direct); - Ok(perform_trusted_operation::<()>(cli, trusted_args, &function_call) - .map(|_| CliResultOk::None)?) + Ok(send_direct_request(cli, trusted_args, &function_call).map(|_| CliResultOk::None)?) } } diff --git a/cli/src/evm/commands/evm_create.rs b/cli/src/evm/commands/evm_create.rs index d0e693548a..abd7f76fb5 100644 --- a/cli/src/evm/commands/evm_create.rs +++ b/cli/src/evm/commands/evm_create.rs @@ -19,7 +19,7 @@ use crate::{ get_layer_two_evm_nonce, get_layer_two_nonce, trusted_cli::TrustedCli, trusted_command_utils::{get_identifiers, get_pair_from_str}, - trusted_operation::perform_trusted_operation, + trusted_operation::{perform_trusted_operation, send_direct_request}, Cli, CliResult, CliResultOk, }; use ita_stf::{evm_helpers::evm_create_address, Index, TrustedCall, TrustedGetter}; @@ -33,6 +33,7 @@ use pallet_evm::{AddressMapping, HashedAddressMapping}; use sp_core::{crypto::Ss58Codec, Pair, H160, U256}; use sp_runtime::traits::BlakeTwo256; use std::vec::Vec; + #[derive(Parser)] pub struct EvmCreateCommands { /// Sender's incognito AccountId in ss58check format, mnemonic or hex seed @@ -79,7 +80,7 @@ impl EvmCreateCommands { .sign(&from.into(), nonce, &mrenclave, &shard) .into_trusted_operation(trusted_args.direct); - perform_trusted_operation::<()>(cli, trusted_args, &top)?; + send_direct_request(cli, trusted_args, &top)?; let execution_address = evm_create_address(sender_evm_acc, evm_account_nonce); info!("trusted call evm_create executed"); diff --git a/cli/src/guess_the_number/commands/guess.rs b/cli/src/guess_the_number/commands/guess.rs index 932d1786e5..20c2e9df92 100644 --- a/cli/src/guess_the_number/commands/guess.rs +++ b/cli/src/guess_the_number/commands/guess.rs @@ -23,6 +23,7 @@ use crate::{ Cli, CliResult, CliResultOk, }; +use crate::trusted_operation::send_direct_request; use ita_stf::{ guess_the_number::GuessTheNumberTrustedCall, Getter, Index, TrustedCall, TrustedCallSigned, }; @@ -55,6 +56,6 @@ impl GuessCommand { ) .sign(&KeyPair::Sr25519(Box::new(signer)), nonce, &mrenclave, &shard) .into_trusted_operation(trusted_args.direct); - Ok(perform_trusted_operation::<()>(cli, trusted_args, &top).map(|_| CliResultOk::None)?) + Ok(send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?) } } diff --git a/cli/src/guess_the_number/commands/push_by_one_day.rs b/cli/src/guess_the_number/commands/push_by_one_day.rs index 1a1cfb2358..8588b42068 100644 --- a/cli/src/guess_the_number/commands/push_by_one_day.rs +++ b/cli/src/guess_the_number/commands/push_by_one_day.rs @@ -23,6 +23,7 @@ use crate::{ Cli, CliResult, CliResultOk, }; +use crate::trusted_operation::send_direct_request; use ita_stf::{ guess_the_number::GuessTheNumberTrustedCall, Getter, Index, TrustedCall, TrustedCallSigned, }; @@ -53,6 +54,6 @@ impl PushByOneDayCommand { ) .sign(&KeyPair::Sr25519(Box::new(signer)), nonce, &mrenclave, &shard) .into_trusted_operation(trusted_args.direct); - Ok(perform_trusted_operation::<()>(cli, trusted_args, &top).map(|_| CliResultOk::None)?) + Ok(send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?) } } diff --git a/cli/src/guess_the_number/commands/set_winnings.rs b/cli/src/guess_the_number/commands/set_winnings.rs index beab2475a2..db59e89090 100644 --- a/cli/src/guess_the_number/commands/set_winnings.rs +++ b/cli/src/guess_the_number/commands/set_winnings.rs @@ -19,7 +19,7 @@ use crate::{ get_layer_two_nonce, trusted_cli::TrustedCli, trusted_command_utils::{get_identifiers, get_pair_from_str}, - trusted_operation::perform_trusted_operation, + trusted_operation::{perform_trusted_operation, send_direct_request}, Cli, CliResult, CliResultOk, }; use ita_parentchain_interface::integritee::Balance; @@ -59,6 +59,6 @@ impl SetWinningsCommand { ) .sign(&KeyPair::Sr25519(Box::new(signer)), nonce, &mrenclave, &shard) .into_trusted_operation(trusted_args.direct); - Ok(perform_trusted_operation::<()>(cli, trusted_args, &top).map(|_| CliResultOk::None)?) + Ok(send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?) } } diff --git a/cli/src/trusted_base_cli/commands/set_balance.rs b/cli/src/trusted_base_cli/commands/set_balance.rs index 7d8dc1f193..3a89ef50e0 100644 --- a/cli/src/trusted_base_cli/commands/set_balance.rs +++ b/cli/src/trusted_base_cli/commands/set_balance.rs @@ -19,7 +19,7 @@ use crate::{ get_layer_two_nonce, trusted_cli::TrustedCli, trusted_command_utils::{get_identifiers, get_pair_from_str}, - trusted_operation::perform_trusted_operation, + trusted_operation::send_direct_request, Cli, CliResult, CliResultOk, }; use ita_parentchain_interface::integritee::Balance; @@ -59,6 +59,6 @@ impl SetBalanceCommand { ) .sign(&KeyPair::Sr25519(Box::new(signer)), nonce, &mrenclave, &shard) .into_trusted_operation(trusted_args.direct); - Ok(perform_trusted_operation::<()>(cli, trusted_args, &top).map(|_| CliResultOk::None)?) + Ok(send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?) } } diff --git a/cli/src/trusted_base_cli/commands/transfer.rs b/cli/src/trusted_base_cli/commands/transfer.rs index 427d9054f6..fb8474a8ca 100644 --- a/cli/src/trusted_base_cli/commands/transfer.rs +++ b/cli/src/trusted_base_cli/commands/transfer.rs @@ -19,7 +19,7 @@ use crate::{ get_layer_two_nonce, trusted_cli::TrustedCli, trusted_command_utils::{get_accountid_from_str, get_identifiers, get_pair_from_str}, - trusted_operation::perform_trusted_operation, + trusted_operation::{perform_trusted_operation, send_direct_request}, Cli, CliResult, CliResultOk, }; use base58::ToBase58; @@ -65,8 +65,7 @@ impl TransferCommand { TrustedCall::balance_transfer(from.public().into(), to, self.amount) .sign(&KeyPair::Sr25519(Box::new(from)), nonce, &mrenclave, &shard) .into_trusted_operation(trusted_args.direct); - let res = - perform_trusted_operation::<()>(cli, trusted_args, &top).map(|_| CliResultOk::None)?; + let res = send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?; info!("trusted call transfer executed"); Ok(res) } diff --git a/cli/src/trusted_base_cli/commands/unshield_funds.rs b/cli/src/trusted_base_cli/commands/unshield_funds.rs index 9d0e66d07b..1d22bdbfb2 100644 --- a/cli/src/trusted_base_cli/commands/unshield_funds.rs +++ b/cli/src/trusted_base_cli/commands/unshield_funds.rs @@ -19,7 +19,7 @@ use crate::{ get_layer_two_nonce, trusted_cli::TrustedCli, trusted_command_utils::{get_accountid_from_str, get_identifiers, get_pair_from_str}, - trusted_operation::perform_trusted_operation, + trusted_operation::{perform_trusted_operation, send_direct_request}, Cli, CliResult, CliResultOk, }; use ita_parentchain_interface::integritee::Balance; @@ -31,6 +31,7 @@ use itp_stf_primitives::{ use log::*; use sp_core::{crypto::Ss58Codec, Pair}; use std::boxed::Box; + #[derive(Parser)] pub struct UnshieldFundsCommand { /// Sender's incognito AccountId in ss58check format, mnemonic or hex seed @@ -63,6 +64,6 @@ impl UnshieldFundsCommand { TrustedCall::balance_unshield(from.public().into(), to, self.amount, shard) .sign(&KeyPair::Sr25519(Box::new(from)), nonce, &mrenclave, &shard) .into_trusted_operation(trusted_args.direct); - Ok(perform_trusted_operation::<()>(cli, trusted_args, &top).map(|_| CliResultOk::None)?) + Ok(send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?) } } diff --git a/cli/src/trusted_operation.rs b/cli/src/trusted_operation.rs index 3cda720cc7..219c85d9fe 100644 --- a/cli/src/trusted_operation.rs +++ b/cli/src/trusted_operation.rs @@ -72,6 +72,10 @@ pub(crate) enum TrustedOperationError { Default { msg: String }, } +pub(crate) fn into_default_trusted_op_err(err_msg: impl ToString) -> TrustedOperationError { + TrustedOperationError::Default { msg: err_msg.to_string() } +} + impl From for TrustedOperationError { fn from(error: ApiClientError) -> Self { Self::ApiClient(error) @@ -87,9 +91,12 @@ pub(crate) fn perform_trusted_operation( ) -> TrustedOpResult { match top { TrustedOperation::indirect_call(_) => send_indirect_request::(cli, trusted_args, top), - TrustedOperation::direct_call(_) => send_direct_request::(cli, trusted_args, top), TrustedOperation::get(getter) => execute_getter_from_cli_args::(cli, trusted_args, getter), + TrustedOperation::direct_call(_) => Err(TrustedOperationError::Default { + msg: "Invalid call to `perform_trusted_operation`, use `send_direct_request` directly" + .into(), + }), } } @@ -268,11 +275,11 @@ pub fn read_shard(trusted_args: &TrustedCli) -> StdResult( +pub(crate) fn send_direct_request( cli: &Cli, trusted_args: &TrustedCli, operation_call: &TrustedOperation, -) -> TrustedOpResult { +) -> TrustedOpResult { let encryption_key = get_shielding_key(cli).unwrap(); let shard = read_shard(trusted_args).unwrap(); let jsonrpc_call: String = get_json_request(shard, operation_call, encryption_key); @@ -288,13 +295,19 @@ pub fn send_direct_request( direct_api.watch(jsonrpc_call, sender); // the first response of `submitAndWatch` is just the plain top hash - let top_hash = receiver.recv().map_err(|e| { + let response_string = receiver.recv().map_err(|e| { error!("failed to receive rpc response: {:?}", e); direct_api.close().unwrap(); - TrustedOperationError::Default { msg: "failed to receive rpc response".to_string() } + into_default_trusted_op_err("failed to receive rpc response") })?; - debug!("subscribing to updates for top with hash: {top_hash}"); + let response = RpcResponse::from_hex(&response_string) + .map_err(|e| into_default_trusted_op_err(format!("{e:?}")))?; + + let top_hash = Hash::from_hex(&response.result) + .map_err(|e| into_default_trusted_op_err(format!("{e:?}")))?; + + debug!("subscribing to updates for top with hash: {top_hash:?}"); loop { debug!("waiting for update"); @@ -327,13 +340,13 @@ pub fn send_direct_request( } if connection_can_be_closed(status) { direct_api.close().unwrap(); - return Ok(Default::default()) + return Ok(top_hash) } }, DirectRequestStatus::Ok => { debug!("request status is ignored"); direct_api.close().unwrap(); - return Ok(Default::default()) + return Ok(top_hash) }, } }; From 1808e6650b0f095ea401ceed06144291f697cf2f Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sat, 26 Oct 2024 20:26:27 +0200 Subject: [PATCH 05/24] fix deserializing rpc response --- cli/src/trusted_operation.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cli/src/trusted_operation.rs b/cli/src/trusted_operation.rs index 219c85d9fe..f5e0f4ee9c 100644 --- a/cli/src/trusted_operation.rs +++ b/cli/src/trusted_operation.rs @@ -301,11 +301,12 @@ pub(crate) fn send_direct_request( into_default_trusted_op_err("failed to receive rpc response") })?; - let response = RpcResponse::from_hex(&response_string) - .map_err(|e| into_default_trusted_op_err(format!("{e:?}")))?; + let response: RpcResponse = serde_json::from_str(&response_string).map_err(|e| { + into_default_trusted_op_err(format!("Error deserializing RpcResponse: {e:?}")) + })?; let top_hash = Hash::from_hex(&response.result) - .map_err(|e| into_default_trusted_op_err(format!("{e:?}")))?; + .map_err(|e| into_default_trusted_op_err(format!("Error decoding top hash: {e:?}")))?; debug!("subscribing to updates for top with hash: {top_hash:?}"); From bb0cefac04e60989590315704623a01c535cd20f Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sat, 26 Oct 2024 20:34:28 +0200 Subject: [PATCH 06/24] [cli/trusted_operation] remove unnecessary nesting --- cli/src/trusted_operation.rs | 87 ++++++++++++++++++------------------ 1 file changed, 43 insertions(+), 44 deletions(-) diff --git a/cli/src/trusted_operation.rs b/cli/src/trusted_operation.rs index f5e0f4ee9c..1bb545e336 100644 --- a/cli/src/trusted_operation.rs +++ b/cli/src/trusted_operation.rs @@ -312,54 +312,53 @@ pub(crate) fn send_direct_request( loop { debug!("waiting for update"); - - match receiver.recv() { - Ok(response) => { - debug!("received response"); - let subscription_update: RpcSubscriptionUpdate = - serde_json::from_str(&response).unwrap(); - trace!("successfully decoded rpc response: {:?}", subscription_update); - if let Ok(direct_request_status) = - DirectRequestStatus::from_hex(&subscription_update.params.result) - { - debug!("successfully decoded request status: {:?}", direct_request_status); - match direct_request_status { - DirectRequestStatus::Error => { - let err = subscription_update.params.error.unwrap_or("{}".into()); - debug!("request status is error"); - direct_api.close().unwrap(); - return Err(TrustedOperationError::Default { - msg: format!("[Error] DirectRequestStatus::Error: {err}"), - }) - }, - DirectRequestStatus::TrustedOperationStatus(status) => { - debug!("request status is: {:?}", status); - if let Ok(value) = - Hash::from_hex(&subscription_update.params.subscription) - { - println!("Trusted call {:?} is {:?}", value, status); - } - if connection_can_be_closed(status) { - direct_api.close().unwrap(); - return Ok(top_hash) - } - }, - DirectRequestStatus::Ok => { - debug!("request status is ignored"); - direct_api.close().unwrap(); - return Ok(top_hash) - }, - } - }; - }, - Err(e) => { - error!("failed to receive rpc response: {:?}", e); + let response = receiver.recv().map_err(|e| { + error!("failed to receive rpc response: {:?}", e); + direct_api.close().unwrap(); + into_default_trusted_op_err("failed to receive rpc response") + })?; + debug!("received response"); + + let subscription_update: RpcSubscriptionUpdate = + serde_json::from_str(&response).map_err(|e| { + into_default_trusted_op_err(format!( + "Error deserializing subscription update: {e:?}" + )) + })?; + + trace!("successfully decoded rpc response: {:?}", subscription_update); + + let direct_request_status = + DirectRequestStatus::from_hex(&subscription_update.params.result).map_err(|e| { + into_default_trusted_op_err(format!("Error decoding direct_request_status: {e:?}")) + })?; + + debug!("successfully decoded request status: {:?}", direct_request_status); + match direct_request_status { + DirectRequestStatus::Error => { + let err = subscription_update.params.error.unwrap_or("{}".into()); + debug!("request status is error"); direct_api.close().unwrap(); return Err(TrustedOperationError::Default { - msg: "failed to receive rpc response".to_string(), + msg: format!("[Error] DirectRequestStatus::Error: {err}"), }) }, - }; + DirectRequestStatus::TrustedOperationStatus(status) => { + debug!("request status is: {:?}", status); + if let Ok(value) = Hash::from_hex(&subscription_update.params.subscription) { + println!("Trusted call {:?} is {:?}", value, status); + } + if connection_can_be_closed(status) { + direct_api.close().unwrap(); + return Ok(top_hash) + } + }, + DirectRequestStatus::Ok => { + debug!("request status is ignored"); + direct_api.close().unwrap(); + return Ok(top_hash) + }, + } } } From 61d9cd3ab6243b7cdea39a2fdd46a422f1874c57 Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sat, 26 Oct 2024 20:43:01 +0200 Subject: [PATCH 07/24] fix clippy --- core/direct-rpc-server/src/rpc_watch_extractor.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/direct-rpc-server/src/rpc_watch_extractor.rs b/core/direct-rpc-server/src/rpc_watch_extractor.rs index bcc514a521..982e7d2f29 100644 --- a/core/direct-rpc-server/src/rpc_watch_extractor.rs +++ b/core/direct-rpc-server/src/rpc_watch_extractor.rs @@ -66,7 +66,7 @@ where return Ok(Some(hash)) } - return Err(DirectRpcError::Other(format!("{:?}", e).into())).into() + return Err(DirectRpcError::Other(format!("{:?}", e).into())) }, }; From da566a0a217a676a5e04e8d5c4ebf61bdd061bce Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sat, 26 Oct 2024 20:43:50 +0200 Subject: [PATCH 08/24] extract await status update --- cli/src/trusted_operation.rs | 47 +++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/cli/src/trusted_operation.rs b/cli/src/trusted_operation.rs index 1bb545e336..1725185340 100644 --- a/cli/src/trusted_operation.rs +++ b/cli/src/trusted_operation.rs @@ -312,25 +312,11 @@ pub(crate) fn send_direct_request( loop { debug!("waiting for update"); - let response = receiver.recv().map_err(|e| { - error!("failed to receive rpc response: {:?}", e); - direct_api.close().unwrap(); - into_default_trusted_op_err("failed to receive rpc response") - })?; - debug!("received response"); - - let subscription_update: RpcSubscriptionUpdate = - serde_json::from_str(&response).map_err(|e| { - into_default_trusted_op_err(format!( - "Error deserializing subscription update: {e:?}" - )) - })?; - - trace!("successfully decoded rpc response: {:?}", subscription_update); - - let direct_request_status = - DirectRequestStatus::from_hex(&subscription_update.params.result).map_err(|e| { - into_default_trusted_op_err(format!("Error decoding direct_request_status: {e:?}")) + let (subscription_update, direct_request_status) = + await_status_update(&receiver).map_err(|e| { + error!("Error getting status update: {:?}", e); + direct_api.close().unwrap(); + e })?; debug!("successfully decoded request status: {:?}", direct_request_status); @@ -362,6 +348,29 @@ pub(crate) fn send_direct_request( } } +fn await_status_update( + receiver: &Receiver, +) -> TrustedOpResult<(RpcSubscriptionUpdate, DirectRequestStatus)> { + let response = receiver.recv().map_err(|e| { + into_default_trusted_op_err(format!("failed to receive rpc response: {e:?}")) + })?; + debug!("received response"); + + let subscription_update: RpcSubscriptionUpdate = + serde_json::from_str(&response).map_err(|e| { + into_default_trusted_op_err(format!("Error deserializing subscription update: {e:?}")) + })?; + + trace!("successfully decoded rpc response: {:?}", subscription_update); + + let direct_request_status = DirectRequestStatus::from_hex(&subscription_update.params.result) + .map_err(|e| { + into_default_trusted_op_err(format!("Error decoding direct_request_status: {e:?}")) + })?; + + Ok((subscription_update, direct_request_status)) +} + fn decode_response_value( value: &mut I, ) -> StdResult { From a2e4aaef95cbe5562dced120ebf0dc231de35430 Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sat, 26 Oct 2024 21:10:31 +0200 Subject: [PATCH 09/24] [cli] fix correctly extract hash in benchmarks --- cli/src/benchmark/mod.rs | 13 ++++++++----- cli/src/trusted_operation.rs | 30 +++++++++++++++++++----------- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/cli/src/benchmark/mod.rs b/cli/src/benchmark/mod.rs index b08e7b87c8..e89f97f13e 100644 --- a/cli/src/benchmark/mod.rs +++ b/cli/src/benchmark/mod.rs @@ -20,7 +20,10 @@ use crate::{ get_layer_two_nonce, trusted_cli::TrustedCli, trusted_command_utils::{get_identifiers, get_keystore_path, get_pair_from_str}, - trusted_operation::{get_json_request, get_state, perform_trusted_operation, wait_until}, + trusted_operation::{ + await_subscription_response, get_json_request, get_state, perform_trusted_operation, + wait_until, + }, Cli, CliResult, CliResultOk, SR25519_KEY_TYPE, }; use codec::Decode; @@ -33,9 +36,8 @@ use itp_stf_primitives::{ }; use itp_types::{ AccountInfo, Balance, ShardIdentifier, TrustedOperationStatus, - TrustedOperationStatus::InSidechainBlock, H256, + TrustedOperationStatus::InSidechainBlock, }; -use itp_utils::FromHexPrefixed; use log::*; use rand::Rng; use rayon::prelude::*; @@ -336,8 +338,9 @@ fn wait_for_top_confirmation( let started = Instant::now(); // the first response of `submitAndWatch` is just the plain top hash - let submitted = match client.receiver.recv() { - Ok(hash) => Some((H256::from_hex(&hash).unwrap(), Instant::now())), + + let submitted = match await_subscription_response(&client.receiver) { + Ok(hash) => Some((hash, Instant::now())), Err(e) => { error!("recv error: {e:?}"); None diff --git a/cli/src/trusted_operation.rs b/cli/src/trusted_operation.rs index 1725185340..417e886795 100644 --- a/cli/src/trusted_operation.rs +++ b/cli/src/trusted_operation.rs @@ -295,19 +295,12 @@ pub(crate) fn send_direct_request( direct_api.watch(jsonrpc_call, sender); // the first response of `submitAndWatch` is just the plain top hash - let response_string = receiver.recv().map_err(|e| { - error!("failed to receive rpc response: {:?}", e); + let top_hash = await_subscription_response(&receiver).map_err(|e| { + error!("Error getting subscription response: {:?}", e); direct_api.close().unwrap(); - into_default_trusted_op_err("failed to receive rpc response") - })?; - - let response: RpcResponse = serde_json::from_str(&response_string).map_err(|e| { - into_default_trusted_op_err(format!("Error deserializing RpcResponse: {e:?}")) + e })?; - let top_hash = Hash::from_hex(&response.result) - .map_err(|e| into_default_trusted_op_err(format!("Error decoding top hash: {e:?}")))?; - debug!("subscribing to updates for top with hash: {top_hash:?}"); loop { @@ -348,7 +341,22 @@ pub(crate) fn send_direct_request( } } -fn await_status_update( +pub(crate) fn await_subscription_response(receiver: &Receiver) -> TrustedOpResult { + let response_string = receiver.recv().map_err(|e| { + into_default_trusted_op_err(format!("failed to receive rpc response: {e:?}")) + })?; + + let response: RpcResponse = serde_json::from_str(&response_string).map_err(|e| { + into_default_trusted_op_err(format!("Error deserializing RpcResponse: {e:?}")) + })?; + + let top_hash = Hash::from_hex(&response.result) + .map_err(|e| into_default_trusted_op_err(format!("Error decoding top hash: {e:?}")))?; + + Ok(top_hash) +} + +pub(crate) fn await_status_update( receiver: &Receiver, ) -> TrustedOpResult<(RpcSubscriptionUpdate, DirectRequestStatus)> { let response = receiver.recv().map_err(|e| { From f311585cb584bd67429c1efa6d20a9ca51c65527 Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sat, 26 Oct 2024 21:11:29 +0200 Subject: [PATCH 10/24] fmt --- cli/src/benchmark/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/cli/src/benchmark/mod.rs b/cli/src/benchmark/mod.rs index f7c9fadc09..2329c582e2 100644 --- a/cli/src/benchmark/mod.rs +++ b/cli/src/benchmark/mod.rs @@ -341,7 +341,6 @@ fn wait_for_top_confirmation( let started = Instant::now(); // the first response of `submitAndWatch` is just the plain top hash - let submitted = match await_subscription_response(&client.receiver) { Ok(hash) => Some((hash, Instant::now())), Err(e) => { From 86fed6bd212a07114004d19e2f4a1688b9806eb9 Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sat, 26 Oct 2024 21:16:27 +0200 Subject: [PATCH 11/24] add check that we can only us direct_calls in send direct request --- cli/src/trusted_operation.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cli/src/trusted_operation.rs b/cli/src/trusted_operation.rs index 417e886795..eb9e23d987 100644 --- a/cli/src/trusted_operation.rs +++ b/cli/src/trusted_operation.rs @@ -280,6 +280,11 @@ pub(crate) fn send_direct_request( trusted_args: &TrustedCli, operation_call: &TrustedOperation, ) -> TrustedOpResult { + if !matches!(operation_call, TrustedOperation::direct_call(_)) { + return Err(TrustedOperationError::Default { + msg: "can only use direct_calls in this function".into(), + }) + } let encryption_key = get_shielding_key(cli).unwrap(); let shard = read_shard(trusted_args).unwrap(); let jsonrpc_call: String = get_json_request(shard, operation_call, encryption_key); From 68e5da76b3f911ce1a0971e76de9df6837f1c7e0 Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sat, 26 Oct 2024 21:24:58 +0200 Subject: [PATCH 12/24] improve logs --- cli/src/trusted_operation.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cli/src/trusted_operation.rs b/cli/src/trusted_operation.rs index eb9e23d987..508f25bcd1 100644 --- a/cli/src/trusted_operation.rs +++ b/cli/src/trusted_operation.rs @@ -365,16 +365,16 @@ pub(crate) fn await_status_update( receiver: &Receiver, ) -> TrustedOpResult<(RpcSubscriptionUpdate, DirectRequestStatus)> { let response = receiver.recv().map_err(|e| { - into_default_trusted_op_err(format!("failed to receive rpc response: {e:?}")) + into_default_trusted_op_err(format!("error receiving subscription update: {e:?}")) })?; - debug!("received response"); + debug!("received subscription update"); let subscription_update: RpcSubscriptionUpdate = serde_json::from_str(&response).map_err(|e| { - into_default_trusted_op_err(format!("Error deserializing subscription update: {e:?}")) + into_default_trusted_op_err(format!("error deserializing subscription update: {e:?}")) })?; - trace!("successfully decoded rpc response: {:?}", subscription_update); + trace!("successfully decoded subscription update: {:?}", subscription_update); let direct_request_status = DirectRequestStatus::from_hex(&subscription_update.params.result) .map_err(|e| { From 876ffdd7f4dc61c265e464cf018e5ba0b0b40bd0 Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sat, 26 Oct 2024 21:30:34 +0200 Subject: [PATCH 13/24] add comment about storing initial top hash --- core/direct-rpc-server/src/rpc_responder.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/core/direct-rpc-server/src/rpc_responder.rs b/core/direct-rpc-server/src/rpc_responder.rs index 9ecde0c2db..1a4a163e18 100644 --- a/core/direct-rpc-server/src/rpc_responder.rs +++ b/core/direct-rpc-server/src/rpc_responder.rs @@ -108,6 +108,11 @@ where self.encode_and_send_subscription_update(connection_token, &sub)?; if do_watch { + // We just store back the initial response, which is the top hash. + // This was implemented before we added the `RpcSubscriptionUpdate` + // and should probably be refactored in the future. + // + // But for now this is fine, as we only use it to track ongoing connections. self.connection_registry.store(hash, connection_token, rpc_response); } From 477ed7ff30324a9c86206c5c3a2c9941f20e281d Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sat, 26 Oct 2024 21:37:25 +0200 Subject: [PATCH 14/24] add todos for rpc refactor. --- core/direct-rpc-server/src/rpc_responder.rs | 2 +- core/direct-rpc-server/src/rpc_watch_extractor.rs | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/core/direct-rpc-server/src/rpc_responder.rs b/core/direct-rpc-server/src/rpc_responder.rs index 1a4a163e18..8e12783630 100644 --- a/core/direct-rpc-server/src/rpc_responder.rs +++ b/core/direct-rpc-server/src/rpc_responder.rs @@ -110,7 +110,7 @@ where if do_watch { // We just store back the initial response, which is the top hash. // This was implemented before we added the `RpcSubscriptionUpdate` - // and should probably be refactored in the future. + // and should probably be refactored in the future, see #1624. // // But for now this is fine, as we only use it to track ongoing connections. self.connection_registry.store(hash, connection_token, rpc_response); diff --git a/core/direct-rpc-server/src/rpc_watch_extractor.rs b/core/direct-rpc-server/src/rpc_watch_extractor.rs index 982e7d2f29..8d688d1d18 100644 --- a/core/direct-rpc-server/src/rpc_watch_extractor.rs +++ b/core/direct-rpc-server/src/rpc_watch_extractor.rs @@ -21,7 +21,7 @@ use codec::Decode; use itp_rpc::{RpcResponse, RpcReturnValue}; use itp_types::DirectRequestStatus; use itp_utils::FromHexPrefixed; -use log::info; +use log::debug; use std::marker::PhantomData; pub struct RpcWatchExtractor @@ -59,10 +59,14 @@ where let rpc_return_value = match RpcReturnValue::from_hex(&rpc_response.result) { Ok(return_value) => return_value, Err(e) => { - // if the return value is a hash + // `author_submitAndWatchExtrinsic` does currently only return the top hash + // as the first subscription response in order to comply with JSON RPC 2.0. + // + // We support this for now with this hack here, but it should be properly + // refactored in #1624. if let Ok(hash) = Self::Hash::from_hex(&rpc_response.result) { - // fixme: hack to support json rpc 2.0 spec without rpc-server refactor. - info!("Returning hash as connection token"); + // fixme: fix hack in #1624. + debug!("returning hash as connection token: {hash:?}"); return Ok(Some(hash)) } From 9c36e7451df826b76c54b07f54638a01067b45d9 Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sat, 26 Oct 2024 21:39:21 +0200 Subject: [PATCH 15/24] better comment --- core/direct-rpc-server/src/rpc_responder.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/core/direct-rpc-server/src/rpc_responder.rs b/core/direct-rpc-server/src/rpc_responder.rs index 8e12783630..ed39f17586 100644 --- a/core/direct-rpc-server/src/rpc_responder.rs +++ b/core/direct-rpc-server/src/rpc_responder.rs @@ -109,10 +109,12 @@ where if do_watch { // We just store back the initial response, which is the top hash. - // This was implemented before we added the `RpcSubscriptionUpdate` - // and should probably be refactored in the future, see #1624. + // This was implemented before we added the `RpcSubscriptionUpdate`, + // which can't be stored due to type incompatibilities. + // This should probably be refactored in the future, see #1624. // - // But for now this is fine, as we only use it to track ongoing connections. + // But for now this is fine, as we only use the connection token + // to track ongoing connections, the response is ignored. self.connection_registry.store(hash, connection_token, rpc_response); } From 8d6ce332ed625b2bfd87704f37a6d6cad46b34a5 Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sat, 26 Oct 2024 21:41:13 +0200 Subject: [PATCH 16/24] add more comments --- sidechain/rpc-handler/src/direct_top_pool_api.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sidechain/rpc-handler/src/direct_top_pool_api.rs b/sidechain/rpc-handler/src/direct_top_pool_api.rs index c1291d53b9..6b64726254 100644 --- a/sidechain/rpc-handler/src/direct_top_pool_api.rs +++ b/sidechain/rpc-handler/src/direct_top_pool_api.rs @@ -60,6 +60,9 @@ pub fn add_top_pool_direct_rpc_methods( ]) .unwrap_or_else(|e| error!("failed to update prometheus metric: {:?}", e)); let json_value = match author_submit_extrinsic_inner(local_author.clone(), params) { + // Only return hash to support JSON RPC 2.0. + // Other methods will follow this pattern when + // we tackle #1624. Ok(hash_value) => hash_value.to_hex(), Err(error) => compute_hex_encoded_return_error(error.as_str()), }; From f9cd82d976a55925edb10a5fa20d9449019c4cbe Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sun, 27 Oct 2024 08:43:07 +0100 Subject: [PATCH 17/24] fix handling of indirect calls --- cli/src/evm/commands/evm_call.rs | 8 +++++++- cli/src/evm/commands/evm_create.rs | 6 +++++- cli/src/guess_the_number/commands/guess.rs | 8 +++++++- cli/src/guess_the_number/commands/push_by_one_day.rs | 8 +++++++- cli/src/guess_the_number/commands/set_winnings.rs | 8 +++++++- cli/src/trusted_base_cli/commands/set_balance.rs | 10 ++++++++-- cli/src/trusted_base_cli/commands/transfer.rs | 11 ++++++++++- cli/src/trusted_base_cli/commands/unshield_funds.rs | 8 +++++++- 8 files changed, 58 insertions(+), 9 deletions(-) diff --git a/cli/src/evm/commands/evm_call.rs b/cli/src/evm/commands/evm_call.rs index 690d3720ed..bf72494185 100644 --- a/cli/src/evm/commands/evm_call.rs +++ b/cli/src/evm/commands/evm_call.rs @@ -82,6 +82,12 @@ impl EvmCallCommands { ) .sign(&KeyPair::Sr25519(Box::new(sender)), nonce, &mrenclave, &shard) .into_trusted_operation(trusted_args.direct); - Ok(send_direct_request(cli, trusted_args, &function_call).map(|_| CliResultOk::None)?) + + if trusted_args.direct { + Ok(send_direct_request(cli, trusted_args, &function_call).map(|_| CliResultOk::None)?) + } else { + Ok(perform_trusted_operation::<()>(cli, trusted_args, &function_call) + .map(|_| CliResultOk::None)?) + } } } diff --git a/cli/src/evm/commands/evm_create.rs b/cli/src/evm/commands/evm_create.rs index abd7f76fb5..04be461cfe 100644 --- a/cli/src/evm/commands/evm_create.rs +++ b/cli/src/evm/commands/evm_create.rs @@ -80,7 +80,11 @@ impl EvmCreateCommands { .sign(&from.into(), nonce, &mrenclave, &shard) .into_trusted_operation(trusted_args.direct); - send_direct_request(cli, trusted_args, &top)?; + if trusted_args.direct { + send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?; + } else { + perform_trusted_operation::<()>(cli, trusted_args, &top).map(|_| CliResultOk::None)?; + } let execution_address = evm_create_address(sender_evm_acc, evm_account_nonce); info!("trusted call evm_create executed"); diff --git a/cli/src/guess_the_number/commands/guess.rs b/cli/src/guess_the_number/commands/guess.rs index 20c2e9df92..38ec145cee 100644 --- a/cli/src/guess_the_number/commands/guess.rs +++ b/cli/src/guess_the_number/commands/guess.rs @@ -56,6 +56,12 @@ impl GuessCommand { ) .sign(&KeyPair::Sr25519(Box::new(signer)), nonce, &mrenclave, &shard) .into_trusted_operation(trusted_args.direct); - Ok(send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?) + + if trusted_args.direct { + Ok(send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?) + } else { + Ok(perform_trusted_operation::<()>(cli, trusted_args, &top) + .map(|_| CliResultOk::None)?) + } } } diff --git a/cli/src/guess_the_number/commands/push_by_one_day.rs b/cli/src/guess_the_number/commands/push_by_one_day.rs index 8588b42068..8b9d3377bf 100644 --- a/cli/src/guess_the_number/commands/push_by_one_day.rs +++ b/cli/src/guess_the_number/commands/push_by_one_day.rs @@ -54,6 +54,12 @@ impl PushByOneDayCommand { ) .sign(&KeyPair::Sr25519(Box::new(signer)), nonce, &mrenclave, &shard) .into_trusted_operation(trusted_args.direct); - Ok(send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?) + + if trusted_args.direct { + Ok(send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?) + } else { + Ok(perform_trusted_operation::<()>(cli, trusted_args, &top) + .map(|_| CliResultOk::None)?) + } } } diff --git a/cli/src/guess_the_number/commands/set_winnings.rs b/cli/src/guess_the_number/commands/set_winnings.rs index db59e89090..f7b814d2e1 100644 --- a/cli/src/guess_the_number/commands/set_winnings.rs +++ b/cli/src/guess_the_number/commands/set_winnings.rs @@ -59,6 +59,12 @@ impl SetWinningsCommand { ) .sign(&KeyPair::Sr25519(Box::new(signer)), nonce, &mrenclave, &shard) .into_trusted_operation(trusted_args.direct); - Ok(send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?) + + if trusted_args.direct { + Ok(send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?) + } else { + Ok(perform_trusted_operation::<()>(cli, trusted_args, &top) + .map(|_| CliResultOk::None)?) + } } } diff --git a/cli/src/trusted_base_cli/commands/set_balance.rs b/cli/src/trusted_base_cli/commands/set_balance.rs index 3a89ef50e0..74cd3c9f34 100644 --- a/cli/src/trusted_base_cli/commands/set_balance.rs +++ b/cli/src/trusted_base_cli/commands/set_balance.rs @@ -19,7 +19,7 @@ use crate::{ get_layer_two_nonce, trusted_cli::TrustedCli, trusted_command_utils::{get_identifiers, get_pair_from_str}, - trusted_operation::send_direct_request, + trusted_operation::{perform_trusted_operation, send_direct_request}, Cli, CliResult, CliResultOk, }; use ita_parentchain_interface::integritee::Balance; @@ -59,6 +59,12 @@ impl SetBalanceCommand { ) .sign(&KeyPair::Sr25519(Box::new(signer)), nonce, &mrenclave, &shard) .into_trusted_operation(trusted_args.direct); - Ok(send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?) + + if trusted_args.direct { + Ok(send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?) + } else { + Ok(perform_trusted_operation::<()>(cli, trusted_args, &top) + .map(|_| CliResultOk::None)?) + } } } diff --git a/cli/src/trusted_base_cli/commands/transfer.rs b/cli/src/trusted_base_cli/commands/transfer.rs index fb8474a8ca..ab22f812ac 100644 --- a/cli/src/trusted_base_cli/commands/transfer.rs +++ b/cli/src/trusted_base_cli/commands/transfer.rs @@ -65,7 +65,16 @@ impl TransferCommand { TrustedCall::balance_transfer(from.public().into(), to, self.amount) .sign(&KeyPair::Sr25519(Box::new(from)), nonce, &mrenclave, &shard) .into_trusted_operation(trusted_args.direct); - let res = send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?; + + let mut res; + + if trusted_args.direct { + res = Ok(send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?)?; + } else { + res = Ok(perform_trusted_operation::<()>(cli, trusted_args, &top) + .map(|_| CliResultOk::None)?)?; + } + info!("trusted call transfer executed"); Ok(res) } diff --git a/cli/src/trusted_base_cli/commands/unshield_funds.rs b/cli/src/trusted_base_cli/commands/unshield_funds.rs index 1d22bdbfb2..fffe4737b8 100644 --- a/cli/src/trusted_base_cli/commands/unshield_funds.rs +++ b/cli/src/trusted_base_cli/commands/unshield_funds.rs @@ -64,6 +64,12 @@ impl UnshieldFundsCommand { TrustedCall::balance_unshield(from.public().into(), to, self.amount, shard) .sign(&KeyPair::Sr25519(Box::new(from)), nonce, &mrenclave, &shard) .into_trusted_operation(trusted_args.direct); - Ok(send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?) + + if trusted_args.direct { + Ok(send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?) + } else { + Ok(perform_trusted_operation::<()>(cli, trusted_args, &top) + .map(|_| CliResultOk::None)?) + } } } From 1bed405cfc4faa84af071d88bb82967fa7348949 Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sun, 27 Oct 2024 09:19:14 +0100 Subject: [PATCH 18/24] derive copy for trusted operation status --- core-primitives/types/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core-primitives/types/src/lib.rs b/core-primitives/types/src/lib.rs index 08ff827db2..19563a7de5 100644 --- a/core-primitives/types/src/lib.rs +++ b/core-primitives/types/src/lib.rs @@ -82,7 +82,7 @@ pub enum DirectRequestStatus { Error, } -#[derive(Debug, Clone, PartialEq, Encode, Decode)] +#[derive(Debug, Clone, PartialEq, Encode, Decode, Copy)] pub enum TrustedOperationStatus { /// TrustedOperation is submitted to the top pool. Submitted, From 749ffb74d1b49f2ed398a461b90a56b1e5739985 Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sun, 27 Oct 2024 09:19:42 +0100 Subject: [PATCH 19/24] fix type inference in trusted transfer cmd --- cli/src/trusted_base_cli/commands/transfer.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/cli/src/trusted_base_cli/commands/transfer.rs b/cli/src/trusted_base_cli/commands/transfer.rs index ab22f812ac..b2e050adbb 100644 --- a/cli/src/trusted_base_cli/commands/transfer.rs +++ b/cli/src/trusted_base_cli/commands/transfer.rs @@ -66,16 +66,11 @@ impl TransferCommand { .sign(&KeyPair::Sr25519(Box::new(from)), nonce, &mrenclave, &shard) .into_trusted_operation(trusted_args.direct); - let mut res; - if trusted_args.direct { - res = Ok(send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?)?; + Ok(send_direct_request(cli, trusted_args, &top).map(|_| CliResultOk::None)?) } else { - res = Ok(perform_trusted_operation::<()>(cli, trusted_args, &top) - .map(|_| CliResultOk::None)?)?; + Ok(perform_trusted_operation::<()>(cli, trusted_args, &top) + .map(|_| CliResultOk::None)?) } - - info!("trusted call transfer executed"); - Ok(res) } } From b5195218d939be0b8780b319e617f063ed7b6ae8 Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sun, 27 Oct 2024 09:20:33 +0100 Subject: [PATCH 20/24] [cli] merge await status update from benchmark and send_direct_request into one function. --- cli/src/benchmark/mod.rs | 10 ++-- cli/src/trusted_operation.rs | 103 ++++++++++++----------------------- 2 files changed, 39 insertions(+), 74 deletions(-) diff --git a/cli/src/benchmark/mod.rs b/cli/src/benchmark/mod.rs index 2329c582e2..feb3d019ad 100644 --- a/cli/src/benchmark/mod.rs +++ b/cli/src/benchmark/mod.rs @@ -21,8 +21,8 @@ use crate::{ trusted_cli::TrustedCli, trusted_command_utils::{get_identifiers, get_keystore_path, get_pair_from_str}, trusted_operation::{ - await_subscription_response, get_json_request, get_state, perform_trusted_operation, - wait_until, + await_status, await_subscription_response, get_json_request, get_state, + perform_trusted_operation, }, Cli, CliResult, CliResultOk, SR25519_KEY_TYPE, }; @@ -352,10 +352,10 @@ fn wait_for_top_confirmation( let confirmed = if wait_for_sidechain_block { // We wait for the transaction hash that actually matches the submitted hash loop { - let transaction_information = wait_until(&client.receiver, is_sidechain_block); - if let Some((hash, _)) = transaction_information { + let transaction_information = await_status(&client.receiver, is_sidechain_block).ok(); + if let Some((hash, _status)) = transaction_information { if hash == submitted.unwrap().0 { - break transaction_information + break Some((hash, Instant::now())) } } } diff --git a/cli/src/trusted_operation.rs b/cli/src/trusted_operation.rs index 508f25bcd1..b95bfcaeb1 100644 --- a/cli/src/trusted_operation.rs +++ b/cli/src/trusted_operation.rs @@ -42,7 +42,6 @@ use std::{ fmt::Debug, result::Result as StdResult, sync::mpsc::{channel, Receiver}, - time::Instant, }; use substrate_api_client::{ ac_compose_macros::compose_extrinsic, GetChainInfo, SubmitAndWatch, SubscribeEvents, XtStatus, @@ -308,12 +307,28 @@ pub(crate) fn send_direct_request( debug!("subscribing to updates for top with hash: {top_hash:?}"); + match await_status(&receiver, connection_can_be_closed) { + Ok((_hash, status)) => { + debug!("Trusted operation reached status {status:?}"); + Ok(top_hash) + }, + Err(e) => { + error!("Error submitting top: {e:?}"); + direct_api.close().unwrap(); + Err(e) + }, + } +} + +pub(crate) fn await_status( + receiver: &Receiver, + wait_until: impl Fn(TrustedOperationStatus) -> bool, +) -> TrustedOpResult<(Hash, TrustedOperationStatus)> { loop { debug!("waiting for update"); let (subscription_update, direct_request_status) = await_status_update(&receiver).map_err(|e| { error!("Error getting status update: {:?}", e); - direct_api.close().unwrap(); e })?; @@ -322,25 +337,29 @@ pub(crate) fn send_direct_request( DirectRequestStatus::Error => { let err = subscription_update.params.error.unwrap_or("{}".into()); debug!("request status is error"); - direct_api.close().unwrap(); - return Err(TrustedOperationError::Default { - msg: format!("[Error] DirectRequestStatus::Error: {err}"), - }) + return Err(into_default_trusted_op_err(format!( + "[Error] DirectRequestStatus::Error: {err}" + ))) }, DirectRequestStatus::TrustedOperationStatus(status) => { debug!("request status is: {:?}", status); - if let Ok(value) = Hash::from_hex(&subscription_update.params.subscription) { - println!("Trusted call {:?} is {:?}", value, status); - } - if connection_can_be_closed(status) { - direct_api.close().unwrap(); - return Ok(top_hash) + let hash = + Hash::from_hex(&subscription_update.params.subscription).map_err(|e| { + into_default_trusted_op_err(format!("Invalid subscription top hash: {e:?}")) + })?; + + println!("Trusted call {:?} is {:?}", hash, status); + + if wait_until(status) { + return Ok((hash, status)) } }, DirectRequestStatus::Ok => { - debug!("request status is ignored"); - direct_api.close().unwrap(); - return Ok(top_hash) + // Todo: #1625. When sending `author_submitAndWatchExtrinsic` this can never happen. + // our cli tries to do too much in one method, we should have better separation of + // concerns. + panic!("`DirectRequestStatus::Ok` should never occur with `author_submitAndWatchExtrinsic`.\ + This is a bug in the usage of the cli."); }, } } @@ -408,60 +427,6 @@ pub(crate) fn get_json_request( .unwrap() } -pub(crate) fn wait_until( - receiver: &Receiver, - until: impl Fn(TrustedOperationStatus) -> bool, -) -> Option<(H256, Instant)> { - debug!("waiting for rpc response"); - loop { - match receiver.recv() { - Ok(response) => { - debug!("received response: {}", response); - let parse_result: StdResult = serde_json::from_str(&response); - if let Ok(response) = parse_result { - if let Ok(return_value) = RpcReturnValue::from_hex(&response.result) { - debug!("successfully decoded rpc response: {:?}", return_value); - match return_value.status { - DirectRequestStatus::Error => { - debug!("request status is error"); - if let Ok(value) = - String::decode(&mut return_value.value.as_slice()) - { - error!("{}", value); - } - return None - }, - DirectRequestStatus::TrustedOperationStatus(status) => { - debug!("request status is: {:?}", status); - if let Ok(value) = Hash::decode(&mut return_value.value.as_slice()) - { - println!("Trusted call {:?} is {:?}", value, status); - if until(status.clone()) { - return Some((value, Instant::now())) - } else if status == TrustedOperationStatus::Invalid { - error!("Invalid request"); - return None - } - } - }, - DirectRequestStatus::Ok => { - debug!("request status is ignored"); - return None - }, - } - }; - } else { - error!("Could not parse response"); - }; - }, - Err(e) => { - error!("failed to receive rpc response: {:?}", e); - return None - }, - }; - } -} - fn connection_can_be_closed(top_status: TrustedOperationStatus) -> bool { !matches!( top_status, From 9f99f249bbf58386149e6dbd820a8e63425d0611 Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sun, 27 Oct 2024 09:39:10 +0100 Subject: [PATCH 21/24] [cli] partially fix benchmarks --- cli/src/benchmark/mod.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/cli/src/benchmark/mod.rs b/cli/src/benchmark/mod.rs index feb3d019ad..eb5aa559f6 100644 --- a/cli/src/benchmark/mod.rs +++ b/cli/src/benchmark/mod.rs @@ -50,7 +50,7 @@ use sp_keystore::Keystore; use std::{ boxed::Box, string::ToString, - sync::mpsc::{channel, Receiver}, + sync::mpsc::{channel, Receiver, Sender}, thread, time, time::Instant, vec::Vec, @@ -92,6 +92,7 @@ struct BenchmarkClient { account: sr25519_core::Pair, current_balance: u128, client_api: DirectClient, + sender: Sender, receiver: Receiver, } @@ -107,8 +108,8 @@ impl BenchmarkClient { debug!("setup sender and receiver"); let (sender, receiver) = channel(); - client_api.watch(initial_request, sender); - BenchmarkClient { account, current_balance: initial_balance, client_api, receiver } + client_api.watch(initial_request, sender.clone()); + BenchmarkClient { account, current_balance: initial_balance, client_api, sender, receiver } } } @@ -164,7 +165,7 @@ impl BenchmarkCommand { &mrenclave, &shard, ) - .into_trusted_operation(trusted_args.direct); + .into_trusted_operation(true); // For the last account we wait for confirmation in order to ensure all accounts were setup correctly let wait_for_confirmation = i == self.number_clients - 1; @@ -221,7 +222,8 @@ impl BenchmarkCommand { let last_iteration = i == self.number_iterations - 1; let jsonrpc_call = get_json_request(shard, &top, shielding_pubkey); - client.client_api.send(&jsonrpc_call).unwrap(); + + client.client_api.watch(jsonrpc_call, client.sender.clone()); let result = wait_for_top_confirmation( self.wait_for_confirmation || last_iteration, &client, From 7e657bcfa0c33b75c3a8b8f9143d36205537b9fa Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sun, 27 Oct 2024 10:01:33 +0100 Subject: [PATCH 22/24] fix clippy --- cli/src/trusted_operation.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/src/trusted_operation.rs b/cli/src/trusted_operation.rs index b95bfcaeb1..ac3a1f522b 100644 --- a/cli/src/trusted_operation.rs +++ b/cli/src/trusted_operation.rs @@ -327,7 +327,7 @@ pub(crate) fn await_status( loop { debug!("waiting for update"); let (subscription_update, direct_request_status) = - await_status_update(&receiver).map_err(|e| { + await_status_update(receiver).map_err(|e| { error!("Error getting status update: {:?}", e); e })?; From bc95fe92331d131c01869da03287e0de821652fb Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sun, 27 Oct 2024 10:57:54 +0100 Subject: [PATCH 23/24] fix local setup for two workers --- local-setup/config/two-workers.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/local-setup/config/two-workers.json b/local-setup/config/two-workers.json index 07b9d8b6ef..a52d79fc52 100644 --- a/local-setup/config/two-workers.json +++ b/local-setup/config/two-workers.json @@ -63,8 +63,7 @@ ], "subcommand_flags": [ "--skip-ra", - "--dev", - "--request-state" + "--dev" ] } ] From 0d2cade18edeb465a90e3c7853ff9eb72d549b74 Mon Sep 17 00:00:00 2001 From: Christian Langenbacher Date: Sun, 27 Oct 2024 11:05:42 +0100 Subject: [PATCH 24/24] [cli] fix: always close client in trusted_operation and return if the status is invalid. --- cli/src/trusted_operation.rs | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/cli/src/trusted_operation.rs b/cli/src/trusted_operation.rs index ac3a1f522b..321e8626d4 100644 --- a/cli/src/trusted_operation.rs +++ b/cli/src/trusted_operation.rs @@ -310,6 +310,7 @@ pub(crate) fn send_direct_request( match await_status(&receiver, connection_can_be_closed) { Ok((_hash, status)) => { debug!("Trusted operation reached status {status:?}"); + direct_api.close().unwrap(); Ok(top_hash) }, Err(e) => { @@ -350,6 +351,11 @@ pub(crate) fn await_status( println!("Trusted call {:?} is {:?}", hash, status); + if is_cancelled(status) { + debug!("trusted call has been cancelled"); + return Ok((hash, status)) + } + if wait_until(status) { return Ok((hash, status)) } @@ -428,12 +434,11 @@ pub(crate) fn get_json_request( } fn connection_can_be_closed(top_status: TrustedOperationStatus) -> bool { - !matches!( - top_status, - TrustedOperationStatus::Submitted - | TrustedOperationStatus::Future - | TrustedOperationStatus::Ready - | TrustedOperationStatus::Broadcast - | TrustedOperationStatus::Invalid - ) + use TrustedOperationStatus::*; + !matches!(top_status, Submitted | Future | Ready | Broadcast) +} + +fn is_cancelled(top_status: TrustedOperationStatus) -> bool { + use TrustedOperationStatus::*; + matches!(top_status, Invalid | Usurped | Dropped | Retracted) }