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: grc20 dynamic call with registered interface #1275

Conversation

r3v4s
Copy link
Contributor

@r3v4s r3v4s commented Oct 21, 2023

allow certain contract to call grc20 functions(Transfer,TransferFrom,BalanceOf) without import by using registered interface

// co-worker @mconcat

relate pr

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
  • Added new benchmarks to generated graphs, if any. More info here.

@r3v4s r3v4s requested a review from a team as a code owner October 21, 2023 15:22
@github-actions github-actions bot added 🧾 package/realm Tag used for new Realms or Packages. 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Oct 21, 2023
@codecov
Copy link

codecov bot commented Oct 21, 2023

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 56.66%. Comparing base (a2b5f29) to head (36ef3bb).
Report is 596 commits behind head on master.

Files with missing lines Patch % Lines
gnovm/pkg/gnolang/realm.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1275      +/-   ##
==========================================
+ Coverage   56.17%   56.66%   +0.49%     
==========================================
  Files         439      425      -14     
  Lines       66242    64786    -1456     
==========================================
- Hits        37209    36709     -500     
+ Misses      26143    25225     -918     
+ Partials     2890     2852      -38     
Flag Coverage Δ
go-1.21.x ∅ <ø> (∅)
misc ∅ <ø> (∅)
misc-_test.genstd ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@n0izn0iz
Copy link
Contributor

Also related to #1072 I believe

@harry-hov
Copy link
Contributor

@r3v4s you might need to run update gno.mod files. CI will start complaining once you rebase it with master

You have 4 ways:

Option1:

Check imports and update each `gno.mod` manually

Option2:

Run `gno mod tidy` per package. 

Option3:

$ cd examples/gno.land/r/x/grc20_dynamic_call
$ find . -name "gno.mod" -execdir gno mod tidy \; 

Option4:

$ cd examples
$ make tidy

I would personally prefer Option4 :)

@r3v4s r3v4s force-pushed the feat/grc20-dynamic-call-with-registered-interface branch from 537bf53 to db05596 Compare December 7, 2023 05:08
@r3v4s r3v4s marked this pull request as draft December 7, 2023 05:14
@r3v4s
Copy link
Contributor Author

r3v4s commented Dec 7, 2023

converting to draft to fix testcase

Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Thank you for this detailed PR 🙏

Please check out @harry-hov's comments about the unused import, otherwise it looks good 💯

examples/gno.land/r/x/grc20_dynamic_call/foo/foo.gno Outdated Show resolved Hide resolved
@@ -0,0 +1,130 @@
package baz

import (
Copy link
Member

Choose a reason for hiding this comment

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

I suggest using either a bar or baz instead of exposing public methods. Create a safe object with the following code:

var baz = grc20.NewAdminToken(...)
var Baz = baz.GRC20()
// call Baz.Approve, etc directly

This recommendation reduces the codebase and eliminates redundancy. It demonstrates that the registry can function with standard public-facing grc20 implementations or grc20 objects. Ideally, keep a single foo file with public methods, while keeping the other files around 30 lines long. This approach will make the example more powerful and concise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

calling Baz.Transfer() does panic VM call panic: cannot modify external-realm or non-realm object because of updating its token balance.

as @tbruyelle mentioned, #1257 fix need to be settle down for this pr to use grc20 object instead of public methods

@tbruyelle
Copy link
Contributor

tbruyelle commented Jan 12, 2024

@r3v4s Why do you need this wrapper ? Why not calling directly register in the related contract (foo/bar/baz) ?

Is this to avoid this error : VM call panic: cannot modify external-realm or non-realm object ?

Because the bug behind this error is fixed here #1257.

@r3v4s r3v4s force-pushed the feat/grc20-dynamic-call-with-registered-interface branch from 900da31 to 36ef3bb Compare February 13, 2024 12:19
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Feb 13, 2024
@n0izn0iz n0izn0iz mentioned this pull request Jul 5, 2024
7 tasks
@moul moul mentioned this pull request Jul 5, 2024
@thehowl
Copy link
Member

thehowl commented Nov 2, 2024

Is this PR still relevant?

@r3v4s
Copy link
Contributor Author

r3v4s commented Nov 3, 2024

Is this PR still relevant?

#2516 implements more simple way, therefore closing this issue.

@r3v4s r3v4s closed this Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: Done
Status: 🌟 Wanted for Launch
Status: Done
Status: No status
Development

Successfully merging this pull request may close these issues.

8 participants