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

Implement ResultQuery #11257

Merged
merged 22 commits into from Aug 24, 2022
Merged

Conversation

KiChjang
Copy link
Contributor

@KiChjang KiChjang commented Apr 21, 2022

Fixes #11010.

Adds a new query type called ResultQuery to the list of possible query kinds for storage items. We now have OptionQuery, ValueQuery and ResultQuery.

To use it properly, one must also declare a pallet error enum, since that is what the ResultQuery uses as the Err type. When specifying the query type as ResultQuery, a type parameter is expected, and instead of providing an actual type to it, the macro expects a type path that points to a variant of an enum that implements PalletError, which will be used as the Err return value when a value is not found under the supplied storage key, i.e. StorageValue<_, u8, ResultQuery<PalletError<T>::EnumVariantName>> is the correct way to declare a StorageValue that returns a Result containing an Err value of Err(PalletError::<T>::EnumVariantName) when a value under the storage key is not found.

@KiChjang KiChjang added A0-please_review Pull request needs code review. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Apr 21, 2022
@KiChjang KiChjang added this to In progress in Runtime via automation Apr 21, 2022
@shawntabrizi shawntabrizi moved this from In progress to Please Review in Runtime May 5, 2022
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

In general looking good. However, I would pass the Error explicitly, e.g. Error::SomeVariant. This makes it easier to know where SomeVariant is coming from.

frame/support/procedural/src/pallet/expand/storage.rs Outdated Show resolved Hide resolved
frame/support/procedural/src/pallet/expand/storage.rs Outdated Show resolved Hide resolved
frame/support/test/tests/pallet_instance.rs Outdated Show resolved Hide resolved
@shawntabrizi
Copy link
Contributor

bot rebase

@paritytech-processbot
Copy link

Rebasing

@KiChjang KiChjang requested a review from ggwpez July 12, 2022 06:14
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

My review of macros was a bit shallow but tests and the new API all look good to me.

Should be documented in at least one more place, and should mention what error types should be used.

Runtime automation moved this from Please Review to Needs Audit Jul 12, 2022
@stale
Copy link

stale bot commented Aug 11, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Aug 11, 2022
@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Aug 15, 2022
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Just one thing with the syntax.

frame/support/procedural/src/pallet/expand/storage.rs Outdated Show resolved Hide resolved
frame/support/test/tests/pallet.rs Show resolved Hide resolved
frame/support/test/tests/pallet_instance.rs Show resolved Hide resolved
Copy link
Contributor

@sam0x17 sam0x17 left a comment

Choose a reason for hiding this comment

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

This looks good to me. One quibble I have is the nesting/indenting gets very very deep at one point in the macro but this can merge regardless of whether that is fixed

frame/support/procedural/src/pallet/parse/storage.rs Outdated Show resolved Hide resolved
@@ -1436,6 +1532,8 @@ fn test_storage_info() {
traits::{StorageInfo, StorageInfoTrait},
};

// Storage max size is calculated by adding up all the hasher size, the key type size and the
// value type size
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KiChjang
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Head SHA changed from 46d53f7 to 17b41b5

@KiChjang
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit bf77292 into master Aug 24, 2022
@paritytech-processbot paritytech-processbot bot deleted the kckyeung/storage-result-query branch August 24, 2022 17:47
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Implement ResultQuery

* Fix test expectations

* Add more tests

* Fix test expectations

* Clean up some names

* Silence warnings

* Specify error type when supplying error type to ResultQuery

* cargo fmt

* Add support for type parameters in parameter_types macro

* Reduce deeply indented code

* Fixes

* Update test expectation

* Rewrite and document formula for calculating max storage size

* More docs

* cargo fmt

* formatting

Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
Archived in project
Runtime
  
Needs Audit
Development

Successfully merging this pull request may close these issues.

ResultQuery/ custom Err type for Storage*::try_get
7 participants