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

gnoland/gnovm: purify p/ even more #2192

Open
moul opened this issue May 25, 2024 · 5 comments
Open

gnoland/gnovm: purify p/ even more #2192

moul opened this issue May 25, 2024 · 5 comments
Labels
🌟 must have 🌟 Mandatory work needed to complete a project 📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related

Comments

@moul
Copy link
Member

moul commented May 25, 2024

Currently, p/ packages lack a data state (global vars) and cannot import r/, which is great. Yet, there are remaining ambiguities with r/ to resolve for clarity and ensure they are entirely pure and stateless.

Essentially, p/ should be seen as a platform for sharing source code among developers, while r/ is where apps (and some dependencies) reside for users to interact with.

Changes to consider:

  • Prohibit the use of maketx call with a p/ entrypoint.
  • Retain q_eval for now.
  • Trigger a panic if a p/ attempts to create a banker, but allow interaction with an already instantiated banker from r/.
  • Conceal std.DerivePkgPath for p/ and emphasize their unique identifier, PkgPath. Consider using std.GetCallerAt() Frame to prevent p/'s std.Address from appearing in logs and causing confusion.

cc @leohhhn

@moul moul changed the title Purify p/ even more Purify p/ even more May 25, 2024
@moul moul changed the title Purify p/ even more gnoland/gnovm: purify p/ even more May 25, 2024
@leohhhn
Copy link
Contributor

leohhhn commented May 28, 2024

On top of this, we should disable direct calls to Gno stdlibs via maketx call (refrence comment).

@moul, I think its okay that p/ has top-level variables such as constants, since these can be useful. This is currently possible - read about it in this issue.

Closing #2224 in favor of this.

@ltzmaxwell
Copy link
Contributor

@moul, I think its okay that p/ has top-level variables such as constants, since these can be useful. This is currently possible - read about it in #1538.

I think that p/ should be OK to contain package-level variables.

While the following is off-topic:

It's peculiar that you need to constantly remember that this is a p/ and that the variable isn't meant to be persisted, similar to how you must be mindful that it will be persisted when working within a Realm (comment in packages in r/). All these rules are implicit, which can be confusing.

@moul
Copy link
Member Author

moul commented May 28, 2024

For p/, package-level constants are allowed, but variables/state that can change are not. The Go linter should prohibit this.

Do you have examples of package-level variables that would make sense?

@ltzmaxwell
Copy link
Contributor

I mean something like this:

errXXX := errors.New("a common error shares within packages").

var ErrLimit = errors.New("flowrate: flow rate limit exceeded")

but of course, we actually don't want to modify it.

@Kouteki Kouteki added 📦 🤖 gnovm Issues or PRs gnovm related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels May 31, 2024
thehowl added a commit that referenced this issue May 31, 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 #2192 and [TheHowl's
comment](#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 <morgan@morganbaz.com>
DIGIX666 pushed a commit to DIGIX666/gno that referenced this issue 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 <morgan@morganbaz.com>
omarsy pushed a commit to TERITORI/gno that referenced this issue 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 <morgan@morganbaz.com>
@leohhhn
Copy link
Contributor

leohhhn commented Jun 5, 2024

Prohibit the use of maketx call with a p/ entrypoint.

Completed in #2242. We could make the error a bit more informative: invalid package path

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟 must have 🌟 Mandatory work needed to complete a project 📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: 📥 Inbox
Status: Backlog
Development

No branches or pull requests

5 participants