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: Multi Token Standard Contract [NEP] #245

Merged
merged 108 commits into from
Apr 4, 2022
Merged

Conversation

zcstarr
Copy link
Contributor

@zcstarr zcstarr commented Jul 28, 2021

This is a pr for the NEP for a new Multi Token Standard

@zcstarr
Copy link
Contributor Author

zcstarr commented Jul 28, 2021

Issue no: #246

@bowenwang1996
Copy link
Collaborator

Please make the title of the PR more descriptive. Thanks!

@zcstarr zcstarr changed the title feat: initial NEP feat: Multi Token Standard Contract [NEP] Jul 28, 2021
@danielwpz
Copy link
Contributor

any plan on when to merge this?

@zcstarr
Copy link
Contributor Author

zcstarr commented Aug 16, 2021

any plan on when to merge this?

Actually we're having a meeting tomorrow to just go over the game plan for this NEP, and fix up any outstanding issues with it. so. I'll ping you here with an update post that.

@mikedotexe
Copy link
Contributor

Would be great to have a version at the top, probably 1.0.0 like we have in FT here:
https://github.com/near/NEPs/blame/273aa15150c1bd804ce8b1f47b45cac1257804e1/specs/Standards/FungibleToken/Core.md#L3

Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

Comments and some suggestions added throughout. One thing that I think would be nice is if we used 'real looking' account / contract names with their suffixes on, rather than abbreviations, but if we're avoiding using a properly suffixed account to avoid using someone's actual account in our examples, feel free to ignore those suggestions :)

specs/Standards/MultiToken/README.md Outdated Show resolved Hide resolved
specs/Standards/MultiToken/README.md Outdated Show resolved Hide resolved
specs/Standards/MultiToken/README.md Outdated Show resolved Hide resolved
specs/Standards/MultiToken/README.md Outdated Show resolved Hide resolved
specs/Standards/MultiToken/README.md Outdated Show resolved Hide resolved
specs/Standards/MultiToken/README.md Outdated Show resolved Hide resolved
specs/Standards/MultiToken/README.md Outdated Show resolved Hide resolved
specs/Standards/MultiToken/README.md Outdated Show resolved Hide resolved
specs/Standards/MultiToken/README.md Outdated Show resolved Hide resolved
specs/Standards/MultiToken/README.md Outdated Show resolved Hide resolved
@frol
Copy link
Collaborator

frol commented Sep 14, 2021

I have only scanned through the proposal. I have a few questions:

  • I wonder if we want to shrink the scope by only defining batch interfaces? (one can always use batch API to send a single operation and given the nature of this standard implying usage of multiple tokens, I feel that it is expected to have a batch-oriented interface)
  • Do I understand correctly that this standard wants to supersede FT and NFT standards?

I see that you postponed the decision about events, but that is a crucial part to get any standard to be adopted by off-chain services (we struggle with it for FT / NFT standards at the moment: #254 (comment))

@zcstarr
Copy link
Contributor Author

zcstarr commented Sep 14, 2021

I have only scanned through the proposal. I have a few questions:

  • I wonder if we want to shrink the scope by only defining batch interfaces? (one can always use batch API to send a single operation and given the nature of this standard implying usage of multiple tokens, I feel that it is expected to have a batch-oriented interface)
  • Do I understand correctly that this standard wants to supersede FT and NFT standards?

I see that you postponed the decision about events, but that is a crucial part to get any standard to be adopted by off-chain services (we struggle with it for FT / NFT standards at the moment: #254 (comment))

Yep we discussed shrinking the api and just having a single interface, but we wanted to stay some what familiar to users that use 1155s out in the wild. So voted against combining the interface.

This isn't to really supersede FT or NFT standards, it's just another tool in the toolkit. So a lot of erc-1155 use cases come from gaming, and then wanting to cheaply represent alot of different tokens in a single contract.

So if you're a market place like say opensea or rarible, you can offer your users the ability to just keep adding new art work to a single collection contract, that represent many different NFTs. This allows the artist to not have to deploy separate contract for every artwork, reducing cost. Additionally in some cases the contracts are just large contracts that are deployed once and everyone writes their art metadata to it, so minting costs are really cheap.

From a video game perspective, it's probably more focused on cost reduction, to be able to exchange many different types of tokens, in a single contract, reducing the number of contracts under management and/or cross contract calls.

The events portion, we at the time decided to be kicked down the field, because it didn't seem to have the infrastructure to do it properly. Things to consider I think there is a limit to the number events that can be emitted in a tx maybe 100. Parsing strings for event seems error prone.

That said, yesterday we re-evaluated that need and it seems like indexer and other consumers want/need the data.

I'll put up a proposal for events for this contract. Would be great to coordinate with you all about this. With regards to events, I like transfer and approval events and metadata_uri events. Where transfer/transfer_batch are encoded with

mint: event transfer(to: "someaccount.near", from: null, token_ids:[9,10], amounts:[100,1])
transfer: event transfer(to: "myaccount.near", from:"someaccount.near", token_ids:[9], amounts:[50])
burn: event transfer( to: null , from: "myaccount.near", token_ids: [9], amounts: [25])

@zcstarr
Copy link
Contributor Author

zcstarr commented Mar 31, 2022

@BenKurrek @telezhnaya I wanted to ping you since you both had a look a this a while back. Since this was last looked at we made an additional change to the interface for the mt standard. In the original draft we forgot to consider ft style tokens in approvals, where many users may hold the same token_id.
In this standard since we needed to extend and change things around a bit to allow for people to iterate over the approvals. The interface change is documented here with in this commit.

the changes are to mt_transfer* and mt_resolve_transfer
principally because the token_id in the multi token standard has may have many different owners we needed to specify which owner the approval applies to within the transfer functions.

This required us to change the method signature to specify the owner_id and approval_id, when referring to approvals. Please see the section in the spec about the changes.

Additionally, it means removing the approvals from the Token structure for mt, and returning back a new structure that allows you to iterate over the tokens.

There is further discussion here: shipsgold#3

I don't want to hold things up just wanted to point this out these changes, and if anyone has any objection to this let us know. The current community wants to move forward with this proposal.

Commit referenced: d413ca3

@BenKurrek
Copy link
Contributor

@zcstarr Unfortunately I won't have time to do an in-depth review again for another 4 weeks as I'm busy with final exams at school. If you want me to take a quick look, I can or I can wait and do an in-depth look later.

@zcstarr
Copy link
Contributor Author

zcstarr commented Apr 1, 2022

@BenKurrek I'd be cool with a quick look I could step you through it too if you ping me on discord, shouldn't be too long. Just unclear what the protocol for the NEP should be.

@telezhnaya
Copy link
Contributor

hey @zcstarr , thank you for your work!
I looked diagonally (sorry I can't find time to dig into that), looks fine for me except approval stuff. The approach looks different from FT/NFT NEPs, it's fine, I'm just worried about Events section. Do we need to update events section as well?

@zcstarr
Copy link
Contributor Author

zcstarr commented Apr 1, 2022

@telezhnaya thanks for taking a peek! This shouldn't affect any of the events.

Copy link
Contributor

@10d9e 10d9e left a comment

Choose a reason for hiding this comment

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

Looking good, just need to add some obligatory warning messages to the Docusaurus markdown and it's ready to merge. Nice work!

specs/Standards/MultiToken/Enumeration.md Show resolved Hide resolved
specs/Standards/MultiToken/Core.md Show resolved Hide resolved
specs/Standards/MultiToken/Events.md Show resolved Hide resolved
specs/Standards/MultiToken/Metadata.md Show resolved Hide resolved
@jriemann
Copy link

jriemann commented Apr 1, 2022

Hi all, I have published a draft PR for a reference impl - near/near-sdk-rs#776 . It should be up to date with most recent changes here. Please take a look if you have a chance.

@zcstarr
Copy link
Contributor Author

zcstarr commented Apr 2, 2022

@jlogelin PTAL I think that's it it's ready for the landing, thanks for having a look at this and getting the ball rollin' again much appreciated!

@10d9e
Copy link
Contributor

10d9e commented Apr 2, 2022

@zcstarr Just a couple of spelling errors to address and it's good to merge. :)

@zcstarr
Copy link
Contributor Author

zcstarr commented Apr 2, 2022

@jlogelin typo patch is in lmk if you hit see anything else that needs patchin! Also thanks for having a look !

@10d9e 10d9e dismissed MaximusHaximus’s stale review April 2, 2022 20:39

All review tasks completed

@10d9e 10d9e merged commit c4dba0b into near:master Apr 4, 2022
@telezhnaya
Copy link
Contributor

🎉 🎉 🎉

@10d9e
Copy link
Contributor

10d9e commented Apr 4, 2022

Great work to all involved with the Draft delivery of this spec. Given the ample discussion already injected, I am updating its status to Review, as a formality. @zcstarr if you feel it's too early, please revert back to Draft status.

@zcstarr
Copy link
Contributor Author

zcstarr commented Apr 4, 2022

Hey thanks @jlogelin. I think review is fine. I couldn't have made it without help from the yourself and the rest of the Near squad @mikedotexe @telezhnaya @mattlockyer @BenKurrek @MaximusHaximus @frol . As well as the community support @jlogelin @marco-sundsk and riqi. Thanks all for taking the time to help make this happen! 🎉

@frol
Copy link
Collaborator

frol commented Dec 14, 2022

Hey folks, I want to give an update here as it seems that the work regarding this NEP is not visible in this PR.

Thanks to @uncle-T0ny, there is a solid PR with the implementation in near-sdk-rs (near/near-sdk-rs#950). The implementation was partially reviewed and @uncle-T0ny took it offline to test this implementation in his own contract before proceeding with submitting a NEP extension to the current MT standard (he identified a couple of rough edges here and there). Feel free to review the implementation and test it out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.