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 r/grc20reg #2516

Draft
wants to merge 72 commits into
base: master
Choose a base branch
from
Draft

feat: add r/grc20reg #2516

wants to merge 72 commits into from

Conversation

moul
Copy link
Member

@moul moul commented Jul 5, 2024

Depends on #2743
Continues #1072
Related #1275, #1262
Depends on #1257
Depends on #2551
Alternative to #3135

@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Jul 5, 2024
@moul moul mentioned this pull request Jul 5, 2024
7 tasks
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Jul 5, 2024
Copy link

codecov bot commented Jul 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@moul moul changed the title feat: grc20 registry feat: add r/grc20reg + p/fqname Jul 5, 2024
Signed-off-by: moul <[email protected]>
@moul moul force-pushed the dev/moul/grc20reg branch from 94f30b5 to 3f83d01 Compare July 5, 2024 22:02
@moul
Copy link
Member Author

moul commented Aug 29, 2024

Blocked by #2743

@moul moul mentioned this pull request Sep 17, 2024
7 tasks
thehowl added a commit that referenced this pull request Sep 18, 2024
Extracted from #2551 (also #2516).

<!-- please provide a detailed description of the changes made in this
pull request. -->

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

- [ ] 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
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Signed-off-by: moul <[email protected]>
Co-authored-by: Morgan <[email protected]>
@moul moul changed the title feat: add r/grc20reg + p/fqname feat: add r/grc20reg Sep 21, 2024
@r3v4s
Copy link
Contributor

r3v4s commented Oct 27, 2024

First of all, I know that this pr is sill working in progress, but couldn't wait for charming of supporting multiple grc20 tokens by default in gno-eco system(in this case grc20reg) and did some test.

Currently, grc20reg takes object from other realm to register it, and it seems like affects stack-frame. (tested with _test.gno)

On the other hand, testing with txtar slightly returns different result, cannot modify external-realm or non-realm object.
Is this due to #2743 you mentioned blocked by ??

@moul
Copy link
Member Author

moul commented Oct 27, 2024

Yes, this grc20reg is no longer a work in progress, but it depends on the changes in the PR, specifically those in #2743.

For a preview of fully working examples, you can check my meta PR #2551, which includes some VM fixes, this completed realm, and other realms that use it, such as the atomicswap realm, for instance.

@moul moul mentioned this pull request Nov 16, 2024
6 tasks
moul added a commit that referenced this pull request Dec 3, 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
#2743.
- [x] Create a demo in `atomicswap` or a similar application.
(#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 #2516  
NOT(!) depending on #2743

---------

Signed-off-by: moul <[email protected]>
@r3v4s
Copy link
Contributor

r3v4s commented Dec 6, 2024

While testing with current grc20reg working version(#3135), I come up with few questions.

  1. How are we(as gnoland) going to handle tokens that aren't registered to grc20reg?
  • if registering token (to grc20reg) become as mandatory(for grc20 spec) you can ignore this question.
  1. First of all, I thought re-entrancy might be possible because grc20reg allows us to call certain contract's function without import it, so tested with txtar.
  • found out that calling transfer using grc20reg, is transfer from teller so re-ent isn't possible.

However, if token contract has certain logic located at before or after calling transfer(e.g), (since calling transfer by grc20reg calls teller.transfer) that logic won't be executed.

What do you think of this case??

@Gno2D2
Copy link
Collaborator

Gno2D2 commented Dec 6, 2024

I'm a bot that assists the Gno Core team in maintaining this repository. My role is to ensure that contributors understand and follow our guidelines, helping to streamline the development process.

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

Manual Checks

No manual checks match this pull request.

Debug
Automated Checks
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 not satisfied
└── 🔴 Head branch (moul:dev/moul/grc20reg) is up to date with base (master): behind by 60 / ahead by 72

@moul
Copy link
Member Author

moul commented Dec 6, 2024

You should either use the Approve pattern before Transfer or expect the user to use RealmTeller.

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
📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: No status
Status: 🎯 Current Topics
Status: Triage
Development

Successfully merging this pull request may close these issues.

5 participants