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

Set types #2075

Closed
wants to merge 3 commits into from
Closed

Set types #2075

wants to merge 3 commits into from

Conversation

rob-Hitchens
Copy link

Fixes #2061
Implementation of an address Enumerable Set

There are use-cases for enumerable sets of most scalar types. address is appropriate user-oriented cases such as memberships, followers, privileges, etc. bytes32 is appropriate for any sort of contract-generated UID such as a hash generated on the fly. While almost anything can be converted to a bytes32 for a uniqueness test, in practice I found the caller code is ugly (e.g. toBytes32(whatever)) and it's usually best to tweak the basic idea to accommodate keys that are natural from the caller's perspective. I added a string variant to show how a hash can be used internally to make it work.

Implementations are libraries that create Set types. Operations are O(1). Enumeration is done by the caller, one row at a time as I'm not comfortable with returning lists of unbounded size in one move. (count() and keyAtIndex(uint row)). In my opinion, calling for the entire list is wasteful and risky in most use-cases.

E.g.

Get started:

import "./AddressSet.sol";
using AddressSet for AddressSet.Set;
AddressSet.set addressSet;

Typical usage:

Insert
addressSet.insert(msg.sender);

Remove
addressSet.remove(msg.sender);

Check
if(addressSet.exists(msg.sender)) { ... }

Enumerate (Count)
uint count = addressSet.count();

Enumerate (Fetch)
address user = addressSet.keyAtIndex(row);

I realize there is a merged candidate implementation, so this is a little late to the party. I will go ahead and create unit tests and address other feedback if there is interest in this approach. I'm not sure where you would want to put it but a little folder of SetTypes (4 given) seemed intuitive to me.

@Amxx
Copy link
Collaborator

Amxx commented Feb 4, 2020

FYI, I've been working on something similar, expec my version used a templated file to generate sets/maps with arbitrary types. This reduces code duplication.
It's available here: https://github.com/Amxx/SolStruct

@nventuro
Copy link
Contributor

nventuro commented Feb 4, 2020

Hello @rob-Hitchens and @Amxx, thank you for contributing!

The proposed contracts have the same API and implementation as our new EnumerableSet (from a cursory overview), so I'll treat this instead as a request for more types to be added to EnumerableSet.

I'd like to wait a little bit before adding full-fledged support for all primitive types: we've only just included EnumerableSet, and we may want to review parts of it soon. I'm also not sure if primitive types would be enough: many interesting use cases require storing complex structs.

That said, your comment about bytes32 is very interesting: it'd let you easily store any custom struct without having to re-write EnumerableSet to support it. The issue I see with that approach however is that enumerating said hashes would not be useful unless you had the data required to compute them: you wouldn't be able to know the actual contents of the set. Since enumeration is the only difference between EnumerableSet and the native mapping, I fear this would make the whole thing futile.

@rob-Hitchens
Copy link
Author

Hi @nventuro,

Thanks for that. I'll just clarify a bit of my original post you mention here:

you wouldn't be able to know the actual contents of the set.

The idea is to make it completely discoverable. The contract might generate keys somehow, but a client needs to bootstrap from nothing and retrieve everything. Here's a typical usage:

struct Widget {
  ...
}

mapping(bytes32 => Widget) widgets;
Bytes32Set.Set widgetIdSet;

It's handy if the contract is generating keys with a hash function. Generally, if that happens, then it's a good idea for the contract to emit NewWidget(key) (as part of something more comprehensive). So, if that pattern is followed, then clients have one way to know the keys.

I usually incline to completely discoverable states, so it would be typical to include:

function widgetCount() public view returns (uint) {
  return widgetSet.count();
}

function widgetIdAtIndex(uint index) public view returns(bytes32) {
  return widgetSet.keyAtIndex(index);
}

That habit gives a client a second way to unfold the keys. That means the structs are discoverable without knowing their contents.

In any case, devs struggle with sets because a CRUD pattern isn't easy to work out. Something reliable helps keep the focus on what the contract is supposed to do instead of problematic implementation details. One can go nuts with it, like in this layout for a graph:

library GraphLib {
    
    using Bytes32Set for Bytes32Set.Set;
    
    struct EdgeStruct {
        bytes32 source;
        bytes32 target;
        uint weight;
    }
    
    struct NodeStruct {
        Bytes32Set.Set sourceEdgeSet; // in
        BYtes32Set.Set targetEdgeSet; // out
    }
    
    struct Graph {
        Bytes32Set.Set nodeSet;
        Bytes32Set.Set edgeSet;
        mapping(bytes32 => NodeStruct) nodeStructs;
        mapping(bytes32 => EdgeStruct) edgeStructs;
    }

@stale
Copy link

stale bot commented Feb 20, 2020

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

@stale stale bot added the stale label Feb 20, 2020
@nventuro
Copy link
Contributor

nventuro commented Mar 4, 2020

Hm, yes, I can see how that could work, but that'd be straying a bit too far away from the original EnumerableSet :)

I still think that we could add more types to it, but for the moment I'd rather not pollute the library with too many of these yet, and would rather start out with just the ones we're using internally.

@stale stale bot removed the stale label Mar 4, 2020
@stale
Copy link

stale bot commented Mar 19, 2020

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

@stale stale bot added the stale label Mar 19, 2020
@frangio
Copy link
Contributor

frangio commented Mar 20, 2020

Closing for the reasons explained by @nventuro above.

@frangio frangio closed this Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants