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

Copy v13 metadata from substrate, move new scale-info version to v14 #12

Merged
merged 14 commits into from
May 18, 2021

Conversation

ascjones
Copy link
Contributor

@ascjones ascjones commented May 14, 2021

New v13 just been introduced in paritytech/substrate#8635 so we need move the new scale-info stuff to v14 and add the Storage NMap variant.

todo:

  • Add new StorageNMap metadata to v14
  • Update CI checks to check v14

@ascjones ascjones marked this pull request as ready for review May 14, 2021 15:50
@ascjones ascjones requested a review from dvdplm May 17, 2021 16:07
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

Approving this, but I don't really know how to prove to myself that it's complete/correct. It looks like v14.rs is the old v13.rs + NMap but yeah, hard to tell for sure without some serious study.

frame-metadata/src/v13.rs Outdated Show resolved Hide resolved
frame-metadata/src/v13.rs Outdated Show resolved Hide resolved
frame-metadata/src/lib.rs Show resolved Hide resolved
Comment on lines +178 to +179
prefix: self.prefix.into_portable(registry),
entries: registry.map_into_portable(self.entries),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think a macro to implement these IntoPortable impls could work? They're pretty repetitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it would be nice if possible.

frame-metadata/src/v14.rs Outdated Show resolved Hide resolved
@ascjones ascjones merged commit 385bbbd into main May 18, 2021
@ascjones ascjones deleted the aj-v14 branch May 18, 2021 09:34
key2_hasher: StorageHasher,
},
NMap {
keys: T::Type,

Choose a reason for hiding this comment

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

Sorry to comment on a merged PR, but is this intended? keys here is a Vec<&'static str> on substrate master: https://github.com/paritytech/substrate/blob/90cfb952f2e11bc6d327faf04b30dc5f4cf89df8/frame/metadata/src/lib.rs#L304, it would seem to me that this should be Vec<T::Type> instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had it like that originally, but the new metadata types are generated statically (not by the macro) here: https://github.com/paritytech/substrate/blob/25bf1ebe87af73e9cb318f0a9633ed8c9c1cf5b1/frame/support/src/storage/types/nmap.rs#L374.

So for multiple keys where Key::Key is a tuple, the type will point to a tuple type definition which contains all the constituent key types.

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.

3 participants