Skip to content

Commit

Permalink
feat(gno.land): add go type checking to keeper + tx simulation in gno…
Browse files Browse the repository at this point in the history
…key (#1702)

Split from #1695 for ease of reviewing. Merge order:

1. #1700 
2. #1702 (this one!)
3. #1695 \
   #1730

This PR removes `TranspileAndCheckMempkg` in favour of performing the
type checking it was supposed to do using `go/types` with a custom
importer. This importer works together with Gno's `Store`, and can as
such be used to type check Gno packages without ever writing a single
file to disk. It is important to note that by "Go type check" I mean a
variety of compile-time checks the Go compiler performs; in fact, this
is much more powerful than running "gofmt" as we are currently doing.

Additionally, it adds a new flag to gnokey, `-simulate`, to control
transaction simulation before committing a transaction. See [this issue
comment](#1702 (comment))

Resolves #1661.

## Reviewing notes

- transpiler.TranspileAndCheckMempkg has been removed from the gnokey
client and gnoclient, in favour of having this step be performed on the
vm keeper. This paves the way for clients to not have to include the
entire GnoVM, which I call a win.
- Stdlib io had a precompiling error due to an unused variable
(`remaining`); I updated it to the latest code on Go's standard
libraries.
- `Store` changes
- `Store` has been changed to have its `getPackage` method work by
detecting import cycles, without risking race conditions (the current
implementation is not thread-safe). This is done by creating a new
store, `importerStore`, which contains the previously imported paths in
the current chain. Cyclic imports are still (correctly) detected in the
tests.
- `GetMemPackage` has been changed to return nil when a package cannot
be found. This matches its behaviour with `GetMemFile`, which already
did this when the file does not exist.
- `GetMemPackage`, if a package is not found in the store, now attempts
retrieving it using Store.GetPackage first. The underlying reason is
that the Gno importer for the type checker needs to access the source of
the standard libraries; however, these are never in any transaction and
are not executed "per se" when the blockchain start. As a consequence,
they may not exist within the Store; as a solution, when using
GetMemPackage, we ensure that a package does not exist by checking if
GetPackage does not retrieve it through getMemPackage and save it.
  • Loading branch information
thehowl authored May 13, 2024
1 parent dc6eb7d commit 3ea1b47
Show file tree
Hide file tree
Showing 25 changed files with 730 additions and 151 deletions.
49 changes: 28 additions & 21 deletions docs/gno-tooling/cli/gnokey.md
Original file line number Diff line number Diff line change
Expand Up @@ -171,13 +171,14 @@ gnokey maketx addpkg \

#### **SignBroadcast Options**

| Name | Type | Description |
|--------------|---------|--------------------------------------------------------------------------|
| `gas-wanted` | Int64 | The maximum amount of gas to use for the transaction. |
| `gas-fee` | String | The gas fee to pay for the transaction. |
| `memo` | String | Any descriptive text. |
| `broadcast` | Boolean | Broadcasts the transaction. |
| `chainid` | String | Defines the chainid to sign for (should only be used with `--broadcast`) |
| Name | Type | Description |
|--------------|---------|----------------------------------------------------------------------------------------|
| `gas-wanted` | Int64 | The maximum amount of gas to use for the transaction. |
| `gas-fee` | String | The gas fee to pay for the transaction. |
| `memo` | String | Any descriptive text. |
| `broadcast` | Boolean | Broadcasts the transaction. |
| `chainid` | String | The chainid to sign for (should only be used with `--broadcast`) |
| `simulate` | String | One of `test` (default), `skip` or `only` (should only be used with `--broadcast`)[^1] |

#### **makeTx AddPackage Options**

Expand Down Expand Up @@ -208,13 +209,14 @@ gnokey maketx call \

#### **SignBroadcast Options**

| Name | Type | Description |
|--------------|---------|------------------------------------------------------------------|
| `gas-wanted` | Int64 | The maximum amount of gas to use for the transaction. |
| `gas-fee` | String | The gas fee to pay for the transaction. |
| `memo` | String | Any descriptive text. |
| `broadcast` | Boolean | Broadcasts the transaction. |
| `chainid` | String | The chainid to sign for (should only be used with `--broadcast`) |
| Name | Type | Description |
|--------------|---------|----------------------------------------------------------------------------------------|
| `gas-wanted` | Int64 | The maximum amount of gas to use for the transaction. |
| `gas-fee` | String | The gas fee to pay for the transaction. |
| `memo` | String | Any descriptive text. |
| `broadcast` | Boolean | Broadcasts the transaction. |
| `chainid` | String | The chainid to sign for (should only be used with `--broadcast`) |
| `simulate` | String | One of `test` (default), `skip` or `only` (should only be used with `--broadcast`)[^1] |

#### **makeTx Call Options**

Expand Down Expand Up @@ -246,13 +248,14 @@ gnokey maketx send \

#### **SignBroadcast Options**

| Name | Type | Description |
|--------------|---------|-------------------------------------------------------|
| `gas-wanted` | Int64 | The maximum amount of gas to use for the transaction. |
| `gas-fee` | String | The gas fee to pay for the transaction. |
| `memo` | String | Any descriptive text. |
| `broadcast` | Boolean | Broadcasts the transaction. |
| `chainid` | String | The chainid to sign for (implies `--broadcast`) |
| Name | Type | Description |
|--------------|---------|----------------------------------------------------------------------------------------|
| `gas-wanted` | Int64 | The maximum amount of gas to use for the transaction. |
| `gas-fee` | String | The gas fee to pay for the transaction. |
| `memo` | String | Any descriptive text. |
| `broadcast` | Boolean | Broadcasts the transaction. |
| `chainid` | String | The chainid to sign for (should only be used with `--broadcast`) |
| `simulate` | String | One of `test` (default), `skip` or `only` (should only be used with `--broadcast`)[^1] |

#### **makeTx Send Options**

Expand Down Expand Up @@ -302,3 +305,7 @@ Broadcast a signed document with the following command.
```bash
gnokey broadcast {signed transaction file document}
```

[^1]: `only` simulates the transaction as a "dry run" (ie. without committing to
the chain), `test` performs simulation and, if successful, commits the
transaction, `skip` skips simulation entirely and commits directly.
21 changes: 21 additions & 0 deletions gno.land/cmd/gnoland/testdata/addpkg_invalid.txtar
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# test for add package; ensuring type checker catches invalid code.

# start a new node
gnoland start

# add bar package located in $WORK directory as gno.land/r/foobar/bar
! gnokey maketx addpkg -pkgdir $WORK -pkgpath gno.land/r/foobar/bar -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1

# check error message
! stdout .+
stderr 'as string value in return statement'
stderr '"std" imported and not used'

-- bar.gno --
package bar

import "std"

func Render(path string) string {
return 89
}
93 changes: 93 additions & 0 deletions gno.land/cmd/gnoland/testdata/gnokey_simulate.txtar
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
# test for gnokey maketx -simulate options, and how they return any errors

loadpkg gno.land/r/hello $WORK/hello

# start a new node
gnoland start

# Initial state: assert that sequence == 0.
gnokey query auth/accounts/$USER_ADDR_test1
stdout '"sequence": "0"'

# attempt adding the "test" package.
# the package has a syntax error; simulation should catch this ahead of time and prevent the tx.
# -simulate test
! gnokey maketx addpkg -pkgdir $WORK/test -pkgpath gno.land/r/test -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test -simulate test test1
gnokey query auth/accounts/$USER_ADDR_test1
stdout '"sequence": "0"'
# -simulate only
! gnokey maketx addpkg -pkgdir $WORK/test -pkgpath gno.land/r/test -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test -simulate only test1
gnokey query auth/accounts/$USER_ADDR_test1
stdout '"sequence": "0"'
# -simulate skip
! gnokey maketx addpkg -pkgdir $WORK/test -pkgpath gno.land/r/test -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test -simulate skip test1
gnokey query auth/accounts/$USER_ADDR_test1
stdout '"sequence": "1"'

# attempt calling hello.SetName correctly.
# -simulate test and skip should do it successfully, -simulate only should not.
# -simulate test
gnokey maketx call -pkgpath gno.land/r/hello -func SetName -args John -gas-wanted 2000000 -gas-fee 1000000ugnot -broadcast -chainid tendermint_test -simulate test test1
gnokey query auth/accounts/$USER_ADDR_test1
stdout '"sequence": "2"'
gnokey query vm/qeval --data "gno.land/r/hello\nHello()"
stdout 'Hello, John!'
# -simulate only
gnokey maketx call -pkgpath gno.land/r/hello -func SetName -args Paul -gas-wanted 2000000 -gas-fee 1000000ugnot -broadcast -chainid tendermint_test -simulate only test1
gnokey query auth/accounts/$USER_ADDR_test1
stdout '"sequence": "2"'
gnokey query vm/qeval --data "gno.land/r/hello\nHello()"
stdout 'Hello, John!'
# -simulate skip
gnokey maketx call -pkgpath gno.land/r/hello -func SetName -args George -gas-wanted 2000000 -gas-fee 1000000ugnot -broadcast -chainid tendermint_test -simulate skip test1
gnokey query auth/accounts/$USER_ADDR_test1
stdout '"sequence": "3"'
gnokey query vm/qeval --data "gno.land/r/hello\nHello()"
stdout 'Hello, George!'

# attempt calling hello.Grumpy (always panics).
# all calls should fail, however -test skip should increase the account sequence.
# none should change the name (ie. panic rollbacks).
# -simulate test
! gnokey maketx call -pkgpath gno.land/r/hello -func Grumpy -gas-wanted 2000000 -gas-fee 1000000ugnot -broadcast -chainid tendermint_test -simulate test test1
gnokey query auth/accounts/$USER_ADDR_test1
stdout '"sequence": "3"'
gnokey query vm/qeval --data "gno.land/r/hello\nHello()"
stdout 'Hello, George!'
# -simulate only
! gnokey maketx call -pkgpath gno.land/r/hello -func Grumpy -gas-wanted 2000000 -gas-fee 1000000ugnot -broadcast -chainid tendermint_test -simulate only test1
gnokey query auth/accounts/$USER_ADDR_test1
stdout '"sequence": "3"'
gnokey query vm/qeval --data "gno.land/r/hello\nHello()"
stdout 'Hello, George!'
# -simulate skip
! gnokey maketx call -pkgpath gno.land/r/hello -func Grumpy -gas-wanted 2000000 -gas-fee 1000000ugnot -broadcast -chainid tendermint_test -simulate skip test1
gnokey query auth/accounts/$USER_ADDR_test1
stdout '"sequence": "4"'
gnokey query vm/qeval --data "gno.land/r/hello\nHello()"
stdout 'Hello, George!'

-- test/test.gno --
package test

func Render(path string) string {
return 89
}

-- hello/hello.gno --
package hello

var name = "Ringo"

func SetName(newName string) {
name = newName
}

func Hello() string {
return "Hello, " + name + "!"
}

func Grumpy() string {
name = "SCOUNDREL"
panic("YOU MAY NOT GREET ME, " + name)
}
9 changes: 3 additions & 6 deletions gno.land/cmd/gnoland/testdata/issue_1786.txtar
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ loadpkg gno.land/r/demo/wugnot
gnoland start

# add contract
gnokey maketx addpkg -pkgdir $WORK -pkgpath gno.land/r/demo/proxywugnot -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
gnokey maketx addpkg -pkgdir $WORK -pkgpath gno.land/r/demo/proxywugnot -gas-fee 1000000ugnot -gas-wanted 4000000 -broadcast -chainid=tendermint_test test1
stdout OK!

# approve wugnot to `proxywugnot ≈ g1fndyg0we60rdfchyy5dwxzkfmhl5u34j932rg3`
Expand All @@ -24,7 +24,7 @@ stdout '10000 uint64'
# unwrap 500 wugnot
gnokey maketx call -pkgpath gno.land/r/demo/proxywugnot -func ProxyUnwrap -args 500 -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1

# XXX without patching anything it will panic
# XXX without patching anything it will panic
# panic msg: insufficient coins error
# XXX with pathcing only wugnot.gnot it will panic
# panic msg: RealmSendBanker can only send from the realm package address "g1fndyg0we60rdfchyy5dwxzkfmhl5u34j932rg3", but got "g1pf6dv9fjk3rn0m4jjcne306ga4he3mzmupfjl6"
Expand All @@ -46,8 +46,6 @@ import (
"std"

"gno.land/r/demo/wugnot"

"gno.land/p/demo/ufmt"
pusers "gno.land/p/demo/users"
)

Expand All @@ -73,7 +71,6 @@ func ProxyUnwrap(wugnotAmount uint64) {
if wugnotAmount == 0 {
return
}
userOldWugnot := wugnot.BalanceOf(pusers.AddressOrName(std.GetOrigCaller()))

// SEND WUGNOT: USER -> PROXY_WUGNOT
wugnot.TransferFrom(pusers.AddressOrName(std.GetOrigCaller()), pusers.AddressOrName(std.CurrentRealm().Addr()), wugnotAmount)
Expand All @@ -84,4 +81,4 @@ func ProxyUnwrap(wugnotAmount uint64) {
// SEND GNOT: PROXY_WUGNOT -> USER
banker := std.GetBanker(std.BankerTypeRealmSend)
banker.SendCoins(std.CurrentRealm().Addr(), std.GetOrigCaller(), std.Coins{{"ugnot", int64(wugnotAmount)}})
}
}
11 changes: 0 additions & 11 deletions gno.land/pkg/gnoclient/client_txs.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package gnoclient

import (
"github.com/gnolang/gno/gno.land/pkg/sdk/vm"
"github.com/gnolang/gno/gnovm/pkg/transpiler"
"github.com/gnolang/gno/tm2/pkg/amino"
ctypes "github.com/gnolang/gno/tm2/pkg/bft/rpc/core/types"
"github.com/gnolang/gno/tm2/pkg/crypto"
Expand Down Expand Up @@ -145,11 +144,6 @@ func (c *Client) Run(cfg BaseTxCfg, msgs ...MsgRun) (*ctypes.ResultBroadcastTxCo

caller := c.Signer.Info().GetAddress()

// Transpile and validate Gno syntax
if err = transpiler.TranspileAndCheckMempkg(msg.Package); err != nil {
return nil, err
}

msg.Package.Name = "main"
msg.Package.Path = ""

Expand Down Expand Up @@ -263,11 +257,6 @@ func (c *Client) AddPackage(cfg BaseTxCfg, msgs ...MsgAddPackage) (*ctypes.Resul

caller := c.Signer.Info().GetAddress()

// Transpile and validate Gno syntax
if err = transpiler.TranspileAndCheckMempkg(msg.Package); err != nil {
return nil, err
}

// Unwrap syntax sugar to vm.MsgCall slice
vmMsgs = append(vmMsgs, std.Msg(vm.MsgAddPackage{
Creator: caller,
Expand Down
3 changes: 0 additions & 3 deletions gno.land/pkg/gnoclient/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,6 @@ func TestRunSingle_Integration(t *testing.T) {

fileBody := `package main
import (
"std"
"gno.land/p/demo/ufmt"
"gno.land/r/demo/tests"
)
Expand Down Expand Up @@ -305,7 +304,6 @@ func TestRunMultiple_Integration(t *testing.T) {

fileBody1 := `package main
import (
"std"
"gno.land/p/demo/ufmt"
"gno.land/r/demo/tests"
)
Expand All @@ -319,7 +317,6 @@ func main() {

fileBody2 := `package main
import (
"std"
"gno.land/p/demo/ufmt"
"gno.land/r/demo/deep/very/deep"
)
Expand Down
7 changes: 0 additions & 7 deletions gno.land/pkg/keyscli/addpkg.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (

"github.com/gnolang/gno/gno.land/pkg/sdk/vm"
gno "github.com/gnolang/gno/gnovm/pkg/gnolang"
"github.com/gnolang/gno/gnovm/pkg/transpiler"
"github.com/gnolang/gno/tm2/pkg/amino"
"github.com/gnolang/gno/tm2/pkg/commands"
"github.com/gnolang/gno/tm2/pkg/crypto/keys"
Expand Down Expand Up @@ -102,12 +101,6 @@ func execMakeAddPkg(cfg *MakeAddPkgCfg, args []string, io commands.IO) error {
panic(fmt.Sprintf("found an empty package %q", cfg.PkgPath))
}

// transpile and validate syntax
err = transpiler.TranspileAndCheckMempkg(memPkg)
if err != nil {
panic(err)
}

// parse gas wanted & fee.
gaswanted := cfg.RootCfg.GasWanted
gasfee, err := std.ParseCoin(cfg.RootCfg.GasFee)
Expand Down
7 changes: 1 addition & 6 deletions gno.land/pkg/keyscli/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (

"github.com/gnolang/gno/gno.land/pkg/sdk/vm"
gno "github.com/gnolang/gno/gnovm/pkg/gnolang"
"github.com/gnolang/gno/gnovm/pkg/transpiler"
"github.com/gnolang/gno/tm2/pkg/amino"
"github.com/gnolang/gno/tm2/pkg/commands"
"github.com/gnolang/gno/tm2/pkg/crypto/keys"
Expand Down Expand Up @@ -109,11 +108,7 @@ func execMakeRun(cfg *MakeRunCfg, args []string, cmdio commands.IO) error {
if memPkg.IsEmpty() {
panic(fmt.Sprintf("found an empty package %q", memPkg.Path))
}
// transpile and validate syntax
err = transpiler.TranspileAndCheckMempkg(memPkg)
if err != nil {
panic(err)
}

memPkg.Name = "main"
// Set to empty; this will be automatically set by the VM keeper.
memPkg.Path = ""
Expand Down
4 changes: 2 additions & 2 deletions gno.land/pkg/sdk/vm/builtins.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ func (vm *VMKeeper) initBuiltinPackagesAndTypes(store gno.Store) {
// NOTE: native functions/methods added here must be quick operations,
// or account for gas before operation.
// TODO: define criteria for inclusion, and solve gas calculations.
getPackage := func(pkgPath string) (pn *gno.PackageNode, pv *gno.PackageValue) {
getPackage := func(pkgPath string, newStore gno.Store) (pn *gno.PackageNode, pv *gno.PackageValue) {
// otherwise, built-in package value.
// first, load from filepath.
stdlibPath := filepath.Join(vm.stdlibsDir, pkgPath)
Expand All @@ -34,7 +34,7 @@ func (vm *VMKeeper) initBuiltinPackagesAndTypes(store gno.Store) {
PkgPath: "gno.land/r/stdlibs/" + pkgPath,
// PkgPath: pkgPath,
Output: os.Stdout,
Store: store,
Store: newStore,
})
defer m2.Release()
return m2.RunMemPackage(memPkg, true)
Expand Down
26 changes: 25 additions & 1 deletion gno.land/pkg/sdk/vm/errors.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
package vm

import "github.com/gnolang/gno/tm2/pkg/errors"
import (
"strings"

"github.com/gnolang/gno/tm2/pkg/errors"
"go.uber.org/multierr"
)

// for convenience:
type abciError struct{}
Expand All @@ -13,11 +18,21 @@ type (
InvalidPkgPathError struct{ abciError }
InvalidStmtError struct{ abciError }
InvalidExprError struct{ abciError }
TypeCheckError struct {
abciError
Errors []string
}
)

func (e InvalidPkgPathError) Error() string { return "invalid package path" }
func (e InvalidStmtError) Error() string { return "invalid statement" }
func (e InvalidExprError) Error() string { return "invalid expression" }
func (e TypeCheckError) Error() string {
var bld strings.Builder
bld.WriteString("invalid gno package; type check errors:\n")
bld.WriteString(strings.Join(e.Errors, "\n"))
return bld.String()
}

func ErrInvalidPkgPath(msg string) error {
return errors.Wrap(InvalidPkgPathError{}, msg)
Expand All @@ -30,3 +45,12 @@ func ErrInvalidStmt(msg string) error {
func ErrInvalidExpr(msg string) error {
return errors.Wrap(InvalidExprError{}, msg)
}

func ErrTypeCheck(err error) error {
var tce TypeCheckError
errs := multierr.Errors(err)
for _, err := range errs {
tce.Errors = append(tce.Errors, err.Error())
}
return errors.NewWithData(tce).Stacktrace()
}
Loading

0 comments on commit 3ea1b47

Please sign in to comment.