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(examples): wugnot (grc20’s wrapped ugnot) #1356

Merged
merged 5 commits into from
Dec 7, 2023

Conversation

moul
Copy link
Member

@moul moul commented Nov 10, 2023

This PR presents a GNOT wrapper that conforms to the GRC20 interface.

Optimally, the txtar file should be relocated to the wugnot directory, pending resolution of issue #1269.

Enabling the use of gnokey maketx call -func WUGNOT.Transfer over -func Transfer would streamline the wrapper, reducing it to just the initial 55 lines.

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.

Signed-off-by: moul <94029+moul@users.noreply.github.com>
@moul moul self-assigned this Nov 10, 2023
Copy link

netlify bot commented Nov 10, 2023

Deploy Preview for gno-docs2 canceled.

Name Link
🔨 Latest commit 7440f93
🔍 Latest deploy log https://app.netlify.com/sites/gno-docs2/deploys/654eacba4d85f70008cd7774

@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Nov 10, 2023
Copy link

codecov bot commented Nov 10, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (98f384e) 55.93% compared to head (7440f93) 55.93%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1356      +/-   ##
==========================================
- Coverage   55.93%   55.93%   -0.01%     
==========================================
  Files         420      420              
  Lines       65415    65415              
==========================================
- Hits        36592    36589       -3     
- Misses      25966    25969       +3     
  Partials     2857     2857              

see 2 files with indirect coverage changes

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

Signed-off-by: moul <94029+moul@users.noreply.github.com>
@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Nov 10, 2023
Signed-off-by: moul <94029+moul@users.noreply.github.com>
Signed-off-by: moul <94029+moul@users.noreply.github.com>
@moul moul marked this pull request as ready for review November 10, 2023 22:01
@moul moul requested a review from a team as a code owner November 10, 2023 22:01
@moul moul changed the title feat: wugnot (wrapped uGNOT) feat: wugnot (grc20’s wrapped ugnot) Nov 10, 2023
@moul moul changed the title feat: wugnot (grc20’s wrapped ugnot) feat(examples): wugnot (grc20’s wrapped ugnot) Nov 10, 2023
Signed-off-by: moul <94029+moul@users.noreply.github.com>
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.

Great work! LGTM 👍

@r3v4s
Copy link
Contributor

r3v4s commented Nov 11, 2023

code and txtar looks good!

@moul FYI, I've built similar logic in gnoswap

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.

Looks great 💯

I've cross checked it with a wrapped ETH contract, and it looks good.

The only thing I'm concerned about is about is having min / max limits, I don't understand the inherent need for them

Comment on lines +18 to +21
const (
ugnotMinDeposit uint64 = 1000
wugnotMinDeposit uint64 = 1
)
Copy link
Member

Choose a reason for hiding this comment

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

Why is there a min / max for wrapping ugnot?

@r3v4s
Copy link
Contributor

r3v4s commented Dec 1, 2023

this is wrapped ugnot token currently used by gnoswap.

seems like there are little bit difference between manfred's and gnoswap's

[manfred wrapping]

  • can be used by multiple realms
  • has general purpose
  • suitable to wrap coin/token from outer gno (ibc... bridge...)
  • user can use both native ugnot and wrapped ugnot.

[gnoswap wrapping]

  • can be used only by gnoswap contract
  • has single purpose which is only to support swap (used temporary during tx, final result will be always native coin)
  • user can use only native ugnot, not wrapped ugnot. (this is big advantage for better UI/UX in gnoswap-interface)

and of course, gnoswap can support manfred's method by using multi msg to wrap tokens then pass wrapped result into it, but since this process requires more functions to be executed it will spend more gas-fee.

@moul @zivkovicmilos
just wanna ask your opinions about this

cc @dongwon8247 @jinoosss @notJoon

@leohhhn
Copy link
Contributor

leohhhn commented Dec 5, 2023

Regarding the general native currency/token/coin discussion, here are a few points from a discussion @dongwon8247 and I had today. Bear in mind I might not have the most accurate view on this topic regarding how coins work, or how leadership has imagined this topic would be implemented, so please correct me if I'm wrong.

As far as I understand, Gno.land currently has a single coin (struct), GNOT (the staking token), issued by the native banker. This would be the native currency of Gno, as is ETH for Ethereum.

Then, we have the GRC20 standard. This allows us to have a token that conforms to a the ERC20 standard. I propose we call these tokens, as is in Ethereum, for clarity.

An example of this can be the WUGNOT token (@moul the ticker WGNOT makes more sense IMHO, as you wouldn't do a wrapped wei, but just a wrapped ETH), or i.e. my own token, $LEON. As this token conforms to the GRC20 standard, if you pair it up with another GRC20 token, you will be able to create GnoSwap pools with it (i.e. $LEON <> $WUGNOT pool).

Now, @r3v4s, GnoSwap's aim is to make the UX for users better than it currently is with Ethereum when using an exchange (i.e. Uniswap), which is that in order to swap the native currency, ETH, with something else, you first need to wrap it with the WETH contract, and then swap. Because the swap is 2 seperate transactions in itself (approve > transferFrom), this would require a total of 3 transactions, making the UX a little clunky.

The idea to solve this with GnoSwap is to wrap GNOT under the hood into their version of WGNOT. This will be invisible to end users and any time users want to do anything with GNOT, either swap or LP, the UI will simply display all callable functions with the native GNOT. I think this makes sense on the UX side, but I do think this needs to be documented for users that are tech-oriented, so that no confusion/controversy arises. Also, I am sure that further down the line there will be an exchange that will not have this approach.

Here are some general points of discussion, which I hope to clarify and clearly document afterwards:

  • Will Gno.land issue more native coins via BankerTypeRealmIssue (if that is the way you use this type of Banker)?
  • How does issuing tokens using that type of banker work? Do you need to make a realm that issues the native coin? One realm per one coin?
  • What are the functionalities of coins issued by the BankerTypeRealmIssue? How do they function?
  • Let’s clearly define what currency/coin/token is, and I will document it and it will remove any confusion once and for all (or at least until something changes :)

If you believe this discussion should be taken somewhere else, please point me to where 👍

cc @moul @r3v4s

@r3v4s
Copy link
Contributor

r3v4s commented Dec 5, 2023

Hello @leohhhn, I really didn't think about documenting this kind of under hood stuff for tech-orinted user. Thx for pointing this

Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

lgtm otherwise

}

caller := std.PrevRealm().Addr()
pkgaddr := std.GetOrigPkgAddr()
Copy link
Member

Choose a reason for hiding this comment

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

not CurrentRealm().Addr()?

@thehowl
Copy link
Member

thehowl commented Dec 7, 2023

@leohhhn I believe I partly already answered your questions elsewhere, but I'll try to answer what I can at the best of my knowledge.

Here are some general points of discussion, which I hope to clarify and clearly document afterwards:

  • Will Gno.land issue more native coins via BankerTypeRealmIssue (if that is the way you use this type of Banker)?

Yes. My understanding is that the native tokens are the ones which will eventually be exchangeable through IBC. Naturally, as this realm is wrapping the native token ugnot to GRC20, the opposite should also be doable.

  • How does issuing tokens using that type of banker work? Do you need to make a realm that issues the native coin? One realm per one coin?

See-also #986. Native tokens will be namespaced: r/morgan and r/leon can both mint a token with denomination ugnot, but they are != and they are also != to the "real" ugnot, imagine the realm path as being a prefix: gno.land/r/morgan.ugnot != gno.land/r/leon.ugnot != ugnot.

  • What are the functionalities of coins issued by the BankerTypeRealmIssue? How do they function?

It's still fake internet money as always, but it can work on IBC. Additionally, I think Manfred mentioned something about the native token being more secure, and likely the go-to choice that realms should use eventually.

  • Let’s clearly define what currency/coin/token is, and I will document it and it will remove any confusion once and for all (or at least until something changes :)

as far as I can tell, mostly synonyms? maybe "currency" implies that the token has some clout.

I'd avoid calling native tokens "coins" and grc20 tokens "tokens", because I think it leads to more confusion and as this PR shows creating a realm to convert back and forth is trivial. When you need to specify, call them grc20/native.

@moul
Copy link
Member Author

moul commented Dec 7, 2023

Merging, and will reply to the native tokens and GRC20 questions in the effective gno documentation next week.

@moul moul merged commit a338929 into gnolang:master Dec 7, 2023
189 checks passed
@moul moul deleted the dev/moul/wugnot branch December 7, 2023 22:19
@dongwon8247
Copy link
Member

dongwon8247 commented Dec 13, 2023

this is wrapped ugnot token currently used by gnoswap.

seems like there are little bit difference between manfred's and gnoswap's

[manfred wrapping]

can be used by multiple realms
has general purpose
suitable to wrap coin/token from outer gno (ibc... bridge...)
user can use both native ugnot and wrapped ugnot.
[gnoswap wrapping]

can be used only by gnoswap contract
has single purpose which is only to support swap (used temporary during tx, final result will be always native coin)
user can use only native ugnot, not wrapped ugnot. (this is big advantage for better UI/UX in gnoswap-interface)

We have decided to use this Manfred's wgunot realm to support wrapping for $GNOT in Gnoswap. The reasons are:

  • This wugnot realm has general-purpose usage (ugnot -> wugnot, wugnot -> ugnot).
  • It's more scalable, given that we can apply the same logic to all native coins (IBC, etc) in the future.
  • Gas spending wouldn't be a problem, given how Uniswap/Osmosis is operating.

gfanton pushed a commit to moul/gno that referenced this pull request Jan 18, 2024
This PR presents a GNOT wrapper that conforms to the GRC20 interface.

Optimally, the txtar file should be relocated to the wugnot directory,
pending resolution of [issue
gnolang#1269](gnolang#1269 (comment)).

Enabling the use of `gnokey maketx call -func WUGNOT.Transfer` over
`-func Transfer` would streamline the wrapper, reducing it to just the
initial 55 lines.

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

<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
- [x] Provided any useful hints for running manual tests
- [x] 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 <94029+moul@users.noreply.github.com>
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 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: ✅ Done
Status: 🌟 Wanted for Launch
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants