fix: minting additional GNOTs #393

dongwon8247 · 2022-11-19T14:01:50Z

I found two significant issues regarding minting and burning tokens with the package minter.

Issue Analysis

GNOTs can be minted additionally

Balance before minting
Balance after minting
TX Log: https://gnoscan.io/test3/contract/55048_0 (Click Show Logs)
Newly minted GNOTs can be transferred to other addresses.

Existing GNOTs can be burned

Tx log: https://gnoscan.io/test3/contract/55233_0
If you put others' addresses in arg, it can also burn their existing GNOTs.

Solution

For the additional minting of GNOTs, I Initially tried to make an exception in the Banker logic here pkgs/sdk/vm/builtins.go but It seems that it's set up to provide standard functions for common & consistent contract development. so instead I've made an exception in pkgs/std/coin.go to solve the problem with this PR.
For being able to burn GNOTs, I don't have a clear solution yet, but it is necessary to check if the caller address can be found in the variables of SDKBanker -> VMKeeper type in RemoveCoin. In the future, it would be best if we can prevent users from calling IssueCoin, RemoveCoin (Change to authentication & authority check or switch it to internal functions).

dongwon8247 · 2022-11-19T14:07:32Z

package minter source found in Gnoscan

package minter

import (
	"std"
	"gno.land/p/demo/avl"
)

var (
	denoms = avl.NewMutTree() // id -> *minter
)

func Mint(addr std.Address, denom string, amount int64) {
	caller := std.GetOrigCaller()
	if denoms.Has(denom) {
		data, _ := denoms.Get(denom)
		minter := data.(std.Address)
		if minter != caller {
			println(minter)
			println(caller)
			panic("not minter")
		}
	} else {
		denoms.Set(denom, caller)
	}

	issuer := std.GetBanker(std.BankerTypeRealmIssue)
	issuer.IssueCoin(addr, denom, amount)
}
// original code
// func Burn(denom string, amount int64) {
// 	caller := std.GetOrigCaller()
// 	issuer := std.GetBanker(std.BankerTypeRealmIssue)
// 	issuer.RemoveCoin(caller, denom, amount)
// }

// what we modified to test to burn other wallet's tokens
func Burn(addr std.Address, denom string, amount int64) {
	caller := std.GetOrigCaller()
	issuer := std.GetBanker(std.BankerTypeRealmIssue)
	issuer.RemoveCoin(addr, denom, amount)
}


func UpdateMinter(denom string) {
	caller := std.GetOrigCaller()
	if !denoms.Has(denom) {
		panic("no such denom")
	}

	data, _ := denoms.Get(denom)
	minter := data.(std.Address)
	if minter != caller {
		panic("not minter")
	}
	denoms.Set(denom, caller)
}

moul · 2022-11-20T17:25:16Z

Hey @dongwon8247, thank you.

I need to dig more this one.

I think there is a bug for minting, but not sure about the one for burning.
If this is just that we can burn tokens, I think this is OK.
But if we can burn non-owned tokens, we’ve another issue, for sure.

anarcher · 2022-11-21T04:12:07Z

I'm not sure but std.BankerTypeRealmIssue seems to return native Banker. With this banker, It looks like that any realms can issue and burn any coin.
What does sdk.BankerTypeRealmIssue mean? Realms can issue and burn native coins (not grc20)? :-)

piux2 · 2022-11-22T09:25:21Z

BankerTypeRealmIssue

I'm not sure, but std.BankerTypeRealmIssue seems to return native Banker. With this banker, It looks like any realm can issue and burn any coin. What does sdk.BankerTypeRealmIssue mean? Realms can issue and burn native coins (not grc20)? :-)

It seems to break the invariance of the total supply
If we cannot find a specific use case in the Smart Contract, shall we remove the BankerTypeRealmIssue for now? We can always add it back correctly when we see good use cases.

r3v4s · 2022-11-22T09:57:41Z

BankerTypeRealmIssue

I'm not sure, but std.BankerTypeRealmIssue seems to return native Banker. With this banker, It looks like any realm can issue and burn any coin. What does sdk.BankerTypeRealmIssue mean? Realms can issue and burn native coins (not grc20)? :-)

It seems to break the invariance of the total supply If we cannot find a specific use case in the Smart Contract, shall we remove the BankerTypeRealmIssue for now? We can always add it back correctly when we see good use cases.

It breaks privileges system too since it can burn non-owned tokens not only invariance of the total supply.
Since we're on testnet, I think we can find ways to fix without removing it
However removing it for now, will be primary but most obvious way to proceed

dongwon8247 · 2022-11-22T10:19:47Z

I think we need a clear direction for this.

Do we want to have both native tokens (other than GNOT) and GRC20 tokens?

If we think about GRC20 tokens being able to use IBCs seamlessly, it's better to have both native tokens & GRC20 tokens together. Otherwise, we'd need some kind of a bridge for GRC20 tokens to be able to use IBC, If I'm understood correctly.

Do we want to have one native token and GRC20 for other tokens? Like Ethereum where only $ETH exists as the native token and other fungible tokens are all ERC20.

This is the easier way for now. but I'm not sure it helps scalability as a L1 smart contract platform. It can be inconvenient for GRC20 tokens too, in my opinion.

What do you think? @jaekwon @moul

moul · 2022-11-22T15:02:24Z

Do we want to have both native tokens (other than GNOT) and GRC20 tokens?

Yes, it should be possible for the chain to have multiple native tokens. I.e.: IBC tokens, second native token (GNOSH, PHOTON, other dual-token approaches).

If we think about GRC20 tokens being able to use IBCs seamlessly, it's better to have both native tokens & GRC20 tokens together. Otherwise, we'd need some kind of a bridge for GRC20 tokens to be able to use IBC, If I'm understood correctly.

This is a complex topic that needs to be addressed differently. Maybe it will be supported with IBC2; maybe with a custom gnoland contract that will act like an escrow; maybe we won't support GRC20 IBC transfers by default.

My feeling -> we'll write a r/gnoland/escrow contract and provide a transparent mapping for IBC1.

Do we want to have one native token and GRC20 for other tokens? Like Ethereum where only $ETH exists as the native token and other fungible tokens are all ERC20.

We need to technically support multiple native tokens, at least for IBC.
We can still encourage people to use GRC20. But not even sure that it's better to encourage it.
At this point, I can say that we need to support multiple tokens, but don't have an opinion on what should be suggested.
I think we'll discover during GameOfRealms what builders will do.

This is the easier way for now. but I'm not sure it helps scalability as a L1 smart contract platform. It can be inconvenient for GRC20 tokens too, in my opinion.

I'm not sure it's that hard, we're based on Tendermint core and have many Cosmos-SDK developers in the ecosystem that could help us improve this part.

anarcher · 2022-11-22T18:00:53Z

small idea.

denom: [email protected]/r/pkg

"BaseAccount": {
   "coins": "[email protected]/r/pkg",
}

std.BankerTypeRealmIssue has a wrapper with the native banker, simply the wrapper checks the caller's path with denom's path.

piux2 · 2022-11-22T18:47:38Z

After a bit more digging, I think Realm Issue Banks is used to mint native tokens from a smart contract, which the current IBC can support. Other chains do not need to integrate Gno VM to use tokens.

For other chains to use GRC20, IBC needs to support message calls through the memo field or some other field; other chains probably will need to integrate Gno VM to mirror the contract.

We can implement a RealmIssueBanker for the BankTypeRealmIssue instead of using the default SDKBanker. In RealmIssueBanker, we need to check OrigPkgAddress so that the issued token stays in the contract and ensure the denom is unique and owned by the contract. The denom part is not that straightforward and needs more thought. We can open an issue to spec it out.

piux2 · 2022-11-22T18:49:19Z

denom: [email protected]/r/pkg

There is a format regexp for the denom.

In Gno
// Denominations can be 3 ~ 16 characters long.
reDnmString = [a-z][a-z0-9]{2,15}
reDnm = regexp.MustCompile(fmt.Sprintf(^%s$, reDnmString))

In Cosmos SDK.

// Denominations can be 3 ~ 128 characters long and support letters, followed by either
// a letter, a number or a separator ('/', ':', '.', '_' or '-').
reDnmString = [a-zA-Z][a-zA-Z0-9/:._-]{2,127}

Since the main usage of RealmIssueBanker is issuing native tokens that can be used by other chains through IBC from a Smart Contract. We can do the following

match denom regexp validation with Cosmos SDK
base denom = "utoken/gno.land/r/pkg" or "utoken:gno.land/r/pkg"

As for IBC transfer, the denom is ibcDenom = "ibc/" + hash(trace path + "/" + base denom)
so we are covered.

@ALL

What do you all think?

moul · 2022-11-22T21:22:46Z

small idea.

denom: [email protected]/r/pkg
"BaseAccount": {
   "coins": "[email protected]/r/pkg",
}
std.BankerTypeRealmIssue has a wrapper with the native banker, simply the wrapper checks the caller's path with denom's path.

Yep, this is the idea.

If we keep the GRC20+IBC topic for later and focus on native token support improvements.

We need to add a prefix/suffix system so that a native coin belongs to its contract. I suggest prefixes to follow IBC + improve sorting.
Use this case as an example of code that should receive extra precautions when being developed. We could, for instance, by default prevent those calls from being called by another package and require the developer to explicitly allow more usages.

Let's fix 1. in this PR or another one, and keep 2. for a more extensive discussion.

anarcher · 2022-11-23T08:55:50Z

osmosis example:

gamm/pool/577 -> osmos's LP pool token
ibc/9712DBB13B9631EDFA9BF61B55F1B2D290B2ADB67E3A4EB3A875F3B6081B3B84 -> IRC transfered token (target chain)

"balances": [
        {
            "denom": "gamm/pool/577",
            "amount": "100"
        },
        {
            "denom": "ibc/9712DBB13B9631EDFA9BF61B55F1B2D290B2ADB67E3A4EB3A875F3B6081B3B84",
             "amount": "100"
        }
]

anarcher · 2022-11-23T09:02:39Z

Just for fun. (not serious :-)

denom: utoken:gno.land/r/pkg/...

giansalex · 2022-11-29T04:20:34Z

maybe BankerIssue was intended to mint tokens (e.g staking rewards), in this case, a whitelist could be added initially

issuerAllowed := []crypto.Address{
   gno.DerivePkgAddr("gno.land/r/system/minter"),
}

moul · 2022-11-29T08:58:31Z

maybe BankerIssue was intended to mint tokens (e.g staking rewards), in this case, a whitelist could be added initially
issuerAllowed := []crypto.Address{
   gno.DerivePkgAddr("gno.land/r/system/minter"),
}

On gno.land with proof-of-contributions, we won't use that feature; we don't want to mint additional gnot, but your suggestion makes sense for future Gnolang-based or Tendermint2-based chains.

Let's just support minting from any package with a prefix/suffix for this issue and consider extending the allowed issuer list later in another PR.

giansalex · 2022-11-30T03:44:57Z

On gno.land with proof-of-contributions, we won't use that feature; we don't want to mint additional gnot, but your suggestion makes sense for future Gnolang-based or Tendermint2-based chains.

How will the staking rewards work?

Let's just support minting from any package with a prefix/suffix for this issue and consider extending the allowed issuer list later in another PR.

and about Slashing... tokens need to be burned here, unless GNO handles it differently.

For minting new denoms, a realm could be created like TokenFactory module, and only this package would have access to BankerTypeRealmIssue

for Smart Contracts to issue native tokens and easy IBC adoption gnolang#393

piux2 · 2022-12-02T23:28:41Z

On gno.land with proof-of-contributions, we won't use that feature; we don't want to mint additional gnot, but your suggestion makes sense for future Gnolang-based or Tendermint2-based chains.

How will the staking rewards work?

Let's just support minting from any package with a prefix/suffix for this issue and consider extending the allowed issuer list later in another PR.

and about Slashing... tokens need to be burned here, unless GNO handles it differently.

For minting new denoms, a realm could be created like TokenFactory module, and only this package would have access to BankerTypeRealmIssue

The validator tokens to secure the POS chain through BFT are defined in the genesis file. The tokens for other purposes can be issued from the BankerTypeRealmIssue, which should be permissionless, just like ERC20. The BankerTypeRealmIssue-supported contracts serve the same function as TokenFactory Module since the business logic in the module-based AppChain is implemented as realm packages in the Gno-enabled chains.

It leads to another topic. Should we develop another set of token standards for Gno Native Token? ERC20 converted GRC20 is for a single native token chain like Ethereum. On the other hand, Gno supports multiple native tokens and can be transferred through IBC without other chains implementing GnoVM.

moul · 2023-01-20T10:38:19Z

How will the staking rewards work?

Blockchain fees, including gas fees, will be sent into a bucket.

Won't be staking. This bucket will regularly be distributed to:

members of the contributors DAO -- v1: tiered members, ; v2: $GNOSH token holders
package/realm authors -- licensing by usage; planned for later, probably after IBC2/ICS1 interchain licensing
validators -- in the validator set decided by the contributors DAO

moul · 2023-01-20T10:40:35Z

And about Slashing... tokens need to be burned here, unless GNO handles it differently.

Slashing is to be more defined, but we believe we won't have common issues because we're in a more trusted environment, kind of PoA backed by a trusted DAO.

But anyway, slashing methods will probably be done by sending tokens to a bucket instead of burning them.

I hope we'll have a better overview of what we can do in the future, during GoR, especially with #407.

moul · 2023-01-20T10:42:18Z

For minting new denoms, a realm could be created like TokenFactory module, and only this package would have access to BankerTypeRealmIssue

Not on gno.land instances. We'll only have realm-prefixed tokens.

But this feature makes sense for other instances running GnoVM in the future.

This PR should not make anything going in that direction, but anyone can open an issue right now, or wait that it makes more sense; when GnoVM will be more ready to be used to run new blockchains.

moul · 2023-01-20T10:43:35Z

It leads to another topic. Should we develop another set of token standards for Gno Native Token? ERC20 converted GRC20 is for a single native token chain like Ethereum. On the other hand, Gno supports multiple native tokens and can be transferred through IBC without other chains implementing GnoVM.

Sounds great, need more details about what you've in mind.

moul · 2023-01-20T10:45:40Z

So, to merge this PR, we should implement this -> #393 (comment)

And then, we can continue related discussions in a dedicated issue or new PRs.

dongwon8247 · 2023-06-01T08:19:22Z

Will reopen this PR soon

cc @r3v4s

r3v4s · 2023-06-05T02:57:47Z

Option 1

I think one of simple ways to fix this, we can just issue coin with denom with following format.

{denom_param_value} @ { called_address_or_pkgpath )
for example gnot issued from gno.land/r/minter => [email protected]/r/minter

we can derive second value perhaps from this pr #667

however, as @piux2 mentioned above there are some format regexp for the denom which I think very inflexible.

gno/tm2/pkg/std/coin.go

Lines 618 to 634 in 24d5493

    
           var ( 
        
           	// Denominations can be 3 ~ 16 characters long. 
        
           	reDnmString = `[a-z][a-z0-9]{2,15}` 
        
           	reAmt       = `[[:digit:]]+` 
        
           	reDecAmt    = `[[:digit:]]*\.[[:digit:]]+` 
        
           	reSpc       = `[[:space:]]*` 
        
           	reDnm       = regexp.MustCompile(fmt.Sprintf(`^%s$`, reDnmString)) 
        
           	reCoin      = regexp.MustCompile(fmt.Sprintf(`^(%s)%s(%s)$`, reAmt, reSpc, reDnmString)) 
        
           	reDecCoin   = regexp.MustCompile(fmt.Sprintf(`^(%s)%s(%s)$`, reDecAmt, reSpc, reDnmString)) 
        
           ) 
        
           func validateDenom(denom string) error { 
        
           	if !reDnm.MatchString(denom) { 
        
           		return fmt.Errorf("invalid denom: %s", denom) 
        
           	} 
        
           	return nil 
        
           }

I think we need new regex(or at least new standard) to fix this issue :D

Option2

once #384 merges then we can remove those regex conditions

r3v4s · 2023-06-07T09:37:40Z

new pr #875

@piux2

# Previous & Related PR #393 # Description ## BREAKING CHANGE: banker will not use raw(input) denom for minting Currently, there is a bug that additional coins are mintable using banker. Lots of discussing were made in #393, this PR include following. 1. It aims to use `pkg_path` that called (banker or Mint) as prefix for denom(Similar to IBC Spec) ``` ibc_denom := 'ibc/' + hash('path' + 'base_denom') denom_in_this_pr := '{pkg_path}:denom' ``` ~2. As @piux2 mentioned [here](#393 (comment)) currently gno has very inflexible format regexp for denom -- Changed to same as Cosmos's regex, but without capitals~ ~### is some issue about using `std.GetOrigCaller` or `std.PrevRealm` Some of you might be wonder why I made some packaged called `istd`, I made a issue: #876~ Update: After @thehowl's native binding PR, doesn't need this kind of work around ## Contributors Checklist - [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 - [ ] 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 - [ ] 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 --------- Co-authored-by: Morgan <[email protected]>

@piux2

# Previous & Related PR gnolang#393 # Description ## BREAKING CHANGE: banker will not use raw(input) denom for minting Currently, there is a bug that additional coins are mintable using banker. Lots of discussing were made in gnolang#393, this PR include following. 1. It aims to use `pkg_path` that called (banker or Mint) as prefix for denom(Similar to IBC Spec) ``` ibc_denom := 'ibc/' + hash('path' + 'base_denom') denom_in_this_pr := '{pkg_path}:denom' ``` ~2. As @piux2 mentioned [here](gnolang#393 (comment)) currently gno has very inflexible format regexp for denom -- Changed to same as Cosmos's regex, but without capitals~ ~### is some issue about using `std.GetOrigCaller` or `std.PrevRealm` Some of you might be wonder why I made some packaged called `istd`, I made a issue: gnolang#876~ Update: After @thehowl's native binding PR, doesn't need this kind of work around ## Contributors Checklist - [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 - [ ] 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 - [ ] 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 --------- Co-authored-by: Morgan <[email protected]>

moul mentioned this pull request Nov 29, 2022

exp: add std.ExecAsPkg #335

Closed

piux2 added a commit to piux2/gno that referenced this pull request Dec 2, 2022

Compatible native token denom

e8a9ddc

for Smart Contracts to issue native tokens and easy IBC adoption gnolang#393

piux2 mentioned this pull request Dec 2, 2022

extended native token denom #412

Closed

moul assigned dongwon8247 Jan 19, 2023

moul added the help wanted label Jan 19, 2023

moul requested review from moul and a team January 19, 2023 18:34

moul mentioned this pull request Mar 31, 2023

RFC Proposal for a new stack/frames management #683

Open

moul mentioned this pull request Apr 19, 2023

META contract-contract interactions on a single chain #757

Open

moul mentioned this pull request May 31, 2023

[META] 🏗4️⃣ test4.gno.land #362

Closed

5 tasks

dongwon8247 closed this Jun 1, 2023

dongwon8247 force-pushed the fix-minting-gnots branch 2 times, most recently from 12f7055 to 24d5493 Compare June 1, 2023 08:04

r3v4s mentioned this pull request Jun 7, 2023

feat(stdlibs/std)!: namespace minted coins with realm path #875

Merged

9 tasks

moul mentioned this pull request Jul 27, 2023

Banker should use PrevRealm instead of OrigPkgAddr #986

Closed

dongwon8247 mentioned this pull request Oct 16, 2023

Gno Core Issue Tracker gnoswap-labs/gno#7

Open

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: minting additional GNOTs #393

fix: minting additional GNOTs #393

dongwon8247 commented Nov 19, 2022

dongwon8247 commented Nov 19, 2022

moul commented Nov 20, 2022

anarcher commented Nov 21, 2022 •

edited

Loading

piux2 commented Nov 22, 2022

r3v4s commented Nov 22, 2022

dongwon8247 commented Nov 22, 2022 •

edited

Loading

moul commented Nov 22, 2022

anarcher commented Nov 22, 2022

piux2 commented Nov 22, 2022

piux2 commented Nov 22, 2022 •

edited

Loading

moul commented Nov 22, 2022

anarcher commented Nov 23, 2022 •

edited

Loading

anarcher commented Nov 23, 2022

giansalex commented Nov 29, 2022

moul commented Nov 29, 2022

giansalex commented Nov 30, 2022 •

edited

Loading

piux2 commented Dec 2, 2022 •

edited

Loading

moul commented Jan 20, 2023

moul commented Jan 20, 2023

moul commented Jan 20, 2023

moul commented Jan 20, 2023

moul commented Jan 20, 2023

dongwon8247 commented Jun 1, 2023

r3v4s commented Jun 5, 2023 •

edited

Loading

r3v4s commented Jun 7, 2023

fix: minting additional GNOTs #393

fix: minting additional GNOTs #393

Conversation

dongwon8247 commented Nov 19, 2022

Issue Analysis

Solution

dongwon8247 commented Nov 19, 2022

moul commented Nov 20, 2022

anarcher commented Nov 21, 2022 • edited Loading

piux2 commented Nov 22, 2022

r3v4s commented Nov 22, 2022

dongwon8247 commented Nov 22, 2022 • edited Loading

moul commented Nov 22, 2022

anarcher commented Nov 22, 2022

piux2 commented Nov 22, 2022

piux2 commented Nov 22, 2022 • edited Loading

moul commented Nov 22, 2022

anarcher commented Nov 23, 2022 • edited Loading

anarcher commented Nov 23, 2022

giansalex commented Nov 29, 2022

moul commented Nov 29, 2022

giansalex commented Nov 30, 2022 • edited Loading

piux2 commented Dec 2, 2022 • edited Loading

moul commented Jan 20, 2023

moul commented Jan 20, 2023

moul commented Jan 20, 2023

moul commented Jan 20, 2023

moul commented Jan 20, 2023

dongwon8247 commented Jun 1, 2023

r3v4s commented Jun 5, 2023 • edited Loading

Option 1

Option2

r3v4s commented Jun 7, 2023

anarcher commented Nov 21, 2022 •

edited

Loading

dongwon8247 commented Nov 22, 2022 •

edited

Loading

piux2 commented Nov 22, 2022 •

edited

Loading

anarcher commented Nov 23, 2022 •

edited

Loading

giansalex commented Nov 30, 2022 •

edited

Loading

piux2 commented Dec 2, 2022 •

edited

Loading

r3v4s commented Jun 5, 2023 •

edited

Loading