hyperledger / solang

Solidity Compiler for Solana and Polkadot
https://solang.readthedocs.io/
Apache License 2.0
1.23k stars 206 forks source link

Add function annotations 🚀 #1557

Closed LucasSte closed 9 months ago

LucasSte commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

@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?

codecov[bot] commented 9 months ago

Codecov Report

:exclamation: 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