Make UnstructuredStorage a library

[sohkai]

Also moves the pinnedCode implementation details to AppProxyPinned as it's only relevant there, and unstructured storage is meant to hide these types of abstractions :).

[coveralls]

Coverage remained the same at 98.678% when pulling 0831b9d on unstructured_storage_library into c992f0a on unstructured_storage.

[bingen]

This looks good to me, just two comments:

• What's the advantage of using a Library versus inheriting a contract? Is it to save gas? How are we currently deploying our contracts regarding libs (like SafeMath)? Are we reusing them? Shouldn't we document library addresses we are using here Move AragonApp to unstructured storage #376 (comment)? What about upgradeability? In case we need it, are we supposed to deploy a new library version and link new contracts using the new address?
• See my comment here: Move AragonApp to unstructured storage #376 (comment)

[sohkai]

So actually this is buried in the solidity docs, but internal functions in a library are copied just the same as base class functions. In this case, UnstructuredStorage being a library (with only internal functions) has two stylistic advantages:

• No need to add it to the inheritance graph (:heart:)
• Allows you to access them as if they were part of the type, e.g. we can mark bytes32 as a storage position.

[bingen]

What about the bytecode size? Did you check it? Is it happening the same as in #382 ? In that case, would we wait to 0.4.24 too? (#382 (comment))

[izqui]

LGTM. Loving how it simplifies the inheritance graph and being able to move pinnedCode out of AppStorage 💥

[sohkai]

Admittedly this was before #382 so I hadn't thought of checking the bytecode size... can do some tests soon.

[bingen]

I'm fine with the changes, but I guess not that excited, haha.
I agree about how better is to put pinnedCode stuff in AppProxyPinned, but we can do that without converting to a library.
About simplifying inheritance graph, I see it more like "hiding" complexity than actually simplifying anything. When I look at that graph, I like to realize that AppStorage is UnstructuredStorage. Besides, we are only removing 2/3 dependencies in a huge graph.
Anyway this is highly subjective, so, again, I'm fine with the changes. But I definitely wouldn't do it if it comes with an objective loss like increasing bytecode size.

[sohkai]

For the library change (f322c87) against branch target:

                              CODE DEPOSIT COST    DEPLOYED BYTES     INITIALIZATION BYTES
ACL.json                      47400 less gas       -237               0
APMRegistry.json              132600 less gas      -663               0
AppProxyFactory.json          82800 less gas       -414               0
AppProxyPinned.json           44400 less gas       -222               +29
AppProxyUpgradeable.json      47800 less gas       -239               +18
AppStorage.json               53000 less gas       -265               0
AragonApp.json                50000 less gas       -250               0
ENSSubdomainRegistrar.json    47400 less gas       -237               0
EVMScriptRegistry.json        47400 less gas       -237               0
EVMScriptRegistryFactory.json 82800 less gas       -414               -237
EVMScriptRunner.json          53000 less gas       -265               0
Initializable.json            57400 less gas       -287               -1
Kernel.json                   138600 less gas      -693               0
Repo.json                     50000 less gas       -250               0
UnstructuredStorage.json      69000 less gas       -345               -3

Awkwardly, this seems to be a gas cost decrease for some reason that #382 was not.

Against current dev HEAD:

                              CODE DEPOSIT COST    DEPLOYED BYTES     INITIALIZATION BYTES
ACL.json                      19200 more gas       +96                0
APMRegistry.json              235200 more gas      +1176              0
AppProxyFactory.json          225000 more gas      +1125              0
AppProxyPinned.json           57600 more gas       +288               +375
AppProxyUpgradeable.json      40800 more gas       +204               +258
AppStorage.json               19600 more gas       +98                0
AragonApp.json                11800 more gas       +59                0
ENSSubdomainRegistrar.json    23400 more gas       +117               0
EVMScriptRegistry.json        25200 more gas       +126               0
EVMScriptRegistryFactory.json 225000 more gas      +1125              +126
EVMScriptRunner.json          12600 more gas       +63                0
Initializable.json            30000 less gas       -150               -1
Kernel.json                   209800 more gas      +1049              0
Repo.json                     11600 more gas       +58                0

Which is much better than #376 initially.

The pinnedCode change has no length diff.

[sohkai]

Given that the bytecode increment seems to be halved by making UnstructuredStorage a library, I'd say there's objective improvement with this change :).

I also don't see this as "hiding" any complexity, since we're still declaring that bytes32 can be unstructured storage positions (using UnstructoredStorage for bytes32). I prefer libraries for these kinds of contracts because inheriting in solidity can be misleading due to the C3 linearization, whereas libraries are plain and obvious. In that aspect, I think complexity is greatly reduced when the inheritance list is smaller.

I've also opted to keep 91b4431 (removing .call()) as this might've been something with a contract or mock not declaring the getters as view, but we can revisit when we update the integration tests.

[bingen]

Wow! AragonApp from 61800 to 11800!! This is definitely a huge objective improvement! End of discussion ;-)
(Although I still kind of disagree about the complexity part)