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.

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

feat: Storage Layout Overrides #2593

Merged
merged 30 commits into from
Jan 7, 2022
Merged

feat: Storage Layout Overrides #2593

merged 30 commits into from
Jan 7, 2022

Conversation

abdullathedruid
Copy link
Contributor

@abdullathedruid abdullathedruid commented Jan 6, 2022

This is my first contribution, so would be grateful for everyones time spent reviewing this PR
Targeting #2572

What I did

  • Added flag --storage-layout-file to compiler which takes a list of storage layout files
  • Added some primitive checks to ensure the user does not accidentally cause storage collision

How I did it

  • Updated compile_code function to take an additional optional argument, storage_layout_overrides. If provided, this is used to custom data position for storage. If it is omitted, then the compiler functions as normally

How to verify it

  • Test script added

Description for the changelog

Added the ability to override storage slots for variables

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2022

Codecov Report

Merging #2593 (02b1b20) into master (fe74711) will decrease coverage by 0.02%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2593      +/-   ##
==========================================
- Coverage   86.29%   86.26%   -0.03%     
==========================================
  Files          90       90              
  Lines        9291     9351      +60     
  Branches     2354     2370      +16     
==========================================
+ Hits         8018     8067      +49     
- Misses        789      796       +7     
- Partials      484      488       +4     
Impacted Files Coverage Δ
vyper/cli/vyper_compile.py 72.58% <28.57%> (-2.64%) ⬇️
vyper/semantics/validation/data_positions.py 90.90% <88.00%> (-4.22%) ⬇️
vyper/compiler/__init__.py 87.17% <100.00%> (+1.46%) ⬆️
vyper/compiler/phases.py 94.05% <100.00%> (+0.05%) ⬆️
vyper/exceptions.py 92.72% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe74711...02b1b20. Read the comment docs.

@abdullathedruid abdullathedruid marked this pull request as draft January 6, 2022 03:49
@abdullathedruid abdullathedruid marked this pull request as ready for review January 6, 2022 20:43
Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

Nice approach, I think your approach is basically pretty clean. I made some suggestions which should make it easier to follow. I think the main thing would be to refactor StorageCollision so it only has a single publicly facing method, reserve_slot_range(self, slot: int, n_slots: int). This makes its usage clearer since there is only "one way to use it". Then, the internal implementation could be slightly simpler if you use a set instead of a dict.

vyper/semantics/validation/data_positions.py Outdated Show resolved Hide resolved
vyper/typing.py Outdated Show resolved Hide resolved
vyper/semantics/validation/data_positions.py Outdated Show resolved Hide resolved
vyper/semantics/validation/data_positions.py Outdated Show resolved Hide resolved
vyper/semantics/validation/data_positions.py Outdated Show resolved Hide resolved
vyper/semantics/validation/data_positions.py Outdated Show resolved Hide resolved
vyper/semantics/validation/data_positions.py Outdated Show resolved Hide resolved
vyper/semantics/validation/data_positions.py Outdated Show resolved Hide resolved
vyper/semantics/validation/data_positions.py Outdated Show resolved Hide resolved
Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

Left a few comments, also I think in Python double underscores are usually used for dunder methods; for private methods please use single underscores. Shaping up nicely, thank you!

tests/cli/outputs/test_storage_layout_overrides.py Outdated Show resolved Hide resolved
vyper/semantics/validation/data_positions.py Outdated Show resolved Hide resolved
vyper/semantics/validation/data_positions.py Outdated Show resolved Hide resolved
vyper/semantics/validation/data_positions.py Outdated Show resolved Hide resolved
vyper/semantics/validation/data_positions.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants