Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Output the storage layout of a contract via storageLayout artifact #7589

Merged
merged 1 commit into from
Nov 14, 2019
Merged

Output the storage layout of a contract via storageLayout artifact #7589

merged 1 commit into from
Nov 14, 2019

Conversation

leonardoalt
Copy link
Member

Fixes #3736

@leonardoalt leonardoalt changed the title Output the storage layout of a contract via storageLayout artifact [WIP] Output the storage layout of a contract via storageLayout artifact Oct 30, 2019
@leonardoalt
Copy link
Member Author

leonardoalt commented Oct 30, 2019 •
edited
Loading

Missing

  • Docs [X]
  • Tests [X]

@leonardoalt
Copy link
Member Author

From #3736 (comment):
Do we also want the "encoding method" to be part of this output?

@leonardoalt leonardoalt mentioned this pull request Oct 30, 2019
Closed
@MrChico
Copy link
Member

MrChico commented Oct 30, 2019

From #3736 (comment):
Do we also want the "encoding method" to be part of this output?

Might not be necessary if both type and length are included. Then the encoding method should be derivable from those.

@axic
Copy link
Member

axic commented Oct 30, 2019

Did you check #4017 (comment)?

@leonardoalt
Copy link
Member Author

leonardoalt commented Oct 31, 2019 •
edited
Loading

@axic Yes. Since this PR is simpler I considered that it might be the case that we want the storage layout to look simpler as well, since the comment you mentioned says that the flat structure would simplify recursive data structures.

@leonardoalt
Copy link
Member Author

It's too tempting to go ahead and implement the Struct case as well, it's too close :p

@leonardoalt leonardoalt changed the title [WIP] Output the storage layout of a contract via storageLayout artifact Output the storage layout of a contract via storageLayout artifact Nov 7, 2019
@leonardoalt
Copy link
Member Author

This is ready for review.

@leonardoalt leonardoalt force-pushed the storage_layout branch 3 times, most recently from 495875e to 5ba3597 Compare November 8, 2019 14:55
@leonardoalt
Copy link
Member Author

@ekpyron removed ptr, adjusted tests, docs, and rebased

ekpyron
ekpyron requested changes Nov 8, 2019
View reviewed changes
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but the documentation is lacking... do we want to fix that here?

docs/miscellaneous.rst Outdated Show resolved Hide resolved
docs/miscellaneous.rst Show resolved Hide resolved
libsolidity/interface/StorageLayout.cpp Outdated Show resolved Hide resolved
libsolidity/interface/StorageLayout.cpp Outdated Show resolved Hide resolved
test/cmdlineTests/storage_layout_dyn_array/output.json Outdated Show resolved Hide resolved
@ekpyron
Copy link
Member

ekpyron commented Nov 8, 2019

Should we explicitly document this as "experimental and subject to change in non-breaking releases" for now and communicate with the "debugging format working group" about this, since there will be overlap?

@ekpyron ekpyron mentioned this pull request Nov 8, 2019
Closed
@leonardoalt
Copy link
Member Author

Should we explicitly document this as "experimental and subject to change in non-breaking releases" for now and communicate with the "debugging format working group" about this, since there will be overlap?

Fine with me

@ekpyron
Copy link
Member

ekpyron commented Nov 8, 2019

Should we explicitly document this as "experimental and subject to change in non-breaking releases" for now and communicate with the "debugging format working group" about this, since there will be overlap?

Fine with me

Not sure about it myself

ekpyron
ekpyron reviewed Nov 8, 2019
View reviewed changes
docs/miscellaneous.rst Outdated Show resolved Hide resolved
leonardoalt
leonardoalt commented Nov 10, 2019
View reviewed changes
test/cmdlineTests/storage_layout_value_types_packed/output.json
{"contracts":{"fileA":{"A":{"storageLayout":{"storage":[{"ast_id":"2","contract":"A","label":"x","offset":"0","slot":"0","type":"t_uint64"},{"ast_id":"4","contract":"A","label":"y","offset":"8","slot":"0","type":"t_uint128"},{"ast_id":"6","contract":"A","label":"z","offset":"0","slot":"1","type":"t_uint128"},{"ast_id":"8","contract":"A","label":"addr","offset":"0","slot":"2","type":"t_address"},{"ast_id":"12","contract":"A","label":"array","offset":"0","slot":"3","type":"t_array(t_uint256)2_storage"}],"types":{"t_address":{"encoding":"inplace","label":"address","numberOfBytes":"20","numberOfSlots":"1"},"t_array(t_uint256)2_storage":{"base":"t_uint256","encoding":"inplace","label":"uint256[2]","numberOfBytes":"32","numberOfSlots":"2"},"t_uint128":{"encoding":"inplace","label":"uint128","numberOfBytes":"16","numberOfSlots":"1"},"t_uint256":{"encoding":"inplace","label":"uint256","numberOfBytes":"32","numberOfSlots":"1"},"t_uint64":{"encoding":"inplace","label":"uint64","numberOfBytes":"8","numberOfSlots":"1"}}}}}},"errors":[{"component":"general","formattedMessage":"fileA:1:1: Warning: Source file does not specify required compiler version!
contract A { uint64 x; uint128 y; uint128 z; address addr; uint[2] array; }
^-------------------------------------------------------------------------^
","message":"Source file does not specify required compiler version!","severity":"warning","sourceLocation":{"end":75,"file":"fileA","start":0},"type":"Warning"}],"sources":{"fileA":{"id":0}}}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chriseth I don't remember precisely, but is this message Source file does not specify required compiler version!... suppose to be removed automatically?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it should be removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, no, it's not, only the recommended compiler version is removed.

@leonardoalt
Copy link
Member Author

@ekpyron Updated and also added the note regarding this feature being subject to changes.
What about the test outputs? Do we want to wait for #7665?

@ekpyron
Copy link
Member

ekpyron commented Nov 11, 2019

I don't think we should wait for #7665 with this.

@ekpyron
Copy link
Member

ekpyron commented Nov 11, 2019

Adding the "to discuss" label for deciding about the "still subject to change" thing.

@ekpyron
Copy link
Member

ekpyron commented Nov 11, 2019

We might not need the "still subject to change" thing, if we create wrappers in solc-js later.

@leonardoalt
Copy link
Member Author

@ekpyron I'm strongly against the solc-js wrapper idea :p

chriseth
chriseth reviewed Nov 13, 2019
View reviewed changes
docs/miscellaneous.rst Outdated Show resolved Hide resolved
chriseth
chriseth reviewed Nov 13, 2019
View reviewed changes
docs/miscellaneous.rst Outdated Show resolved Hide resolved
chriseth
chriseth reviewed Nov 13, 2019
View reviewed changes
docs/miscellaneous.rst Outdated Show resolved Hide resolved
chriseth
chriseth reviewed Nov 13, 2019
View reviewed changes
docs/miscellaneous.rst Outdated Show resolved Hide resolved
chriseth
chriseth reviewed Nov 13, 2019
View reviewed changes
docs/miscellaneous.rst Outdated Show resolved Hide resolved
chriseth
chriseth reviewed Nov 13, 2019
View reviewed changes
docs/miscellaneous.rst Outdated Show resolved Hide resolved
chriseth
chriseth reviewed Nov 13, 2019
View reviewed changes
docs/miscellaneous.rst Outdated Show resolved Hide resolved
chriseth
chriseth reviewed Nov 13, 2019
View reviewed changes
docs/miscellaneous.rst Outdated Show resolved Hide resolved
chriseth
chriseth reviewed Nov 13, 2019
View reviewed changes
docs/miscellaneous.rst Outdated Show resolved Hide resolved
chriseth
chriseth reviewed Nov 13, 2019
View reviewed changes
docs/miscellaneous.rst Outdated Show resolved Hide resolved
@leonardoalt
Copy link
Member Author

@chriseth updated all comments

@chriseth
Copy link
Contributor

Build error.

@leonardoalt
Copy link
Member Author

@chriseth tests passing

@leonardoalt
Copy link
Member Author

Do we also want a command line option?

chriseth
chriseth reviewed Nov 14, 2019
View reviewed changes
docs/miscellaneous.rst


{
"astId": 2,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed these to be numbers again and added some spaces.

chriseth
chriseth reviewed Nov 14, 2019
View reviewed changes
docs/miscellaneous.rst
- ``bytes``: single slot or Keccak-256 hash-based depending on the data size (see :ref:`above <bytes-and-string>`).

- ``label`` is the canonical type name.
- ``numberOfBytes`` is the number of used bytes (as a decimal string). Note that if ``numberOfBytes > 32`` this means that more than one slot is used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added string clarification.

@chriseth chriseth mentioned this pull request Nov 14, 2019
Closed
34 tasks
@chriseth chriseth merged commit d1c6ab8 into develop Nov 14, 2019
@chriseth chriseth deleted the storage_layout branch November 14, 2019 10:22
@chriseth chriseth mentioned this pull request Jan 23, 2020
Closed
3 tasks
@nikugogoi nikugogoi mentioned this pull request Apr 6, 2023
Open
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chriseth chriseth chriseth approved these changes

@ekpyron ekpyron ekpyron approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Storage Keys to ABI
5 participants
@leonardoalt @MrChico @axic @ekpyron @chriseth