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 'paginated' getters for EnumerableSet and Map #2390

Open
nventuro opened this issue Oct 19, 2020 · 5 comments
Open

Add 'paginated' getters for EnumerableSet and Map #2390

nventuro opened this issue Oct 19, 2020 · 5 comments
Labels
feature New contracts, functions, or helpers.

Comments

@nventuro
Copy link
Contributor

nventuro commented Oct 19, 2020

The current API only provides access via .length() and .at(). It'd be great to have a .list(start, end) that returned a partial view of the set's elements.

The reasoning behind making this a non-complete view is to let the caller decide how much gas they want to spend. Calls to view functions from off-chain sources (i.e. eth_call) are also capped in gas, and a large enough set can cause them to fail.

The implementation should be quite simple:

function list(
    EnumerableSet.AddressSet storage set,
    uint256 start,
    uint256 end
) internal view returns (address[] memory) {
    address[] memory elements = new address[](end - start);

    for (uint256 i = 0; i < elements.length; ++i) {
        elements[i] = set.at(i + start);
    }

    return elements;
}
@abcoathup
Copy link
Contributor

Hi @nventuro ! Thanks for the suggestion, it is really appreciated.

The project owner will review your suggestion as soon as they can.

Please wait until we have discussed this idea before writing any code or submitting a Pull Request, so we can go through the design beforehand. We don’t want you to waste your time!

@abcoathup abcoathup added the feature New contracts, functions, or helpers. label Oct 20, 2020
@ducquangkstn
Copy link

I would like to have some kind of listAll function

function listAll(
    EnumerableSet.AddressSet storage set
) internal view returns (address[] memory);

@Amxx
Copy link
Collaborator

Amxx commented Jul 13, 2021

Hello @ducquangkstn

This was already requested many times, but the issue remains the same. AddressSet, and all the other sets, use a bytes32[] based underlying storage. This means that the values are stored as a bytes32[]. Unfortunatelly, it is not possible to cast a bytes32[] to a address[] easily. This casting would require creating a new array, and casting all the elements one by one.

This would very gas expensive to do it onchain, and it feels like gettings elements on request using the .at() function is a better option, with no hidden costs. If you really need the array of values, you can access the bytes32[] version, but you'll have to process it accordingly.

@nventuro
Copy link
Contributor Author

Unfortunatelly, it is not possible to cast a bytes32[] to a address[] easily

Actually I think this is not that difficult using inline assembly:

pragma solidity ^0.8.0;

contract A {
    bytes32[] private _foo;
    
    function foo() internal view returns (bytes32[] storage) {
        return _foo;
    }
    
    function fooAsAddress() internal view returns (address[] storage) {
        this; // Silence state mutability warning, as this should be view
        
        address[] storage fooAddress;
        
        
        assembly {
            fooAddress.slot := _foo.slot
        }
        
        return fooAddress;
    }
}    

From the docs, a storage array should always have an offset of 0, so we don't need to assign that. I did a quick test on remix and it seems to work.

@frangio
Copy link
Contributor

frangio commented Sep 30, 2022

Bumping this. Contracts with EnumerableSet/Map variables that want to expose that data through getters currently have two options:

  1. Use values()
  2. Use at(i) and length()

Option 1 is not recommended because values() returns an array of unbounded length. However, option 2 has some downsides:

  • It potentially requires more boilerplate if you want to know the length prior to getting all indices. Alternatively, you can just use at(i) until you get a revert from being past the end of the array, but for this to be reliable we probably should add a custom error for out of bounds (and it's not a nice solution).
  • It's inefficient because it requires many more function calls.

Note that I'm mainly thinking about off-chain purposes but the same downsides apply on-chain.

A paginated getter would be the best of both worlds. You have to expose just one function, you can bound the array to whatever length you want, and you can just keep going until the array you get has shorter length than the page size that was requested. And you can safely and efficiently use it on-chain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New contracts, functions, or helpers.
Projects
None yet
Development

No branches or pull requests

5 participants