feat!: forbid importing '/r' from '/p'

[harry-hov]

Packages shouldn't be importing realms!

Depends on:

1. refactor!: split r/demo/users #1433
2. refactor!: split r/demo/boards #1989

[codecov]

Codecov Report

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

Project coverage is 45.64%. Comparing base (56b5bc0) to head (3fdb303).
Report is 2 commits behind head on master.

Files	Patch %	Lines
tm2/pkg/std/memfile.go	80.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1393      +/-   ##
==========================================
- Coverage   48.38%   45.64%   -2.75%     
==========================================
  Files         409      445      +36     
  Lines       62035    66249    +4214     
==========================================
+ Hits        30018    30241     +223     
- Misses      29517    33512    +3995     
+ Partials     2500     2496       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[albttx]

Why would we want that ? Just for security issues ?

I would suggest something else, since calling realm from package could be really useful !

For example, in the case of the merkle-airdrop contract i wrote. there is a package p/demo/airdrop/merkle-airdrop-grc20 that you call from your realm and you send it a grc20.IGRC20 (eg: foo20.IGRC20()).

I already propose couple time ago to extends the gno.mod file with something like a realm "whitelist" which will allow the realms to be called and their versions once we will have versioning solutions.

[moul]

I believe it's the ideal location for early arrivals, package uploads, and early cleanups. @piux2, could you please verify?

[piux2]

In the Gno language environment, ParseMemPackage() in gnolang/nodes.go is called in six distinct scenarios, each with different contexts. These scenarios can be divided into three main categories: blockchain application, file testing, and local development.

Blockchain Application:

This involves the Gno language as an application running on the GnoLand blockchain. There are two specific scenarios under this category:

1. gno.land start
   When this command is executed, the Gno virtual machine (VM) only reads the gnovm/stdlibs contracts from the local file system.
   Instead of loading everything into memory, it initializes a package getter and loads packages during runtime as needed.

2. gnokey maketx addpkg
   In this case, the VM parses contracts loaded from the GnoLand app store on-chain.
   All non-stdlib contracts (identified as "p" and "r") in the Gno app store are derived from the Tendermint Consensus and genesis transactions.

File Testing:

This scenario involves loading packages into a machine instance from a testing store, without involving blockchain applications. Packages can be loaded from the local file system.

3. go test ./tests -run '^TestFiles$'

import.TestStore() is used.

Gno Local Development and Test:

4. gno run imports.TestStore() is used.

5. gno repl imports.TestStore() is used.

6. gno test ./examples

Gno modules (Gno mod) are used to facilitate local development and testing. They are not entities on the blockchain.
The goal is to avoid mixing the logic of on-chain production, local development, and testing within the gnolang/nodes.go functions. Each scenario handles the parsing and loading of packages differently. therefore the logic should be implemted outside of the pure gnolang function in a language pkg for the specific needs.

graph TD
    A[1 'gno.land start'] -->AB[makeGenesis] -->B[NewApp]
    B --> C[VMKeeper.Initialize]
    C --> D[m.PreprocessAllFilesAndSaveBlockNodes]
    D --> E[gnolang/nodes.go: ParseMemPackage]

    F[2 'gnokey maketx addpkg'] -->|Network| H[tm2.PRC]
    H --> I[vmHandler]
    I --> J[VMKeeper.AddPackage]
    J --> U[m.RunMemPackage] 

    K[3 'go test ./tests -run '^TestFiles$''] --> L[RunFileTest]
    L --> U 

    M[4 'gno run'] --> N[import.TestStore]
    N --> U 

    O[5 'gno repl'] --> P[import.TestStore]
    P --> U 

    Q['6 gno test ./examples'] --> R[execTest]
    R -->S[gnoTestPkg]
    S --> U 
    U --> E

     style A stroke:#1548c6,stroke-width:2px
     style F stroke:#1548c6,stroke-width:2px
     style K stroke:#111111,stroke-width:2px
     style M stroke:#111111,stroke-width:2px
     style O stroke:#111111,stroke-width:2px
     style Q stroke:#111111,stroke-width:2px
   

     style E stroke:#b548c6,stroke-width:2px
     style U stroke:#b548c6,stroke-width:2px
     style D stroke:#b548c6,stroke-width:2px
     
 

Loading

[harry-hov]

Here's the list of pkgs importing realm:

gno.land/p/demo/grc/grc1155 imports:
        - gno.land/r/demo/users
gno.land/p/demo/grc/grc721 imports:
        - gno.land/r/demo/users
gno.land/p/demo/groups imports:
        - gno.land/r/demo/boards
gno.land/p/demo/microblog imports:
        - gno.land/r/demo/users
gno.land/p/demo/tests imports:
        - gno.land/r/demo/tests
panic: stop

Should I mark them as Draft until we figure out alternate way to call realm methods in pkgs?

cc/ @moul @piux2 @albttx

[moul]

• r/demo/users shoudl have a p/demo/users with interfaces
• p/ should import p/ for interfaces instead of r/ with the implementation OR just remove the import (r/users is probably useless in grc)

[thehowl]

So, I've taken a look at this and tried to figure out where best this check could sit.

Where current validation about import paths resides is in tm2/pkg/std/memfile.go. It seems obvious to me that this is less than ideal. The MemPackage/MemFile types themselves already should probably not sit within tm2, but more importantly the Validate function should not sit in there. And this is even more apparent when taking a look at its callers:

gno.land/pkg/gnoland/genesis.go|108 col 20| if err := memPkg.Validate(); err != nil {
gno.land/pkg/sdk/vm/keeper.go|154 col 24| if err := msg.Package.Validate(); err != nil {
gno.land/pkg/sdk/vm/keeper.go|310 col 24| if err := msg.Package.Validate(); err != nil {
gnovm/pkg/gnolang/store.go|529 col 9| memPkg.Validate() // NOTE: duplicate validation.

It's always called internally within gno.land already, and even when it is called outside of gno.land it does absolutely nothing.

So, my proposed plan of action is the following:

• Move the Validate function within gno.land/pkg/sdk/vm
• Add to it a call to Go's parser, only getting its imported packages
• Verify that if it is a package it is only calling other packages

[thehowl]

@albttx

Why would we want that ? Just for security issues ?

I would suggest something else, since calling realm from package could be really useful !

For example, in the case of the merkle-airdrop contract i wrote. there is a package p/demo/airdrop/merkle-airdrop-grc20 that you call from your realm and you send it a grc20.IGRC20 (eg: foo20.IGRC20()).

I already propose couple time ago to extends the gno.mod file with something like a realm "whitelist" which will allow the realms to be called and their versions once we will have versioning solutions.

The restriction really is on importing rather than calling.

I think it makes a ton of sense to forbid importing from a package. A package is stateless, and importing a package and running its functions should not have side-effects unless clearly specified.

Remember that the "realm" is considered an entity, which has its own coins in the banker and can send them to other realms. It can make calls to other realms which will identify the calls as coming from it through PrevRealm.

So it is important that "important" actions like calling other realms and using the banker are strictly authorised within the realm's source code. (The banker can currently be freely called by a package, I'm pretty sure, but I made a note that this should be fixed eventually).

That said, if the realm does trust a package, it can very freely give the keys to its heart...

package myrlm

import (
	"std"
	"strings"
	"gno.land/p/demo/mypkg"
	"gno.land/r/demo/boards"
	pboards "gno.land/p/demo/boards"
)

func LendBanker() {
	bnk := std.GetBanker(std.BankerTypeRealmSend)
	// mypkg can now freely send coins from the realm's bank
	mypkg.Lend(bnk)
}

func LendCall() {
	// now mypkg can call boards.CreatePost because it was passed by a closure!
	mypkg.LendCall(boards.CreatePost)
	
	// but it can even decide not to "trust" mypkg...
	mypkg.LendCall(func(x pboards.Post) {
		if strings.Contains(strings.ToLower(x.Content), "ethereum") {
			panic("bad package! >:(")
		}

		boards.CreatePost(x)
	})
}

This feature we're talking about is not about forbidding calls from packages; but rather making sure that what they do is not stateful unless explicitly authorised by the realm. It allows realm authors to know that while they always have to audit their dependencies, they can at least be sure their dependencies can never accidentally bankrupt them.

[Kouteki]

Let's find some time during the retreat to discuss this PR