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

Experimental: StorageKey feature #4297

Merged
merged 12 commits into from
Apr 13, 2023
Merged

Conversation

mohammadfawaz
Copy link
Contributor

@mohammadfawaz mohammadfawaz commented Mar 15, 2023

Description

This PR introduces StorageHandle to the core as an experimental feature. StorageHandle is a descriptor of a location in storage containing two fields:

  1. A key pointing to a storage slot s.
  2. An offset pointing to a particular word in s or in its subsequent slots. This offset is new and is not described in the RFC. I will rectify that shortly.
    The standard library introduces helper methods read, try_read, and write to StorageHandle that allow reading from and writing to the location pointed to by the handle. try_read returns an Option wrapping the data while read automatically internally unwraps the data and could panic.

The summary of this change is as follows:

  • New struct core::experimental::storage::StorageHandle
  • New storage library std::experimental::storage::* that should eventually replace std::storage::*. This new library mirrors the old one to the extent possible and introduces the helper methods for StorageHandle. Other data structures such as StorageVec and StorageBytes will be introduced in a separate PR.
  • Add an experimental flag --experimental-storage to forc that enables all the required code in the compiler to support StorageHandle as described in the Introducing StorageKey sway-rfcs#23.
    • Type checking phases assumes the existence of StorageHandle and monomorphizes it as needed.
    • Storage accesses now always return StorageHandle and storage reassignment are no longer relevant.
    • IR gen handles storage accesses by "filling" the key and the offset in an aggregate representing the StorageHandle. The key and the offset are statically determined based on the index of the storage variable in the storage block and the field accessed, if applicable.
    • Storage initializers are now handled based on the new model
    • The new approach packs struct fields by default but does not pack across storage variables. This offers the most amount of flexibility. This is a deviation from the RFC which I will update shortly.
    • To implement StorageMap and other dynamic storage data structures, we now write impl StorageHandle<StorageMap<K, V>> instead of impl StorageMap<K, V> directly. Also, note that the get method now returns a StorageHandle instead of the actual data. To get the actual data, read() or try_read() should be called on the resulting handle. This is needed for multiple reasons including proper support for nested dynamic storage types. Rust also does the same, in a way (where get actually returns & which happens to coerce to the real data in certain places).
  • I added various tests but they're not comprehensive. Some tests on my list:
    • Extensive tests for storage maps in structs (which now work!)
    • Extensive tests for storage accesses with various types and struct layouts

I still need to figure out how to do nested dynamic storage types with this approach. The stuff I have in map_in_map_access in test_projects/experimental_storage/src/main.sw is just an attempt but not ergonomic at all of course.

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@mohammadfawaz mohammadfawaz force-pushed the mohammadfawaz/storage_key branch 2 times, most recently from c89c376 to 032acc8 Compare March 16, 2023 20:58
@mohammadfawaz mohammadfawaz self-assigned this Mar 16, 2023
@mohammadfawaz mohammadfawaz added lib: std Standard library language feature Core language features visible to end users compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler: parser Everything to do with the parser labels Mar 16, 2023
@mohammadfawaz mohammadfawaz force-pushed the mohammadfawaz/storage_key branch 7 times, most recently from ce83390 to 895e01c Compare March 22, 2023 20:43
@mohammadfawaz mohammadfawaz requested review from a team March 22, 2023 21:00
@mohammadfawaz mohammadfawaz marked this pull request as ready for review March 22, 2023 21:01
otrho
otrho previously approved these changes Mar 23, 2023
sway-core/src/ir_generation/function.rs Show resolved Hide resolved
sway-core/src/ir_generation/function.rs Outdated Show resolved Hide resolved
otrho
otrho previously approved these changes Mar 23, 2023
@otrho otrho requested a review from a team March 23, 2023 22:55
@mohammadfawaz
Copy link
Contributor Author

Ah conflicts.. let me fix.

otrho
otrho previously approved these changes Mar 23, 2023
Copy link
Contributor

@nfurfaro nfurfaro left a comment

Choose a reason for hiding this comment

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

looks great! I just left a couple comments/questions.

sway-lib-core/src/raw_ptr.sw Outdated Show resolved Hide resolved
sway-lib-std/src/experimental/storage.sw Show resolved Hide resolved
@otrho otrho requested a review from a team April 5, 2023 07:55
otrho
otrho previously approved these changes Apr 6, 2023
@otrho otrho requested a review from a team April 6, 2023 01:51
mitchmindtree
mitchmindtree previously approved these changes Apr 12, 2023
Copy link
Contributor

@mitchmindtree mitchmindtree left a comment

Choose a reason for hiding this comment

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

Looking good to me! Left some minor suggestions, but will leave it up to you if you think they're worth addressing.

sway-lib-core/src/experimental/storage.sw Outdated Show resolved Hide resolved
sway-lib-std/src/experimental/storage.sw Outdated Show resolved Hide resolved
sway-lib-std/src/experimental/storage.sw Outdated Show resolved Hide resolved
sway-lib-std/src/experimental/storage.sw Outdated Show resolved Hide resolved
@mohammadfawaz mohammadfawaz changed the title Experimental: StorageHandle feature Experimental: StorageKey feature Apr 12, 2023
@mohammadfawaz mohammadfawaz enabled auto-merge (squash) April 12, 2023 13:51
@mohammadfawaz mohammadfawaz dismissed stale reviews from mitchmindtree and otrho via 52cf865 April 12, 2023 14:00
@JoshuaBatty JoshuaBatty requested a review from a team April 12, 2023 23:20
@mitchmindtree mitchmindtree requested review from a team April 12, 2023 23:48
@mohammadfawaz mohammadfawaz merged commit 9685fab into master Apr 13, 2023
@mohammadfawaz mohammadfawaz deleted the mohammadfawaz/storage_key branch April 13, 2023 00:24
mohammadfawaz added a commit that referenced this pull request Apr 21, 2023
…` RFC (#4464)

## Description
Implement FuelLabs/sway-rfcs#23

Closes #3419 (technically via the
RFC)
Closes #3202
Closes #3043
Closes #2639
Closes #2465
Closes #4304

This is a big one but mostly removes stuff. It's hard to review but the
summary of changes below should be sufficient. Most of the changes here
are things that I just had to do to make CI pass.

- Removed the `--experimental-storage` flag and made its
[behavior](#4297) the default. Also
removed everything that was required for the previous storage APIs in
favour of the new ones.
- Break down the `std::storage` into multiple submodules:
 - `storage_key` implements the API for `std::core::StorageKey`
- `storage_api` implements the free functions `read` (previously `get`),
`write` (previously `store`), and `clear`.
- 4 more modules for the dynamic storage types which now use the new
`StorageKey` API.
- `#[storage(write)]` now allows reading from storage as well. This is
needed because the way we pack structs in storage now requires that we
sometimes read from storage first if we were to write a portion of a
slot.
- Removed the "storage only types" checks and the corresponding tests.
- Removed the `__get_storage_key` intrinsic and the `get_storage_key` IR
instruction and all corresponding tests. Also removed the `state_index`
metadata as it is no longer required.
- Added tests and example to showcase nested storage maps and nested
storage vectors.

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [x] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [x] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.

Co-authored-by: Joshua Batty <joshpbatty@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen compiler: parser Everything to do with the parser formatter language feature Core language features visible to end users lib: std Standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants