feat: std.TestSetPrevRealm

[r3v4s]

Description

Thanks to @albttx, we now have std.PrevRealm #667

However it seems like doesn't work on testcase(that can be run by gno test command) as what we expected.

gno test uses its own Context which makes few other functions (like GetRealmPath) doesn't work neither.

So I made little helper very similar to std.TestSetOrigCaller

---

Discussion Need

std.TestSetOrigCaller takes single parameter and two different type can be passed

1. If std.Address type is passed, -> TestSetPrevRealm will take it as user address => return user address not realm.
2. If string type is passed, -> TestSetPrevRealm will take it as pkg_path => return pkg address(using bech32) and pkg_path

Since string parameter is being used without any verification in this pr, How about reusing verification logic from here ??

gno/tm2/pkg/std/memfile.go

Lines 33 to 68 in 408fc68

	const (
	reDomainPart = `gno\.land`
	rePathPart = `[a-z][a-z0-9_]*`
	rePkgName = `^[a-z][a-z0-9_]*$`
	rePkgPath = reDomainPart + `/p/` + rePathPart + `(/` + rePathPart + `)*`
	reRlmPath = reDomainPart + `/r/` + rePathPart + `(/` + rePathPart + `)*`
	rePkgOrRlmPath = `^(` + rePkgPath + `|` + reRlmPath + `)$`
	reFileName = `^[a-zA-Z0-9_]*\.[a-z0-9_\.]*$`
	)
	// path must not contain any dots after the first domain component.
	// file names must contain dots.
	// NOTE: this is to prevent conflicts with nested paths.
	func (mempkg *MemPackage) Validate() error {
	ok, _ := regexp.MatchString(rePkgName, mempkg.Name)
	if !ok {
	return errors.New(fmt.Sprintf("invalid package name %q", mempkg.Name))
	}
	ok, _ = regexp.MatchString(rePkgOrRlmPath, mempkg.Path)
	if !ok {
	return errors.New(fmt.Sprintf("invalid package/realm path %q", mempkg.Path))
	}
	fnames := map[string]struct{}{}
	for _, memfile := range mempkg.Files {
	ok, _ := regexp.MatchString(reFileName, memfile.Name)
	if !ok {
	return errors.New(fmt.Sprintf("invalid file name %q", memfile.Name))
	}
	if _, exists := fnames[memfile.Name]; exists {
	return errors.New(fmt.Sprintf("duplicate file name %q", memfile.Name))
	}
	fnames[memfile.Name] = struct{}{}
	}
	return nil
	}

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
• Provided any useful hints for running manual tests

Maintainers Checklist

• Checked that the author followed the guidelines in CONTRIBUTING.md
• Checked the conventional-commit (especially PR title and verb, presence of BREAKING CHANGE: in the body)
• Ensured that this PR is not a significant change or confirmed that the review/consideration process was appropriate for the change

[tbruyelle]

Thanks for raising this, I checked on my side and I noticed an other issue related to PackageValue.IsRealm() method. If we fix it, maybe we won't need this extra std.TestSetPrevRealm function.

PackageValue.IsRealm() is used in std.PrevRealm to determine if a frame is a realm, and if not, the frame is ignored.

PackageValue.IsRealm() calls IsRealmPath() under the hood, which has the following body :

func IsRealmPath(pkgPath string) bool {
	// TODO: make it more distinct to distinguish from normal paths.
	if strings.HasPrefix(pkgPath, "gno.land/r/") {
		return true
	} else {
		return false
	}
}

The problem is that realms under _test.gno file have a realm path equals to examples/gno.land/r/... so this function returns always false, which breaks the behavior of std.PrevRealm().

I'll try to provide a fix and see if that solves also the problem you raised.

[r3v4s]

@tbruyelle Thanks for response. I found other issue in this pr maybe related your IsRealm.

If I pass weird string to std.TestSetPrevRealm("this_is_strange_string") it breaks.

Would be nice if your fix does fix every issue on our hand, but if it doesn't I might be have to use IsRealm in PrevRealm to validate it.

[tbruyelle]

@r3v4s I was able to fix the issue I had, can you compile gno from this PR #896 and see if you still have the issue on your side ? THx!

[zivkovicmilos]

Looks good 💯

I've left some comments that should be addressed, and pinged a few people to take another look.

Please check out @tbruyelle's recent comment 🙏

[r3v4s]

Just removed logic for setting caller to PrevRealm, looks like st.TestSetOrigCaller() does that

[r3v4s]

• TestSetPrevAddr removes previous pkg path, and sets address (just like user called it directly)

• TestSetPrevRealm keeps origin caller, and sets previous pkg path

[r3v4s]

update: remove nested testcase, and made simple one in gno.land/r/demo/test

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.51%. Comparing base (281815a) to head (201a27f).
Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #891   +/-   ##
=======================================
  Coverage   47.51%   47.51%           
=======================================
  Files         388      388           
  Lines       61331    61331           
=======================================
  Hits        29144    29144           
  Misses      29747    29747           
  Partials     2440     2440           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tbruyelle]

My take on this PR stays the same, considering #1048, we don't need those extra TestSetPrev* functions. But since #1048 is blocked, do what you think is best.

See comment #891 (comment)

[piux2]

We shouldn't need to inject a TestSetPrevRealm as a native method for testing.

We will need a separate RFC for the reasons outlined below:

Given the numerous incidents on other chains where millions were mistakenly sent to contract addresses instead of user addresses, it's crucial that we distinguish between contract entities (represented by package paths) and EOA (Externally Owned Account) entities (represented by addresses) in gno contract.

• Instead of returning an address for PrevRealm, we should return the realm package.

• Furthermore, we need to address the ambiguity between returning user account addresses and realm identity from PrevRealm. We might want to explicitly check the returned value to determine whether we are validating an EOA or a contract for entry method we try guard.

• Not sure we have enough strong use cases where we had to pass contract addresses instead of realm package paths between contracts in gno.

[r3v4s]

here is short briefing for why I implemented it.

let's say... certain contact want to receive grc20 token from user(wallet, address).
to test that logic with test case, something like this will be used.

# gno.mod ( gno.land/r/demo/receive )
# contract_test.gno

import (
	"std"
	"testing"
	foo "gno.land/r/demo/foo20"
)


const sender = std.Address("g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5")


func TestFunc(t *testing.T) {
	std.TestSetOrigCaller(sender)

	foo.Transfer(std.GetOrigPkgAddr(), "12345")
}

because foo uses caller address from std.PrevRealm().Addr()

gno/examples/gno.land/r/demo/foo20/foo20.gno

Line 51 in dce345f

caller := std.PrevRealm().Addr()

actual caller will be same with std.DerivePkgAddr("gno.land/r/demo/receive") rather than my expected value g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5

[thehowl]

Keeping the comment for the edit history, but I'll need to take another look at this, nevermind what I just said

[thehowl]

So, I did a second review and agreed that the approach here is not correct.

• Instead of overwriting the previous frames, we should add a new frame that marks the PrevRealm
• When setting a PrevRealm, the address should still be set.
• The current approach pollutes the frame stack, presumably because of a missing return (so fixable), but I think a different approach in all is warranted.

I have been discussing about this with @albttx on how to best do this and he's working on a PR to iterate and improve on your work, @r3v4s. I also have some ideas on a different API we could have for the std.TestSetPrevRealm functionality; I'll make a separate issue for it and add it to our agenda for today's call.

---

For @piux2's concerns,

We shouldn't need to inject a TestSetPrevRealm as a native method for testing.

We should, (or at least have a similar mechanism), otherwise the only way to test a realm when being called from other realms is through filetests, which I personally think is inflexible (ie. can't create table tests, trying to call from different realms) and creates pollution in a realm's directory as a realm grows in complexity.

We will need a separate RFC for the reasons outlined below:

Given the numerous incidents on other chains where millions were mistakenly sent to contract addresses instead of user addresses, it's crucial that we distinguish between contract entities (represented by package paths) and EOA (Externally Owned Account) entities (represented by addresses) in gno contract.

This is still one of the design goals in the implementation of PrevRealm and it can be easily done. On std.Realm, you can determine whether the caller (or previous-realm, in the paradigm proposed by Jae whereby even users can be considered "realms"), is an end user or a published gno realm by calling the method IsUser(). More specifically, this works by testing return r.pkgPath == "" -- ie. end-users don't have a pkgPath, published realms do.

* Instead of returning an address for PrevRealm, we should return the realm package.

PrevRealms should have addresses, as realms have addresses with funds, and a caller may be looking for that information.

[tbruyelle]

My take on this PR stays the same, considering #1048, we don't need those extra TestSetPrev* functions. But since #1048 is blocked, do what you think is best.

So I though I fixed that last week but I probably forgot to click the button...

This comment from me mentions the wrong PR, I wanted to mention this PR #896, which is merged since a long time ago. IMO, this PR mitigates the needs of TestSetPrevRealm because it ensures the _test files have a realm (which wasn't the case before).

The way this works is the following, if there's a gno.mod file, the realm is the one from the gno.mod, so the _test shares the same realm as the tested realm. If there's no gno.mod file, the realm of the _test is generated randomly, so it's different from the tested realm. Looks weird when read like that but anyway I don't think _test files should be used to test PrevRealm situration.

_filtest on the other hand are goods for PrevRealm testing, and for them the solution exists from the beginning, you just need to add a // PKGPATH: instruction to define the realm.

[tbruyelle]

Instead of overwriting the previous frames, we should add a new frame that marks the PrevRealm

I don't think it's a good idea to manually manipulate the frames. The thing is complex enough, let's not add other way to confuse ourselves.

[thehowl]

@tbruyelle my comment goes in a direction of removing filetests which I kind of had subconsciously but hadn't written/talked about it yet 😛 . I've created an issue so we can discuss that and understand if it is a common feeling / my proposal makes sense: #1346

[r3v4s]

After #764 merged, mocking(or overwriting) codes got really short (only few lines)

In my local testing, these TestSet is really helpful for situation like I've mentioned above.

Comments not replied to since October, even though Blake resolved and pinged

[leohhhn]

Revisiting this - We should definitely have TestSetPrevRealm, just like we do with other ctx variables.

@r3v4s, can you please write txtar tests for this, and possibly simplify them to not use a separate package)? 🙏

[r3v4s]

@leohhhn I don't think with txtar testing I can call Test* functions. Aren't they meant to only callable from gno test ??

[zivkovicmilos]

Merged master locally and all tests are passing 💯