Support different Soroban storage types #1664

salaheldinsoliman · 2024-08-24T11:35:26Z

This PR addresses that Soroban has 3 different storage types: persistent, temporary and instance.
It modifies the parser to accept persistent, temporary and instance keywords during contract variable instantiation. If no storage type is provided, the storage defaults to persistent and raises a warning telling the developer: variable x has no storage type defined, defaulting to persistent.

Signed-off-by: salaheldinsoliman <[email protected]>

…eldinsoliman/solang into feat/soroban_storage_types

Signed-off-by: salaheldinsoliman <[email protected]>

seanyoung · 2024-10-04T06:45:36Z

src/sema/variables.rs

@@ -233,9 +235,22 @@ pub fn variable_decl<'a>(

                visibility = Some(v.clone());
            }
+            pt::VariableAttribute::StorageType(s) => {
+                storage_type = Some(s.clone());


Here you don't check for duplicate entries. For example:

contract c { int temporary persistent v; }

This confused me for a second, but I assume you're suggesting that we add a check to prevent declaring two storage type modifiers for the same variable in a contract. However, in Soroban, each storage 'map' (e.g., temporary, persistent, instance) operates independently of the others. It's possible to store different (or even the same) values for the same key across different storage types. For example, the key 123_u32 might hold the value 100_u32 in temporary storage, while in persistent storage, the same key could store abcd or 100_u32.

So, if both modifiers are handled correctly, Soroban would technically allow this. That said, while it's not typically recommended—since it consumes more storage—there could be edge cases where this pattern is useful, perhaps as a way to initialize data across namespaces. In those cases, such declarations might be valid.

I think @seanyoung meant this case:
uint64 public temporary instance persistent sesa = 1;

seanyoung · 2024-10-04T06:46:21Z

src/sema/variables.rs

+                def.name.as_ref().unwrap().name
+            ),
+        ));
+    }


If the storage_type is specified and the target is not Soroban, we should give an error and not silently ignore.

Would be nice to have test cases for these

seanyoung · 2024-10-04T06:47:43Z

src/emit/binary.rs

@@ -1015,6 +1015,7 @@ impl<'a> Binary<'a> {
                Type::FunctionSelector => {
                    self.llvm_type(&Type::Bytes(ns.target.selector_length()), ns)
                }
+                Type::Void => BasicTypeEnum::IntType(self.context.i64_type()),


There is a llvm/inkwell self.context.void_type().into() for this.

There is a llvm/inkwell self.context.void_type().into() for this.

like this?

Type::Void => self.context.void_type().into(),

Soroban functions always return a value encoded in 64 bits. If functions return type is void, that means it returns 2_u64. However, I should have explained this in a comment

That is Soroban specific, and that doesn't belong in the llvm type system.

What's the return value? Is it a success/failure code? That's the same as Solana and Polkadot, and that's implemented in the function emit code.

@seanyoung The return value is not a success/failure, it is just the returned data
If no data is returned, it returns void (which is a uint64 in Soroban)

seanyoung · 2024-10-04T06:50:06Z

src/codegen/expression.rs

+                let var = ns.contracts[contract_no].variables.get(var_no).unwrap();
+
+                storage_type = var.storage_type.clone();
+            }


you could do

let storage_type = if let ast::Expression::StorageVariable { loc: _, ty: _, var_no, contract_no, } = *expr.clone() { let var = ns.contracts[contract_no].variables.get(var_no).unwrap(); var.storage_type.clone() } else { None };

seanyoung · 2024-10-04T06:50:18Z

src/codegen/expression.rs

+        let var = ns.contracts[*contract_no].variables.get(*var_no).unwrap();
+
+        storage_type = var.storage_type.clone();
+    }


seanyoung · 2024-10-04T06:50:31Z

src/codegen/expression.rs

+                let var = ns.contracts[*contract_no].variables.get(*var_no).unwrap();
+
+                storage_type = var.storage_type.clone();
+            }


seanyoung · 2024-10-04T06:51:37Z

solang-parser/src/lexer.rs

@@ -553,6 +561,9 @@ static KEYWORDS: phf::Map<&'static str, Token> = phf_map! {
    "unchecked" => Token::Unchecked,
    "assembly" => Token::Assembly,
    "let" => Token::Let,
+    "persistent" => Token::Persistent,
+    "temporary" => Token::Temporary,
+    "instance" => Token::Instance,


This does affect Polkadot/Solana, but never mind, I don't know what other solution there is (other than a better parser generator).

Can they be made context-sensitive so that they are only treated as keywords when used in the appropriate context, and in other contexts they would be allowed to be used. I'm not sure if that can be done here, but basically have it only if the target is soroban?

Unfortunately the keywords can't be made context-sensitive with the current parser generator. Of course, this is something I would really love and this would be such a nicer feature (for lalrpop).

Having said that, it might be possible to replace the keywords persistent, temporary, and instance with SolIdentifier and then checking for the keywords in sema.

A way around this is to make these keywords Soroban specific, so they do not apply for Solana/Polkadot. This could be implemented in the lexer.

silence48

All in all the code looks good, and seems to implement the storage correctly.

for solidity dev's who might be looking at how this works, and are not familiar with the multiple types of storage, it would be helpful to explicitly document the best practices for choosing a storage type (persistent vs. instance vs. temporary). This could be inline in the code, or a link to an external guide (e.g., Stellar documentation) would suffice.

Consider adding a function or hook to extend the TTL for instance storage (perhaps something like an extend_ttl method), as instance storage typically has a limited lifespan tied to the contract instance. This could be helpful for contracts that need to maintain instance data for longer periods.

Currently, storage types are limited to Soroban targets, but if these types are used in other contexts, you should ensure proper error handling is in place.
I'd suggest adding error handling for these cases on those targets if necessary.

silence48 · 2024-10-04T17:53:46Z

src/sema/variables.rs

@@ -233,9 +235,22 @@ pub fn variable_decl<'a>(

                visibility = Some(v.clone());
            }
+            pt::VariableAttribute::StorageType(s) => {


Suggested change

pt::VariableAttribute::StorageType(s) => {

pt::VariableAttribute::StorageType(s) => {

if storage_type.is_some() {

ns.diagnostics.push(Diagnostic::error(

s.loc().unwrap(),

format!(

"variable `{}` has multiple storage types specified",

def.name.as_ref().unwrap().name

),

));

} else {

storage_type = Some(s.clone());

}

}`

silence48 · 2024-10-04T17:56:15Z

src/sema/variables.rs

        }
    }

+    if ns.target == Target::Soroban && storage_type.is_none() {


Suggested change

if ns.target == Target::Soroban && storage_type.is_none() {

if ns.target != Target::Soroban && storage_type.is_some() {

ns.diagnostics.push(Diagnostic::error(

def.loc,

format!(

"variable `{}`: storage types are only valid for Soroban targets",

def.name.as_ref().unwrap().name

),

));

}

if ns.target == Target::Soroban && storage_type.is_none() {

silence48 · 2024-10-04T17:59:38Z

src/emit/binary.rs

@@ -1015,6 +1015,7 @@ impl<'a> Binary<'a> {
                Type::FunctionSelector => {
                    self.llvm_type(&Type::Bytes(ns.target.selector_length()), ns)
                }
+                Type::Void => BasicTypeEnum::IntType(self.context.i64_type()),


There is a llvm/inkwell self.context.void_type().into() for this.

like this?

Type::Void => self.context.void_type().into(),

silence48 · 2024-10-04T18:13:30Z

tests/soroban_testcases/storage.rs

+#[test]
+fn different_storage_types() {
+    let src = build_solidity(
+        r#"contract sesa {


I'd suggest renaming the storage variables sesa, sesa1, sesa2, and sesa3 to something more descriptive, unless they need to stay like this for some reason

For example, temporaryCount, instanceCount, and persistentCount may help clarify what the variables are intended to test and demonstrate.

I figured i'd mention this because i was trying to figure out what sesa3 was used for,

silence48 · 2024-10-04T18:15:43Z

src/codegen/cfg.rs

@@ -1027,7 +1029,7 @@ impl ControlFlowGraph {
                true_block,
                false_block,
            ),
-            Instr::LoadStorage { ty, res, storage } => format!(
+            Instr::LoadStorage { ty, res, storage, .. } => format!(


a comment clarifying about how the system behaves when the storage type is omitted might be helpful to people looking at this in the future.

This is the implementation of instr_to_string, which is used in Solang if a Solidity dev wants to debug the generated IR (using --emit cfg).
To simplify the output of this, I omitted the storage type option as I think it is not relevant at the IR generation stage.
WDYT?

silence48 · 2024-10-04T18:21:43Z

integration/soroban/storage_types.spec.js

+
+  it('check initial values', async () => {
+    // Check initial values of all storage variables
+    let sesa = await call_contract_function("sesa", server, keypair, contract);


To expand testing coverage, consider adding a test case for scenarios where a variable lacks a specified storage type and defaults to persistent. This would ensure that the defaulting mechanism works as intended in Soroban contracts. (unless that's what sesa3 already represents)

Also, what I said previously about sesa not being a descriptive variable.

silence48 · 2024-10-04T18:27:18Z

src/emit/soroban/target.rs

    ) -> BasicValueEnum<'a> {
+        let storage_type = if let Some(storage_type) = storage_type {


To reduce code duplication and improve maintainability, consider extracting the mapping of StorageType to integer values into a helper function. This will ensure consistency across different functions and make future updates easier.

Both storage_load and storage_store functions contain similar code for mapping StorageType.

fn map_storage_type(storage_type: Option<&StorageType>) -> u64 { if let Some(storage_type) = storage_type { match storage_type { StorageType::Persistent(_) => 0, StorageType::Temporary(_) => 1, StorageType::Instance(_) => 2, } } else { 0 // Default to Persistent } }

Then, in your functions, you can use:

let storage_type = map_storage_type(storage_type);

silence48 · 2024-10-04T18:33:55Z

solang-parser/src/lexer.rs

@@ -553,6 +561,9 @@ static KEYWORDS: phf::Map<&'static str, Token> = phf_map! {
    "unchecked" => Token::Unchecked,
    "assembly" => Token::Assembly,
    "let" => Token::Let,
+    "persistent" => Token::Persistent,
+    "temporary" => Token::Temporary,
+    "instance" => Token::Instance,


Can they be made context-sensitive so that they are only treated as keywords when used in the appropriate context, and in other contexts they would be allowed to be used. I'm not sure if that can be done here, but basically have it only if the target is soroban?

silence48 · 2024-10-04T18:36:39Z

src/emit/polkadot/target.rs

@@ -17,7 +17,7 @@ use inkwell::values::{
    ArrayValue, BasicMetadataValueEnum, BasicValueEnum, FunctionValue, IntValue, PointerValue,
 };
 use inkwell::{AddressSpace, IntPredicate};
-use solang_parser::pt::Loc;
+use solang_parser::pt::{Loc, StorageType};


The storage_type parameter has been added to trait methods and their implementations for all targets, but it's unused in non-Soroban targets.

Ensure that adding the storage_type parameter does not negatively impact other targets. If the parameter is irrelevant for Polkadot and Solana, consider using default implementations or conditional compilation to exclude it from those targets.

silence48 · 2024-10-04T19:00:04Z

src/sema/variables.rs

@@ -233,9 +235,22 @@ pub fn variable_decl<'a>(

                visibility = Some(v.clone());
            }
+            pt::VariableAttribute::StorageType(s) => {
+                storage_type = Some(s.clone());


This confused me for a second, but I assume you're suggesting that we add a check to prevent declaring two storage type modifiers for the same variable in a contract. However, in Soroban, each storage 'map' (e.g., temporary, persistent, instance) operates independently of the others. It's possible to store different (or even the same) values for the same key across different storage types. For example, the key 123_u32 might hold the value 100_u32 in temporary storage, while in persistent storage, the same key could store abcd or 100_u32.

So, if both modifiers are handled correctly, Soroban would technically allow this. That said, while it's not typically recommended—since it consumes more storage—there could be edge cases where this pattern is useful, perhaps as a way to initialize data across namespaces. In those cases, such declarations might be valid.

silence48 · 2024-10-04T19:06:18Z

fmt/src/formatter.rs

@@ -3333,6 +3333,11 @@ impl<'a, W: Write> Visitor for Formatter<'a, W> {
                self.visit_list("", idents, Some(loc.start()), Some(loc.end()), false)?;
                None
            }
+            VariableAttribute::StorageType(s) => match s {


is there any reason the order of the matching should match the order of the enum?

silence48 · 2024-10-04T19:08:31Z

solang-parser/src/pt.rs

+#[derive(Debug, PartialEq, Eq, Clone)]
+#[cfg_attr(feature = "pt-serde", derive(Serialize, Deserialize))]
+#[repr(u8)] // for cmp; order of variants is important
+pub enum StorageType {


This doesn't match the storagetype enum order on soroban side

Signed-off-by: salaheldinsoliman <[email protected]>

…eldinsoliman/solang into feat/soroban_storage_types

Signed-off-by: salaheldinsoliman <[email protected]>

silence48

Changes look good!

salaheldinsoliman changed the title ~~feat: soroban storage types~~ Support different Soroban storage types Aug 25, 2024

salaheldinsoliman force-pushed the feat/soroban_storage_types branch 2 times, most recently from 9c914c1 to 9a9733a Compare September 19, 2024 18:20

salaheldinsoliman force-pushed the feat/soroban_storage_types branch from 1463176 to 929e4b6 Compare September 29, 2024 12:45

salaheldinsoliman added 2 commits October 2, 2024 18:48

add tests

92ab9df

Signed-off-by: salaheldinsoliman <[email protected]>

add tests

cf0498d

Signed-off-by: salaheldinsoliman <[email protected]>

salaheldinsoliman force-pushed the feat/soroban_storage_types branch from 929e4b6 to cf0498d Compare October 2, 2024 16:50

Merge branch 'hyperledger-solang:main' into feat/soroban_storage_types

14bc671

salaheldinsoliman marked this pull request as ready for review October 2, 2024 16:51

salaheldinsoliman requested review from LucasSte, seanyoung and xermicus as code owners October 2, 2024 16:51

salaheldinsoliman added 6 commits October 2, 2024 19:01

fix tests

c559b00

Signed-off-by: salaheldinsoliman <[email protected]>

Merge branch 'feat/soroban_storage_types' of https://github.com/salah…

c12608c

…eldinsoliman/solang into feat/soroban_storage_types

fix tests

dfa8aa3

Signed-off-by: salaheldinsoliman <[email protected]>

fix failing parser test

46367ed

Signed-off-by: salaheldinsoliman <[email protected]>

remove residual println

ae8b540

Signed-off-by: salaheldinsoliman <[email protected]>

fix Polkadot integration tests

9105d37

Signed-off-by: salaheldinsoliman <[email protected]>

seanyoung requested changes Oct 4, 2024

View reviewed changes

silence48 reviewed Oct 4, 2024

View reviewed changes

salaheldinsoliman added 3 commits October 5, 2024 20:42

address review comments

9a981e0

Signed-off-by: salaheldinsoliman <[email protected]>

Merge branch 'feat/soroban_storage_types' of https://github.com/salah…

e5dd60a

…eldinsoliman/solang into feat/soroban_storage_types

fix clippy error

2ef39f8

Signed-off-by: salaheldinsoliman <[email protected]>

salaheldinsoliman requested a review from seanyoung October 5, 2024 19:10

silence48 reviewed Oct 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support different Soroban storage types #1664

Support different Soroban storage types #1664

salaheldinsoliman commented Aug 24, 2024 •

edited

Loading

seanyoung Oct 4, 2024

silence48 Oct 4, 2024

salaheldinsoliman Oct 5, 2024

seanyoung Oct 4, 2024

seanyoung Oct 7, 2024

seanyoung Oct 4, 2024

silence48 Oct 4, 2024

salaheldinsoliman Oct 5, 2024

seanyoung Oct 7, 2024

salaheldinsoliman Nov 2, 2024

seanyoung Oct 4, 2024

seanyoung Oct 4, 2024

seanyoung Oct 4, 2024

seanyoung Oct 4, 2024

silence48 Oct 4, 2024

seanyoung Oct 7, 2024

seanyoung Oct 11, 2024

silence48 left a comment

silence48 Oct 4, 2024

silence48 Oct 4, 2024

silence48 Oct 4, 2024

silence48 Oct 4, 2024

silence48 Oct 4, 2024

salaheldinsoliman Oct 5, 2024 •

edited

Loading

silence48 Oct 4, 2024 •

edited

Loading

silence48 Oct 4, 2024

silence48 Oct 4, 2024

silence48 Oct 4, 2024

silence48 Oct 4, 2024

silence48 Oct 4, 2024

silence48 Oct 4, 2024 •

edited

Loading

silence48 left a comment

		) -> BasicValueEnum<'a> {
		let storage_type = if let Some(storage_type) = storage_type {

Support different Soroban storage types #1664

Are you sure you want to change the base?

Support different Soroban storage types #1664

Conversation

salaheldinsoliman commented Aug 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silence48 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salaheldinsoliman Oct 5, 2024 • edited Loading

Choose a reason for hiding this comment

silence48 Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silence48 Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

silence48 left a comment

Choose a reason for hiding this comment

salaheldinsoliman commented Aug 24, 2024 •

edited

Loading

salaheldinsoliman Oct 5, 2024 •

edited

Loading

silence48 Oct 4, 2024 •

edited

Loading

silence48 Oct 4, 2024 •

edited

Loading