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.

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

Unify StorageKey and CacheKey #643

Closed
vlopes11 opened this issue Aug 10, 2023 · 2 comments
Closed

Unify StorageKey and CacheKey #643

vlopes11 opened this issue Aug 10, 2023 · 2 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers Small Use this label for quick cleanup and maintenance tasks

Comments

@vlopes11
Copy link
Contributor

Description

Currently, StorageKey and CacheKey are equivalent types. They historically were different, but as the implementation evolved, they converged to have the same. However, StorageKey implements serialization, whereas CacheKey doesn't.

This limits our implementation as the conversion between the types may force us to take the key as owned for simple operations such as get, when a reference should suffice

let key = key.as_cache_key();

Solution

Deprecate CacheKey and replace its usage by StorageKey. This will allow us to pass references downstream and make the map API more powerful, as the types are just borrowed references and can be passed downstream without any conversion. This will unlock a simplification requested on #427

Alternatives

We could unsafely cast one type to the other, but we have no clear gain to maintain the distinction and would create a potential undefined behavior if the layout of any of the types ever change. This would create a maintenance burden to cross-check both types whenever they are updated.

A working alternative suggested by @preston-evans98 is to use yoke to cast one type to the other. This will allow a zero-cost solution and will preserve both types, in case we want to maintain the distinction. The CacheKey is currently used for memory-only representation, and StorageKey is used for serialization. It might be desirable to maintain this domain separation if we want to have a layer that don't interact with I/O.

vlopes11 added a commit that referenced this issue Aug 10, 2023
This commit updates the API of `StateMap` to behave as common map
implementations in Rust: the argument of a get method will receive a
value that can be borrowed from the concrete key type.

This is useful for recurrent situations such as when we want to perform
a get from a map of `Vec<u8>` without cloning the key index, by using a
slice.

The focus of the PR is to touch the public API of the map and trait,
without changing too much the internals. A new issue is introduced to
propose an upgrade: #643

It also updates the public interface of the storage to take references
by default, instead of owned values. It will allow the user to perform
storage queries without necessarily cloning the values.

Resolves #427
@vlopes11 vlopes11 added Small Use this label for quick cleanup and maintenance tasks enhancement New feature or request good first issue Good for newcomers labels Aug 14, 2023
@bkolad
Copy link
Member

bkolad commented Oct 13, 2023

We could even have a single type StoargeSlot for keys and values.

@citizen-stig
Copy link
Member

As discussed on today's daily:

  • Keep Key and Value types for semantic
  • Rename CacheKey and CacheValue to SlotKey and SlotValue
  • Remove StorageKey and StorageValue in favour of SlotKey(previously CacheKey) and SlotValue (previously CacheValue)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers Small Use this label for quick cleanup and maintenance tasks
Projects
None yet
Development

No branches or pull requests

4 participants