test: unable to use std.AssertOriginCall in _test files

[tbruyelle]

In test context, std.AssertOriginCall is overrided by the following code :

			func(m *gno.Machine) {
				isOrigin := len(m.Frames) == 3
				if !isOrigin {
					panic("invalid non-origin call")
				}
			}

Which works well for _filetest files, but not for _test files because the number of m.Frames is not 3.

I would like to make it compatible for both scenario, but for that I need to understand why checking the number of frames asserts that the caller is actually the signer of the tx. Any hint would be appreciated.

---

The frames for _filetest (dumped by spew.Dump()) :

([]gnolang.Frame) (len=3 cap=4) {
 (gnolang.Frame) [FRAME FUNC:main RECV:(undefined) (0 args) 2/0/0/0/1 LASTPKG:main LASTRLM:Realm(nil)],
 (gnolang.Frame) [FRAME FUNC:CreateContrib RECV:(undefined) (2 args) 5/1/0/2/2 LASTPKG:main LASTRLM:Realm(nil)],
 (gnolang.Frame) [FRAME FUNC:AssertOriginCall RECV:(undefined) (0 args) 7/2/0/3/3 LASTPKG:gno.land/r/gnoland/dao/eval LASTRLM:Realm{Path:"gno.land/r/gnoland/dao/eval",Time:4}#EA8DA22C5FB56500FCF01D8C35C2F076DFA9771B]
}

The frames for _test :

([]gnolang.Frame) (len=7 cap=8) {
 (gnolang.Frame) [FRAME FUNC:runtest RECV:(undefined) (1 args) 1/0/0/0/1 LASTPKG:./examples/gno.land/r/gnoland/dao/eval LASTRLM:Realm(nil)],
 (gnolang.Frame) [FRAME LABEL:  2/1/0/1/2],
 (gnolang.Frame) [FRAME FUNC:RunTest RECV:(undefined) (3 args) 6/2/0/3/4 LASTPKG:./examples/gno.land/r/gnoland/dao/eval LASTRLM:Realm(nil)],
 (gnolang.Frame) [FRAME FUNC:tRunner RECV:(undefined) (3 args) 8/3/0/4/5 LASTPKG:testing LASTRLM:Realm(nil)],
 (gnolang.Frame) [FRAME FUNC:TestCreateContrib RECV:(undefined) (1 args) 11/4/0/6/6 LASTPKG:testing LASTRLM:Realm(nil)],
 (gnolang.Frame) [FRAME FUNC:CreateContrib RECV:(undefined) (2 args) 14/5/0/8/7 LASTPKG:./examples/gno.land/r/gnoland/dao/eval LASTRLM:Realm(nil)],
 (gnolang.Frame) [FRAME FUNC:AssertOriginCall RECV:(undefined) (0 args) 16/6/0/9/8 LASTPKG:./examples/gno.land/r/gnoland/dao/eval LASTRLM:Realm(nil)]
}

[moul]

Thank you @tbruyelle,

I agree this is a bug and we should fix it.
The current hardcoded number works on real blockchain runtime and with the _filetest.gno runtime, but not with the special context of a _test.gno file.

I think there are multiple ways to manage this case.
Some ideas so we can discuss:

• update AssertOriginCall so it will use on various locations.
• update the testing helpers so they generate a custom context that looks more like a standard call stack.
• add more helpers to std so we can simulate various stacks.

[tbruyelle]

• update AssertOriginCall so it will use on various locations.

This could be basically handled by checking in the test helpers if the number of Frames is equal to 3 or 7.

• update the testing helpers so they generate a custom context that looks more like a standard call stack.

So in this scenario we somehow manipulates m.Frames in the test helpers, so it looks like a standard call, and thus it's no longer needed to override AssertOriginCall (and maybe others funcs), right ?

• add more helpers to std so we can simulate various stacks.

In this scenario we have 2 different test setups, one for _filetest and one for _test files, and each of them have a different overrides of AssertOriginCall (and maybe others funcs), is this what you mean ?

---

If I understood correctly, the role of AssertOriginCall is to assert the call is triggered by a tx. So checking the number of frames for that purpose sounds a little weak for me, maybe this is something planned for improvement. So with this context in mind, I would go for the simplest solution, so the first one. WDYT ?

[moul]

• update AssertOriginCall so it will use on various locations.

This could be basically handled by checking in the test helpers if the number of Frames is equal to 3 or 7.

For 3, yes;
For 7, I think it will be more dynamic, i.e., when calling nested t.Run, it could probably become 8, 9.

• update the testing helpers so they generate a custom context that looks more like a standard call stack.

So in this scenario we somehow manipulates m.Frames in the test helpers, so it looks like a standard call, and thus it's no longer needed to override AssertOriginCall (and maybe others funcs), right ?

We can manipulate m.Frames, or maybe just regenerate a fresh context.
I don't know yet, I suggest we try and write unit tests against edge cases.

• add more helpers to std so we can simulate various stacks.

In this scenario we have 2 different test setups, one for _filetest and one for _test files, and each of them have a different overrides of AssertOriginCall (and maybe others funcs), is this what you mean ?

I mean adding new helpers that you can call when you're in the context of a unit test, so you can tell the VM to consider that the runtime is happening under certain conditions.

We already have some test-specific std helpers, they all start with std.Test..., and allow you to simulate custom caller, height, date, etc.

Maybe we can add extend test-only helpers to play with the frames/stack.

If I understood correctly, the role of AssertOriginCall is to assert the call is triggered by a tx. So checking the number of frames for that purpose sounds a little weak for me, maybe this is something planned for improvement. So with this context in mind, I would go for the simplest solution, so the first one. WDYT ?

Yep, there is room for improvement. As a contract developer, I want to be able to assert (non-exhaustive):

• that I'm called directly by a TX,
• that I'm not called directly by a TX, but through another front-facing contract,
• that I'm called by a specific contract,
• that, even if there is an intermediary contract, somewhere in the stack, there is a specific contract/caller.

[tbruyelle]

I have tested this "naive" change in AssertOrigCaller, which in my opinion better matches the purpose of this function:

 		pn.DefineNative("AssertOriginCall",
 			gno.Flds( // params
 			),
 			gno.Flds( // results
 			),
 			func(m *gno.Machine) {
-				isOrigin := len(m.Frames) == 2
-				if !isOrigin {
+				ctx := m.Context.(ExecContext)
+				if ctx.Msg == nil || len(ctx.Msg.GetSigners()) == 0 {
 					panic("invalid non-origin call")
 				}
 			},
 		)

With that, we don't need any more to override the function for testing, if, in addition, we do the following change in testMachineCustom :

 func testMachineCustom(store gno.Store, pkgPath string, stdout io.Writer, maxAlloc int64, send std.Coins) *gno.Machine {
 	// FIXME: create a better package to manage this, with custom constructors

 	pkgAddr := gno.DerivePkgAddr(pkgPath)                      // the addr of the pkgPath called.
 	caller := gno.DerivePkgAddr(pkgPath)                       // NOTE: for the purpose of testing, the caller is generally the "main" package, same as pkgAddr.
 	pkgCoins := std.MustParseCoins("200000000ugnot").Add(send) // >= send.
 	banker := newTestBanker(pkgAddr.Bech32(), pkgCoins)
 	ctx := stdlibs.ExecContext{
 		ChainID:       "dev",
 		Height:        123,
 		Timestamp:     1234567890,
-		Msg:           nil,
+		Msg:           testutils.NewTestMsg(crypto.MustAddressFromString(caller.String())),
 		OrigCaller:    caller.Bech32(),
 		OrigPkgAddr:   pkgAddr.Bech32(),
 		OrigSend:      send,
 		OrigSendSpent: new(std.Coins),
 		Banker:        banker,
 	}
 	m := gno.NewMachineWithOptions(gno.MachineOptions{
 		PkgPath:       "", // set later.
 		Output:        stdout,
 		Store:         store,
 		Context:       ctx,
 		MaxAllocBytes: maxAlloc,
 	})
 	return m
 }

Which fills the signers of the message. Do you think it's acceptable ?

[moul]

It looks great 👍

I need to think about about the first snippet, about eventual edge cases.

For the second snippet, I can't think any issue, only improvements.

Please open a PR so we can continue the review and also rely on the CI to help us identify eventual edge cases.

[grepsuzette]

I really like this bug, or rather the discussion.
I also get this problem.

That is,

• From a _test file, if there is a call std.AssertOrigCall, it will write "invalid non-origin call".
  So same as this issue.

But in my case; if I use _filetest instead,

then gnodev test pass, even when it's supposed to fail (t.Fail()). Showing, with -verbose:

Edit: no it was me who didn't understand what a filetest was and how to use it.

[moul]

If you liked the discussion, please look at #495.