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 float as arguments to maketx call #1434

Merged
merged 7 commits into from
Feb 1, 2024

Conversation

deelawn
Copy link
Contributor

@deelawn deelawn commented Dec 12, 2023

Addresses #1427

This allows passing in float types to contract functions

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@deelawn deelawn requested a review from a team as a code owner December 12, 2023 12:08
@deelawn deelawn linked an issue Dec 12, 2023 that may be closed by this pull request
@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Dec 12, 2023
Copy link

codecov bot commented Dec 12, 2023

Codecov Report

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

Comparison is base (d6f0cde) 56.24% compared to head (5db1525) 56.04%.

Files Patch % Lines
gno.land/pkg/sdk/vm/convert.go 0.00% 30 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1434      +/-   ##
==========================================
- Coverage   56.24%   56.04%   -0.21%     
==========================================
  Files         425      439      +14     
  Lines       64428    66182    +1754     
==========================================
+ Hits        36237    37091     +854     
- Misses      25369    26205     +836     
- Partials     2822     2886      +64     
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.

@deelawn
Copy link
Contributor Author

deelawn commented Dec 21, 2023

Discussed with @piux2. Closing due to potential nondeterministic results encoding amino float values:

### Floating points

@deelawn deelawn closed this Dec 21, 2023
@deelawn
Copy link
Contributor Author

deelawn commented Dec 21, 2023

Actually this may not be an issue since the float value is encoded as a string when it is sent. I will discuss with @piux2 and reevaluate.

@deelawn deelawn reopened this Dec 21, 2023
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

Basically, looking at this I'm simply wondering why we're doing this argument parsing at all instead of just executing the function as an expression on the GnoVM. Ie. maketx call -func A -args 1 -args 2, to me should just be executed by the VM Keeper as the expression realm.A(1, 2).

I think it's worth chatting with @jaekwon on what could be a good approach here.

gno.land/pkg/sdk/vm/convert.go Outdated Show resolved Hide resolved
@thehowl thehowl changed the title fix: realm func float args feat(sdk/vm): support float as arguments to maketx call Jan 11, 2024
@thehowl
Copy link
Member

thehowl commented Jan 11, 2024

Had a discussion with jae in review call. Posting the details here; tldr is that maketx call arg parsing should be very simple and not call the machine for anything, and the argument distinction be made very clear.

If, however, the float parsing done in convert.go and the one done by the gnovm are the same (so likely, Apd -> apd.Float32/Float64), then it's probably fine.

Call notes

>>> maketx call -func A -args 1 -args 2 <<<

Current approach

- Get A
- Parse each arg individually with a "custom parser" (convert.go)
- Call A with the TypedValue

Suggested approach

- Get A
- A only accepts "safe types"
- Sanitization of arguments based on <func>
- <func> ( <arg1> , <arg2> )
- Machine.RunExpression(string)

-----------------

run -pkgpath gno.land/r/demo/morgan -expr morgan.A(1, 2.4)

run -file 'package main; import "gno.land/r/demo/morgan"; func main() { morgan.A(1, 2.4) }'

Ledger dev: only passes strings
call: strings, integers, bools? (float -- or we do APD -> BasicLit -> TypedValue // we don't use strconv.ParseFloat)

@deelawn deelawn requested a review from moul as a code owner January 25, 2024 01:45
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

Looks good for now.

I think we can pick it up at one point and optimise it, as you said, to call strconv.ParseFloat directly. In any case, I'd say we go ahead and merge this :)

Copy link
Contributor

@harry-hov harry-hov left a comment

Choose a reason for hiding this comment

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

Just some minor suggestions. Rest LGTM 💯

gno.land/pkg/sdk/vm/convert.go Show resolved Hide resolved
gno.land/pkg/sdk/vm/convert.go Show resolved Hide resolved
gno.land/pkg/sdk/vm/convert.go Outdated Show resolved Hide resolved
@deelawn deelawn merged commit 988ca91 into gnolang:master Feb 1, 2024
181 of 182 checks passed
@deelawn deelawn deleted the bug/float-func-args branch February 1, 2024 22:43
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.

Unexpected primitive type: float32 and float64
4 participants