-
Notifications
You must be signed in to change notification settings - Fork 375
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
Conversation
Codecov ReportAttention:
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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. |
There was a problem hiding this 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.
maketx call
Had a discussion with jae in review call. Posting the details here; tldr is that If, however, the float parsing done in Call notes
|
There was a problem hiding this 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 :)
There was a problem hiding this 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 💯
Co-authored-by: Hariom Verma <hariom18599@gmail.com>
Addresses #1427
This allows passing in float types to contract functions
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description