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

fix(sdk/vm): improve MsgCall panic error message for wrong number of args #1610

Merged

Conversation

jefft0
Copy link
Contributor

@jefft0 jefft0 commented Feb 1, 2024

If a realm function Call has too many arguments, VMKeeper Call panics with an uninformative index error message like "index out of range [1] with length 1" when it is checking the arguments. One of our devs lost time trying to figure this out, as would other devs presumably.

This PR adds explicit checks for number of arguments with informative panic messages like "not enough arguments in call to Echo" or "too many arguments in call to Echo" . (These are the same errors as the Go compiler.)

(I couldn't find an existing issue for this. If you already have other plans to improve this error message, then you can close this PR.)

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 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.

…mber of arguments

Signed-off-by: Jeff Thompson <jeff@thefirst.org>
@jefft0 jefft0 requested a review from moul as a code owner February 1, 2024 08:57
@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Feb 1, 2024
@jefft0 jefft0 changed the title chore: In VMKeeper Call, panic with useful error message for wrong number of args chore: In VMKeeper Call, panic with informative message for wrong number of args Feb 1, 2024
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.

greatly appreciated!

I think we can be more explicit about how many args we want, so here is a better version imo.

(note: I wondered what happened if we had a variadic function; turns out we don't support these in maketx call currently, though it would be nice if we did :) going to make an issue)

gno.land/pkg/sdk/vm/keeper.go Outdated Show resolved Hide resolved
@thehowl thehowl changed the title chore: In VMKeeper Call, panic with informative message for wrong number of args fix(sdk/vm): improve MsgCall panic error message for wrong number of args Feb 1, 2024
@jefft0
Copy link
Contributor Author

jefft0 commented Feb 1, 2024

I think we can be more explicit about how many args we want, so here is a better version imo.

Looks good. I clicked "Add suggestion to batch".

I wondered what happened if we had a variadic function

You probably saw that this check for the number of arguments comes just after excluding variadics.

panic("variadic calls not yet supported")

jefft0 and others added 2 commits February 1, 2024 12:16
Co-authored-by: Morgan <git@howl.moe>
Signed-off-by: Jeff Thompson <jeff@thefirst.org>
@jefft0
Copy link
Contributor Author

jefft0 commented Feb 1, 2024

I also updated the test to use the revised expected panic message.

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7382d7c) 55.79% compared to head (52085e4) 56.05%.
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1610      +/-   ##
==========================================
+ Coverage   55.79%   56.05%   +0.26%     
==========================================
  Files         436      438       +2     
  Lines       66168    66720     +552     
==========================================
+ Hits        36917    37400     +483     
- Misses      26360    26420      +60     
- Partials     2891     2900       +9     
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 merged commit 059a3b8 into gnolang:master Feb 7, 2024
180 of 181 checks passed
leohhhn pushed a commit to leohhhn/gno that referenced this pull request Feb 8, 2024
…args (gnolang#1610)

If a realm function Call has too many arguments, VMKeeper Call panics
with an uninformative index error message like "index out of range [1]
with length 1" when it is [checking the
arguments](https://github.com/gnolang/gno/blob/12b4b458e1b13d491a5797aa11b2002242f012bd/gno.land/pkg/sdk/vm/keeper.go#L244).
One of our devs lost time trying to figure this out, as would other devs
presumably.

This PR adds explicit checks for number of arguments with informative
panic messages like "not enough arguments in call to Echo" or "too many
arguments in call to Echo" . (These are the same errors as the Go
compiler.)

(I couldn't find an existing issue for this. If you already have other
plans to improve this error message, then you can close this PR.)

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation not needed
- [x] 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](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Signed-off-by: Jeff Thompson <jeff@thefirst.org>
Co-authored-by: Morgan <git@howl.moe>
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.

3 participants