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

Add function annotations 🚀 #1557

Merged
merged 3 commits into from
Oct 13, 2023
Merged

Conversation

LucasSte
Copy link
Contributor

@LucasSte LucasSte commented Oct 3, 2023

This PR introduces account annotations for developers to declare accounts in a Solidity function. This is one of the task items in #1251.

The new syntax tremendously facilitates writing contracts and is less error prone as we do not need to both pass the account address as an argument and in the accounts field.

I wrote some tests where I noticed they were necessary and I attempted to refactor all existing Solana Solidity to use the annotations, so that my implementation would be stress tested in all those cases.

There are a few items this PR did not accomplish, but are going to be implemented in future changes:

  1. I did not validate accounts during runtime, i.e. I am not checking if the bits are properly set before dispatching the function, but I wonder if this is really needed.
  2. When generating the interface for an IDL, I am not creating the account annotations, but this is an amazing feature to be added soon.
  3. The address.balance function is useless on Solana, but has not been eliminated yet. We can now just do tx.accounts.myAccount.lamports.
  4. The address.send and address.transfer are also not necessary, but I did not purge the compiler of them.

@LucasSte LucasSte changed the title Add function annotations Add function annotations 🚀 Oct 3, 2023
@LucasSte LucasSte added the solana The Solana target label Oct 3, 2023
@LucasSte LucasSte marked this pull request as ready for review October 3, 2023 19:52
Copy link
Contributor

@seanyoung seanyoung left a comment

Choose a reason for hiding this comment

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

How the annotations work with constructors and virtual functions?

solang-parser/src/solidity.lalrpop Outdated Show resolved Hide resolved
src/sema/ast.rs Outdated Show resolved Hide resolved
src/sema/ast.rs Show resolved Hide resolved
stdlib/solana.c Show resolved Hide resolved
src/emit/binary.rs Outdated Show resolved Hide resolved
src/emit/binary.rs Outdated Show resolved Hide resolved
src/emit/solana/mod.rs Outdated Show resolved Hide resolved
Comment on lines +1064 to +1114
.context
.struct_type(
&[
binary
.module
.get_struct_type("struct.SolPubkey")
.unwrap()
.ptr_type(AddressSpace::default())
.as_basic_type_enum(),
binary
.llvm_type(&Type::Struct(StructType::AccountMeta), ns)
.ptr_type(AddressSpace::default())
.as_basic_type_enum(),
binary.context.i64_type().as_basic_type_enum(),
binary
.context
.i8_type()
.ptr_type(AddressSpace::default())
.as_basic_type_enum(),
binary.context.i64_type().as_basic_type_enum(),
],
false,
)
.as_basic_type_enum();
Copy link
Contributor

Choose a reason for hiding this comment

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

declare_externals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there is a way to add struct definitions to the module.

Copy link
Contributor

Choose a reason for hiding this comment

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

There must be a way to types to a module, how else would you build a complete IR for a file.

Having said that, I don't know how it is done. I can't find it either

integration/solana/calls.spec.ts Show resolved Hide resolved
docs/language/functions.rst Outdated Show resolved Hide resolved
@LucasSte
Copy link
Contributor Author

LucasSte commented Oct 4, 2023

How the annotations work with constructors and virtual functions?

As constructors are just like any other function on Solana, the annotations work as in an external function. A test case for constructors is missing, though.

I missed the case about virtual functions. We should either disallow annotations or require that the overriding function repeat the account declaration (I'd extend these rules to interfaces). Do you have any opinion on the subject?

@LucasSte LucasSte force-pushed the rsm-acc branch 2 times, most recently from 63d035c to fd6b43c Compare October 6, 2023 22:13
@LucasSte
Copy link
Contributor Author

LucasSte commented Oct 9, 2023

@seanyoung I am requiring now that overriding functions declared the same accounts as their respective virtual ones. I may not have conceived the most ideal error message, though. Can you please check if you like it?

src/sema/contracts.rs Outdated Show resolved Hide resolved
src/sema/contracts.rs Outdated Show resolved Hide resolved
src/sema/contracts.rs Outdated Show resolved Hide resolved
src/sema/contracts.rs Outdated Show resolved Hide resolved

res = await callee.program.methods.getX()
.accounts({ dataAccount: callee.storage.publicKey })
.view();
.view({commitment: "confirmed"});
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than providing this for each call, I think this can be set once for the connection as a second argument to AnchorProvider.local() in setup.ts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding {commitment: "confirmed"} to AnchorProvider.local causes all tests to fail.

Copy link
Contributor

@xermicus xermicus left a comment

Choose a reason for hiding this comment

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

LGTM

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

codecov bot commented Oct 12, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@817831a). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1557   +/-   ##
=======================================
  Coverage        ?   87.33%           
=======================================
  Files           ?      133           
  Lines           ?    64143           
  Branches        ?        0           
=======================================
  Hits            ?    56021           
  Misses          ?     8122           
  Partials        ?        0           

@LucasSte LucasSte merged commit 2c29fd7 into hyperledger-solang:main Oct 13, 2023
19 checks passed
@LucasSte LucasSte deleted the rsm-acc branch October 13, 2023 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solana The Solana target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants