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

feat: add grc20reg that works... today #3135

Merged
merged 26 commits into from
Dec 3, 2024
Merged

Conversation

moul
Copy link
Member

@moul moul commented Nov 16, 2024

  • Switch to storing a type XXX func() grc20.Token instead of a grc20.Token directly.
  • Implement grc20reg.
  • Add new tests in gnovm/tests to demonstrate the current VM's management of the cross-realm feature and support potential changes in Gno Realm and Ownership Spec #2743.
  • Create a demo in atomicswap or a similar application. (feat: add r/demo/atomicswap #2510 (comment))
  • Try using a Token.Getter() helper. (Works! f99654e)
  • Demonstrate how to manage "disappearing" functions during garbage collection by checking if the function pointer is nil or non-resolvable.

Alternative to #2516
NOT(!) depending on #2743

@moul moul self-assigned this Nov 16, 2024
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Nov 16, 2024
Signed-off-by: moul <[email protected]>
Copy link

codecov bot commented Nov 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

This was referenced Nov 16, 2024
moul added 6 commits November 16, 2024 08:39
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Nov 16, 2024
moul added 11 commits November 16, 2024 09:33
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
@moul moul mentioned this pull request Nov 16, 2024
5 tasks
@dongwon8247
Copy link
Member

@onlyhyde @notJoon @r3v4s

@moul moul marked this pull request as ready for review November 16, 2024 10:15
@moul
Copy link
Member Author

moul commented Nov 16, 2024

It seems to work, unless we consider the current behavior a bug that we want to fix. In that case, we can create gno object registries by avoiding the storage of remotely defined objects. Instead, we can store remotely defined functions that dynamically return the remote object when needed.

I’m not a big fan of this indirection because I believe Go interfaces are excellent. However, since we fully preserve Go's type safety, I think it’s a good compromise that allows for greater independence between realms.

The last checkbox in the original comment raises an open question about how we plan to manage realm deprovisionning in the future. One idea I have that would avoid the need to track which remote realms are storing a locally defined function (remote backref). We could implement a special rule in the VM so that if you:

  1. Store a remotely defined function,
  2. Deprovision the remote realm,
  3. Attempt to call the locally stored (now dead) function, instead of panicking, we could simulate that the pointer is now nil at runtime.

This would mean we only need to check if the pointer is nil to manage custom error handling, or we could allow the VM to panic due to the missing pointer.

@n0izn0iz
Copy link
Contributor

n0izn0iz commented Nov 16, 2024

Did you try to call mutating functions from the calling realm?
The tests seem to be read-only

@moul
Copy link
Member Author

moul commented Nov 17, 2024

Here's one: https://github.com/gnolang/gno/pull/3135/files#diff-57ef3b955ce6dd26e824d52cfa9fd0d84e2b4f67e50abdd3f5d117458a14e59cR15

You're right; the atomic swap wasn't tested, and I forgot to remove a "panic not implemented" blocker. However, the test output you shared isn't related to mutation. It's due to reusing the same "sender" and "recipient" addresses in independent tests. In other words, if you run a single unit test with -run, it works, but running everything together does not. Here is a commit that makes the sender and recipient independent for each test: 3a5acf1.

And here a21ddd5 is the commit that removes the panic and adds the missing tests for atomic swap. It works.

$>  gno test -v ./examples/gno.land/r/demo/atomicswap
=== RUN   TestNewCustomCoinSwap_Claim
--- PASS: TestNewCustomCoinSwap_Claim (0.00s)
=== RUN   TestNewCustomCoinSwap_Refund
--- PASS: TestNewCustomCoinSwap_Refund (0.00s)
=== RUN   TestNewCustomGRC20Swap_Claim
--- PASS: TestNewCustomGRC20Swap_Claim (0.00s)
=== RUN   TestNewCustomGRC20Swap_Refund
--- PASS: TestNewCustomGRC20Swap_Refund (0.00s)
=== RUN   TestNewGRC20Swap_Claim
--- PASS: TestNewGRC20Swap_Claim (0.00s)
=== RUN   TestNewGRC20Swap_Refund
--- PASS: TestNewGRC20Swap_Refund (0.00s)
=== RUN   TestRender
--- PASS: TestRender (0.00s)
ok      ./examples/gno.land/r/demo/atomicswap   1.44s

@Kouteki Kouteki added this to the 🚀 Mainnet launch milestone Nov 18, 2024
@Kouteki Kouteki added the in focus Core team is prioritizing this work label Nov 18, 2024
examples/gno.land/r/demo/grc20reg/grc20reg.gno Outdated Show resolved Hide resolved
Copy link
Member

@notJoon notJoon left a comment

Choose a reason for hiding this comment

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

It looks valid to me 👍

@ltzmaxwell
Copy link
Contributor

ltzmaxwell commented Nov 21, 2024

  1. this pattern is no different with this, except for the addition of Getter for convenience, right?
// PKGPATH: gno.land/r/crossrealm_test
package crossrealm_test

import (
	"std"

	crossrealm "gno.land/r/demo/tests/crossrealm"
)

type fooer struct {
	v string
}

func (f *fooer) Foo() { println("hello " + f.v + " " + std.CurrentRealm().PkgPath()) }

func (f *fooer) Mutate() {
	f.v = "b"
}

var f *fooer

func init() {
	f = &fooer{v: "a"} // attach first
}

func main() {
	crossrealm.SetFooer(f)
	crossrealm.CallFooerFoo()

	crossrealm.MutateFooer()
	crossrealm.CallFooerFoo()
}

// Output:
// hello a gno.land/r/crossrealm_test
// hello b gno.land/r/crossrealm_test
  1. There are underlying bugs that we need to fix (though they do not block the Getter pattern):
// PKGPATH: gno.land/r/crossrealm_test
package crossrealm_test

import (
	"std"

	crossrealm "gno.land/r/demo/tests/crossrealm"
)

type fooer struct {
	s string
}

func (f *fooer) Foo() {
	println("hello " + f.s + " " + std.CurrentRealm().PkgPath())
}

func (f *fooer) Mutate() {}

func init() {
	f := &fooer{s: "B"} // XXX, note this is incorrectly attached to "r/demo/crossrealm", by the next line.
	fg := func() crossrealm.Fooer { return f }
	crossrealm.SetFooerGetter(fg)
	crossrealm.CallFooerGetterFoo()

	f.s = "C" // XXX, so this cause a panic while modifying external realm obejct
	crossrealm.CallFooerGetterFoo()
}

func main() {
	print(".")
}

// Error:
// cannot modify external-realm or non-realm object

Copy link
Contributor

@ltzmaxwell ltzmaxwell left a comment

Choose a reason for hiding this comment

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

LGTM.

}

// Error:
// gno.land/r/crossrealm_test/main.gno:19:2: name CallFoo not declared
Copy link
Contributor

Choose a reason for hiding this comment

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

This error message doesn’t seem intended for this pattern. Consider modifying the code or simply removing the message entirely to eliminate any ambiguity.

Copy link
Member Author

Choose a reason for hiding this comment

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

@moul
Copy link
Member Author

moul commented Nov 25, 2024

This pattern is no different with this, except for the addition of Getter for convenience, right?

Not exactly—it's not just for convenience but because the behavior is fundamentally different. With cross-realm objects, we encounter ownership errors due to the constraints of realm isolation. However, by using the Getter pattern to store a pointer to a function (either top-level or lambda), we avoid these ownership issues entirely. This makes the Getter pattern a unique and powerful approach that enables interactions across realms in ways that aren’t possible with direct object storage.

There are underlying bugs that we need to fix (though they do not block the Getter pattern):

👍

@Gno2D2
Copy link
Collaborator

Gno2D2 commented Nov 28, 2024

Merge Requirements

The following requirements must be fulfilled before a pull request can be merged.
Some requirement checks are automated and can be verified by the CI, while others need manual verification by a staff member.

These requirements are defined in this configuration file.

Automated Checks

🟢 Maintainers must be able to edit this pull request
🟢 The pull request head branch must be up-to-date with its base

Details
Maintainers must be able to edit this pull request

If

🟢 Condition met
└── 🟢 On every pull request

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

The pull request head branch must be up-to-date with its base

If

🟢 Condition met
└── 🟢 On every pull request

Then

🟢 Requirement satisfied
└── 🟢 Head branch (moul:dev/moul/grc20reg-v2) is up to date with base (master): behind by 0 / ahead by 26

Manual Checks

No manual checks match this pull request.

Signed-off-by: moul <[email protected]>
@moul moul mentioned this pull request Dec 2, 2024
@moul moul merged commit bc44a39 into gnolang:master Dec 3, 2024
102 of 103 checks passed
@moul moul deleted the dev/moul/grc20reg-v2 branch December 3, 2024 13:23
r3v4s pushed a commit to gnoswap-labs/gno that referenced this pull request Dec 10, 2024
- [x] Switch to storing a `type XXX func() grc20.Token` instead of a
`grc20.Token` directly.
-  [x] Implement `grc20reg`.  
- [x] Add new tests in `gnovm/tests` to demonstrate the current VM's
management of the cross-realm feature and support potential changes in
gnolang#2743.
- [x] Create a demo in `atomicswap` or a similar application.
(gnolang#2510 (comment))
-  [x] Try using a `Token.Getter()` helper.  (Works! f99654e)
- [ ] Demonstrate how to manage "disappearing" functions during garbage
collection by checking if the function pointer is nil or non-resolvable.

Alternative to gnolang#2516  
NOT(!) depending on gnolang#2743

---------

Signed-off-by: moul <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in focus Core team is prioritizing this work 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

8 participants