-
Notifications
You must be signed in to change notification settings - Fork 240
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
Typed Storage Keys #1419
Typed Storage Keys #1419
Conversation
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] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* 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] <[email protected]> * Update Cargo.lock --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: James Wilson <[email protected]> Co-authored-by: Niklas Adolfsson <[email protected]>
* subxt/tx: Derive std::cmp traits Signed-off-by: Alexandru Vasile <[email protected]> * subxt/runtime_api: Derive std::cmp traits Signed-off-by: Alexandru Vasile <[email protected]> * subxt/constants: Derive std::cmp traits Signed-off-by: Alexandru Vasile <[email protected]> * subxt/custom_values: Derive std::cmp traits Signed-off-by: Alexandru Vasile <[email protected]> * subxt/storage: Derive std::cmp traits Signed-off-by: Alexandru Vasile <[email protected]> * subxt: Fix non_canonical_partial_ord_impl clippy introduced in 1.73 Signed-off-by: Alexandru Vasile <[email protected]> * subxt: Add comment wrt derivative issue that triggers clippy warning Signed-off-by: Alexandru Vasile <[email protected]> * Update subxt/src/backend/mod.rs * Update subxt/src/constants/constant_address.rs --------- Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: Niklas Adolfsson <[email protected]>
subxt/src/storage/storage_address.rs
Outdated
pub trait StorageMultiKey { | ||
/// Iterator over the storage keys, each key implements EncodeAsType to | ||
/// give the corresponding bytes to a `StorageHasher`. | ||
fn keys_iter(&self) -> impl ExactSizeIterator<Item = &dyn EncodeAsType>; |
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.
Ah ok, so while &dyn EncodeAsType
won't work any more when I make EncodeAsType
non-object-safe (ie generic over TypeResolvers), since Subxt will probably continue to use concrete types like PortableRegistry
instead of relying too much on TypeResolver
, it'll be easy enough to make a wrapper trait that is object safe in the same way I had to in scale-encode
:)
subxt/src/storage/storage_address.rs
Outdated
@@ -18,6 +20,8 @@ use subxt_metadata::{StorageEntryType, StorageHasher}; | |||
pub trait StorageAddress { | |||
/// The target type of the value that lives at this address. | |||
type Target: DecodeWithMetadata; | |||
/// The keys type used to construc this address. |
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.
s/construc
/construct
subxt/src/storage/storage_address.rs
Outdated
validation_hash: Option<[u8; 32]>, | ||
_marker: std::marker::PhantomData<(ReturnTy, Fetchable, Defaultable, Iterable)>, | ||
} | ||
|
||
/// A storage key, mostly used for static encoded values. | ||
/// The original value is only given during construction, but can be |
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.
dq: Unfinished doc
subxt/src/storage/storage_address.rs
Outdated
_hashers_and_ty_ids: &[(StorageHasher, u32)], | ||
_metadata: &Metadata, | ||
) -> Option<Result<Self, Error>> { | ||
if address_bytes.len() < 16 { |
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.
Nit: I would add a comment that for a valid storage key, we'd need at least 8 bytes for the pallet hash and 8 for the entry
subxt/src/error/mod.rs
Outdated
@@ -206,6 +206,9 @@ pub enum StorageAddressError { | |||
/// The number of fields in the metadata for this storage entry. | |||
fields: usize, | |||
}, | |||
/// The bytes of a storage address are not the expected address for decoding the storage keys of the address. | |||
#[error("Storage address bytes are not the expected format")] |
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.
I would add a small section about keys needing at least 16 bytes (pallet ++ entry)
Edit: ah this can also mean we are needing 8 bytes and so on (prob good to add a u8 here?)
subxt/src/storage/utils.rs
Outdated
} | ||
} | ||
|
||
/// Strips the first few bytes off a hash produced byt a concat hasher. |
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.
byt a concat
?
/// Strips the first 16 bytes (8 for the pallet hash, 8 for the entry hash) off some storage address bytes. | ||
fn strip_storage_addess_root_bytes(address_bytes: &mut &[u8]) -> Result<(), StorageAddressError> { | ||
if address_bytes.len() >= 16 { | ||
*address_bytes = &address_bytes[16..]; |
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.
this may end up with an empty slice if address_bytes.len == 16 ok?
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.
Yes, that is true, but it is okay I think.
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.
Yeah I think that's ok; if it matters (ie we expect a non-empty hash next) then the next thing will error instead
} | ||
|
||
/// Returns true if the key used to produce the hash is appended to the hash itself. | ||
pub fn ends_with_key(&self) -> bool { |
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.
I find this is a bit hard to hard to read, maybe we could do contains_key
, contains_plaintext
or is_concat_with_key
... not sure
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.
I chanegd it from contains_key
or something I think! I went with ends_
just to fit in more nicely with the above method which is giving the length of the hash apart from the key bit.
contains_key
would work too though for me!
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.
Either looks good to me :D
subxt/src/storage/storage_key.rs
Outdated
pub fn next_or_err(&mut self) -> Result<(StorageHasher, u32), Error> { | ||
self.next().ok_or_else(|| { | ||
StorageAddressError::TooManyKeys { | ||
expected: self.hashers_and_ty_ids.len(), | ||
} | ||
.into() | ||
}) | ||
} |
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.
Should this be a part of the public API? Maybe it could just be a small internal helper fn instead :)
subxt/src/storage/storage_key.rs
Outdated
pub fn reset(&mut self) { | ||
self.idx = 0; | ||
} |
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.
Nit: reset()
type functions on iterators feel a bit unidiomatic to me (and mutating an iterator scares me a little). Not really a big deal but we could be more idiomatic perhaps by just making the iteration be a separate thing like so:
// rename `StorageHAshersIter` to this but keep methods the same:
pub struct StorageHashers {
// same methods..
pub fn iter(&'_ self) -> StorageHashersIter<'_> {
StorageHashersIter { hashers: self, idx: 0 }
}
}
// impl `Iterator` on a separate type instead (then we can iterate over them
// as many times as we want and no iterators are being mutated. Maybe a bit like:
pub struct StorageHashersIter<'a> {
idx: usize,
hashers: &'a StorageHashers
}
impl <'a> Iterator for StorageHashersIter<'a> {
fn next(&mut self) {
let item = self.hashers.hashers_and_ty_ids.get(self.idx).copied()?;
self.idx += 1;
Some(item)
}
}
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.
Maybe this is a bit of a pain though, and if so I'm happy to stick with the current approach!
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.
Oh yeah, the reset
is a bit ugly, I implemented a seperate StorageHashersIter<'a>
struct now.
This looks great to me now; just a couple of very small suggestions! Great job @tadeohepperle! |
subxt/src/storage/storage_key.rs
Outdated
@@ -182,6 +202,7 @@ impl<K: ?Sized> StorageKey for StaticStorageKey<K> { | |||
hashers: &mut StorageHashersIter, | |||
types: &PortableRegistry, | |||
) -> Result<(), Error> { | |||
println!("{:?}", hashers); |
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.
Intentional? :)
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.
One println!()
that I think is unintended but otherwise looks awesome :)
for h0 in hashers { | ||
for h1 in key_preserving_hashers { | ||
for h2 in key_preserving_hashers { | ||
for h3 in key_preserving_hashers { |
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.
Nice! 👍
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.
Nice job here 👍
Fixes #1201
Background Info
Previously, static storage keys were represented as a
Static<Encoded>
(wrapper around aVec<u8>
) that is created from a concrete typeT: Encode
by scale encoding it.Previously the
storage::Address
struct was generic over the type of key:K
, and held aVec<K>
as a value. ThisK
wasStatic<Encoded>
for static storage keys andscale_value::Value
for dynamic storage keys.What changed
For static storage keys, used in the codegen, we add a new type
StorageKey<K>
that is just a value of K scale encoded into bytes, but keeps the type information of K in aPhantomData
field.A new trait
StorageMultiKey
is introduced, that can be implemented by anything that represents one or multiple storage keys:This trait is then implemented for:
StorageKey<K>
,(StorageKey<A>, StorageKey<B>, ...)
Vec<scale_value::Value>
The
Address
struct representing a storage address is now generic over aKeys: StorageMultiKey
type, instead of holding a Vec.This allows us to construct storage addresses that use tuples of storage keys
(StorageKey<A>, StorageKey<B>, ...)
in the generated code.Also when getting
StorageResponse
's back from the Backend, we can now attempt to reconstruct the respectiveStorageMultiKey
from the address bytes.