Skip to content

Conversation

@area
Copy link
Member

@area area commented May 27, 2024

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
Copy link
Member Author

area commented May 27, 2024

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).

@area area force-pushed the maint/storage-checks branch from 75db161 to d21f695 Compare May 28, 2024 05:52
@kronosapiens kronosapiens merged commit 608d766 into develop May 28, 2024
@kronosapiens kronosapiens deleted the maint/storage-checks branch May 28, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants