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

feat(sdk/vm): support return values from main in msgrun #1591

Closed
wants to merge 2 commits into from

Conversation

thehowl
Copy link
Member

@thehowl thehowl commented Jan 26, 2024

Something I wondered about what we could do with MsgRun. I saw that in the current implementation, the output that is returned to the user is the output of calls to the "Output", ie. println. I don't think we should support this OR endorse any usage of println as a stable function in production code; hence this PR.

This PR adds support for adding return values to the main() function in MsgRun. It substitutes the current way to get data back from a MsgRun call -- println -- to using return values, as we do for MsgCall. This creates an inconsistency with the "model" of inspiration for MsgRun -- ie. gno run -- but I think it is the right call.

Let's discuss!

@thehowl thehowl added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Jan 26, 2024
@thehowl thehowl self-assigned this Jan 26, 2024
@thehowl thehowl requested a review from moul as a code owner January 26, 2024 15:19
@thehowl
Copy link
Member Author

thehowl commented Jan 26, 2024

Example of execution:

$ cat sos.gno
package main

func main() int {
	return 11
}
$ gnokey maketx run -broadcast -gas-wanted 1000000 -gas-fee 1ugnot gentest sos.gno
Enter password.
(11 int)
OK!
GAS WANTED: 1000000
GAS USED:   65914

Copy link

codecov bot commented Jan 26, 2024

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (b619351) 55.80% compared to head (52d8b0d) 55.79%.
Report is 1 commits behind head on master.

Files Patch % Lines
gno.land/pkg/sdk/vm/keeper.go 57.14% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1591      +/-   ##
==========================================
- Coverage   55.80%   55.79%   -0.01%     
==========================================
  Files         436      436              
  Lines       66168    66169       +1     
==========================================
- Hits        36922    36920       -2     
- Misses      26356    26358       +2     
- Partials     2890     2891       +1     
Flag Coverage Δ
go-1.21.x ∅ <ø> (∅)
misc ∅ <ø> (∅)
misc-_test.genstd ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thehowl thehowl requested a review from gfanton as a code owner January 26, 2024 16:27
@thehowl
Copy link
Member Author

thehowl commented Jan 29, 2024

Reporting what we discussed in our weekly today:

@moul thinks that this PR is not an improvement, as he sees MsgRun first and foremost as a tool for humans and a tool that should have its closest equivalent to Go's main(), whereby unlike C it doesn't take arguments or return values, and that is what a Go developer expects.

This has a few major issues, in my opinion:

  • It creates a notion of an "output" on the blockchain, which before MsgRun was possible but never visible to any end user (it was just printed to the node stdout, and still is on MsgCall)
  • Consequently, it makes the printing format of println something that the users will depend upon, even though this should definitely not be depended on.
  • ... especially because the buffer, effectively, can be written on by any function being called by Run, even maliciously.
    • ie. the caller of Run reads the first JSON object printed in the result, but when calling run which calls println(json.Marshal(myrlm.A())), myrlm.A is actually maliciously updated and now outputs an object of his own, hijacking the output.

Consequently, we convened that:

  • There should be a notion of "output" (ie. "stdout"), but this is meant for debugging purposes and not something that we should encourage users to rely on.
  • We can create consistency in our calls by allowing each of call, run, addpkg to have both an output and a return value.
  • There is a larger feature which can be adopted by MsgRun, and that is specifying an expression to be executed, similar to gno run -expr
    • ie maketx run -expr 'Hello()' -code 'package main; func Hello() int { return 42 }. In this case it makes sense that the user can specify a return value; however, if main() were still the only function allowed to run in maketx run and it did not support println + allowed return values, it would deviate too much from what programmers think as familiar with main().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

1 participant