Skip to content

Commit

Permalink
Storage domains (#6466)
Browse files Browse the repository at this point in the history 
## Description

This PR fixes #6317 by implementing `storage_domains` experimental
feature.

This feature introduces two _storage domains_, one for the storage
fields defined inside of the `storage` declaration, and a different one
for storage slots generated inside of the `StorageMap`.

The PR strictly fixes the issue described in #6317 by applying the
recommendation proposed in the issue.
A general approach to storage key domains will be discussed as a part of
the [Configurable and composable storage
RFC](FuelLabs/sway-rfcs#40).

Additionally, the PR:
- adds expressive diagnostics for the duplicated storage keys warning.
- removes the misleading internal compiler error that always followed
the storage key type mismatch error (see demo below).

Closes #6317, #6701.

## Breaking Changes

The PR changes the way how storage keys are generated for `storage`
fields. Instead of `sha256("storage::ns1::ns2.field_name")` we now use
`sha256((0u8, "storage::ns1::ns2.field_name"))`.

This is a breaking change for those who relied on the storage key
calculation.

## Demo

Before, every type-mismatch was always followed by an ICE, because the
compilation continued despite the type-mismatch.

![Mismatched types and ICE -
Before](https://github.com/user-attachments/assets/ac7915f7-3458-409e-a2bb-118dd4925234)

This is solved now, and the type-mismatch has a dedicated help message.

![Mismatched types and ICE -
After](https://github.com/user-attachments/assets/570aedd3-4c9c-4945-bfd0-5f12d68dbead)

## Checklist

- [x] I have linked to any relevant issues.
- [x] 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).
- [ ] If my change requires substantial documentation changes, I have
[requested support from the DevRel
team](https://github.com/FuelLabs/devrel-requests/issues/new/choose)
- [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: Sophie Dankel <[email protected]>
Co-authored-by: IGI-111 <[email protected]>
  • Loading branch information
@ironcev @sdankel @IGI-111
3 people authored Nov 15, 2024
1 parent 6e31144 commit 3b07f49
Show file tree
Hide file tree
Showing 49 changed files with 2,236 additions and 127 deletions.
11 changes: 9 additions & 2 deletions sway-core/src/ir_generation/function.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4338,6 +4338,14 @@ impl<'eng> FnCompiler<'eng> {
base_type: &Type,
span_md_idx: Option<MetadataIndex>,
) -> Result<TerminatorValue, CompileError> {
// Use the `struct_field_names` to get a field id that is unique even for zero-sized values that live in the same slot.
// We calculate the `unique_field_id` early, here, before the `storage_filed_names` get consumed by `get_storage_key` below.
let unique_field_id = get_storage_field_id(
&storage_field_names,
&struct_field_names,
context.experimental,
);

// Get the actual storage key as a `Bytes32` as well as the offset, in words,
// within the slot. The offset depends on what field of the top level storage
// variable is being accessed.
Expand Down Expand Up @@ -4371,7 +4379,7 @@ impl<'eng> FnCompiler<'eng> {
// particular slot is the remaining offset, in words.
(
add_to_b256(
get_storage_key(storage_field_names.clone(), key.clone()),
get_storage_key(storage_field_names, key, context.experimental),
offset_in_slots,
),
offset_remaining,
Expand Down Expand Up @@ -4428,7 +4436,6 @@ impl<'eng> FnCompiler<'eng> {
.add_metadatum(context, span_md_idx);

// Store the field identifier as the third field in the `StorageKey` struct
let unique_field_id = get_storage_field_id(storage_field_names, struct_field_names); // use the struct_field_names to get a field id that is unique even for zero-sized values that live in the same slot
let field_id = convert_literal_to_value(context, &Literal::B256(unique_field_id.into()))
.add_metadatum(context, span_md_idx);
let gep_2_val =
Expand Down
73 changes: 53 additions & 20 deletions sway-core/src/ir_generation/storage.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use crate::fuel_prelude::{
fuel_tx::StorageSlot,
fuel_types::{Bytes32, Bytes8},
};
use sway_features::ExperimentalFeatures;
use sway_ir::{
constant::{Constant, ConstantValue},
context::Context,
Expand All @@ -20,28 +21,31 @@ enum InByte8Padding {
}

/// Hands out storage keys using storage field names or an existing key.
/// Basically returns sha256("storage::<storage_namespace_name1>::<storage_namespace_name2>.<storage_field_name>")
/// Basically returns sha256((0u8, "storage::<storage_namespace_name1>::<storage_namespace_name2>.<storage_field_name>"))
/// or key if defined.
pub(super) fn get_storage_key(storage_field_names: Vec<String>, key: Option<U256>) -> Bytes32 {
if let Some(key) = key {
return key.to_be_bytes().into();
pub(super) fn get_storage_key(
storage_field_names: Vec<String>,
key: Option<U256>,
experimental: ExperimentalFeatures,
) -> Bytes32 {
match key {
Some(key) => key.to_be_bytes().into(),
None => hash_storage_key_string(get_storage_key_string(&storage_field_names), experimental),
}

Hasher::hash(get_storage_key_string(storage_field_names))
}

pub fn get_storage_key_string(storage_field_names: Vec<String>) -> String {
pub fn get_storage_key_string(storage_field_names: &[String]) -> String {
if storage_field_names.len() == 1 {
format!(
"{}{}{}",
sway_utils::constants::STORAGE_DOMAIN,
sway_utils::constants::STORAGE_TOP_LEVEL_NAMESPACE,
sway_utils::constants::STORAGE_FIELD_SEPARATOR,
storage_field_names.last().unwrap(),
)
} else {
format!(
"{}{}{}{}{}",
sway_utils::constants::STORAGE_DOMAIN,
sway_utils::constants::STORAGE_TOP_LEVEL_NAMESPACE,
sway_utils::constants::STORAGE_NAMESPACE_SEPARATOR,
storage_field_names
.iter()
Expand All @@ -56,10 +60,11 @@ pub fn get_storage_key_string(storage_field_names: Vec<String>) -> String {
}

/// Hands out unique storage field ids using storage field names and struct field names.
/// Basically returns sha256("storage::<storage_namespace_name1>::<storage_namespace_name2>.<storage_field_name>.<struct_field_name1>.<struct_field_name2>")
/// Basically returns sha256((0u8, "storage::<storage_namespace_name1>::<storage_namespace_name2>.<storage_field_name>.<struct_field_name1>.<struct_field_name2>")).
pub(super) fn get_storage_field_id(
storage_field_names: Vec<String>,
struct_field_names: Vec<String>,
storage_field_names: &[String],
struct_field_names: &[String],
experimental: ExperimentalFeatures,
) -> Bytes32 {
let data = format!(
"{}{}",
Expand All @@ -74,7 +79,32 @@ pub(super) fn get_storage_field_id(
)
}
);
Hasher::hash(data)

hash_storage_key_string(data, experimental)
}

fn hash_storage_key_string(
storage_key_string: String,
experimental: ExperimentalFeatures,
) -> Bytes32 {
let mut hasher = Hasher::default();
// Certain storage types, like, e.g., `StorageMap` allow
// storage slots of their contained elements to be defined
// based on developer's input. E.g., the `key` in a `StorageMap`
// used to calculate the storage slot is a developer input.
//
// To ensure that pre-images of such storage slots can never
// be the same as a pre-image of compiler generated key of storage
// field, we prefix the pre-images with a single byte that denotes
// the domain. Storage types like `StorageMap` must have a different
// domain prefix than the `STORAGE_DOMAIN` which is 0u8.
//
// For detailed elaboration see: https://github.com/FuelLabs/sway/issues/6317
if experimental.storage_domains {
hasher.input(sway_utils::constants::STORAGE_DOMAIN);
}
hasher.input(storage_key_string);
hasher.finalize()
}

use uint::construct_uint;
Expand Down Expand Up @@ -109,17 +139,18 @@ pub fn serialize_to_storage_slots(
key: Option<U256>,
ty: &Type,
) -> Vec<StorageSlot> {
let experimental = context.experimental;
match &constant.value {
ConstantValue::Undef => vec![],
// If not being a part of an aggregate, single byte values like `bool`, `u8`, and unit
// are stored as a byte at the beginning of the storage slot.
ConstantValue::Unit if ty.is_unit(context) => vec![StorageSlot::new(
get_storage_key(storage_field_names, key),
get_storage_key(storage_field_names, key, experimental),
Bytes32::new([0; 32]),
)],
ConstantValue::Bool(b) if ty.is_bool(context) => {
vec![StorageSlot::new(
get_storage_key(storage_field_names, key),
get_storage_key(storage_field_names, key, experimental),
Bytes32::new([
if *b { 1 } else { 0 },
0,
Expand Down Expand Up @@ -158,7 +189,7 @@ pub fn serialize_to_storage_slots(
}
ConstantValue::Uint(b) if ty.is_uint8(context) => {
vec![StorageSlot::new(
get_storage_key(storage_field_names, key),
get_storage_key(storage_field_names, key, experimental),
Bytes32::new([
*b as u8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0,
Expand All @@ -168,7 +199,7 @@ pub fn serialize_to_storage_slots(
// Similarly, other uint values are stored at the beginning of the storage slot.
ConstantValue::Uint(n) if ty.is_uint(context) => {
vec![StorageSlot::new(
get_storage_key(storage_field_names, key),
get_storage_key(storage_field_names, key, experimental),
Bytes32::new(
n.to_be_bytes()
.iter()
Expand All @@ -182,13 +213,13 @@ pub fn serialize_to_storage_slots(
}
ConstantValue::U256(b) if ty.is_uint_of(context, 256) => {
vec![StorageSlot::new(
get_storage_key(storage_field_names, key),
get_storage_key(storage_field_names, key, experimental),
Bytes32::new(b.to_be_bytes()),
)]
}
ConstantValue::B256(b) if ty.is_b256(context) => {
vec![StorageSlot::new(
get_storage_key(storage_field_names, key),
get_storage_key(storage_field_names, key, experimental),
Bytes32::new(b.to_be_bytes()),
)]
}
Expand Down Expand Up @@ -224,8 +255,10 @@ pub fn serialize_to_storage_slots(
type_size_in_bytes,
ty.as_string(context)
);

let storage_key = get_storage_key(storage_field_names, key, experimental);
(0..(type_size_in_bytes + 31) / 32)
.map(|i| add_to_b256(get_storage_key(storage_field_names.clone(), key.clone()), i))
.map(|i| add_to_b256(storage_key, i))
.zip((0..packed.len() / 4).map(|i| {
Bytes32::new(
Vec::from_iter((0..4).flat_map(|j| *packed[4 * i + j]))
Expand Down
20 changes: 19 additions & 1 deletion sway-core/src/language/ty/declaration/storage.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@ use sway_types::{Ident, Named, Span, Spanned};

use crate::{
engine_threading::*,
language::{parsed::StorageDeclaration, ty::*},
ir_generation::storage::get_storage_key_string,
language::parsed::StorageDeclaration,
transform::{self},
ty::*,
type_system::*,
Namespace,
};
Expand Down Expand Up @@ -252,6 +254,22 @@ pub struct TyStorageField {
pub attributes: transform::AttributesMap,
}

impl TyStorageField {
/// Returns the full name of the [TyStorageField], consisting
/// of its name preceded by its full namespace path.
/// E.g., "storage::ns1::ns1.name".
pub fn full_name(&self) -> String {
get_storage_key_string(
&self
.namespace_names
.iter()
.map(|i| i.as_str().to_string())
.chain(vec![self.name.as_str().to_string()])
.collect::<Vec<_>>(),
)
}
}

impl EqWithEngines for TyStorageField {}
impl PartialEqWithEngines for TyStorageField {
fn eq(&self, other: &Self, ctx: &PartialEqWithEnginesContext) -> bool {
Expand Down
34 changes: 22 additions & 12 deletions sway-core/src/semantic_analysis/ast_node/declaration/declaration.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -441,22 +441,32 @@ impl TyDecl {
let initializer =
ty::TyExpression::type_check(handler, ctx.by_ref(), &initializer)?;

let mut key_ty_expression = None;
if let Some(key_expression) = key_expression {
let mut key_ctx =
ctx.with_type_annotation(engines.te().id_of_b256());

key_ty_expression = Some(ty::TyExpression::type_check(
handler,
key_ctx.by_ref(),
&key_expression,
)?);
}
let key_expression = match key_expression {
Some(key_expression) => {
let key_ctx = ctx
.with_type_annotation(engines.te().id_of_b256())
.with_help_text("Storage keys must have type \"b256\".");

// TODO: Remove the `handler.scope` once https://github.com/FuelLabs/sway/issues/5606 gets solved.
// We need it here so that we can short-circuit in case of a `TypeMismatch` error which is
// not treated as an error in the `type_check()`'s result.
let typed_expr = handler.scope(|handler| {
ty::TyExpression::type_check(
handler,
key_ctx,
&key_expression,
)
})?;

Some(typed_expr)
}
None => None,
};

fields_buf.push(ty::TyStorageField {
name,
namespace_names: namespace_names.clone(),
key_expression: key_ty_expression,
key_expression,
type_argument,
initializer,
span: field_span,
Expand Down
42 changes: 18 additions & 24 deletions sway-core/src/semantic_analysis/ast_node/declaration/storage.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@ use crate::{
decl_engine::parsed_id::ParsedDeclId,
fuel_prelude::fuel_tx::StorageSlot,
ir_generation::{
const_eval::compile_constant_expression_to_constant,
storage::{get_storage_key_string, serialize_to_storage_slots},
const_eval::compile_constant_expression_to_constant, storage::serialize_to_storage_slots,
},
language::{
parsed::StorageDeclaration,
Expand Down Expand Up @@ -54,32 +53,27 @@ impl ty::TyStorageDecl {
let slots = f.get_initialized_storage_slots(engines, context, md_mgr, module);

// Check if slot with same key was already used and throw warning.
if let Ok(slots) = slots.clone() {
for s in slots.into_iter() {
if let Ok(slots) = &slots {
for s in slots.iter() {
if let Some(old_field) = slot_fields.insert(*s.key(), f.clone()) {
handler.emit_warn(CompileWarning {
span: f.span(),
warning_content:
sway_error::warning::Warning::DuplicatedStorageKey {
key: format!("{:X} ", s.key()),
field1: get_storage_key_string(
old_field
.namespace_names
.iter()
.map(|i| i.as_str().to_string())
.chain(vec![old_field
.name
.as_str()
.to_string()])
.collect::<Vec<_>>(),
),
field2: get_storage_key_string(
f.namespace_names
.iter()
.map(|i| i.as_str().to_string())
.chain(vec![f.name.as_str().to_string()])
.collect::<Vec<_>>(),
),
first_field: (&old_field.name).into(),
first_field_full_name: old_field.full_name(),
first_field_key_is_compiler_generated: old_field
.key_expression
.is_none(),
second_field: (&f.name).into(),
second_field_full_name: f.full_name(),
second_field_key_is_compiler_generated: f
.key_expression
.is_none(),
key: format!("0x{:x}", s.key()),
experimental_storage_domains: context
.experimental
.storage_domains,
},
})
}
Expand Down Expand Up @@ -151,7 +145,7 @@ impl ty::TyStorageField {
Ok(Some(key))
} else {
Err(CompileError::Internal(
"Expected B256 key",
"Storage keys must have type \"b256\".",
key_expression.span.clone(),
))
}
Expand Down
6 changes: 3 additions & 3 deletions sway-core/src/semantic_analysis/ast_node/expression/typed_expression/enum_instantiation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,9 +136,9 @@ pub(crate) fn instantiate_enum(
.with_help_text(help_text)
.with_type_annotation(enum_variant_type_id);

// TODO-IG: Remove the `handler.scope` once https://github.com/FuelLabs/sway/issues/5606 gets solved.
// We need it here so that we can short-circuit in case of a `TypeMismatch` error which is
// not treated as an error in the `type_check()`'s result.
// TODO: Remove the `handler.scope` once https://github.com/FuelLabs/sway/issues/5606 gets solved.
// We need it here so that we can short-circuit in case of a `TypeMismatch` error which is
// not treated as an error in the `type_check()`'s result.
let typed_expr = handler
.scope(|handler| ty::TyExpression::type_check(handler, enum_ctx, single_expr))?;

Expand Down
6 changes: 3 additions & 3 deletions sway-core/src/semantic_analysis/ast_node/expression/typed_expression/struct_instantiation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -397,9 +397,9 @@ fn type_check_field_arguments(
.with_type_annotation(struct_field.type_argument.type_id)
.with_unify_generic(true);

// TODO-IG: Remove the `handler.scope` once https://github.com/FuelLabs/sway/issues/5606 gets solved.
// We need it here so that we can short-circuit in case of a `TypeMismatch` error which is
// not treated as an error in the `type_check()`'s result.
// TODO: Remove the `handler.scope` once https://github.com/FuelLabs/sway/issues/5606 gets solved.
// We need it here so that we can short-circuit in case of a `TypeMismatch` error which is
// not treated as an error in the `type_check()`'s result.
let typed_expr = handler
.scope(|handler| ty::TyExpression::type_check(handler, ctx, &field.value));

Expand Down
Loading

0 comments on commit 3b07f49

Please sign in to comment.