-
Notifications
You must be signed in to change notification settings - Fork 242
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
V15 Metadata: Support accessing custom types #1106
Conversation
subxt/src/lib.rs
Outdated
@@ -5,7 +5,7 @@ | |||
//! Subxt is a library for interacting with Substrate based nodes. Using it looks something like this: | |||
//! | |||
//! ```rust,ignore | |||
#![doc = include_str!("../examples/tx_basic.rs")] | |||
#![doc = include_str ! ("../examples/tx_basic.rs")] |
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: Is this intended?
subxt/src/custom_types/mod.rs
Outdated
|
||
impl<'a> CustomValueMetadata<'a> { | ||
/// the scale encoded value | ||
pub fn encoded(&self) -> &[u8] { |
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 name this value
and update the documentation to say The custom value associated with the type
, since developers are free to place any binary representation/value 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.
I wouldn't call it value
, because that overlaps with a lot of naming we use where value
returns a scale_value::Value
. encoded()
is consistent with other areas of the API at least (.bytes()
would also work to me, to signal that it's quite arbitrary and may not necessarily be a scale encoded value)
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.
Lovely PR and useful to expose those custom fields to users!
Do you think it would be worth exposing the custom types in the codegen as well?
That's an interesting thought. Being quite arbitrary, some of the types and values may not line up (and so codegen should be lenient and just ignore any errors), and the string names are also arbitrary and need not be valid idents or anything, but one could expose codegen for the cases where things appear to line up. It'd be cool to be able to do I'll raise an issue so we can consider this as a follow-up |
metadata/src/lib.rs
Outdated
pub fn custom_metadata(&self) -> &CustomMetadata<PortableForm> { | ||
&self.custom | ||
} |
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 should hide the internals (frame-metadata stuff) a little better here and have an api more like:
let custom = metadata.custom();
let Some(value) = custom.get("Foo") else { return };
let type_id: u32 = value.ty();
let data: &[u8] = value.bytes();
// so maybe thats implemented something like:
pub struct CustomMetadata {
// We can leave the frame_metadata map as is:
values: BTreeMap<String, frame_metadata::CustomValueMetadata<T>>
}
pub struct CustomMetadataValue<'a> {
type_id: u32,
data: &'a Vec<u8>
}
impl CustomMetadata {
pub fn get(&'a self, &str) -> Option<CustomMetadataValue<'a>>
}
impl CustomMetadataValue {
pub fn ty(&self) -> u32;
pub fn bytes(&Self) -> &[u8]
}
subxt/src/client/online_client.rs
Outdated
/// Access custom types. | ||
pub fn custom_types(&self) -> CustomTypes { |
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's really the values that we are accessing (and the values happen to come with type info) so maybe custom_values -> CustomValues
makes more sense?
metadata/src/from_into/v15.rs
Outdated
@@ -17,6 +17,7 @@ use std::collections::HashMap; | |||
// Converting from V15 metadata into our Subxt repr. | |||
mod from_v15 { | |||
use super::*; | |||
use crate::CustomMetadata; |
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.
We don't need this for all of the other types?
metadata/src/lib.rs
Outdated
/// Metadata of custom types with custom values, basically the same as `frame_metadata::v15::CustomMetadata<PortableForm>>`. | ||
#[derive(Debug, Clone)] | ||
pub struct CustomMetadata { | ||
pub(crate) map: BTreeMap<String, frame_metadata::v15::CustomValueMetadata<PortableForm>>, |
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 think the pub(crate)
is necessary (since sub-modules can access stuff in parent ones anyway, and it's not needed anywhere else)
metadata/src/lib.rs
Outdated
pub fn bytes(&self) -> &[u8] { | ||
&self.data | ||
} |
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.
pub fn bytes(&self) -> &[u8] { | |
&self.data | |
} | |
pub fn bytes(&self) -> &'a [u8] { | |
&self.data | |
} |
Just to tie the lifetime to that of the bytes and not that of this CustomMetadataValue
instance
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.
Oh yeah true, nice catch!
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.
Typo in the file name, should be custom_value_address.rs
:)
fn name(&self) -> &str; | ||
} | ||
|
||
impl CustomValueAddress for str { |
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.
Maybe for consistency we just impl this on &str
so that we don't need the + ?Sized
bound below? I'm a bit torn; it doesn't really matter either way afaik :)
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 wanted to use str directly, because of this interface:
at<Address: CustomValueAddress + ?Sized>(&self, address: &Address)
When I used &str in the trait implementation, this is expecting &&str
to be passed as address
. And then it cannot be easily used as e.g. custom_value_client.at("Foo")
anymore.
map: { | ||
let mut m = BTreeMap::new(); | ||
m.insert("Person".to_string(), person_value_metadata); | ||
m | ||
}, |
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.
Just FYI a nice alternate way to declare this is:
map: { | |
let mut m = BTreeMap::new(); | |
m.insert("Person".to_string(), person_value_metadata); | |
m | |
}, | |
map: BTreeMap::from_iter([ | |
("Person".to_string(), person_value_metadata) | |
]), |
Just some minor comments but looks great overall; nice one! |
//! | ||
//! Note: Names of custom values are converted to __snake_case__ to produce a valid function name during code generation. | ||
//! If there are multiple values where the names would be equal when converted to __snake_case__, functions might not be statically generated for some of them, because of naming conflicts. | ||
//! Make sure names in the custom values of your metadata differ significantly. |
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.
//! Make sure names in the custom values of your metadata differ significantly. |
Just because I think it's covered above, and users often won't have the ability to influence chain metadata anyway :)
//! let api = OnlineClient::<PolkadotConfig>::new().await?; | ||
//! let custom_value_client = api.custom_values(); | ||
//! | ||
//! // Now the `at()` function already decodes the value into the Foo type: |
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.
//! // Now the `at()` function already decodes the value into the Foo type: | |
//! // Now, the `at()` function decodes the value into the correct type: |
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.
Lovely docs; nice one!
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.
Two small things you'll need to do to make tests pass:
- import metadata from the right location (look at other doc examples for this)
- wrap the doc stuff in a hidden
#[tokio::main] async fn main()
thing (use#
to hide this; check out other examples to see :))
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.
Lovely docs indeed! 🚀
//! | ||
//! ``` | ||
//! | ||
//! Alternatively we also provide a statically generated api for custom values: |
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 guess the logic for this is a followup PR, but as long as we don't do another release before the static stuff merges too then it's ok to leave the docs 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.
Looks great to me!
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! Amazing job here!
fixes #1042.
Custom types are now available from
OfflineClientT
like this: