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(stdlibs/std)!: namespace minted coins with realm path #875

Merged
merged 16 commits into from
May 14, 2024

Conversation

r3v4s
Copy link
Contributor

@r3v4s r3v4s commented Jun 7, 2023

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 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

  • 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

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

@github-actions github-actions bot added 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 🤖 gnovm Issues or PRs gnovm related labels Jun 7, 2023
@r3v4s r3v4s changed the title Fix/gnot mintable fix coins are being minted by banker Jun 7, 2023
@r3v4s r3v4s changed the title fix coins are being minted by banker Change denom of coin that minted by banker Jun 13, 2023
@r3v4s r3v4s marked this pull request as ready for review June 13, 2023 03:59
@r3v4s r3v4s requested review from jaekwon and moul as code owners June 13, 2023 03:59
gnovm/stdlibs/std/banker.gno Outdated Show resolved Hide resolved
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.

Hi @r3v4s, we discussed on how to best tackle this, both to try avoiding duplicating std into internal/std and also to have the code here effectively make sense.

Let's work to do this instead:

  • Remove stdlibs/internal/std and the current changes to stdlibs.go (to add PrevRealm to internal/std)
  • Create a new type of banker inside of stdlibs/banker.go, called RealmIssueBanker.
    • This shall be returned by the native GetBanker with BankerType BankerTypeRealmIssue
    • The realm is initialized with the PrevRealm values pkgPath and addr. Maybe split out the internal code of PrevRealm (with the for-loop etc.) into a function which returns 2 strings: (pkgPath, addr string)
  • When RealmIssueBanker.IssueCoins is called, you add the code you inserted into gnovm/stdlibs/std/banker.gno, and you remove it from there.

WDYT?

Thanks!

@r3v4s
Copy link
Contributor Author

r3v4s commented Sep 8, 2023

@thehowl Looking good! That will remove inevitable use of code redundancy and logic will be more simple. Will work on that

// UPDATE
stdlibs.go already had BankerTypeRealmIssue type which native GetBanker was returning banker it self. I couldn't find any good reason to return banker, so changed it to return RealmIssueBanker

@r3v4s r3v4s requested a review from a team as a code owner September 15, 2023 11:31
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Sep 15, 2023
@r3v4s r3v4s changed the title Change denom of coin that minted by banker feat: change denom of coin that minted by banker Sep 15, 2023
@r3v4s r3v4s marked this pull request as draft March 29, 2024 08:02
@r3v4s r3v4s marked this pull request as ready for review March 29, 2024 13:01
@r3v4s r3v4s requested a review from thehowl March 29, 2024 13:01
@harry-hov harry-hov self-requested a review April 8, 2024 13:44
Copy link
Contributor

@leohhhn leohhhn left a comment

Choose a reason for hiding this comment

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

Hey @r3v4s, I left some comments, please check them out 🙏

@thehowl, can you take a look at this one final time so that we can get it merged?

gnovm/stdlibs/std/banker.go Outdated Show resolved Hide resolved
@r3v4s r3v4s requested a review from a team as a code owner May 7, 2024 03:20
@thehowl thehowl mentioned this pull request May 7, 2024
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.

I am approving these changes, because this PR has been stuck in review forever and it already does an important step in the right direction. Please, tackle these last comments and we're good to go. Sorry for the long wait.

That said, soon after this is merged, I think we'll need to change this API again. This current implementation creates an inconsistency within the Banker API. All other banker methods now require you to pass in the full realm path to the token you're referring to, but IssueCoin and RemoveCoin do not.

Thus, I think a few more changes are in order:

  1. There should be a RealmDenom(pkgpath, denom string) function in std, which creates a realm denomination (ie. /gno.land/r/morgan:bitcoin). There can be a helper method Realm.Denom(denom string) (so you can do std.CurrentRealm().Denom("bitcoin")
  2. Instead of modifying denom's value in the native function, we should check it matches what we expect. ie. strings.HasPrefix(denom, RealmDenom(std.CurrentRealm().PkgPath()), then check the last part of the denom to see that it matches the Gno regex. (This can all be done in gno, without needing to put it in native code)

This should make it more consistent & easier to use realm denominations. Still, it's a UX issue, while this one tackles what is effectively a security issue; so let's start from here.

gnovm/stdlibs/std/banker.go Outdated Show resolved Hide resolved
tm2/pkg/std/coin.go Outdated Show resolved Hide resolved
@thehowl thehowl changed the title feat: change denom of coin that minted by banker feat(stdlibs/td)!: namespace minted coins with realm path May 7, 2024
@thehowl thehowl changed the title feat(stdlibs/td)!: namespace minted coins with realm path feat(stdlibs/std)!: namespace minted coins with realm path May 8, 2024
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.

Sorry. The test LGTM

gnovm/stdlibs/std/banker.go Outdated Show resolved Hide resolved
gnovm/stdlibs/std/banker.go Outdated Show resolved Hide resolved
@zivkovicmilos zivkovicmilos merged commit c30d0f1 into gnolang:master May 14, 2024
222 checks passed
jefft0 pushed a commit to jefft0/gno that referenced this pull request May 15, 2024
<!-- Please provide a brief summary of your changes in the Title above
-->
# 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]>
harry-hov pushed a commit that referenced this pull request May 16, 2024
Introduced in #875, but only managed to review after it was merged.
leohhhn pushed a commit to leohhhn/gno that referenced this pull request May 21, 2024
…#2106)

Introduced in gnolang#875, but only managed to review after it was merged.
thehowl added a commit that referenced this pull request May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ 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: Done
Status: 🚀 Needed for Launch
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants