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

Support different Soroban storage types #1664

Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion fmt/src/formatter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::{
};
use alloy_primitives::Address;
use itertools::{Either, Itertools};
use solang_parser::pt::{PragmaDirective, VersionComparator};
use solang_parser::pt::{PragmaDirective, StorageType, VersionComparator};
use std::{fmt::Write, str::FromStr};
use thiserror::Error;

Expand Down Expand Up @@ -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 {

Choose a reason for hiding this comment

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

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

StorageType::Instance(_) => Some("instance".to_string()),
StorageType::Temporary(_) => Some("temporary".to_string()),
StorageType::Persistent(_) => Some("persistent".to_string()),
},
};
if let Some(token) = token {
let loc = attribute.loc();
Expand Down
12 changes: 12 additions & 0 deletions fmt/src/solang_ext/ast_eq.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,17 @@ impl AstEq for VariableDefinition {
}
}

impl AstEq for StorageType {
fn ast_eq(&self, other: &Self) -> bool {
matches!(
(self, other),
(StorageType::Instance(_), StorageType::Instance(_))
| (StorageType::Persistent(_), StorageType::Persistent(_))
| (StorageType::Temporary(_), StorageType::Temporary(_))
)
}
}

impl AstEq for FunctionDefinition {
fn ast_eq(&self, other: &Self) -> bool {
// attributes
Expand Down Expand Up @@ -726,5 +737,6 @@ derive_ast_eq! { enum VariableAttribute {
Constant(loc),
Immutable(loc),
Override(loc, idents),
StorageType(s)
_
}}
4 changes: 2 additions & 2 deletions integration/polkadot/try_catch.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ contract TryCatchCaller {
constructor() payable {}

function test(uint128 div) public payable returns (uint128) {
TryCatchCallee instance = new TryCatchCallee();
TryCatchCallee contract_instance = new TryCatchCallee();

try instance.test(div) returns (uint128) {
try contract_instance.test(div) returns (uint128) {
return 4;
} catch Error(string reason) {
assert(reason == "foo");
Expand Down
1 change: 0 additions & 1 deletion integration/soroban/.gitignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
*.js
*.so
*.key
*.json
Expand Down
4 changes: 2 additions & 2 deletions integration/soroban/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
"mocha": "^10.4.0"
},
"scripts": {
"build": "solang compile *.sol --target soroban",
"build": "solang compile *.sol --target soroban && solang compile storage_types.sol --target soroban --release",
"setup": "node setup.js",
"test": "mocha *.spec.js --timeout 20000"
"test": "mocha *.spec.js --timeout 100000"
},
"devDependencies": {
"@eslint/js": "^9.4.0",
Expand Down
51 changes: 51 additions & 0 deletions integration/soroban/runtime_error.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import * as StellarSdk from '@stellar/stellar-sdk';
import { readFileSync } from 'fs';
import { expect } from 'chai';
import path from 'path';
import { fileURLToPath } from 'url';
import { call_contract_function } from './test_helpers.js';

const __filename = fileURLToPath(import.meta.url);
const dirname = path.dirname(__filename);

describe('Runtime Error', () => {
let keypair;
const server = new StellarSdk.SorobanRpc.Server(
"https://soroban-testnet.stellar.org:443",
);

let contractAddr;
let contract;
before(async () => {

console.log('Setting up runtime_error.sol contract tests...');

// read secret from file
const secret = readFileSync('alice.txt', 'utf8').trim();
keypair = StellarSdk.Keypair.fromSecret(secret);

let contractIdFile = path.join(dirname, '.soroban', 'contract-ids', 'Error.txt');
// read contract address from file
contractAddr = readFileSync(contractIdFile, 'utf8').trim().toString();

// load contract
contract = new StellarSdk.Contract(contractAddr);

// initialize the contract
await call_contract_function("init", server, keypair, contract);

// call decrement once. The second call however will result in a runtime error
await call_contract_function("decrement", server, keypair, contract);
});

it('get correct initial counter', async () => {

// decrement the counter again, resulting in a runtime error
let res = await call_contract_function("decrement", server, keypair, contract);

expect(res).to.contain('runtime_error: math overflow in runtime_error.sol:6:9-19');
});

});


21 changes: 21 additions & 0 deletions integration/soroban/storage_types.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
contract storage_types {

uint64 public temporary sesa = 1;
uint64 public instance sesa1 = 1;
uint64 public persistent sesa2 = 2;
uint64 public sesa3 = 2;

function inc() public {
sesa++;
sesa1++;
sesa2++;
sesa3++;
}

function dec() public {
sesa--;
sesa1--;
sesa2--;
sesa3--;
}
}
87 changes: 87 additions & 0 deletions integration/soroban/storage_types.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import * as StellarSdk from '@stellar/stellar-sdk';
import { readFileSync } from 'fs';
import { expect } from 'chai';
import path from 'path';
import { fileURLToPath } from 'url';
import { call_contract_function } from './test_helpers.js'; // Helper to interact with the contract

const __filename = fileURLToPath(import.meta.url);
const dirname = path.dirname(__filename);

describe('StorageTypes', () => {
let keypair;
const server = new StellarSdk.SorobanRpc.Server(
"https://soroban-testnet.stellar.org:443",
);

let contractAddr;
let contract;
before(async () => {
console.log('Setting up storage_types contract tests...');

// Read secret from file
const secret = readFileSync('alice.txt', 'utf8').trim();
keypair = StellarSdk.Keypair.fromSecret(secret);

let contractIdFile = path.join(dirname, '.soroban', 'contract-ids', 'storage_types.txt');
// Read contract address from file
contractAddr = readFileSync(contractIdFile, 'utf8').trim().toString();

// Load contract
contract = new StellarSdk.Contract(contractAddr);

// Initialize the contract if necessary (adapt according to your contract logic)
await call_contract_function("init", server, keypair, contract);
});

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

@silence48 silence48 Oct 4, 2024

Choose a reason for hiding this comment

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

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.

expect(sesa.toString()).eq("1");

let sesa1 = await call_contract_function("sesa1", server, keypair, contract);
expect(sesa1.toString()).eq("1");

let sesa2 = await call_contract_function("sesa2", server, keypair, contract);
expect(sesa2.toString()).eq("2");

let sesa3 = await call_contract_function("sesa3", server, keypair, contract);
expect(sesa3.toString()).eq("2");
});

it('increment values', async () => {
// Increment all values by calling the inc function
await call_contract_function("inc", server, keypair, contract);

// Check the incremented values
let sesa = await call_contract_function("sesa", server, keypair, contract);
expect(sesa.toString()).eq("2");

let sesa1 = await call_contract_function("sesa1", server, keypair, contract);
expect(sesa1.toString()).eq("2");

let sesa2 = await call_contract_function("sesa2", server, keypair, contract);
expect(sesa2.toString()).eq("3");

let sesa3 = await call_contract_function("sesa3", server, keypair, contract);
expect(sesa3.toString()).eq("3");
});

it('decrement values', async () => {
// Decrement all values by calling the dec function
await call_contract_function("dec", server, keypair, contract);

// Check the decremented values
let sesa = await call_contract_function("sesa", server, keypair, contract);
expect(sesa.toString()).eq("1");

let sesa1 = await call_contract_function("sesa1", server, keypair, contract);
expect(sesa1.toString()).eq("1");

let sesa2 = await call_contract_function("sesa2", server, keypair, contract);
expect(sesa2.toString()).eq("2");

let sesa3 = await call_contract_function("sesa3", server, keypair, contract);
expect(sesa3.toString()).eq("2");
});
});
7 changes: 6 additions & 1 deletion solang-parser/src/helpers/fmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
//!
//! [ref]: https://docs.soliditylang.org/en/latest/style-guide.html

use crate::pt;
use crate::pt::{self, StorageType};
use std::{
borrow::Cow,
fmt::{Display, Formatter, Result, Write},
Expand Down Expand Up @@ -1169,6 +1169,11 @@ impl Display for pt::VariableAttribute {
}
Ok(())
}
Self::StorageType(storage) => match storage {
StorageType::Instance(_) => f.write_str("instance"),
StorageType::Temporary(_) => f.write_str("temporary"),
StorageType::Persistent(_) => f.write_str("persistent"),
},
}
}
}
Expand Down
9 changes: 9 additions & 0 deletions solang-parser/src/helpers/loc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,14 @@ impl OptionalCodeLocation for pt::Visibility {
}
}

impl OptionalCodeLocation for pt::StorageType {
fn loc_opt(&self) -> Option<Loc> {
match self {
Self::Persistent(l) | Self::Temporary(l) | Self::Instance(l) => *l,
}
}
}

impl OptionalCodeLocation for pt::SourceUnit {
#[inline]
fn loc_opt(&self) -> Option<Loc> {
Expand Down Expand Up @@ -431,6 +439,7 @@ impl_for_enums! {

pt::VariableAttribute: match self {
Self::Visibility(ref l, ..) => l.loc_opt().unwrap_or_default(),
Self::StorageType(ref l, ..) => l.loc_opt().unwrap_or_default(),
Self::Constant(l, ..)
| Self::Immutable(l, ..)
| Self::Override(l, ..) => l,
Expand Down
11 changes: 11 additions & 0 deletions solang-parser/src/lexer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,11 @@ pub enum Token<'input> {
Default,
YulArrow,

// Storage types for Soroban
Persistent,
Temporary,
Instance,

Annotation(&'input str),
}

Expand Down Expand Up @@ -316,6 +321,9 @@ impl<'input> fmt::Display for Token<'input> {
Token::Default => write!(f, "default"),
Token::YulArrow => write!(f, "->"),
Token::Annotation(name) => write!(f, "@{name}"),
Token::Persistent => write!(f, "persistent"),
Token::Temporary => write!(f, "temporary"),
Token::Instance => write!(f, "instance"),
}
}
}
Expand Down Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

};

impl<'input> Lexer<'input> {
Expand Down
18 changes: 18 additions & 0 deletions solang-parser/src/pt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -955,6 +955,24 @@ pub enum VariableAttribute {

/// `ovveride(<1>,*)`
Override(Loc, Vec<IdentifierPath>),

/// Storage type.
StorageType(StorageType),
}

/// Soroban storage types.
#[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 {
Copy link

@silence48 silence48 Oct 4, 2024

Choose a reason for hiding this comment

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

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

/// `Temporary`
Temporary(Option<Loc>),

/// `persistent`
Persistent(Option<Loc>),

/// `Instance`
Instance(Option<Loc>),
}

/// A variable definition.
Expand Down
10 changes: 10 additions & 0 deletions solang-parser/src/solidity.lalrpop
Original file line number Diff line number Diff line change
Expand Up @@ -359,8 +359,15 @@ Visibility: Visibility = {
<l:@L> "private" <r:@R> => Visibility::Private(Some(Loc::File(file_no, l, r))),
}

StorageType: StorageType = {
<l:@L> "persistent" <r:@R> => StorageType::Persistent(Some(Loc::File(file_no, l, r))),
<l:@R> "temporary" <r:@R> => StorageType::Temporary(Some(Loc::File(file_no, l, r))),
<l:@R> "instance" <r:@R> => StorageType::Instance(Some(Loc::File(file_no, l, r))),
}

VariableAttribute: VariableAttribute = {
Visibility => VariableAttribute::Visibility(<>),
StorageType => VariableAttribute::StorageType(<>),
<l:@L> "constant" <r:@R> => VariableAttribute::Constant(Loc::File(file_no, l, r)),
<l:@L> "immutable" <r:@R> => VariableAttribute::Immutable(Loc::File(file_no, l, r)),
<l:@L> "override" <r:@R> => VariableAttribute::Override(Loc::File(file_no, l, r), Vec::new()),
Expand Down Expand Up @@ -1312,5 +1319,8 @@ extern {
"switch" => Token::Switch,
"case" => Token::Case,
"default" => Token::Default,
"persistent" => Token::Persistent,
"temporary" => Token::Temporary,
"instance" => Token::Instance,
}
}
4 changes: 2 additions & 2 deletions solang-parser/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ contract 9c {
Diagnostic { loc: File(0, 48, 49), level: Error, ty: ParserError, message: "unrecognised token ';', expected \"*\", \"<\", \"<=\", \"=\", \">\", \">=\", \"^\", \"~\", identifier, number, string".to_string(), notes: vec![] },
Diagnostic { loc: File(0, 62, 65), level: Error, ty: ParserError, message: r#"unrecognised token 'for', expected "(", ";", "=""#.to_string(), notes: vec![] },
Diagnostic { loc: File(0, 78, 79), level: Error, ty: ParserError, message: r#"unrecognised token '9', expected "case", "default", "leave", "revert", "switch", identifier"#.to_string(), notes: vec![] },
Diagnostic { loc: File(0, 95, 96), level: Error, ty: ParserError, message: "unrecognised token '0', expected \"(\", \"++\", \"--\", \".\", \"[\", \"case\", \"constant\", \"default\", \"external\", \"immutable\", \"internal\", \"leave\", \"override\", \"private\", \"public\", \"revert\", \"switch\", \"{\", identifier".to_string(), notes: vec![] },
Diagnostic { loc: File(0, 95, 96), level: Error, ty: ParserError, message: "unrecognised token '0', expected \"(\", \"++\", \"--\", \".\", \"[\", \"case\", \"constant\", \"default\", \"external\", \"immutable\", \"instance\", \"internal\", \"leave\", \"override\", \"persistent\", \"private\", \"public\", \"revert\", \"switch\", \"temporary\", \"{\", identifier".to_string(), notes: vec![] },
Diagnostic { loc: File(0, 116, 123), level: Error, ty: ParserError, message: "unrecognised token 'uint256', expected \"++\", \"--\", \".\", \"[\", \"case\", \"default\", \"leave\", \"switch\", identifier".to_string(), notes: vec![] },
Diagnostic { loc: File(0, 403, 404), level: Error, ty: ParserError, message: "unrecognised token '3', expected \"(\", \"++\", \"--\", \".\", \"[\", \"case\", \"constant\", \"default\", \"external\", \"immutable\", \"internal\", \"leave\", \"override\", \"private\", \"public\", \"revert\", \"switch\", \"{\", identifier".to_string(), notes: vec![] },
Diagnostic { loc: File(0, 403, 404), level: Error, ty: ParserError, message: "unrecognised token '3', expected \"(\", \"++\", \"--\", \".\", \"[\", \"case\", \"constant\", \"default\", \"external\", \"immutable\", \"instance\", \"internal\", \"leave\", \"override\", \"persistent\", \"private\", \"public\", \"revert\", \"switch\", \"temporary\", \"{\", identifier".to_string(), notes: vec![] },
Diagnostic { loc: File(0, 441, 442), level: Error, ty: ParserError, message: r#"unrecognised token '4', expected "(", "case", "default", "leave", "revert", "switch", identifier"#.to_string(), notes: vec![] },
Diagnostic { loc: File(0, 460, 461), level: Error, ty: ParserError, message: "unrecognised token '!', expected \";\", \"case\", \"constant\", \"default\", \"external\", \"immutable\", \"internal\", \"leave\", \"override\", \"payable\", \"private\", \"public\", \"pure\", \"return\", \"returns\", \"revert\", \"switch\", \"view\", \"virtual\", \"{\", identifier".to_string(), notes: vec![] },
Diagnostic { loc: File(0, 482, 483), level: Error, ty: ParserError, message: "unrecognised token '3', expected \"!=\", \"%\", \"%=\", \"&\", \"&&\", \"&=\", \"(\", \"*\", \"**\", \"*=\", \"+\", \"++\", \"+=\", \"-\", \"--\", \"-=\", \".\", \"/\", \"/=\", \";\", \"<\", \"<<\", \"<<=\", \"<=\", \"=\", \"==\", \">\", \">=\", \">>\", \">>=\", \"?\", \"[\", \"^\", \"^=\", \"calldata\", \"case\", \"default\", \"leave\", \"memory\", \"revert\", \"storage\", \"switch\", \"{\", \"|\", \"|=\", \"||\", identifier".to_string(), notes: vec![] },
Expand Down
Loading
Loading