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.

Sign up for GitHub

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

Jump to bottom

Suggestions for storage value decoding #1457

Merged
merged 3 commits into from
Feb 28, 2024
Merged

Suggestions for storage value decoding #1457

merged 3 commits into from
Feb 28, 2024

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Feb 28, 2024 •
edited
Loading

This PR was quite complicated, so here I just offer suggestions for things to improve instead of tryign to write comments for it all!

One thing that is missing I think is some tests in storage_key.rs to check that we can form various tuple keys etc and they will correctly decode some bytes. I spotted one or two things I thought might fail, so would be great to have some test coverage here :)

@jsdw jsdw changed the base branch from master to tadeohepperle/decoding-storage-keys February 28, 2024 13:16
jsdw
jsdw commented Feb 28, 2024
View reviewed changes
metadata/src/lib.rs
/// The hash produced by a [`StorageHasher`] has 1 or 2 of the following components:
/// 1. A fixed size hash. (not present for [`StorageHasher::Identity`])
/// 2. The key that was used as an input to the hasher. (only present for
/// The hash produced by a [`StorageHasher`] can have these two components, in order:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since the functions are a part of our public API here, I tried to give them slightly nicer names

jsdw
jsdw commented Feb 28, 2024
View reviewed changes
subxt/src/error/mod.rs
@@ -211,12 +211,10 @@ pub enum StorageAddressError {
#[error("Storage address bytes are not the expected format. Addresses need to be at least 16 bytes (pallet ++ entry) and follow a structure given by the hashers defined in the metadata.")]
UnexpectedAddressBytes,
/// An invalid hasher was used to reconstruct a value from a chunk of bytes that is part of a storage address. Hashers where the hash does not contain the original value are invalid for this purpose.
#[error("An invalid hasher was used to reconstruct a value of type {ty_name} (id={ty_id}) from a hash formed by a {hasher:?} hasher. This is only possible for concat-style hashers or the identity hasher")]
#[error("An invalid hasher was used to reconstruct a value with type ID {ty_id} from a hash formed by a {hasher:?} hasher. This is only possible for concat-style hashers or the identity hasher")]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the type_name, really jsut to simplify the code a touch since in most other places we only return the type ID anyway for this sort of error.

jsdw
jsdw commented Feb 28, 2024
View reviewed changes
subxt/src/storage/storage_key.rs
fn decode_from_bytes(
cursor: &mut &[u8],
bytes: &mut &[u8],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

both this and the below are "cursors", so I renamed this one

jsdw
jsdw commented Feb 28, 2024
View reviewed changes
subxt/src/storage/storage_key.rs
return Err(StorageAddressError::WrongNumberOfHashers {
hashers: 0,
fields: 1,
// If no hashers, we just do nothing (erroring if ).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When there is a hasher, this also needs to skip over any key bytes if they exist (which is what consume_hash_returning_key_bytes does.

When there is not a hasher, we just exit happily unless there are leftover bytes, which would be unexpected.

jsdw
jsdw commented Feb 28, 2024
View reviewed changes
subxt/src/storage/storage_key.rs
scale_decode::visitor::IgnoreVisitor,
) {
return Err(scale_decode::Error::from(err).into());
// Advance the bytes cursor, returning any key bytes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With the consume_hash_returning_bytes function I thought it would be cleaner then to inline the other code for this impl

jsdw
jsdw commented Feb 28, 2024
View reviewed changes
subxt/src/storage/storage_key.rs

pub fn decode_storage_key_from_hash<K: ?Sized>(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed this specific-to-StaticStorageKey function and made a slightly more general consume_hash_returning_bytes one that could then be used in a couple of other places too, to cut down the number of util functions about

jsdw
jsdw commented Feb 28, 2024
View reviewed changes
subxt/src/storage/storage_key.rs
let value = decoded.to_value()?;
result.push(value.remove_context());
n += 1;
for (hasher, ty_id) in hashers_and_ty_ids.iter() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had a look and DecodedValueThunk::decode_with_metadata looks like it consumes all input bytes (not sure why tbh!), so the value decoding would have failed when more than one hasher I think.

I just decoded directly into a Value and figured it may as well go into a for loop where we check for remaining bytes at the end.

jsdw
jsdw commented Feb 28, 2024
View reviewed changes
subxt/src/storage/storage_key.rs
Ok(result)
}
}

// Skip over the hash bytes (including any key at the end), returning bytes
// representing the key if one exists, or None if the hasher has no key appended.
fn consume_hash_returning_key_bytes<'a>(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This just captures a thing we want to do in a few places, which is to skip over any bytes a StorageHasher represents, and then if there are bytes for the key, then return them.

Using this means you don't have to worry about consuming the right number of bytes anywhere else.

jsdw
jsdw commented Feb 28, 2024
View reviewed changes
subxt/src/storage/storage_type.rs
@@ -311,7 +310,8 @@ where
}
}

pub(crate) fn storage_hasher_type_id_pairs(
/// Return a vec of hashers and the associated type IDs for the keys that are hashed.
fn storage_hasher_type_id_pairs(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

making it not pub didn't seem to break anything offhand :D

jsdw
jsdw commented Feb 28, 2024
View reviewed changes
subxt/src/storage/utils.rs

/// Return the root of a given [`StorageAddress`]: hash the pallet name and entry name
/// and append those bytes to the output.
pub(crate) fn write_storage_address_root_bytes<Address: StorageAddress>(
pub fn write_storage_address_root_bytes<Address: StorageAddress>(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

utils wasn't exporting anything before, so now the module is not exported at all so these fns don't need to be pub(crate) to hide themselves any more.

I moved the added fns that were only used in one place into the place they were used.

tadeohepperle
tadeohepperle approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@tadeohepperle tadeohepperle left a comment

Choose a reason for hiding this comment

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

Looks all good to me!

@tadeohepperle tadeohepperle marked this pull request as ready for review February 28, 2024 14:24
@tadeohepperle tadeohepperle requested a review from a team as a code owner February 28, 2024 14:24
@tadeohepperle tadeohepperle merged commit bd01aab into tadeohepperle/decoding-storage-keys Feb 28, 2024
1 check passed
@tadeohepperle tadeohepperle deleted the jsdw-decoding-storage-values branch February 28, 2024 14:35
tadeohepperle added a commit that referenced this pull request Mar 6, 2024
@tadeohepperle @dependabot
@jsdw @niklasad1 @lexnv
Typed Storage Keys (#1419)
a2ee750 
* first iteration on storage multi keys

* decoding values from concat style hashers

* move util functions and remove comments

* change codegen for storage keys and fix examples

* trait bounds don't match scale value...

* fix trait bounds and examples

* reconstruct storage keys in iterations

* build(deps): bump js-sys from 0.3.67 to 0.3.68 (#1428)

Bumps [js-sys](https://github.com/rustwasm/wasm-bindgen) from 0.3.67 to 0.3.68.
- [Release notes](https://github.com/rustwasm/wasm-bindgen/releases)
- [Changelog](https://github.com/rustwasm/wasm-bindgen/blob/main/CHANGELOG.md)
- [Commits](https://github.com/rustwasm/wasm-bindgen/commits)

---
updated-dependencies:
- dependency-name: js-sys
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): bump clap from 4.4.18 to 4.5.0 (#1427)

Bumps [clap](https://github.com/clap-rs/clap) from 4.4.18 to 4.5.0.
- [Release notes](https://github.com/clap-rs/clap/releases)
- [Changelog](https://github.com/clap-rs/clap/blob/master/CHANGELOG.md)
- [Commits](clap-rs/clap@v4.4.18...clap_complete-v4.5.0)

---
updated-dependencies:
- dependency-name: clap
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): bump either from 1.9.0 to 1.10.0 (#1425)

Bumps [either](https://github.com/rayon-rs/either) from 1.9.0 to 1.10.0.
- [Commits](rayon-rs/either@1.9.0...1.10.0)

---
updated-dependencies:
- dependency-name: either
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): bump thiserror from 1.0.56 to 1.0.57 (#1424)

Bumps [thiserror](https://github.com/dtolnay/thiserror) from 1.0.56 to 1.0.57.
- [Release notes](https://github.com/dtolnay/thiserror/releases)
- [Commits](dtolnay/thiserror@1.0.56...1.0.57)

---
updated-dependencies:
- dependency-name: thiserror
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): bump jsonrpsee from 0.21.0 to 0.22.0 (#1426)

* build(deps): bump jsonrpsee from 0.21.0 to 0.22.0

Bumps [jsonrpsee](https://github.com/paritytech/jsonrpsee) from 0.21.0 to 0.22.0.
- [Release notes](https://github.com/paritytech/jsonrpsee/releases)
- [Changelog](https://github.com/paritytech/jsonrpsee/blob/master/CHANGELOG.md)
- [Commits](paritytech/jsonrpsee@v0.21.0...v0.22.0)

---
updated-dependencies:
- dependency-name: jsonrpsee
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Update Cargo.lock

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: James Wilson <james@jsdw.me>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

* subxt: Derive `std::cmp` traits for subxt payloads and addresses (#1429)

* subxt/tx: Derive std::cmp traits

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt/runtime_api: Derive std::cmp traits

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt/constants: Derive std::cmp traits

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt/custom_values: Derive std::cmp traits

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt/storage: Derive std::cmp traits

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt: Fix non_canonical_partial_ord_impl clippy introduced in 1.73

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* subxt: Add comment wrt derivative issue that triggers clippy warning

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Update subxt/src/backend/mod.rs

* Update subxt/src/constants/constant_address.rs

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

* fix clippy

* add integration tests

* fix doc tests

* change hashing logic for hashers=1

* refactor

* clippy and fmt

* regenerate polkadot file which got changed by the automatic PR

* nested design for storage keys

* refactor codegen

* codegen adjustments

* fix storage hasher codegen test

* Suggestions for storage value decoding (#1457)

* Storage decode tweaks

* doc tweak

* more precise error when leftover or not enough bytes

* integrate nits from PR

* add fuzztest for storage keys, fix decoding bug

* clippy and fmt

* clippy

* Niklas Suggestions

* lifetime issues and iterator impls

* fmt and clippy

* regenerate polkadot.rs

* fix storage key encoding for empty keys

* rename trait methods for storage keys

* fix hasher bug...

* impl nits, add iterator struct seperate from `StorageHashers`

* clippy fix

* remove println

---------

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: James Wilson <james@jsdw.me>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tadeohepperle tadeohepperle tadeohepperle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jsdw @tadeohepperle