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

Add a method to delete all Map elements under a prefix #29

Closed
larry0x opened this issue Jan 28, 2023 · 4 comments
Closed

Add a method to delete all Map elements under a prefix #29

larry0x opened this issue Jan 28, 2023 · 4 comments
Milestone

Comments

@larry0x
Copy link
Contributor

larry0x commented Jan 28, 2023

Now we have Map::clear method which deletes all elements in the map, it'd be helpful to also have a Prefix::clear method to delete all elements under a specific prefix, for example:

// (first name, last name) -> Person
const PERSONS: Map<(&str, &str), Person> = Map::new("persons");

// delete all persons whose first name is larry
PERSONS.prefix("larry").clear(&mut storage);

The same can be done for the is_empty method:

// check whether there are persons whose first name is larry
PERSONS.prefix("larry").is_empty(storage);
@larry0x
Copy link
Contributor Author

larry0x commented Jan 28, 2023

@webmaster128 @ueco-jb I did a quick and dirty implementation here: main...steak-enjoyers:cw-storage-plus:prefix-clear

The idea is that clear and is_empty methods are moved into Prefix, while for Map, simply do:

impl Map {
    pub fn clear(&self, store: &mut dyn Storage) {
        self.no_prefix_raw().clear(store)
    }

    pub fn is_empty(&self, store: &dyn Storage) -> bool {
        self.no_prefix_raw().is_empty(store)
    }
}

I consider this implementation "dirty" because here:

impl Prefix {
    pub fn clear(&self, store: &mut dyn Storage) {
        const TAKE: usize = 10;
        let mut cleared = false;

        while !cleared {
            let paths = self
                .keys_raw(store, None, None, Order::Ascending)         // (1)
                .map(|raw_key| concat(&self.storage_prefix, &raw_key)) // (2)
                .take(TAKE)
                .collect::<Vec<_>>();

            paths.iter().for_each(|path| store.remove(path));

            cleared = paths.len() < TAKE;
        }
    }
}

On line (1), in keys_raw function, it trims off the prefix from the full path, while on line (2) we have to add the prefix right back, so this is a bunch of necessary computations.

Thoughts?

I suggest adding a Prefix::paths_raw method that return the full path without trimming off the prefix, then we can do:

impl Prefix {
    pub fn clear(&self, store: &mut dyn Storage) {
        const TAKE: usize = 10;
        let mut cleared = false;

        while !cleared {
            let paths = self
                .paths_raw(store, None, None, Order::Ascending)
                .take(TAKE)
                .collect::<Vec<_>>();

            paths.iter().for_each(|path| store.remove(path));

            cleared = paths.len() < TAKE;
        }
    }
}

@webmaster128
Copy link
Member

Sounds all good to me. I think one think we should introduce here is an optional limit for removals. Otherwise you can never support cases where the overall gas consumtion does not fit in a block. By keeping the limit optional, the existing Map.clear() can be supported by the same implementation in a compatible way.

@webmaster128 webmaster128 added this to the 1.1 milestone Jun 7, 2023
@webmaster128
Copy link
Member

@larry0x could you have a look at #45 and double check if this is what you are looking for?

@chipshort
Copy link
Collaborator

Done in #45. Will be released in 1.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants