openzeppelin-contracts icon indicating copy to clipboard operation
openzeppelin-contracts copied to clipboard

EnumerableSet.clear()

Open olehmisar opened this issue 2 years ago • 5 comments

🧐 Motivation Use case: I need to store a bunch of approvals in a set and once the number of approvals reach a specific length, I need to execute an operation and clear all the approvals.

📝 Details

Right now, it is possible to clear the set with this code:

address[] memory values = approvals.values();
for (uint256 i = 0; i < values.length; i++) {
  approvals.remove(values[i]);
}

but it is rather inefficient because each call to .remove() does .pop() and multiple storage reads/writes.

The implementation of .clear() might look like this:

function clear(Set storage set) private {
  for (uint256 i = 0; i < set._values.length; i++) {
    set._indexes[set._values[i]] = 0;
  }
  // this line is dangerous as it might consume unbounded amount of gas
  set._values = new bytes32[](0); 
}

If set._values = new bytes32[](0); is too dangerous, another way to implement .clear() is to make another struct with .clear() method:

struct ClearableAddressSet {
  uint256 _setId;
  // setId => AddressSet
  mapping(uint256 => AddressSet) _sets;
}

// delegate all functions
function add(ClearableAddressSet storage set, address value) internal returns (bool) {
  return set._sets[set._setId].add(value);
}

function clear() internal {
  // by incrementing `_setId`, a new set is "created".
  set._setId++;
}

olehmisar avatar Mar 10 '22 09:03 olehmisar

Hello @olehmisar

If set._values = new bytes32[](0); is too dangerous, another way to implement .clear() is to make another struct with .clear() method

This is what we have been recommending.

As you showed, clearing a mapping is not trivial, and includes an unbounded loop. This means that the clear() method might become unusable, because of gas limitations, if the size of the set is too big. We fear that some users will not notice that, and shot themselves in the foot. Having a contract locked/unusable because a clear cannot be performed would be terrible.

The only way to ensure that you contract can get a fresh start, with a fresh set, regardless of the size of its predecessor, it unfortunately to use a new storage location.

Amxx avatar Mar 10 '22 22:03 Amxx

Yeah I think the best option is to use an array of EnumerableSet and just move on to the next one when you clear.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.12;

import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";

contract Example {
    using EnumerableSet for EnumerableSet.AddressSet;

    EnumerableSet.AddressSet[] private sets;
    uint setIndex;

    function _getCurrentSet() internal view returns (EnumerableSet.AddressSet storage) {
        return sets[setIndex];
    }

    function _clearSet() internal {
        setIndex += 1;
    }
}

I don't think it makes sense for us to provide this pattern out of the box.

frangio avatar Mar 22 '22 21:03 frangio

Seems not work for upgradeable contracts that never hit constructor. always panic code 032

envatic avatar Nov 08 '22 06:11 envatic

@envatic Try using a mapping instead.

nth-commit avatar Nov 12 '22 07:11 nth-commit

Agree with @envatic, using an array panics, though wasn't necessarily in the case of an upgradable contract. Panic exception is:

Error: VM Exception while processing transaction: reverted with panic code 0x32 (Array accessed at an out-of-bounds or negative index)

Using a mapping instead worked 👍

emizzle avatar Nov 15 '22 05:11 emizzle