search

gnolang / gno

Gno: An interpreted, stack-based Go virtual machine to build succinct and composable apps + Gno.land: a blockchain for timeless code and fair open-source
https://gno.land/
Other
839 stars 342 forks source link

chore: In r/demo/boards public API, change AssertOriginCall() to PrevRealm().IsUser() #2358

Closed jefft0 closed 2 days ago

jefft0 commented 1 week ago

In r/demo/boards, std.AssertOriginCall() blocks calling the API from another realm. But it also prevents using MsgRun for testing as we do in PR #1583. Therefore, we change to check std.PrevRealm().IsUser(). This still blocks another realm from calling the code, but allows being called by MsgRun where a user keypair sends the transaction.

Contributors' checklist... - [x] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs
leohhhn commented 1 week ago

@jefft0 can you please check why the tests are failing? 🙏

jefft0 commented 1 week ago

Hi @leohhhn . A test file is getting the new panic message "invalid non-user call" when calling the realm function. I don't understand. I thought that PrevRealm().IsUser() is more permissive than AssertOriginCall(). If the test doesn't get a panic from AssertOriginCall(), then why get the new panic?

jefft0 commented 1 week ago

Hi @leohhhn . A test file is getting the new panic message "invalid non-user call" when calling the realm function. I don't understand. I thought that PrevRealm().IsUser() is more permissive than AssertOriginCall(). If the test doesn't get a panic from AssertOriginCall(), then why get the new panic?

Hi @leohhhn. I changed the test to !(std.IsOriginCall() || std.PrevRealm().IsUser()) . Allowing isOriginCall() seems to make the tests happy. Is this the right way to do it?

leohhhn commented 1 week ago

@jefft0

Ideally we should work with just PrevRealm. There is an API to set the caller realm in tests. This is the docs for it (in review), but there is currently an issue with the TestSetRealm function.

Due to the issue, right now I suggest going with your latest proposal, and iterating on this once we fix up the issues.