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

Include type id in serialized type registry #114

Merged
merged 4 commits into from Jul 16, 2021
Merged

Include type id in serialized type registry #114

merged 4 commits into from Jul 16, 2021

Conversation

ascjones
Copy link
Member

Currently the position of a type in the list of types corresponds with its lookup id. This is fine for computers but makes human readability of the serialized json a bit more difficult.

Adding the id of the type to the serialized type registry makes developing and debugging with the type registry much easier, since you can manually lookup a type with a text search in the json metadata.

Encoded size increase should be negligible, just an extra u32 per type.

Hat tip to @jacogr for the idea.

@jacogr
Copy link

jacogr commented Jul 15, 2021

Since we are size-sensitive, a Compact would be great. (Actually goes for all the type lookups, we have a lot of zeros everywhere - it has a very visible in the hex output)

for the ids since we have 500-odd at the moment we shave at least 2 bytes where we lookup. (And also here, 3 bytes for the ids below 128)

@dvdplm
Copy link
Contributor

dvdplm commented Jul 16, 2021

Is it fair to assume that all consuming contexts have tooling available to understand compact encoded data? I guess if they don't, they'll be using the json anyway?

@ascjones
Copy link
Member Author

Correct if they are decoding the SCALE encoded metadata then they should handle compacts. Json output will not be affected.

@ascjones ascjones merged commit 244f7ab into master Jul 16, 2021
@ascjones ascjones deleted the aj-type-index branch July 16, 2021 11:01
@jacogr
Copy link

jacogr commented Jul 16, 2021

If you don't understand Compact, you also cannot decode Vec<T> since it starts with Compact<length> nor String (likewise, length-prefixed). So it is pretty much a requirement to consume anything SCALE.

@Robbepop
Copy link
Collaborator

The JSON representation is not used for debug purposes anyways so I am fine with this change, however, we should really not include this superflous index into the SCALE output if we currently do so.

@ascjones
Copy link
Member Author

This could be done by custom Encode/Decode impls which remove and then add back the index respectively, but would it really be worth it?

@ascjones ascjones mentioned this pull request Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants