feat: token hub #3479
feat: token hub #3479
Conversation
github-actions
bot
added
the
🧾 package/realm
🛠 PR Checks Summary🔴 Pending initial approval by a review team member (and label matches review triage state) Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
|
Check this out, it's a similar idea: https://github.com/gnolang/gno/blob/master/examples/gno.land/r/demo/grc20reg/grc20reg.gno |
Thanks for pointing out, I will consider implementation for different token types |
-change tokenhub to be a hub for nfts rather than a hub for fungible tokens
…okens - match registering grc721 to grc20 registration which now go by pck path (and custom slug) instead of symbol - combine the two in the token hub
…ion to grc20reg.gno but this might help with iteration in that realm as well)
-update the getbalances functions so they're easier to parse
There was a problem hiding this comment.
Good start but there's more to be worked on. Check out the comments I left below.
When resolving comments, try to make commits that correspond to each comment, where it makes sense. This way the reviewer can see that you solved their exact concern, instead of making them read the whole code again.
| pageSize = 2 | ||
| test = "" |
There was a problem hiding this comment.
pageSize can be const, test should be removed i assume
There was a problem hiding this comment.
Fixed this in 8c09725
| ErrMTAlreadyRegistered = errors.New("Multi-token already registered") | ||
| ErrMTNotFound = errors.New("Multi-token not found") | ||
| ErrMTInfoNotFound = errors.New("Multi-token info not found") |
There was a problem hiding this comment.
in case the error msg does not start with some abbreviation, line "NFT", that is usually in uppercase, err msgs should start in lowercase
There was a problem hiding this comment.
noted. see 88b0a7e for the fix
|
|
||
| // registering a collection of NFTs | ||
| func RegisterNFT(nftGetter grc721.NFTGetter, collection string, tokenId string) error { | ||
|
|
There was a problem hiding this comment.
removed it in 1c4b72a
| nftOwner, err := nft.OwnerOf(grc721.TokenID(tokenId)) | ||
| if string(nftOwner) == "" || err != nil { | ||
| return ErrNFTtokIDNotExists | ||
| } |
There was a problem hiding this comment.
if nftOwner is std.Address, and I assume it is, use address.IsValid()
There was a problem hiding this comment.
good point, fixed this in : 005e64b
| func RegisterToken(tokenGetter grc20.TokenGetter, slugs ...string) { | ||
| slug := "" | ||
| if len(slugs) > 0 { | ||
| slug = slugs[0] | ||
| } | ||
| grc20reg.Register(tokenGetter, slug) | ||
| } |
There was a problem hiding this comment.
I don't get why there can be multiple slugs. Can you elaborate?
If there is a reason for them to exist, you're basically ignoring all but the first one, for some reason.
There was a problem hiding this comment.
Now I realize this might be an overkill, but what I was going for is this: slugs should be either 0 length or 1, if it is more the rest is ignored. I changed this to just be one string in : 005e64b
| type GRC1155TokenInfo struct { | ||
| Collection grc1155.MultiTokenGetter | ||
| TokenID string | ||
| } |
There was a problem hiding this comment.
I'm wondering if this is the right place for this. Is it specific to your hub, or maybe should it be something that exists in the 1155 package itself? Put some godoc as to why you need this.
There was a problem hiding this comment.
Actually, I think grc1155 need pretty big changes as I was able to transfer other people's tokens using this getter so I think adding this to it wouldn't make much difference.
I think tokenhub should keep this info for now as I expect having to change it in the future with the GRC1155 changes. Please correct me if this is a wrong approach
| } | ||
| sb.WriteString(md.BulletList(links)) | ||
|
|
||
| case strings.HasPrefix(path, "token"): // grc20 |
There was a problem hiding this comment.
If a string need a comment to explain it like this, maybe it's better to define it as a constant.
There was a problem hiding this comment.
fixed in 43b4b52
| } | ||
|
|
||
| func getBalances(input string) string { | ||
|
|
There was a problem hiding this comment.
removed in 1c4b72a
| registeredNFTs.Iterate("", "", func(key string, value interface{}) bool { | ||
| nftGetter := value.(grc721.NFTGetter) | ||
| nft := nftGetter() | ||
| key_parts := strings.Split(key, ".") | ||
| owner, err := nft.OwnerOf(grc721.TokenID(key_parts[len(key_parts)-1])) | ||
| if err == nil && addr == owner { // show only the nfts owner owns | ||
| balances.Set("nft:"+key, 1) | ||
| } | ||
| return false | ||
| }) | ||
|
|
||
| grc20reg.Iterate(func(key string, tokenGetter grc20.TokenGetter) bool { | ||
| token := tokenGetter() | ||
| balance := token.BalanceOf(addr) | ||
| balances.Set("token:"+key, balance) | ||
| return false | ||
| }) | ||
|
|
||
| registeredMTs.Iterate("", "", func(key string, value interface{}) bool { | ||
| info := value.(GRC1155TokenInfo) | ||
| mt := info.Collection() | ||
| balance, err := mt.BalanceOf(addr, grc1155.TokenID(info.TokenID)) | ||
| if err == nil { | ||
| balances.Set("mt:"+key, balance) | ||
| } | ||
| return false | ||
| }) |
There was a problem hiding this comment.
This will be a pretty bad case when your hub gains popularity. You will most likely break gas limits.
Try to optimize this somehow, split it up, not sure exactly how. Iterating trees (especially 3 of them), should be avoided wherever possible.
There was a problem hiding this comment.
As we agreed in person, the only thing I could to to optimize this is iterate the lists separately instead of all at once. This was done in 9b9767f
| func getAddressForUsername(addrOrName string) std.Address { | ||
| addr := std.Address(addrOrName) | ||
|
|
||
| if addr.IsValid() { | ||
| return addr | ||
| } | ||
| if user := users.GetUserByName(addrOrName); user != nil { | ||
| return user.Address | ||
| } | ||
|
|
||
| return "" | ||
| } |
There was a problem hiding this comment.
| func getAddressForUsername(addrOrName string) std.Address { | |
| addr := std.Address(addrOrName) | |
| if addr.IsValid() { | |
| return addr | |
| } | |
| if user := users.GetUserByName(addrOrName); user != nil { | |
| return user.Address | |
| } | |
| return "" | |
| } | |
| func getAddressForUsername(addrOrName string) std.Address { | |
| addr := std.Address(addrOrName) | |
| if addr.IsValid() { | |
| return addr | |
| } | |
| if user := users.GetUserByName(addrOrName); user != nil { | |
| return user.Address | |
| } | |
| return "" | |
| } |
There was a problem hiding this comment.
Is this just a suggestion to move \n? If so - fixed in 387ebd9
- use addr.isvalid
- remove comments
…ller address rename getusrebalancesall to withoutzeros
Gno2D2
added
the
review/triage-pending
…(and a simple test)
- instead of iteration in grc20reg use read only tree from avl
-remove test
All comments have been addressed and changes made. I have also made tests for new funcs. Can you please check out the "Question" section of the PR description and maybe provide an opinion 🙏 Thanks!! |
Description
Token hub is a registry for tokens and NFTs created on gno.land. Since all the tokens are visible in each their own realms, as it is right now there is no easy way that a user can check which tokens belong to him by his address. Token hub bypasses this by concentrating each each token in it's own realm and allows operations over these tokens providing a easier way to communicate with the realms in which the tokens are originally made. Since grc20 registry already exists the idea is to combine it's functionality with similar concept used for other tokens to create a realm that will contain information about all tokens, this way you will be able to get all your token balances in one place.
How it works
In the realm where token is being made, tokenhub needs to be imported and the newly made token has to be registered to the token hub by using RegisterToken function and passing the token as an argument. This is everything token hub needs to be able to operate. Simiraly how it is done here for GRC20 token hub registers a pointer to the token in it's registry.
More Details
In order to make this work I had to make minor additions to other realms as one would probably notice in the code - I had to add Getters (pointers) in the grc721 and grc1155 so storing the token instance itself is avoided, like it is done with GRC20 for grc20reg.gno purposes. I also needed to add Iterate and IterateN functions to grc20 only to be able to get the tokens of that registry which shouldn't undermine any of it's functionality or security.
Contributor's checklist
Dillema
Since the hub uses existing grc20reg and extends it's usage, if a token is registered through the hub it would mean an incorrect realm path would be written in the grc20reg since the prevrealm would now point to the hub. I have not thought of an ideal solution to this, the best I got is transfering the whole realm to the /r/demo/grc20reg package, if I got the author's permission (@moul)
(because of this probelm this is not yet ready to be merged but I would like an opinion)
All feedback is welcome