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!: autoload storage slots #1120

Merged
merged 13 commits into from
Sep 1, 2023
Merged

Conversation

segfault-magnet
Copy link
Contributor

closes: #1117 #1118

Storage slots are now loaded automatically by default (an opt-out exists).

If the StorageConfiguration indicates that autoload should take place, Contract::load_from will try and discover the storage slots file in the same directory as the contract binary to be loaded.

If it is for some reason missing this constitutes an error. The user will be guided to either provide the file or disable autoloading.

Any additional storage slots configured in StorageConfiguration act as an override taking precedence over what was loaded from the storage slots file.

Enabling the autoloading uncovered a bug where we didn't account for storage slots when calculating the heap data offset for predicates.

Breaking

The storage configuration interface changed
Autoloading by default might change behavior.

Checklist

  • I have linked to any relevant issues.
  • I have updated the documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary labels.
  • 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.

@segfault-magnet segfault-magnet added bug Something isn't working enhancement New feature or request labels Aug 31, 2023
@segfault-magnet segfault-magnet self-assigned this Aug 31, 2023
Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

Very clean implementation and good UX choices. Left just a couple of nits/suggestions.

docs/src/deploying/storage-slots.md Outdated Show resolved Hide resolved
examples/contracts/src/lib.rs Show resolved Hide resolved
Copy link
Contributor

@hal3e hal3e left a comment

Choose a reason for hiding this comment

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

IMO it is a bit weird that we try to get the storage slots path from the bin path. What do you think that instead we ask for the sway project folder. From there we can get both the bin and the storage slots and we can generate nice error messages. Something like this:

    let contract = Contract::load_from(
        "some/path/sway_project_name",
        LoadConfiguration::default().
    )?;

@digorithm
Copy link
Member

IMO it is a bit weird that we try to get the storage slots path from the bin path. What do you think that instead we ask for the sway project folder. From there we can get both the bin and the storage slots and we can generate nice error messages. Something like this:

    let contract = Contract::load_from(
        "some/path/sway_project_name",
        LoadConfiguration::default().
    )?;

That's a good point. In the implementation we're essentially just grabbing the project dir anyway:

    let dir = contract_binary.parent()?;

    let binary_filename = contract_binary.file_stem()?.to_str()?;

    Some(dir.join(format!("{binary_filename}-storage_slots.json")))

And then grabbing the project name (which is the bin name) and slapping the storage_slots string in front of it... Might as well just grab the dir name.

I don't have super strong opinions on this, but I agree it could be confusing for the end user.

@segfault-magnet
Copy link
Contributor Author

IMO it is a bit weird that we try to get the storage slots path from the bin path. What do you think that instead we ask for the sway project folder. From there we can get both the bin and the storage slots and we can generate nice error messages. Something like this:

    let contract = Contract::load_from(
        "some/path/sway_project_name",
        LoadConfiguration::default().
    )?;

I share your feelings, but have the following reservations:

If we start giving project paths to the Contract::load_from then we're implicitly expecting a certain layout. The bin file must be named as the dir is, the ABI has -abi.json appended etc.

We would expect that the user gave us the root dir of the project (the one where Forc.toml lives) because we need it to extract the project name (then to get the contract bin name etc).

But is the build a debug or a release one? We would need to add a flag for that as well due to the artifacts landing in out/debug and out/release respectively.

Is the project titled the same as the dir? The name property of the Forc.toml can differ from the project rootdir name. From the name property all the generated files derive their names.

What if somebody just wants to load a contract binary from outside a project? We would need to support them and provide an alternative, custom-path-loading mechanism. Or they would need to recreate this folder structure by hand.

If we start accepting project paths, then we would probably want to be consistent in the abigen! as well. So that users might have the same experience of "give the thing a path to your project". Otherwise, they would remember abigen! gets the full path to the JSON, contracts get the project, and predicates again get the custom path (or they get the project).

Although it seems scripty (for lack of a better word), the fact remains that for projects it will detect the storage slots reliably, for one-off contract binaries the autoloading can be disabled and both parties are content.

@hal3e
Copy link
Contributor

hal3e commented Sep 1, 2023

IMO it is a bit weird that we try to get the storage slots path from the bin path. What do you think that instead we ask for the sway project folder. From there we can get both the bin and the storage slots and we can generate nice error messages. Something like this:

    let contract = Contract::load_from(
        "some/path/sway_project_name",
        LoadConfiguration::default().
    )?;

I share your feelings, but have the following reservations:

If we start giving project paths to the Contract::load_from then we're implicitly expecting a certain layout. The bin file must be named as the dir is, the ABI has -abi.json appended etc.

We would expect that the user gave us the root dir of the project (the one where Forc.toml lives) because we need it to extract the project name (then to get the contract bin name etc).

But is the build a debug or a release one? We would need to add a flag for that as well due to the artifacts landing in out/debug and out/release respectively.

Is the project titled the same as the dir? The name property of the Forc.toml can differ from the project rootdir name. From the name property all the generated files derive their names.

What if somebody just wants to load a contract binary from outside a project? We would need to support them and provide an alternative, custom-path-loading mechanism. Or they would need to recreate this folder structure by hand.

If we start accepting project paths, then we would probably want to be consistent in the abigen! as well. So that users might have the same experience of "give the thing a path to your project". Otherwise, they would remember abigen! gets the full path to the JSON, contracts get the project, and predicates again get the custom path (or they get the project).

Although it seems scripty (for lack of a better word), the fact remains that for projects it will detect the storage slots reliably, for one-off contract binaries the autoloading can be disabled and both parties are content.

I agree. Let's use this until we have something better!

examples/contracts/src/lib.rs Show resolved Hide resolved
@segfault-magnet segfault-magnet merged commit 34f9216 into master Sep 1, 2023
35 checks passed
@segfault-magnet segfault-magnet deleted the feat/autoload_storage_slots branch September 1, 2023 17:16
calldelegation added a commit to FuelLabs/sway that referenced this pull request Oct 11, 2023
## Description
- Bump harness template to v0.48.0 for [intro-to-sway
guide](FuelLabs/docs-hub#55)
- Used auto loading [storage slots
](FuelLabs/fuels-rs#1120)

## Checklist

- [x] I have linked to any relevant issues.
- [ ] 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: Igor Rončević <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add storage slot initialization for contract deployment
4 participants