Represent contracts by their program id #1474

LucasSte · 2023-08-04T20:24:06Z

This PR changes the inner representation of Solana contracts from their data account to their program id.
All the changes are listed here: #1430 (except the last item, which will have a dedicated PR).

Constructors will still require an account to be initialized, even if the contract has no storage variables and no function requires a data account. This setting does not influence the overall functionality of Solang, but it is something we must solve as I have described in #1480 .

There are small changes in Polkadot tests, because I found the mutability check to be incomplete. We were not recursing down all expressions, so reads and writes from the state went unnoticed. I fixed this as well.

seanyoung

Not a complete review yet, sorry. I'll get back to this later today.

contract foo {
	function meh() external {
		for (uint64 i = 0; i < 64; i++) {
			X a = new X();
		}
	}
}

@program_id("Foo5mMfYo5RhRcWa4NZ2bwFn4Kdhe8rNK5jchxsKrivA")
contract X {}

Results in:

thread 'main' panicked at 'assertion failed: contract_args.accounts.is_some()', src/emit/solana/target.rs:1268:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

docs/language/contracts.rst

docs/language/managing_values.rst

src/sema/expression/function_call.rs

src/sema/mutability.rs

seanyoung · 2023-08-11T09:17:11Z

src/sema/mutability.rs

-            state.read(loc)
+            state.data_account |= DataAccountUsage::READ;
+            state.read(loc);
+            return false;


why stop here?

If we continue recursing, there would be two mutability errors. One for the subscript and other for the storage variable nested within the subscript expression. Perhaps the best solution here is just to track the variable access instead of the subscript.

Perhaps the best solution here is just to track the variable access instead of the subscript.

There is a test case that fails if I do that. I need to keep the expressions separated and not recurse on on them.

It's fundamentally broken.

contract C { bool x; int[4] a; function test() public view returns (int) { return foo()[1]; } function foo() internal returns (int[4] storage) { x = true; return a; } }

Please, do not say it is broken without testing. The IDL generated for this contract is correct:

{ "name": "test", "accounts": [ { "name": "dataAccount", "isMut": true, "isSigner": false, "isOptional": false } ], "args": [], "returns": "i256" }

The collection of accounts between function calls happens in codegen.

My goal here was not to make mutability check perfect, but make it work for the tests we have while collecting the data account. Not continuing the traversal was an existing problem and its solution is not simple to include in this PR. I am happy to work on this separately.

The point is this: you can't just not traverse parts of the ast when looking for mutability.

Right now, the main branch stops traversing when we match one of the cases in fn read_expression.

I just replicated the main branch's behavior by stopping.

If you want to work on this on separate PR, sure.

If you want to work on this on separate PR, sure.

Thanks! I should make a start on this today or tomorrow. As the problem also exists in the main branch, I believe it deserves a separate piece of work.

I'll track this issue at #1493.

src/sema/mutability.rs

src/bin/solang.rs

src/codegen/dispatch/solana.rs

src/codegen/solana_accounts/account_collection.rs

src/sema/statements.rs

seanyoung · 2023-08-16T10:46:49Z

I've been thinking a lot of about this change. The thing that bother is that contract variables (i.e. Type::Contract(,,)) now only store a fixed value, the program address for the contract.

First of all, we could allow calling contracts without a contract variable:

contract A {
     function test() public {
          int value = B.foo();
     }
}

contract B {
   int a;
   function foo() public returns (int) { return a; }
}

There is a slight complication there: B could be a base contract of A:

contract A is B {
     function test() public {
          int value = B.foo();
     }
}

contract B {
   int a;
   function foo() public returns (int) { return a; }
}

This would default to a call to base contract unless accounts are specified.

Secondly, the contract variable types make no sense. They do not hold useful information. I think they should be disallowed completely, as they are not intuitive on Solana.

LucasSte · 2023-08-16T12:51:43Z

I've been thinking a lot of about this change. The thing that bother is that contract variables (i.e. Type::Contract(,,)) now only store a fixed value, the program address for the contract.

First of all, we could allow calling contracts without a contract variable:
contract A {
     function test() public {
          int value = B.foo();
     }
}

contract B {
   int a;
   function foo() public returns (int) { return a; }
}
There is a slight complication there: B could be a base contract of A:
contract A is B {
     function test() public {
          int value = B.foo();
     }
}

contract B {
   int a;
   function foo() public returns (int) { return a; }
}
This would default to a call to base contract unless accounts are specified.

Secondly, the contract variable types make no sense. They do not hold useful information. I think they should be disallowed completely, as they are not intuitive on Solana.

We still allow contracts to be declared without a program id. In your example, for instance, calling B.foo wouldn't work because we don't know B's program id. Are we going to enforce that all contracts have a declared program id?

Also, what would be the syntax for the constructor?

seanyoung · 2023-08-16T13:23:36Z

We still allow contracts to be declared without a program id. In your example, for instance, calling B.foo wouldn't work because we don't know B's program id. Are we going to enforce that all contracts have a declared program id?

Good point.

Also, what would be the syntax for the constructor?

You can still do new A(), but the result would be Type;:Void -- which is also not intuitive.

LucasSte · 2023-08-16T13:26:29Z

Also, what would be the syntax for the constructor?

You can still do new A(), but the result would be Type;:Void -- which is also not intuitive.

Perhaps, we should get rid of the account management at this point and explicitly require the accounts to be passed when instantiating a contract. Even if new does not return anything, making it clear that it needs an account sounds more intuitive.

LucasSte · 2023-08-31T17:17:19Z

This PR is already large enough (it touches 110 files). I decided to merge this first and open another PR for the new contracts syntax (there will be another one to refactor the mutability check as well). Please, review it when possible @seanyoung and @xermicus.

seanyoung

Looks great, just some nitpicks

src/sema/expression/mod.rs

src/sema/function_annotation.rs

src/sema/expression/constructor.rs

src/sema/variables.rs

docs/language/contracts.rst

src/codegen/solana_accounts/account_collection.rs

xermicus · 2023-09-04T14:21:37Z

src/codegen/solana_accounts/account_management.rs

+    cfg_no: usize,
+    ast_no: usize,
+) {
+    if contracts[contract_no].cfg[cfg_no].blocks.is_empty() {


I'm aware that this is already in main, but it lets me wondering whether it should be an assert! instead. Unless being able to add empty CFGs has a fair reason.

When we have a modifier, an empty CFG is generated.

Sure, I guess an empty function body not returning anything might also add an empty CFG. But this could be detected optimized away.

src/sema/expression/constructor.rs

xermicus · 2023-09-04T14:27:27Z

src/sema/expression/mod.rs

+
+impl ExprContext {
+    pub fn enter_loop(&mut self) {
+        self.loop_counter += 1;


Could use checked arithmetic and error out if too many loops. Also could indicate a compiler bug if that happens.

As the variable is of type usize, Rust will already warn of an overflow. This won't be, however, an error for the user.

src/sema/expression/mod.rs

xermicus

LGTM

Signed-off-by: Lucas Steuernagel <lucas.tnagel@gmail.com>

LucasSte force-pushed the program-id branch 3 times, most recently from 151504a to 4c8695b Compare August 8, 2023 15:22

LucasSte marked this pull request as ready for review August 8, 2023 17:10

LucasSte requested review from seanyoung and xermicus as code owners August 8, 2023 17:10

LucasSte force-pushed the program-id branch from 4c8695b to 49497c2 Compare August 8, 2023 17:22

seanyoung reviewed Aug 11, 2023

View reviewed changes

LucasSte force-pushed the program-id branch from 53f949a to 366fd50 Compare August 14, 2023 14:58

LucasSte requested a review from seanyoung August 14, 2023 15:05

seanyoung reviewed Aug 15, 2023

View reviewed changes

LucasSte force-pushed the program-id branch 2 times, most recently from a427b0d to 307ecd8 Compare August 17, 2023 19:51

LucasSte marked this pull request as draft August 18, 2023 13:25

LucasSte force-pushed the program-id branch 2 times, most recently from 966a62b to 7a9b94f Compare August 31, 2023 15:40

LucasSte marked this pull request as ready for review August 31, 2023 17:16

LucasSte requested a review from seanyoung August 31, 2023 17:16

seanyoung approved these changes Sep 4, 2023

View reviewed changes

xermicus reviewed Sep 4, 2023

View reviewed changes

LucasSte force-pushed the program-id branch from 7a9b94f to db77e61 Compare September 4, 2023 20:36

LucasSte requested a review from xermicus September 4, 2023 21:35

xermicus approved these changes Sep 5, 2023

View reviewed changes

Represent contracts by their program id

aede954

Signed-off-by: Lucas Steuernagel <lucas.tnagel@gmail.com>

LucasSte force-pushed the program-id branch from db77e61 to aede954 Compare September 5, 2023 13:24

LucasSte merged commit d5e7289 into hyperledger:main Sep 5, 2023
16 checks passed

LucasSte deleted the program-id branch September 5, 2023 15:08

LucasSte mentioned this pull request Sep 5, 2023

Remove unnecessary constructors and data accounts from Solana contracts #1480

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Represent contracts by their program id #1474

Represent contracts by their program id #1474

LucasSte commented Aug 4, 2023 •

edited

seanyoung left a comment

seanyoung Aug 11, 2023

LucasSte Aug 11, 2023

LucasSte Aug 11, 2023

seanyoung Aug 15, 2023

LucasSte Aug 15, 2023

LucasSte Aug 15, 2023

LucasSte Aug 15, 2023

seanyoung Aug 16, 2023

LucasSte Aug 16, 2023

LucasSte Aug 17, 2023

seanyoung commented Aug 16, 2023

LucasSte commented Aug 16, 2023

seanyoung commented Aug 16, 2023

LucasSte commented Aug 16, 2023

LucasSte commented Aug 31, 2023

seanyoung left a comment

xermicus Sep 4, 2023

LucasSte Sep 4, 2023

xermicus Sep 5, 2023

xermicus Sep 4, 2023

LucasSte Sep 4, 2023

xermicus left a comment

Represent contracts by their program id #1474

Represent contracts by their program id #1474

Conversation

LucasSte commented Aug 4, 2023 • edited

seanyoung 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

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

seanyoung commented Aug 16, 2023

LucasSte commented Aug 16, 2023

seanyoung commented Aug 16, 2023

LucasSte commented Aug 16, 2023

LucasSte commented Aug 31, 2023

seanyoung 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

xermicus left a comment

Choose a reason for hiding this comment

LucasSte commented Aug 4, 2023 •

edited