Add more robust storage slot checking #1258
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 propertyastId, 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 ofastIds 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
normalizefunction 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-layoutsdirectories 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.