Skip to content
This repository has been archived by the owner on Aug 20, 2021. It is now read-only.

lib/build.js: rm special formatting for storage blocks #397

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

d-xo
Copy link
Contributor

@d-xo d-xo commented Apr 10, 2020

Removes all special formatting for storage blocks in the reports.

This was interacting badly with the multiline storage conditions that are commonly used in k-uniswap. It also fixes the issue where storage blocks are indented differently to the rest of the spec. An example of both issues can be seen here

Readable formatting for storage blocks is now the responsibility of the spec author.

@d-xo d-xo requested a review from a team April 10, 2020 12:37
Removes all special formatting for storage blocks. This was interacting
badly with the multiline storage conditions that are commonly used in
k-uniswap.

Readable formatting for storage blocks is now the responsibility of the
spec author.
@d-xo d-xo force-pushed the rm-storage-formatting branch from e342791 to 45d45c3 Compare April 10, 2020 12:39
@asymmetric
Copy link
Contributor

asymmetric commented Apr 10, 2020

Do you have a report produced by this branch?

Does the code you removed only affect how proofs are displayed in the reports?

@d-xo
Copy link
Contributor Author

d-xo commented Apr 11, 2020

Do you have a report produced by this branch?

https://ipfs.io/ipfs/QmeZzK4VjXKd95Ab2UvD67SE6dWFuiL1qCUJxbaAwtgd8Q/

Does the code you removed only affect how proofs are displayed in the reports?

Very good question, and something I had not fully considered. The code I removed is called as part of klab build, but afaict it just attaches an html representation of the spec to each object in the parsed_tokens array here. I diffed a spec produced by klab build before and after this change, and the only change was the module hash (see here).

@mhhf is this safe to remove?

@d-xo
Copy link
Contributor Author

d-xo commented Apr 15, 2020

This breaks storage comments (see e.g. here for an example).

More work needed before merging.

@d-xo
Copy link
Contributor Author

d-xo commented Apr 15, 2020

Does the code you removed only affect how proofs are displayed in the reports?

Confirmed with @mhhf that this was the case out of band.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants