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

chore: attempt to improve OriginCall funcs #495

Closed

Conversation

tbruyelle
Copy link
Contributor

@tbruyelle tbruyelle commented Jan 30, 2023

AssertOriginCall and IsOriginCall relies on the number of frames to determine if the function was called by a gno transaction (definition taken from examples/gno.land/r/demo/banktest/README.md). Since this number is different in the context of test and filetest, the 2 functions are overrided during test setup, but the override works only for filetest files and not for test files.

This change allows AssertOriginCall and IsOriginCall to rely on message's signers instead of frames. If the message has at least one signer, we consider the function was called by a gno tx. As a result, the functions no longer needs to be overrided in test setup, and they work both for filetest files and test files.

Fix #481

`AssertOriginCall` and `IsOriginCall` relies on the number of frames to
determine if the function was called by a gno transaction (definition
taken from `examples/gno.land/r/demo/banktest/README.md`). Since this
number is different in the context of test and filetest, the 2 functions
are overrided during test setup, but the override works only for
filetest files and not for test files.

This change makes `AssertOriginCall` and `IsOriginCall` to rely on
message's signers instead of frames. If the message has at east one
signer, we consider the function was called by a gno tx. As a result,
the functions no longer needs to be overrided in test setup, and they
work both for filetest files and test files.

Fix gnolang#481
@tbruyelle tbruyelle requested a review from a team as a code owner January 30, 2023 17:14
@moul
Copy link
Member

moul commented Mar 2, 2023

Hey @tbruyelle, sorry for the delay; everything looks good, however, to make sure that it's stable and stays stable over time.

It would be nice to add tests checking the consistency of this approach in the three main conditions:

  • simulating a TX,
  • testing with _test.gno,
  • testing with _filetest.gno.

We may add a unit test suite simulating the various Machine contexts so we can add more and more tests for std over time.
It may be done in another PR and by someone else, but I was wondering what do you think.

@tbruyelle
Copy link
Contributor Author

Hey @moul no problem for the delay. You're right some tests are missing for a _test.gno file (which was the root problem addressed by this change actually!).

For _filetest.gno files, only AssertOriginCall is actually tested (for instance in demo/banktest), but not IsOriginCall, I will add a case for that.

About simulating a TX, could you give more details? Is it some kind of integration test ?

@tbruyelle tbruyelle force-pushed the chore/improve-assertOriginCall branch 2 times, most recently from f4d832e to 405f13f Compare March 5, 2023 22:33
@tbruyelle tbruyelle force-pushed the chore/improve-assertOriginCall branch from 405f13f to dde938d Compare March 5, 2023 22:40
@tbruyelle
Copy link
Contributor Author

tbruyelle commented Mar 5, 2023

I added some tests for AssertOriginCall and IsOriginCall, but that required some changes in the body of AssertOriginCall.

Indeed, the panic in this function occurred in go code, so it wasn't recoverable by gno code. Switching from panic to machine.Panic fixed that. Not sure if this switch has other consequences, I can just assert that in both scenario, the program is stopped.

@tbruyelle tbruyelle marked this pull request as draft March 30, 2023 07:09
@tbruyelle
Copy link
Contributor Author

After discussions with @moul, I realized my attempt to change OriginCall functions actually broke their initial behavior. So I put this PR in draft until I fix that.

My initial understanding on these functions was wrong. I though they were ensuring that the call comes from a transaction, but it's not only that, they also ensure that this call comes directly from a transaction, and not from an other contract. This is why they were relying on the number of frames to determine that origin call property.

@moul
Copy link
Member

moul commented Mar 31, 2023

Please share your thoughts on PR #683.

@tbruyelle
Copy link
Contributor Author

Closed in favor of #703

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

test: unable to use std.AssertOriginCall in _test files
2 participants