Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

pallet macro: always generate storage info on pallet struct #9246

Merged
4 commits merged into from
Jul 5, 2021
Merged

pallet macro: always generate storage info on pallet struct #9246

4 commits merged into from
Jul 5, 2021

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Jul 1, 2021

Currently the trait StorageInfoTrait is implemented on the struct Pallet only when the attribute generate_storage_info is set.
But it can be handy to always generate the storage info even with partial information.

This PR makes storage implement a new trait PartialStorageInfoTrait which allows to return some partial information.
Then depending on whereas the attribute generate_storage_info is set, the Pallet will implement the StorageInfo using PartialStorageInfo or using StorageInfo

as a side note, the span should be improved in a follow up by #8850
the implementation should also be improved currently we allocate some small vec way too much. But we can do in a follow-up.

cc @shawntabrizi

@gui1117 gui1117 changed the title Always generate storage info pallet macro: always generate storage info on pallet struct Jul 1, 2021
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jul 1, 2021
@gui1117 gui1117 added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jul 1, 2021
@shawntabrizi shawntabrizi requested a review from KiChjang July 1, 2021 23:11
shawntabrizi
shawntabrizi approved these changes Jul 1, 2021
View reviewed changes
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

lgtm

KiChjang
KiChjang approved these changes Jul 2, 2021
View reviewed changes
Copy link
Contributor

@KiChjang KiChjang left a comment

Choose a reason for hiding this comment

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

Looks good overall. I think one question that I would have now is the discoverability of storage items that were implemented with PartialStorageInfo. This is useful when I may later on want to visit those items and implement StorageInfo directly, but I guess this is more of a UI issue than anything else.

@gui1117
Copy link
Contributor Author

gui1117 commented Jul 2, 2021

Looks good overall. I think one question that I would have now is the discoverability of storage items that were implemented with PartialStorageInfo. This is useful when I may later on want to visit those items and implement StorageInfo directly, but I guess this is more of a UI issue than anything else.

The partial storage info doesn't have the max_size bound. I think when the benchmarking process fail to get proper bound for max_size it will return an error or a warning saying that the storage doesn't have valid bound for PoV size computation.

@gui1117
Copy link
Contributor Author

gui1117 commented Jul 5, 2021

bot merge

@ghost
Copy link

ghost commented Jul 5, 2021

Trying merge.

@ghost ghost merged commit cd48bac into master Jul 5, 2021
@ghost ghost deleted the gui-always-generate-storage-info branch July 5, 2021 11:23
@gui1117 gui1117 mentioned this pull request Jul 5, 2021
Merged
@gui1117 gui1117 added D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Jul 5, 2021
ordian added a commit that referenced this pull request Jul 5, 2021
@ordian
Merge branch 'master' into ao-bump-common
7160c16 
* master:
  Bump linregress due to security vulnerability (#9262)
  pallet macro: always generate storage info on pallet struct (#9246)
  Less duplication in test code (#9270)
  Add `Chilled` event to staking chill extrinsics (#9250)
@gui1117 gui1117 mentioned this pull request Jul 5, 2021
Merged
dvdplm added a commit that referenced this pull request Jul 6, 2021
@dvdplm
Merge branch 'master' into dp-jsonrpsee-integration-2
3cb6181 
* master:
  fix staking version in genesis (#9280)
  fix storage info for decl_storage (#9274)
  Authority_discovery: expose assimilate_storage with GenesisBuild (#9279)
  Update CODEOWNERS (#9278)
  Remove in-tree `max-encoded-len` and use the new SCALE codec crate instead (#9163)
  bump a bunch of deps in parity-common (#9263)
  Bump linregress due to security vulnerability (#9262)
  pallet macro: always generate storage info on pallet struct (#9246)
  Less duplication in test code (#9270)
  Add `Chilled` event to staking chill extrinsics (#9250)
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KiChjang KiChjang KiChjang approved these changes

@shawntabrizi shawntabrizi shawntabrizi approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gui1117 @shawntabrizi @KiChjang