Generate transient storage layout #15247

matheusaaguiar · 2024-07-08T05:19:38Z

No description provided.

libsolidity/interface/StorageLayout.cpp

matheusaaguiar · 2024-07-09T22:01:06Z

libsolidity/ast/Types.cpp

+	VariableDeclaration::Location location = [&]() {
+		switch (_location)
+		{
+			case DataLocation::Storage: return VariableDeclaration::Location::Unspecified; // By default state variables have unspecified (storage) data location


I think this should be refactored in a separate PR so variables at storage are explicitly assigned VariableDeclaration::Location::Storage (note this is a different enum from DataLocation which is in Types.h).

Why do we even need two enums for this? It makes it significantly more annoying in the first place. Adding an Unspecified option to the DataLocation enum would be enough; plus it's not a class member enum so it can be aliased as well (unlike VariableDeclaration::Location). But yeah, a problem for another time.

Why do we even need two enums for this?

DataLocation specifies where the data is stored while VariableDeclaration::Location indicates which modifier is on the variable.

I think this should be refactored in a separate PR so variables at storage are explicitly assigned VariableDeclaration::Location::Storage

Unspecified does not necessarily mean Storage. That's the case only for state variables. And even then it may also represent a constant or an immutable.

I think changing the way it works would just make things confusing in a different way. Then we'd be asking why it doesn't reflect what's on the variable :) The current usage does have some logic behind it. This is just another case of things being too vaguely named, omitting important context. Maybe something like DeclaredLocation or LocationModifier would be clearer.

Adding an Unspecified option to the DataLocation enum would be enough;

It would be misleading because this Unspecified would have a different meaning than the one in VariableDeclaration::Location and it still would not be a 1:1 translation, IMO nullopt is a better choice.

Changelog.md

libsolidity/interface/StorageLayout.cpp

libsolidity/ast/Types.cpp

libsolidity/codegen/ir/IRGenerator.cpp

test/cmdlineTests/transient_storage_layout_smoke/output.json

libsolidity/interface/StorageLayout.cpp

libsolidity/ast/Types.cpp

nikola-matic · 2024-07-24T09:56:06Z

libsolidity/codegen/ir/IRGenerator.cpp

@@ -1103,8 +1115,9 @@ void IRGenerator::resetContext(ContractDefinition const& _contract, ExecutionCon
 	m_context = std::move(newContext);

 	m_context.setMostDerivedContract(_contract);
-	for (auto const& var: ContractType(_contract).stateVariables())
-		m_context.addStateVariable(*std::get<0>(var), std::get<1>(var), std::get<2>(var));
+	for (auto const& location: {DataLocation::Storage, DataLocation::Transient})


Might make sense to make the required data location std::optional and set to std::nullopt by default - this would alleviate then need to change both this section of code as well as the one in ContractCompiler.

Yeah, I thought about that at first but expected it to be more complicated than what I have actually come up with.

nikola-matic · 2024-07-24T09:58:53Z

libsolidity/ast/Types.cpp

+	VariableDeclaration::Location location = [&]() {
+		switch (_location)
+		{
+			case DataLocation::Storage: return VariableDeclaration::Location::Unspecified; // By default state variables have unspecified (storage) data location


Why do we even need two enums for this? It makes it significantly more annoying in the first place. Adding an Unspecified option to the DataLocation enum would be enough; plus it's not a class member enum so it can be aliased as well (unlike VariableDeclaration::Location). But yeah, a problem for another time.

test/cmdlineTests/transient_storage_layout_smoke_two_contracts/input.json

test/cmdlineTests/transient_storage_layout_value_types/in.sol

test/cmdlineTests/transient_storage_layout_value_types_interleaved_with_storage/in.sol

test/cmdlineTests/transient_storage_layout_value_types_interleaved_with_storage/input.json

solc/CommandLineParser.cpp

nikola-matic

@cameel please take over.

Changelog.md

cameel

Here are a few initial remarks after going through tests.

So far only minor stuff, other than the fact that we haven't even one test covering CLI output. And that docs have not been updated.

test/cmdlineTests/standard_outputs_on_compilation_error/output.json

test/cmdlineTests/transient_storage_layout_smoke_empty/input.json

test/cmdlineTests/transient_storage_layout_smoke_two_contracts/input.json

cameel · 2024-07-26T11:22:27Z

test/cmdlineTests/transient_storage_layout_value_types_interleaved_with_storage/in.sol

+contract A {
+    uint transient x;
+    uint y;
+    int transient z;
+    int w;
+    address transient addr;
+    address d;
+    bool transient b;
+    bool c;
+}


I'd also add some constants and immutables here to make sure they're not included in either layout.

Also different visibilities (public, private, etc.).

Could also be separate tests, but IMO it would be fine to keep it in one test since CLI tests are pretty verbose.

test/cmdlineTests/transient_storage_layout_value_types/in.sol

test/cmdlineTests/transient_storage_layout_value_types_interleaved_with_storage/input.json

cameel

I went through it all now and did not find anything broken. Just a few more testing/styling/wording remarks.

So overall this is mostly fine, just needs docs, CLI coverage, some small tweaks and then we can merge.

libsolidity/interface/StorageLayout.cpp

libsolidity/ast/Types.cpp

libsolidity/analysis/TypeChecker.cpp

Changelog.md

nikola-matic

You haven't added a cmdline test for --transient-storage-layout yet, right?

Changelog.md

matheusaaguiar · 2024-07-29T12:58:50Z

You haven't added a cmdline test for --transient-storage-layout yet, right?

No I haven't. I wrote a test case in solc/CommandLineInterface.cpp but the output contains the path to a temporary file which changes everytime and I am not sure how to handle it. See 6b6407c.

nikola-matic · 2024-07-29T13:01:11Z

You haven't added a cmdline test for --transient-storage-layout yet, right?

No I haven't. I wrote a test case in solc/CommandLineInterface.cpp but the output contains the path to a temporary file which changes everytime and I am not sure how to handle it. See 6b6407c.

I meant something like this - https://github.com/ethereum/solidity/tree/develop/test/cmdlineTests/ir_subobject_order

matheusaaguiar · 2024-07-29T13:26:49Z

I meant something like this - https://github.com/ethereum/solidity/tree/develop/test/cmdlineTests/ir_subobject_order

Duh, yeah, I forgot we can do that 😁
Added.

matheusaaguiar · 2024-07-29T13:34:43Z

We have this section in the docs describing the storage layout and it mentions storage very frequently. I was thinking of a good way to include the information about transient storage. I think that a note explaining that regarding the layout of state variables, and only that, the same rules apply for both storage and transient storage.
Is that a good approach?

cameel

A couple of tests have syntax errors and we could use a tiny bit more detail in the docs.

I didn't find anything more serious and these are trivial to fix so you can consider it soft-approved.

cameel · 2024-08-01T18:29:21Z

docs/internals/layout_in_storage.rst

@@ -6,6 +6,9 @@ Layout of State Variables in Storage

 .. _storage-inplace-encoding:

+.. note::
+     The rules described in this section apply for both storage and transient storage data locations.


I'd still add a few things:

Maybe change title to "Layout of State Variables in Storage and Transient Storage"?

Say that the layouts are completely independent, i.e. storage variables have zero effect on locations of transient storage variables and vice-versa. They can be safely interleaved.

I'd mention that only value types are supported for transient storage.

There's a note saying that we consider storage layout a part of the external interface. Is that still true for transient storage? If so, we should say that explicitly.

Add an index entry for transient storage, next to storage above.

Also, there's a note way below, saying:

The JSON output format of a contract's storage layout is still considered experimental and is subject to change in non-breaking releases of Solidity.

I don't think it's true any more. When we once broke the layout accidentally in 0.8.x cycle, we considered it very breaking and released a new version a few days later to correct it.

Given that, I wonder if we should now say that about transient storage or is it automatically considered stable by the virtue of being tied to storage layout?

I have applied all suggestions, but not sure about the notes regarding external interface.
What do you think @ekpyron?

TBH this is already wrong on develop so it's fine to merge the PR regardless and then fix it separately in a smaller doc-only PR.

test/cmdlineTests/transient_storage_layout/args

test/cmdlineTests/transient_storage_layout_smoke_two_contracts/input.json

test/cmdlineTests/transient_storage_layout_value_types_interleaved_with_storage/output.json

test/cmdlineTests/transient_storage_layout_value_types_packed/in.sol

cameel · 2024-08-01T18:56:32Z

I think that a note explaining that regarding the layout of state variables, and only that, the same rules apply for both storage and transient storage.
Is that a good approach?

I generally agree in that we don't have to create a separate section and just repeat everything verbatim, but I think we could use a tiny bit more mentions than just a single note :) See #15247 (comment)

nikola-matic · 2024-08-01T21:10:40Z

Gonna need to squash the commits - still got some fixups in there.

matheusaaguiar self-assigned this Jul 8, 2024

matheusaaguiar force-pushed the transientStorageLayout branch from 828652c to 39f0be8 Compare July 9, 2024 18:58

matheusaaguiar marked this pull request as ready for review July 9, 2024 18:59

matheusaaguiar force-pushed the transientStorageLayout branch from 8da9441 to 09890a7 Compare July 9, 2024 22:37

matheusaaguiar commented Jul 9, 2024

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

nikola-matic reviewed Jul 18, 2024

View reviewed changes

matheusaaguiar force-pushed the transientStorageLayout branch from 9545849 to e4961e0 Compare July 23, 2024 18:06

nikola-matic reviewed Jul 24, 2024

View reviewed changes

nikola-matic reviewed Jul 25, 2024

View reviewed changes

solc/CommandLineParser.cpp Outdated Show resolved Hide resolved

matheusaaguiar force-pushed the transientStorageLayout branch from b58bd32 to de3872e Compare July 25, 2024 18:13

nikola-matic previously approved these changes Jul 26, 2024

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

cameel reviewed Jul 26, 2024

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

cameel reviewed Jul 26, 2024

View reviewed changes

libsolidity/interface/StorageLayout.cpp Outdated Show resolved Hide resolved

libsolidity/ast/Types.cpp Outdated Show resolved Hide resolved

libsolidity/analysis/TypeChecker.cpp Show resolved Hide resolved

Changelog.md Outdated Show resolved Hide resolved

matheusaaguiar dismissed nikola-matic’s stale review via eea2800 July 26, 2024 18:07

nikola-matic reviewed Jul 29, 2024

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

matheusaaguiar force-pushed the transientStorageLayout branch from 6b6407c to fc21366 Compare July 29, 2024 13:26

matheusaaguiar force-pushed the transientStorageLayout branch from b18cf12 to 7660610 Compare July 29, 2024 18:43

cameel reviewed Aug 1, 2024

View reviewed changes

matheusaaguiar force-pushed the transientStorageLayout branch from 7660610 to 6ee0b7f Compare August 1, 2024 20:23

cameel approved these changes Aug 1, 2024

View reviewed changes

generate transient storage layout

c5685e7

matheusaaguiar force-pushed the transientStorageLayout branch from 7d13d99 to c5685e7 Compare August 1, 2024 23:22

nikola-matic approved these changes Aug 2, 2024

View reviewed changes

nikola-matic merged commit 7094e30 into develop Aug 2, 2024
72 checks passed

nikola-matic deleted the transientStorageLayout branch August 2, 2024 11:12

nikola-matic mentioned this pull request Aug 20, 2024

Support transient storage state variables in legacy codegen #15257

Merged

cameel mentioned this pull request Sep 19, 2024

High-Level Language Support for Transient Storage #15007

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate transient storage layout #15247

Generate transient storage layout #15247

matheusaaguiar commented Jul 8, 2024

matheusaaguiar Jul 9, 2024

nikola-matic Jul 24, 2024

cameel Jul 26, 2024 •

edited

Loading

nikola-matic Jul 24, 2024

matheusaaguiar Jul 25, 2024

nikola-matic Jul 24, 2024

nikola-matic left a comment

cameel left a comment

cameel Jul 26, 2024 •

edited

Loading

cameel left a comment

nikola-matic left a comment

matheusaaguiar commented Jul 29, 2024

nikola-matic commented Jul 29, 2024

matheusaaguiar commented Jul 29, 2024

matheusaaguiar commented Jul 29, 2024

cameel left a comment

cameel Aug 1, 2024

cameel Aug 1, 2024

matheusaaguiar Aug 1, 2024

cameel Aug 1, 2024

cameel commented Aug 1, 2024 •

edited

Loading

nikola-matic commented Aug 1, 2024

Generate transient storage layout #15247

Generate transient storage layout #15247

Conversation

matheusaaguiar commented Jul 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikola-matic left a comment

Choose a reason for hiding this comment

cameel left a comment

Choose a reason for hiding this comment

cameel Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

cameel left a comment

Choose a reason for hiding this comment

nikola-matic left a comment

Choose a reason for hiding this comment

matheusaaguiar commented Jul 29, 2024

nikola-matic commented Jul 29, 2024

matheusaaguiar commented Jul 29, 2024

matheusaaguiar commented Jul 29, 2024

cameel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel commented Aug 1, 2024 • edited Loading

nikola-matic commented Aug 1, 2024

cameel Jul 26, 2024 •

edited

Loading

cameel Jul 26, 2024 •

edited

Loading

cameel commented Aug 1, 2024 •

edited

Loading