Closed tbruyelle closed 1 year ago
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:
AssertOriginCall
so it will use on various locations.testing
helpers so they generate a custom context that looks more like a standard call stack.std
so we can simulate various stacks.
- 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 ?
- 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 overrideAssertOriginCall
(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 ofAssertOriginCall
(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):
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 ?
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.
I really like this bug, or rather the discussion. I also get this problem.
That is,
_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.
If you liked the discussion, please look at #495.
In test context,
std.AssertOriginCall
is overrided by the following code :Which works well for
_filetest
files, but not for_test
files because the number ofm.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 byspew.Dump()
) :The frames for
_test
: