Adds EnumerableMap.

[MicahZoltu]

I'm not sure what the best way to recommend a library to OZ contracts is, so I figured I would start with a PR and see what happens. This is code is strongly based on the OZ EnumerableSet. The biggest usability difference is that you have to instantiate manually and then call .initialize rather than doing EnumerableMap.newMap(). This is because struct arrays aren't supported in the ways necessary to make EnumerableMap.newMap() work.

The require statements are basically invariants to protect against bugs (and provide an early warning during testing) but could be removed as they should never be hit (really they are assertions with useful error messages).

I also changed the array to be 1-indexed, which differs from EnumerableSet which has a 0-indexed array and then treats the index values as all off-by-one. I personally found the code easier to understand with a placeholder value at the first entry position rather than having to shift the indexes by 1. Since the placeholder is just a 0 value, I suspect there is no meaningful gas difference between the strategies and it is just a matter of style.

The biggest problem here, of course (same as EnumerableSet) is that Solidity doesn't contain generics, so the user will need to change the key and value types to match their code. The nice thing here though is that the key can be any valid mapping key and the value can be anything including structs. The user only needs to change the type in a few places. I chose to use address and uint8 because you can do a find/replace as neither type is used outside the key/value types and it is as reasonable of a choice as any.

Fixes #

[MicahZoltu]

Compile failure is due to a change in Solidity 0.6.x (not sure when exactly). There is now a .push() for dynamic storage arrays which just adds an empty entry.

[nventuro]

Hello @MicahZoltu, thank you for this suggestion! Sorry I took so long to review this.

To clarify for people with less context, EnumerableMap expands on EnumerableSet by adding a value to the members of the set, that is, a traditional hash map with key enumeration.

While I think this would be a useful primitive to have (a true enumerable Solidity mapping), I'm worried about the lack of generics. A set of addresses made immediate sense for tracking accounts with special permissions, but I can't think of a similar generally useful key-value pair. And I'd rather not having people run find-replace on our code (nor copy-pasting it, for that matter), which can lead to all sort of errors. Perhaps its time to give ethereum/solidity#869 the love it needs?

I also changed the array to be 1-indexed, which differs from EnumerableSet which has a 0-indexed array and then treats the index values as all off-by-one. I personally found the code easier to understand with a placeholder value at the first entry position rather than having to shift the indexes by 1. Since the placeholder is just a 0 value, I suspect there is no meaningful gas difference between the strategies and it is just a matter of style.

While I agree that shifting indexes makes the code harder to read (because you need this extra context), I'd argue 1-indexing also has similar added complexity. There's one big difference between the two though: by shifting indexes we can remove the need for the initialize function (which is not required in EnumerableSet, see #2077).

This is significant enough to make me prefer the shifting approach, even if it makes the code slightly more obtuse.

[MicahZoltu]

I inquired with Solidity dev team about generics and while they are on the roadmap, they are fairly low priority, and a pretty big task, so I think we (dev community) should proceed for the time being under the assumption that they won't happen anytime soon.

Given that, the question then becomes is it better to have an implementation that requires a find/replace to change the type, or is it better to have nothing at all. While in some cases I would argue that nothing at all is better, in this case I think this is a widely enough desired data structure that I believe it may be worth having the less-than-ideal version rather than nothing. In my own projects, I have wanted it on several occassions (which is what finally caused me to generalize the solution and type this up). I would rather people in a similar situation not write their own, and instead re-use something that is well established as a "good pattern" (whatever that ends up looking like).

---

Your point about not requiring initialization is convincing enough to me to switch this over to 0-indexed.

[frangio]

Thanks a lot for the contribution!

The issue that we have is that so far we've highly discouraged copy-pasting code and making changes to it, with the argument that it's an error-prone procedure.

We added AddressSet because we felt that it's very generally useful, and we don't expect people to copy-paste it for different types. If there's other types that are commonly used we would be open to adding them to the library, I think.

For EnumerableMap it seems like the recommendation would be to always change it and that seems like a departure from our current recommendations so we have to consider it carefully. It's true, however, that that would be better than not having anything at all and people implementing it from scratch themselves.

[MicahZoltu]

Another option, which isn't great but maybe better than all of the other options, would be to create a folder full of EnumerableMap contracts, one for each of many types that people are likely to want. e.g., address => uint256, address => uint8, uint256 => uint256, etc. Try to limit the number of people who need to copy/paste the code by simply supplying a large set of things.

Yet another option would be to author a code-generator script that will create an EnumerableMap for you from an OZ template. This would be trivially easy in this case, though figuring out how to make it available to users in an easy-to-use + hard-to-break way could be a challenge.

[stale]

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!

[nventuro]

It looks like we may end up implementing a uint256 -> address enumerable map for the owners of ERC721 tokens, as part of the effort in unifying ERC extensions.

[stale]

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!

[MicahZoltu]

Up to @nventuro and @frangio. We can close this if it is decided that copy-paste is worse than nothing or not for this library.

[frangio]

I think we have to add the specific EnumerableMap types that show up as needed in the library or in user requests.

@nventuro Any news about the enumerable map for ERC721?