search

JoinColony / colonyNetwork

Colony Network smart contracts
https://colony.io/
GNU General Public License v3.0
438 stars 106 forks source link

Add more robust storage slot checking #1258

Closed area closed 2 months ago

area commented 2 months ago

We introduced hardhat-storage-layout-changes in #1255, but the way I've set it up is very sensitive.

This is because a lot of the information recorded in the committed files is the astid. This the ID of the relevant node in the Abstract Syntax Tree which can (and does) change based on the particulars of the source code; in particular, it appears to be a function of the codebase as a whole (and then possibly a function of the order in which things are compiled).

This was unexpected enough it seemed at first glance to those unfamiliar with what was going on that it seemed to be broken, so I thought I'd circle back and make it behave a bit more intuitively.

This fundamentally means removing all astIds from the output that is used to detect changes. These appear in two places in the JSON Output from solidity used by the hardhat plugin. The first is the property astId, which is easy to traverse the JSON for and remove. More subtly, however, they also appear in the types of more complicated structures (mappings, arrays). By recursively replacing the type names with the type definitions (which are helpfully also provided in the JSON output), we can eliminate all occurrences of astIds and end up with a structure that should be sensitive to what we actually want.

The implementation of this is not going to winning me any prizes (in particular, repeatedly calling the normalize function until there are no more changes is not performant, but it's easy to write), but it does the job.

Strictly, we don't require both .storage-layouts directories here, but I opted to keep both. The first check in CI (which uses the plugin, and the original directory) will fail if storage slots are moved. The second check in CI (using my script and the normalized files) will fail if new variables have been added but not committed to the storage layout directories. It would also fail if storage slots had been moved, but I believe having the distinction between those two failure cases is worthwhile.

area commented 2 months ago

Build of #1240 based on this branch, as an example (which adds events, and changes the astids, which you can see from the first job, but the second job is still green).