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

[RPC-Spec-V2] chainHead: use integer for block index and adjust RuntimeVersion JSON format #1666

Merged
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion substrate/client/rpc-spec-v2/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ tokio = { version = "1.22.0", features = ["sync"] }
array-bytes = "6.1"
log = "0.4.17"
futures-util = { version = "0.3.19", default-features = false }

[dev-dependencies]
serde_json = "1.0"
tokio = { version = "1.22.0", features = ["macros"] }
Expand Down
1 change: 0 additions & 1 deletion substrate/client/rpc-spec-v2/src/chain_head/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ impl From<Error> for ErrorObject<'static> {
Error::InvalidSubscriptionID => ErrorObject::owned(INVALID_SUB_ID, msg, None::<()>),
Error::InvalidContinue => ErrorObject::owned(INVALID_CONTINUE, msg, None::<()>),
}
.into()
}
}

Expand Down
110 changes: 104 additions & 6 deletions substrate/client/rpc-spec-v2/src/chain_head/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
use serde::{ser::SerializeStruct, Deserialize, Serialize, Serializer};
use sp_api::ApiError;
use sp_version::RuntimeVersion;
use std::collections::BTreeMap;
use thiserror::Error;

/// The operation could not be processed due to an error.
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
Expand All @@ -35,13 +37,108 @@ pub struct ErrorEvent {
/// This event is generated for:
/// - the first announced block by the follow subscription
/// - blocks that suffered a change in runtime compared with their parents
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(
rename_all = "camelCase",
Copy link
Contributor

@lexnv lexnv Sep 25, 2023

Choose a reason for hiding this comment

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

nit: we could move the #[serde(rename_all = "camelCase")] to RuntimeVersionEventWrapper instead and remove it from here

try_from = "RuntimeVersionEventWrapper",
into = "RuntimeVersionEventWrapper"
)]
pub struct RuntimeVersionEvent {
/// The runtime version.
pub spec: RuntimeVersion,
}

// Note: PartialEq mainly used in tests, manual implementation necessary, because BTreeMap in
// RuntimeVersionWrapper does not preserve order of apis vec.
impl PartialEq for RuntimeVersionEvent {
fn eq(&self, other: &Self) -> bool {
RuntimeVersionWrapper::from(self.spec.clone()) ==
RuntimeVersionWrapper::from(other.spec.clone())
}
}

#[derive(Clone, PartialEq, Debug, Serialize, Deserialize)]
struct RuntimeVersionEventWrapper {
spec: RuntimeVersionWrapper,
}

impl TryFrom<RuntimeVersionEventWrapper> for RuntimeVersionEvent {
type Error = ApiFromHexError;

fn try_from(val: RuntimeVersionEventWrapper) -> Result<Self, Self::Error> {
let spec = val.spec.try_into()?;
Ok(Self { spec })
}
}

impl From<RuntimeVersionEvent> for RuntimeVersionEventWrapper {
fn from(val: RuntimeVersionEvent) -> Self {
Self { spec: val.spec.into() }
}
}

#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
struct RuntimeVersionWrapper {
pub spec_name: String,
pub impl_name: String,
pub spec_version: u32,
pub impl_version: u32,
pub apis: BTreeMap<String, u32>,
pub transaction_version: u32,
}

#[derive(Error, Debug)]
enum ApiFromHexError {
#[error("invalid hex string provided")]
FromHexError(#[from] sp_core::bytes::FromHexError),
#[error("buffer must be 8 bytes long")]
InvalidLength,
}

impl TryFrom<RuntimeVersionWrapper> for RuntimeVersion {
type Error = ApiFromHexError;

fn try_from(val: RuntimeVersionWrapper) -> Result<Self, Self::Error> {
let apis = val
.apis
.into_iter()
.map(|(api, version)| -> Result<([u8; 8], u32), ApiFromHexError> {
let bytes_vec = sp_core::bytes::from_hex(&api)?;
let api: [u8; 8] =
bytes_vec.try_into().map_err(|_| ApiFromHexError::InvalidLength)?;
Ok((api, version))
})
.collect::<Result<sp_version::ApisVec, ApiFromHexError>>()?;
Ok(Self {
spec_name: sp_runtime::RuntimeString::Owned(val.spec_name),
impl_name: sp_runtime::RuntimeString::Owned(val.impl_name),
spec_version: val.spec_version,
impl_version: val.impl_version,
apis,
transaction_version: val.transaction_version,
..Default::default()
})
}
}

impl From<RuntimeVersion> for RuntimeVersionWrapper {
fn from(val: RuntimeVersion) -> Self {
Self {
spec_name: val.spec_name.into(),
impl_name: val.impl_name.into(),
spec_version: val.spec_version,
impl_version: val.impl_version,
apis: val
.apis
.into_iter()
.map(|(api, version)| (sp_core::bytes::to_hex(api, false), *version))
.collect(),
transaction_version: val.transaction_version,
}
}
}

/// The runtime event generated if the `follow` subscription
/// has set the `with_runtime` flag.
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
Expand Down Expand Up @@ -391,8 +488,8 @@ mod tests {
let ser = serde_json::to_string(&event).unwrap();
let exp = concat!(
r#"{"event":"initialized","finalizedBlockHash":"0x1","#,
r#""finalizedBlockRuntime":{"type":"valid","spec":{"specName":"ABC","implName":"Impl","authoringVersion":0,"#,
r#""specVersion":1,"implVersion":0,"apis":[],"transactionVersion":0,"stateVersion":0}}}"#,
r#""finalizedBlockRuntime":{"type":"valid","spec":{"specName":"ABC","implName":"Impl","#,
r#""specVersion":1,"implVersion":0,"apis":{},"transactionVersion":0}}}"#,
);
assert_eq!(ser, exp);

Expand Down Expand Up @@ -429,6 +526,7 @@ mod tests {
spec_name: "ABC".into(),
impl_name: "Impl".into(),
spec_version: 1,
apis: vec![([0, 0, 0, 0, 0, 0, 0, 0], 2), ([1, 0, 0, 0, 0, 0, 0, 0], 3)].into(),
..Default::default()
};

Expand All @@ -445,8 +543,8 @@ mod tests {
let ser = serde_json::to_string(&event).unwrap();
let exp = concat!(
r#"{"event":"newBlock","blockHash":"0x1","parentBlockHash":"0x2","#,
r#""newRuntime":{"type":"valid","spec":{"specName":"ABC","implName":"Impl","authoringVersion":0,"#,
r#""specVersion":1,"implVersion":0,"apis":[],"transactionVersion":0,"stateVersion":0}}}"#,
r#""newRuntime":{"type":"valid","spec":{"specName":"ABC","implName":"Impl","#,
r#""specVersion":1,"implVersion":0,"apis":{"0x0000000000000000":2,"0x0100000000000000":3},"transactionVersion":0}}}"#,
);
assert_eq!(ser, exp);

Expand Down
4 changes: 2 additions & 2 deletions substrate/client/rpc-spec-v2/src/chain_head/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,12 +234,12 @@ async fn follow_with_runtime() {
let event: FollowEvent<String> = get_next_event(&mut sub).await;

// it is basically json-encoded substrate_test_runtime_client::runtime::VERSION
let runtime_str = "{\"specName\":\"test\",\"implName\":\"parity-test\",\"authoringVersion\":1,\
let runtime_str = "{\"specName\":\"test\",\"implName\":\"parity-test\",\"authoringVersion\":0,\
\"specVersion\":2,\"implVersion\":2,\"apis\":[[\"0xdf6acb689907609b\",4],\
[\"0x37e397fc7c91f5e4\",2],[\"0xd2bc9897eed08f15\",3],[\"0x40fe3ad401f8959a\",6],\
[\"0xbc9d89904f5b923f\",1],[\"0xc6e9a76309f39b09\",2],[\"0xdd718d5cc53262d4\",1],\
[\"0xcbca25e39f142387\",2],[\"0xf78b278be53f454c\",2],[\"0xab3c0572291feb8b\",1],\
[\"0xed99c5acb25eedf5\",3],[\"0xfbc577b9d747efd6\",1]],\"transactionVersion\":1,\"stateVersion\":1}";
[\"0xed99c5acb25eedf5\",3],[\"0xfbc577b9d747efd6\",1]],\"transactionVersion\":1,\"stateVersion\":0}";

let runtime: RuntimeVersion = serde_json::from_str(runtime_str).unwrap();

Expand Down
5 changes: 2 additions & 3 deletions substrate/client/rpc-spec-v2/src/transaction/event.rs
Copy link
Contributor

Choose a reason for hiding this comment

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

The TransactionBroadcasted would also need to be kept in sync with the number formatting.

We could remove the as_string altogether for now and adjust the testing 👍

Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ pub struct TransactionBlock<Hash> {
/// The hash of the block the transaction was included into.
pub hash: Hash,
/// The index (zero-based) of the transaction within the body of the block.
#[serde(with = "as_string")]
pub index: usize,
}

Expand Down Expand Up @@ -288,7 +287,7 @@ mod tests {
}));
let ser = serde_json::to_string(&event).unwrap();

let exp = r#"{"event":"bestChainBlockIncluded","block":{"hash":"0x0000000000000000000000000000000000000000000000000000000000000001","index":"2"}}"#;
let exp = r#"{"event":"bestChainBlockIncluded","block":{"hash":"0x0000000000000000000000000000000000000000000000000000000000000001","index":2}}"#;
assert_eq!(ser, exp);

let event_dec: TransactionEvent<H256> = serde_json::from_str(exp).unwrap();
Expand All @@ -303,7 +302,7 @@ mod tests {
});
let ser = serde_json::to_string(&event).unwrap();

let exp = r#"{"event":"finalized","block":{"hash":"0x0000000000000000000000000000000000000000000000000000000000000001","index":"10"}}"#;
let exp = r#"{"event":"finalized","block":{"hash":"0x0000000000000000000000000000000000000000000000000000000000000001","index":10}}"#;
assert_eq!(ser, exp);

let event_dec: TransactionEvent<H256> = serde_json::from_str(exp).unwrap();
Expand Down