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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Jun 24, 2024
Merged

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

merged 4 commits into from
Jun 24, 2024

Conversation

jefft0
Copy link
Contributor

@jefft0 jefft0 commented Jun 14, 2024

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...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs

@jefft0 jefft0 requested review from a team as code owners June 14, 2024 09:32
@jefft0 jefft0 requested review from ajnavarro and gfanton and removed request for a team June 14, 2024 09:32
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Jun 14, 2024
@leohhhn
Copy link
Contributor

leohhhn commented Jun 14, 2024

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

@jefft0
Copy link
Contributor Author

jefft0 commented Jun 14, 2024

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
Copy link
Contributor Author

jefft0 commented Jun 17, 2024

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
Copy link
Contributor

leohhhn commented Jun 17, 2024 •
edited
Loading

@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.

leohhhn
leohhhn approved these changes Jun 17, 2024
View reviewed changes
Copy link
Contributor

@leohhhn leohhhn left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM until we fix the issues in the above comment

@leohhhn leohhhn merged commit 900bb2f into gnolang:master Jun 24, 2024
11 checks passed
@jefft0 jefft0 mentioned this pull request Jun 24, 2024
Open
gfanton pushed a commit to gfanton/gno that referenced this pull request Jul 23, 2024
@jefft0 @leohhhn @gfanton
chore(examples): In r/demo/boards public API, change AssertOriginCall…
ff6cc54 
…() to PrevRealm().IsUser() (gnolang#2358)

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 gnolang#1583. Therefore, we change to check `std.PrevRealm().IsUser()`. This
still blocks another realm from calling the code, but allows being
[called by
MsgRun](https://github.com/gnolang/gno/blob/d7f12167eff72cd4a12e9e8b8aaa30dc241bfb6c/misc/stress-test/stress-test-many-posts/main.go#L148-L156)
where a user keypair sends the transaction.

<details><summary>Contributors' checklist...</summary>

- [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
</details>

---------

Signed-off-by: Jeff Thompson <[email protected]>
Co-authored-by: Leon Hudak <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leohhhn leohhhn leohhhn approved these changes

@ajnavarro ajnavarro Awaiting requested review from ajnavarro ajnavarro was automatically assigned from gnolang/tech-staff

@gfanton gfanton Awaiting requested review from gfanton gfanton was automatically assigned from gnolang/tech-staff

Assignees

@jefft0 jefft0

Labels
🧾 package/realm Tag used for new Realms or Packages.
Projects
💪 Bounties & Worx
Status: Done
🧙‍♂️gno.land core team
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jefft0 @leohhhn