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

Gno Studio Connect: issues #2197

Open
MalekLahbib opened this issue May 25, 2024 · 9 comments
Open

Gno Studio Connect: issues #2197

MalekLahbib opened this issue May 25, 2024 · 9 comments
Labels
gno-studio Issues related to Gno Studio

Comments

@MalekLahbib
Copy link
Contributor

MalekLahbib commented May 25, 2024

Description

As seen last Thursday on the "office hours" call with @leohhhn @michelleellen and @iam-agf, when testing Gno Studio Connect, we noticed with the example Leon suggested "wugnot" that the function Deposit is called without any args but is supposed to be a function that the user calls to deposit (send) coins to the contract.

So may be it's necessary to modify the code so it detects such type of functions and allow user to send coins using Gno Studio Connect.

@leohhhn leohhhn changed the title Gno Studio: bug report? Gno Studio Connect: issues May 26, 2024
@leohhhn
Copy link
Contributor

leohhhn commented May 26, 2024

@ilgooz @salmad3

To clarify, we need to add a field for each public function in a realm which will allow users to specify an OrigSend value to their call. Since we do not have a way to specify if a function can or cannot take Coins (unlike Ethereum's payable keyword - in Gno, if they're exported, they can receive coins), we should have this field for every function. OrigSend should take in a a coin denom & amount, which will then later be packed in along with the transaction.

Since the studio/connect repo is not public, what would be a good place for the team & contributors to leave feedback on the UI/UX of Connect?

@MalekLahbib
Copy link
Contributor Author

@leohhhn @ilgooz @salmad3 is there a way to detect that a function is a "payable" function as in solidity?

@leohhhn
Copy link
Contributor

leohhhn commented May 28, 2024

No - every exported function is by default "payable". Gno is different from Solidity, and we want to keep as close as possible to Go syntax.

A consideration would be to add a std.Payable() function which would be put at the beginning of the function you want to be payable. Same thing could be done for "view-only" functions, but not sure what others think. IMHO this would make it much clear what functions are meant for.

cc @moul

@moul
Copy link
Member

moul commented May 28, 2024

I think we should start by playing with standardized comments that can be read both by humans and machines (gnoweb, wallets, SDKs).

Maybe something like this:

// Foo does foo things.
//
// @payable
// @...
func Foo() {}

@leohhhn
Copy link
Contributor

leohhhn commented May 28, 2024

@moul

I see two issues with this:

  1. If this is included in comments, people will forget/will not know to write this
  2. This is not enforceable on a VM level - meaning people will still be able to make mistakes, wrongly calling functions.

I think adding a std.Payable() and std.View() or std.Readonly() will help Gnomes enforce strict rules while not steering away from Go syntax. We can think about naming & in which package to put these functions in later. This also covers both of your points where it's readable by both humans and machines.

The above is Ethereum's way. I researched a bit on Solana & Aptos (Move lang), and they follow a different approach. To summarize, they have a built-in function that reverts if its argument, the amount of tokens required, is not available to be pulled from the callers balance - offloading the burden of sending coins from the user to the developer. Seems that Ethereum's way is much more approachable, considering how we manage our transaction context.

@moul
Copy link
Member

moul commented May 28, 2024

If this is included in comments, people will forget/will not know to write this

I think that as soon as it becomes a standard, it can become more common, and we can use linters. That's what go is doing. https://tip.golang.org/doc/comment

It's also easy to forget std.Payable().

In general, I believe that doc comments are more frequently read than the first lines of a function implementation for things that configure the metadata of a function. Personally, I usually examine the function signature and comments before considering reading the actual code.

This is not enforceable on a VM level - meaning people will still be able to make mistakes, wrongly calling functions.

I don't understand your point about mistakes.

Anyway, I don't understand the connection to the VM level if the goal is to generate better documentation for payable functions. It would make sense to have Go helpers if this also benefits other contracts. However, if this is solely for generating documentation when calling a function as an entry point, I don't see a need for the VM to be aware.

... they have a built-in function that reverts if its argument, the amount of tokens required ...

That's already doable by checking the std.OrigSend. We can eventually make a Assert helper.


I suggest you write some example contracts or pseudocode so we can review something and decide more easily. There was a very old issue about this concept, if I recall correctly, which was named "rules".

Edit: #688, #301

@salmad3
Copy link
Member

salmad3 commented May 28, 2024

@ilgooz @salmad3

To clarify, we need to add a field for each public function in a realm which will allow users to specify an OrigSend value to their call. Since we do not have a way to specify if a function can or cannot take Coins (unlike Ethereum's payable keyword - in Gno, if they're exported, they can receive coins), we should have this field for every function. OrigSend should take in a a coin denom & amount, which will then later be packed in along with the transaction.

Since the studio/connect repo is not public, what would be a good place for the team & contributors to leave feedback on the UI/UX of Connect?

Thanks @leohhhn @MalekLahbib @moul

May we add the feedback using the feedback realm?

Just to recap, the suggestion for Connect is to add an OrigSend field to the interface (by default), allowing users to specify the coin denomination and amount directly when calling functions, since all exported functions in Gno are inherently payable. We could also use standardized comments to clarify the functions' purposes and how they handle coin transfers. Is this correct?

I thought of a suggestion by Alan related to a somewhat related issue (Clarifying Acceptable Argument Values for Function Execution) - to use godocs to create a specific comment structure that provides information on the arguments, so perhaps there is a way to enforce the comments for payable functions.

@ilgooz
Copy link
Contributor

ilgooz commented Jun 9, 2024

@jeronimoalbi

@jeronimoalbi
Copy link
Member

It should be easy to add a payable field to functions as part of the schema generation, I'm going to explore that change. With that Connect could easily identify the case and allow users to send coins.

@Kouteki Kouteki added the gno-studio Issues related to Gno Studio label Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gno-studio Issues related to Gno Studio
Projects
None yet
Development

No branches or pull requests

7 participants