Add JSON spec (ChainTypes) for defining chain specific types #3
Conversation
| /// | ||
| /// # Example | ||
| /// | ||
| /// ```rust |
There was a problem hiding this comment.
This example is a good starting place to understand the format (or see the tests) :)
|
|
||
| /// Extend this chain type registry with the one provided. In case of any matches, the provided types | ||
| /// will overwrite the existing ones. | ||
| pub fn extend(&mut self, other: ChainTypeRegistry) { |
There was a problem hiding this comment.
Ahh cool, so this kind of mirrors the Overrides in PJS. Very cool.
There was a problem hiding this comment.
The reason I added this is mainly that we might want t odefine a load of general substrate types in a JSON file somewhere, and then define eg polkadot specific types in another file, and then load both and merge them to get the ChainTypeRegistry for a chain (polkadot in this case).
There was a problem hiding this comment.
I haven't read the implementation yet but how are we dealing with duplicate JSON keys in the ChainTypeRegistry?
I have done weird stuff with JSON before, duplicate keys not recommended :)
There was a problem hiding this comment.
On a high level, the HashMap serde impl will handle it (ie it probably will overwrite earlier keys with later ones). The enum logic will just assume that an enum has two variants with the same name, and structs would have two fields of the same name.
If you mean dupe keys as in, name mentioned in "global" types and also in some specific spec range or whatever, then any type in a specific "pallet" wins, and any type in the last seen spec range wins!
Overall I think it's ok if such behaviour is "undefined" and it's a user error to provide dupe keys in the same object :D
There was a problem hiding this comment.
Overall I think it's ok if such behaviour is "undefined" and it's a user error to provide dupe keys in the same object :D
Correct
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
src/type_registry.rs
Outdated
| TypeNotFound(String), | ||
| LookupNameInvalid(String, lookup_name::ParseError), | ||
| #[display(fmt = "Type not found")] | ||
| TypeNotFound, |
There was a problem hiding this comment.
Could be useful to know which type/name that wasn't found but there are others that already displays that such as LookupNameInvalid, so what's the reason why it is omitted here?
There was a problem hiding this comment.
Good question; I'll have to remind myself why I removed the string, because yeah it would be useful!
There was a problem hiding this comment.
Ok, so actually I removed TypeNotFound entirely, since I should be calling visitor.visit_not_found() instead, I think the idea there was that the person who hands the visitor also hands the type ID, so they will always know what it is anyway, but I'm not super convinced and will have to play around and see whether we should be removing .visit_not_found() or adding the TypeId as an arg to it or whatever!
| /// can not be parsed into a valid [`TypeName`]. | ||
| pub fn parse_unwrap(input: &str) -> TypeName { | ||
| Self::parse(input).unwrap() | ||
| impl core::convert::TryFrom<&str> for LookupName { |
There was a problem hiding this comment.
I think when we been down this route before but it doesn't work to impl this from TryFrom<AsRef<str>>, no?
There was a problem hiding this comment.
Yeah, impl <T: AsRef<str>> TryFrom<T> is no good because there's already a generic impl on T in the std library (and it doesn't consider trait bounds!
| impl core::fmt::Display for TypeName { | ||
| impl PartialEq for LookupName { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| // Bit of a hack, but it does the trick: |
There was a problem hiding this comment.
Fine by me but can't you implement PartialEq for TypeNameDef and compare them instead?
There was a problem hiding this comment.
I hid this impl behind a cfg(test) for now, since we only care about this comparison stuff in testing, and it's a bit weird :)
| Ok(DeserializableShape::AliasOf(name)) | ||
| } | ||
|
|
||
| // Either '{ _enum: ... }' for enum descriptions, or '{ a: ..., b: ... }' for struct descriptions. |
There was a problem hiding this comment.
dq: is this a notation you came up with to distinguish enums and structs?
There was a problem hiding this comment.
This is copied from how PJS does it (https://polkadot.js.org/docs/api/start/types.extend#user-defined-enum) :)
The main thing we added here is a
ChainTypestype.ChainTypes::for_spec_versionto get something that's ready to resolve types for a specific spec version.Chaintypesinstances via.extendto eg merge a JSON file with basic substrate types with a JSON file with eg polkadot specific types on top.Under this, a
TypeRegistrySetis added, which is a borrowed (or owned) set ofTypeRegistrys that resolves types from them in a specific order, falling back to previous registries if types aren't found, and resolving pallet types before global ones.