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

Allow SovState use Borrow to behave similar to std::collections::HashMap #427

Open
citizen-stig opened this issue Jun 21, 2023 · 0 comments
Assignees
Labels
enhancement New feature or request Needs Grooming

Comments

@citizen-stig
Copy link
Member

Consider following rust code, which compares usage of HashMap and StateMap:

use std::collections::HashMap;
use sov_state::{DefaultStorageSpec, Prefix, ProverStorage, StateMap, WorkingSet};

pub fn hashmap() {
    let mut map: HashMap<Vec<u8>, u64> = HashMap::new();

    map.insert(vec![1, 2, 3], 1);
    map.insert(vec![4, 5, 6], 2);
    map.insert(vec![7, 8, 9], 3);

    println!("HASH MAP: {:?}", map);

    let check_this: [u8; 3] = [1, 2, 3];

    let v = map.get(&check_this[..]);
    assert!(v.is_some());
    assert_eq!(v, Some(&1));
}

pub fn sovstatemap() {
    let tmpdir = tempfile::tempdir().unwrap();
    let mut working_set = WorkingSet::new(ProverStorage::<DefaultStorageSpec>::with_path(tmpdir.path()).unwrap());


    let map: StateMap<Vec<u8>, u64> = StateMap::new(Prefix::new(vec![1, 2, 3]));

    let a = vec![1, 2, 3];
    let b = vec![4, 5, 6];
    let c = vec![7, 8, 9];
    map.set(&a, &1, &mut working_set);
    map.set(&b, &2, &mut working_set);
    map.set(&c, &3, &mut working_set);

    // Uncomment this to get compiler error
    // let check_this: [u8; 3] = [1, 2, 3];
    // let v = map.get(&check_this[..]);
    // assert!(v.is_some());

}

It would be nice to be able to use slice the same as with standard HashMap, it will remove some unnecessary clonings in default STF.

@citizen-stig citizen-stig added enhancement New feature or request Needs Grooming labels Jun 21, 2023
@vlopes11 vlopes11 self-assigned this Aug 10, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Needs Grooming
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants