-
Notifications
You must be signed in to change notification settings - Fork 690
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
[RPC-Spec-V2] chainHead: use integer for block index and adjust RuntimeVersion JSON format #1666
Conversation
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.
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 👍
#[serde( | ||
serialize_with = "custom_serialize::serialize_runtime_version", | ||
deserialize_with = "custom_serialize::deserialize_runtime_version" | ||
)] |
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 do wonder if we could take another approach here, since implementing deserialization is quite complicated in general:
- Create a private
RuntimeVersionWrapper
/RuntimeVersionInner
which contains the fields in final serailization/deserialization shape - Use serde(from = "RuntimeVersionWrapper") and serde(into = "RuntimeVersionWrapper")
- Implement
From
for those structs
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.
@lexnv Good idea, would definitely be simpler! I don't know if it is important, but this would come at a slightly higher runtime cost because we would construct a HashMap every time we create a RuntimeVersionWrapper
from a RuntimeVersion
before serializing it.
@@ -36,6 +36,7 @@ tokio = { version = "1.22.0", features = ["sync"] } | |||
array-bytes = "6.1" | |||
log = "0.4.17" | |||
futures-util = { version = "0.3.19", default-features = false } | |||
impl-serde = { version = "0.4.0", default-features = false } |
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 it still be necessary to have this extra dependency if we use a serde(from = ...
and serde(into =
for the RuntimeVersion
? 🤔
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 think we would still need it, because we would still need to define how ApiId
(which is a [u8; 8]
) is serialized (as a string). The serialization of RuntimeVersion
also relies on a custom serialization using impl-serde
, so we would ahve to do something similar for our wrapper.
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.
Looks good so far! Just a few comments/suggestions regarding the serde implementations and a small consistency with tx event 👍 Also think there are a couple of places were we'd need to adjust the tests
Thanks for taking a look into these 🙏
…er' of https://github.com/paritytech/polkadot-sdk into tadeohepperle/rpc-spec-v2-apis-as-map-and-index-as-number
#[serde(rename_all = "camelCase")] | ||
#[derive(Debug, Clone, Serialize, Deserialize)] | ||
#[serde( | ||
rename_all = "camelCase", |
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.
nit: we could move the #[serde(rename_all = "camelCase")]
to RuntimeVersionEventWrapper
instead and remove it from here
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.
LGTM, nice one! 👍
impl TryFrom<ChainHeadRuntimeVersion> for RuntimeVersion { | ||
type Error = IntoRuntimeVersionError; | ||
|
||
fn try_from(val: ChainHeadRuntimeVersion) -> Result<Self, Self::Error> { | ||
let apis = val | ||
.apis | ||
.into_iter() | ||
.map(|(api, version)| -> Result<([u8; 8], u32), IntoRuntimeVersionError> { | ||
let bytes_vec = sp_core::bytes::from_hex(&api)?; | ||
let api: [u8; 8] = | ||
bytes_vec.try_into().map_err(|_| IntoRuntimeVersionError::InvalidLength)?; | ||
Ok((api, version)) | ||
}) | ||
.collect::<Result<sp_version::ApisVec, IntoRuntimeVersionError>>()?; | ||
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() | ||
}) | ||
} | ||
} |
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.
Is this TryFrom impl used anywhere?
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 wonder whether you can ditch the TryFrom
impl (do we need to be able to convert from new to old?), but otherwise looks good!
* master: (24 commits) Init System Parachain storage versions and add migration check jobs to CI (#1344) no-bound derives: Use absolute path for `core` (#1763) migrate alliance, fast-unstake and bags list to use derive-impl (#1636) Tvl pool staking (#1322) improve service error (#1734) frame-support: `RuntimeDebug\Eq\PartialEq` impls for `Imbalance` (#1717) Point documentation links to monorepo (#1741) [NPoS] Fix for Reward Deficit in the pool (#1255) Move import queue from `ChainSync` to `SyncingEngine` (#1736) Enable mocking contracts (#1331) Revert "fix(review-bot): pull secrets from `master` environment" (#1748) Remove kusama and polkadot runtime crates (#1731) Use `Extensions` to register offchain worker custom extensions (#1719) [RPC-Spec-V2] chainHead: use integer for block index and adjust RuntimeVersion JSON format (#1666) fix(review-bot): pull secrets from `master` environment (#1745) Fix `subkey inspect` output text padding (#1744) archive: Implement height, hashByHeight and call (#1582) rpc/client: Propagate `rpc_methods` method to reported methods (#1713) rococo-runtime: `RococoGenesisExt` removed (#1490) PVF: more filesystem sandboxing (#1373) ...
* tsv-disabling-node-side: (24 commits) Init System Parachain storage versions and add migration check jobs to CI (#1344) no-bound derives: Use absolute path for `core` (#1763) migrate alliance, fast-unstake and bags list to use derive-impl (#1636) Tvl pool staking (#1322) improve service error (#1734) frame-support: `RuntimeDebug\Eq\PartialEq` impls for `Imbalance` (#1717) Point documentation links to monorepo (#1741) [NPoS] Fix for Reward Deficit in the pool (#1255) Move import queue from `ChainSync` to `SyncingEngine` (#1736) Enable mocking contracts (#1331) Revert "fix(review-bot): pull secrets from `master` environment" (#1748) Remove kusama and polkadot runtime crates (#1731) Use `Extensions` to register offchain worker custom extensions (#1719) [RPC-Spec-V2] chainHead: use integer for block index and adjust RuntimeVersion JSON format (#1666) fix(review-bot): pull secrets from `master` environment (#1745) Fix `subkey inspect` output text padding (#1744) archive: Implement height, hashByHeight and call (#1582) rpc/client: Propagate `rpc_methods` method to reported methods (#1713) rococo-runtime: `RococoGenesisExt` removed (#1490) PVF: more filesystem sandboxing (#1373) ...
…meVersion JSON format (paritytech#1666) This PR adjusts the serialized format of the the returned RuntimeVersion in the rpc-spec-v2 methods. This is done to match the format defined here: https://paritytech.github.io/json-rpc-interface-spec/api/chainHead_unstable_follow.html#about-the-runtime - ##### `apis` field as object `apis` field of `RuntimeVersion` is now returned as an object, e.g. ``` "apis": { "0xdf6acb689907609b": 3, "0x37e397fc7c91f5e4": 1, } ``` instead of ``` "apis": [ ["0xdf6acb689907609b", 3], ["0x37e397fc7c91f5e4", 1], ] ``` - ##### removed `stateVersion` and `authoringVersion` `stateVersion` and `authoringVersion` are no longer returned in the `RuntimeVersion` JSON Object. - ##### block index in chain head events as integer ### Related Issues Closes: paritytech#1507 Closes: paritytech#1146 ### Testing Done Adjusted existing tests to make sure data is returned in the correct format.
This PR adjusts the serialized format of the the returned RuntimeVersion in the rpc-spec-v2 methods. This is done to match the format defined here: https://paritytech.github.io/json-rpc-interface-spec/api/chainHead_unstable_follow.html#about-the-runtime
apis
field as objectapis
field ofRuntimeVersion
is now returned as an object, e.g.instead of
removed
stateVersion
andauthoringVersion
stateVersion
andauthoringVersion
are no longer returned in theRuntimeVersion
JSON Object.block index in chain head events as integer
Related Issues
Closes: #1507
Closes: #1146
Testing Done
Adjusted existing tests to make sure data is returned in the correct format.