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)!: only allow using msgcall on realms #2242

Merged
merged 8 commits into from
May 31, 2024

Conversation

thinhnx-var
Copy link
Contributor

@thinhnx-var thinhnx-var commented May 30, 2024

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.

Follow discussion in #2192 and TheHowl's comment, this PR aims to prohibit the use of maketx call with a p/ entrypoint and force maketx call to call to a gno.land/ pkgpath

Behavior:

$ gnokey maketx call -broadcast -pkgpath gno.land/p/ -gas-wanted 10000000 -gas-fee 1000000ugnot -func Demo -remote localhost:26657 testKey

--= Error =--
Data: forbidden/bad package called
Msg Traces:
    0  ....... deliver transaction failed: log:
--= /Error =--
$ gnokey maketx call -broadcast -pkgpath gno.land/p -gas-wanted 10000000 -gas-fee 1000000ugnot -func Demo -remote localhost: 26657 testKey

--= Error =--
Data: forbidden/bad package called
Msg Traces:
    0  ....... deliver transaction failed: log:
--= /Error =--

For stdlibs, force the pkgpath to begins with gno.land/

$ gnokey maketx call -broadcast -pkgpath strconv -gas-wanted 10000000 -gas-fee 1000000ugnot -func Itoa -args 11 testKey

--= Error =--
Data: forbidden/bad package called
Msg Traces:
    0  ....... deliver transaction failed: log:
--= /Error =--

@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label May 30, 2024
@thinhnx-var thinhnx-var force-pushed the feat_gnovm_purify_p_more branch from defc250 to 4bf6b46 Compare May 31, 2024 04:44
@thinhnx-var thinhnx-var requested review from zivkovicmilos and a team as code owners May 31, 2024 04:44
@thinhnx-var thinhnx-var changed the title feat(gnokey) Prohibit the use of maketx call with a p/ entrypoint. feat(gnokey): Prohibit the use of maketx call with a p/ entrypoint. May 31, 2024
Copy link

codecov bot commented May 31, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 54.62%. Comparing base (9c60a23) to head (2fbae1f).
Report is 1 commits behind head on master.

Files Patch % Lines
gno.land/pkg/sdk/vm/msgs.go 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2242      +/-   ##
==========================================
- Coverage   54.62%   54.62%   -0.01%     
==========================================
  Files         578      578              
  Lines       77808    77811       +3     
==========================================
+ Hits        42506    42507       +1     
- Misses      32132    32134       +2     
  Partials     3170     3170              
Flag Coverage Δ
gno.land 61.64% <0.00%> (-0.03%) ⬇️
gnovm 59.98% <ø> (+<0.01%) ⬆️

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 review from piux2 and deelawn as code owners May 31, 2024 14:58
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label May 31, 2024
@thehowl thehowl changed the title feat(gnokey): Prohibit the use of maketx call with a p/ entrypoint. feat(sdk/vm): only allow using msgcall on realms May 31, 2024
@thehowl thehowl changed the title feat(sdk/vm): only allow using msgcall on realms feat(sdk/vm)!: only allow using msgcall on realms May 31, 2024
@thehowl thehowl merged commit 86f5624 into gnolang:master May 31, 2024
49 of 51 checks passed
DIGIX666 pushed a commit to DIGIX666/gno that referenced this pull request Jun 2, 2024
<!-- please provide a detailed description of the changes made in this
pull request. -->

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

- [ ] 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](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

Follow discussion in gnolang#2192 and [TheHowl's
comment](gnolang#1538 (comment)),
this PR aims to prohibit the use of `maketx call` with a `p/` entrypoint
and force `maketx call` to call to a `gno.land/` pkgpath

Behavior:
```
$ gnokey maketx call -broadcast -pkgpath gno.land/p/ -gas-wanted 10000000 -gas-fee 1000000ugnot -func Demo -remote localhost:26657 testKey

--= Error =--
Data: forbidden/bad package called
Msg Traces:
    0  ....... deliver transaction failed: log:
--= /Error =--
```

```
$ gnokey maketx call -broadcast -pkgpath gno.land/p -gas-wanted 10000000 -gas-fee 1000000ugnot -func Demo -remote localhost: 26657 testKey

--= Error =--
Data: forbidden/bad package called
Msg Traces:
    0  ....... deliver transaction failed: log:
--= /Error =--
```

For `stdlibs`, force the pkgpath to begins with `gno.land/`
```
$ gnokey maketx call -broadcast -pkgpath strconv -gas-wanted 10000000 -gas-fee 1000000ugnot -func Itoa -args 11 testKey

--= Error =--
Data: forbidden/bad package called
Msg Traces:
    0  ....... deliver transaction failed: log:
--= /Error =--
```

---------

Co-authored-by: Morgan Bazalgette <[email protected]>
omarsy pushed a commit to TERITORI/gno that referenced this pull request Jun 3, 2024
<!-- please provide a detailed description of the changes made in this
pull request. -->

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

- [ ] 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](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

Follow discussion in gnolang#2192 and [TheHowl's
comment](gnolang#1538 (comment)),
this PR aims to prohibit the use of `maketx call` with a `p/` entrypoint
and force `maketx call` to call to a `gno.land/` pkgpath

Behavior:
```
$ gnokey maketx call -broadcast -pkgpath gno.land/p/ -gas-wanted 10000000 -gas-fee 1000000ugnot -func Demo -remote localhost:26657 testKey

--= Error =--
Data: forbidden/bad package called
Msg Traces:
    0  ....... deliver transaction failed: log:
--= /Error =--
```

```
$ gnokey maketx call -broadcast -pkgpath gno.land/p -gas-wanted 10000000 -gas-fee 1000000ugnot -func Demo -remote localhost: 26657 testKey

--= Error =--
Data: forbidden/bad package called
Msg Traces:
    0  ....... deliver transaction failed: log:
--= /Error =--
```

For `stdlibs`, force the pkgpath to begins with `gno.land/`
```
$ gnokey maketx call -broadcast -pkgpath strconv -gas-wanted 10000000 -gas-fee 1000000ugnot -func Itoa -args 11 testKey

--= Error =--
Data: forbidden/bad package called
Msg Traces:
    0  ....... deliver transaction failed: log:
--= /Error =--
```

---------

Co-authored-by: Morgan Bazalgette <[email protected]>
Comment on lines +52 to +59
## remove due to update to maketx call can only call realm (case 5, 6, 13)
## 5. MsgCall -> p/demo/bar.A -> myrlm.A: user address
gnokey maketx call -pkgpath gno.land/p/demo/bar -func A -gas-fee 100000ugnot -gas-wanted 2000000 -broadcast -chainid tendermint_test test1
stdout ${USER_ADDR_test1}
## gnokey maketx call -pkgpath gno.land/p/demo/bar -func A -gas-fee 100000ugnot -gas-wanted 2000000 -broadcast -chainid tendermint_test test1
## stdout ${USER_ADDR_test1}

## 6. MsgCall -> p/demo/bar.B -> myrlm.B -> r/foo.A: user address
gnokey maketx call -pkgpath gno.land/p/demo/bar -func B -gas-fee 100000ugnot -gas-wanted 2000000 -broadcast -chainid tendermint_test test1
stdout ${USER_ADDR_test1}
## gnokey maketx call -pkgpath gno.land/p/demo/bar -func B -gas-fee 100000ugnot -gas-wanted 2000000 -broadcast -chainid tendermint_test test1
## stdout ${USER_ADDR_test1}
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess these should've been removed, as well as in the other txtar

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 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants