Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
stateful-storage: check hash in signed extension #2137
stateful-storage: check hash in signed extension #2137
Changes from 4 commits
890bb38
43e1c18
3b98a56
83e7b9a
0111ad7
0174b42
95924b5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Check warning on line 22 in pallets/stateful-storage/src/signed_extension.rs
Codecov / codecov/patch
pallets/stateful-storage/src/signed_extension.rs#L22
Check warning on line 24 in pallets/stateful-storage/src/signed_extension.rs
Codecov / codecov/patch
pallets/stateful-storage/src/signed_extension.rs#L24
Check warning on line 49 in pallets/stateful-storage/src/signed_extension.rs
Codecov / codecov/patch
pallets/stateful-storage/src/signed_extension.rs#L48-L49
Check warning on line 52 in pallets/stateful-storage/src/signed_extension.rs
Codecov / codecov/patch
pallets/stateful-storage/src/signed_extension.rs#L52
Check warning on line 72 in pallets/stateful-storage/src/signed_extension.rs
Codecov / codecov/patch
pallets/stateful-storage/src/signed_extension.rs#L71-L72
Check warning on line 76 in pallets/stateful-storage/src/signed_extension.rs
Codecov / codecov/patch
pallets/stateful-storage/src/signed_extension.rs#L76
Check warning on line 84 in pallets/stateful-storage/src/signed_extension.rs
Codecov / codecov/patch
pallets/stateful-storage/src/signed_extension.rs#L82-L84
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we could end up with an error code that’s not entirely accurate, particularly in the case where zero is returned. Using unwrap_or_default() might not be the best approach here, as falling back to the default value could potentially introduce a bug. That said, I might be overlooking something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, just a note: the "Custom Error" code that we get as part of the RPC "1010" error is entirely arbitrary, and does not indicate the pallet. So a code of
9
(StalePageHash) is not meaningful to the end user, because they don't know what pallet enum that comes from. I do think somewhere there is an enum of custom RPC error codes for Frequency; perhaps we should map to that instead? Sadly Polkadot/Substrate does not give us a way to propagate anything more than an integer value for custom RPC errors.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@enddynayn The first
unwrap_or_default()
tries to convert encoded error value (coming from the position of error in the Error enum in the pallet) tou32
which means it can not fail. The secondunwrap_or_default()
tries to convert u32 to u8 but that would only fail if we have more than 256 errors in the same pallet which is highly unlikely.@JoeCap08055 Since this is coming from
validate
it means that after submission you would immediately get that error back. And for any individual extrinsic since the client knows what type of extrinsic they are submitting it is obvious what that error would mean. I have to look into the batch usecase since that would be trickier.In any case since we only have 256 unique values for different errors we might be able to create a universal custom error enum which would be different between the errors in different pallets but that does not seem to scale based on the total number of pallets and errors. I'm open to suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting approach but works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a wider issue than this PR, but the universal enum idea might not be so bad; we wouldn't need to map every error from every pallet; only the ones that can happen in a SignedExtension check. There shouldn't be that many.
Anyway, this approach works for now; maybe it's worth a backlog issue to revisit how we do this across all of our signed extensions, or maybe not.
Check warning on line 116 in pallets/stateful-storage/src/signed_extension.rs
Codecov / codecov/patch
pallets/stateful-storage/src/signed_extension.rs#L114-L116
Check warning on line 120 in pallets/stateful-storage/src/signed_extension.rs
Codecov / codecov/patch
pallets/stateful-storage/src/signed_extension.rs#L119-L120
Check warning on line 129 in pallets/stateful-storage/src/signed_extension.rs
Codecov / codecov/patch
pallets/stateful-storage/src/signed_extension.rs#L128-L129
Check warning on line 170 in pallets/stateful-storage/src/signed_extension.rs
Codecov / codecov/patch
pallets/stateful-storage/src/signed_extension.rs#L170
Check warning on line 189 in pallets/stateful-storage/src/signed_extension.rs
Codecov / codecov/patch
pallets/stateful-storage/src/signed_extension.rs#L189
Check warning on line 197 in pallets/stateful-storage/src/signed_extension.rs
Codecov / codecov/patch
pallets/stateful-storage/src/signed_extension.rs#L197