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

ICS721: Spec for interchain NFT transfer #615

Merged
merged 18 commits into from
May 23, 2022
Merged

ICS721: Spec for interchain NFT transfer #615

merged 18 commits into from
May 23, 2022

Conversation

haifengxi
Copy link
Contributor

@haifengxi haifengxi commented Nov 10, 2021

Adapted from ICS-020, replacing bank with nft as the "asset tracking module"

Copy link
Contributor

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

Some sections are copied from: https://github.com/cosmos/ibc/tree/master/spec/app/ics-020-fungible-token-transfer

Instead of copying, maybe we can reference those sections? What do you think? CC: @crodriguezvega

spec/app/ics-021-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-021-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-021-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-021-nft-transfer/README.md Outdated Show resolved Hide resolved
@haifengxi
Copy link
Contributor Author

haifengxi commented Nov 12, 2021

Some sections are copied from: https://github.com/cosmos/ibc/tree/master/spec/app/ics-020-fungible-token-transfer

Instead of copying, maybe we can reference those sections? What do you think? CC: @crodriguezvega

Will add a reference to ICS20 and some comments at the beginning of ICS721; as discussed earlier, the quickest and most reasonable approach to ICS721 will be an adaptation of ICS20 with NFT specifics.

@MikeSofaer
Copy link

Pylons is primarily concerned with royalties when it comes to NFT IBC transfer. I'd like to make sure that this has been thought about. We have our own modules for item creation, but we would like to have standards for things like image_url in addition to royalties.

@the-frey
Copy link

Pylons is primarily concerned with royalties when it comes to NFT IBC transfer. I'd like to make sure that this has been thought about. We have our own modules for item creation, but we would like to have standards for things like image_url in addition to royalties.

Out of interest, is there anything I can read on your approach to royalties? I think getting royalties interop right is super important, and something we'd want smart contracts and native to line up on as well...

@MikeSofaer
Copy link

Pylons is primarily concerned with royalties when it comes to NFT IBC transfer. I'd like to make sure that this has been thought about. We have our own modules for item creation, but we would like to have standards for things like image_url in addition to royalties.

Out of interest, is there anything I can read on your approach to royalties? I think getting royalties interop right is super important, and something we'd want smart contracts and native to line up on as well...

I don't have anything written, but I can maybe draft something we can work on. I think encumbrances across IBC are actually a larger question that royalties are only a part of. Superfluid staking faces IMO a similar problem with its cross-chain slashing, as does the question of enabling people to get usage value out of locked-up lending collateral, which I think is critical for on-chain economics to compete with off-chain.

@robert-zaremba
Copy link
Contributor

@MikeSofaer - could you share you proto files?

@MikeSofaer
Copy link

@robert-zaremba
Copy link
Contributor

Re Royalties - I think sooner or later it will be a part of almost every NFT project. Especially when going crosschain. So let's reconsider how this will be integrated, what kind of information we need to carry over IBC and if we need to add something to existing NFT or Class proto structures.
Here is a related Ethereum ERC: https://eips.ethereum.org/EIPS/eip-2981

@the-frey
Copy link

Re Royalties - I think sooner or later it will be a part of almost every NFT project. Especially when going crosschain. So let's reconsider how this will be integrated, what kind of information we need to carry over IBC and if we need to add something to existing NFT or Class proto structures. Here is a related Ethereum ERC: https://eips.ethereum.org/EIPS/eip-2981

We've got a small port of EIP-2981 in CW actually, it seems like a good minimal starting point, but imagine there might be other considerations or requirements that we can bring in that the eth folks couldn't - given that they already had a bunch of live marketplaces out in the wild 🤔

@robert-zaremba
Copy link
Contributor

@michaelfig Is royalty a fixed or linear (eg %) amount for each token, or it is a non linear function?
If an NFT is transferred over IBC, how do you expect to carry the royalties? What information do we need to share over IBC?

@MikeSofaer
Copy link

@michaelfig Is royalty a fixed or linear (eg %) amount for each token, or it is a non linear function? If an NFT is transferred over IBC, how do you expect to carry the royalties? What information do we need to share over IBC?

If that's intended for me, we currently support linear royalties, and minimum prices. For payment, I'd expect the royalty to be sent to the creator's account on Pylons over IBC, in whatever token was used to pay, but I haven't really thought about any other options.

@crodriguezvega
Copy link
Contributor

Seems like the general preference would be to rename this specification to ICS-721 (to name it after Ethereum's ERC-721). Would you mind doing that please?

@haifengxi haifengxi changed the title Add ICS-021 spec for interchain NFT transfer Add ICS-721 spec for interchain NFT transfer Nov 17, 2021
@okwme
Copy link

okwme commented Nov 17, 2021

I'm hesitant to complicate the ICS standard with encumbrances at this point. One goal of this latest batch of work is getting the simplest version working and live as soon as possible and part of the reason prior development has taken a year longer than expected is because so much time was spent trying to get encumbrances right on the first try when I think we can all agree they're a complicated thing to get wrong.

If we look at OpenSea as the example for the most popular marketplace they allow royalties to be added to sales in the following manner:

  • prove you control the account that deployed the original smart contract
  • once done you may:
    • set a percentage fee to be applied to all sales
    • set an address to receive the royalties
    • set the design of the page on their site that lists your token class
      If that were the goal then it should only be the account that is sent with the token; basically who is allowed to configure royalties wherever this token goes. The ability to configure things on behalf of the token will only be as functional as the destination allows them.

Not sure if this actually the best path but consideration should be made for the account address that has this kind of "admin" rights on the asset. It should not be a groups module account because then it would not be able to submit transactions to the various destinations, only in a local setting. It could be a single private key account assuming the key is derived with the same path and the proper bech32 prefix on the destination. Same for cryptographic multisig account although certainly more work to set up in the new context.

@MikeSofaer can you confirm what your IBC user story is? From my understanding the bulk of your product story is for NFTs that start and stay on Pylons, correct? Do you have destination marketplace partners in place who are already aware and willing and able to support your royalty structure?

@haifengxi IRIS is the only team who has already moved live NFTs across networks AFAIK. Can you speak to the features that you found necessary with regard to encumbrances like royalties?

I'd like to understand if a simplified encumbrance field that is essentially allows for destination chains to enforce with the following logic: "who can configure encumbrances wherever this NFT is located?", is sufficient to cover the expected use cases at this point. Or if we should entirely defer to future versions of the ICS that allow for a more complex encumbrance metadata transfer.

In any case, it should be possible to begin with a very simple ICS that uses the NFT module under the hood. A future version of the ICS could include encumbrances and still use the NFT module under the hood. This would result in some NFTs that have the same class but came over different channels. Depending on the channel they could have encumbrance information or not. NFTs could migrate to the updated version by going back to the destination and then through the new encumbrance channel. I think that's a permissible path to allow for iteration over the ICS to support the simplest first and the more complex as user demands shape the direction.

@MikeSofaer
Copy link

@MikeSofaer can you confirm what your IBC user story is? From my understanding the bulk of your product story is for NFTs that start and stay on Pylons, correct? Do you have destination marketplace partners in place who are already aware and willing and able to support your royalty structure?

We're interested in moving NFTs to marketplace zones, stargaze is the first zone we've talked to about this. Royalties are core to this story, and Shane and I have talked a bit about how to approach building an IBC connector. I also think the OpenSea flow described above is a crazy hack and we should do better.

I'm also not totally sure how compatible we are with a standard that uses the NFT module under the hood anyway, as we don't use it.

@adu-web3
Copy link

the locked token in the escrow account owned by IBC indicates that NFT has a third status as "locked", so should we describe this status in NFT data structure?

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Thanks for the submission; see comments (and I second Aditya's).

spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

A few nits, and a question about how mutable the NFT URIs are. I believe that will have an effect on the final design of the spec. Getting close though!

spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@mpoke mpoke left a comment

Choose a reason for hiding this comment

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

Thanks for the submission. Also ACK on general architecture. See comments bellow.

spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Show resolved Hide resolved
@mpoke mpoke changed the title Add ICS-721 spec for interchain NFT transfer ICS721: Spec for interchain NFT transfer Mar 9, 2022
@robert-zaremba
Copy link
Contributor

robert-zaremba commented Mar 9, 2022

Who is the PO for this work? We need to put more pressure to finalize it because x/nft is being release with 0.46

@haifengxi haifengxi closed this Mar 10, 2022
@haifengxi haifengxi reopened this Mar 10, 2022
@haifengxi
Copy link
Contributor Author

How is the PO for this work? We need to put more pressure to finalize it because x/nft is being release with 0.46

IMO, this spec is almost ready for implementation.

Copy link
Member

@AdityaSripal AdityaSripal 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 on adopting the ICS-20 logic. I think the only open question for me is the mutability of the original NFT at the source. It needs to be clarified how this should be handled if the "live" version of the NFT is living on a different chain.

This can be addressed in a separate PR since it is a longer discussion. The spec outside of the mutability question looks good to me so unblocking.

spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@mpoke mpoke 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. Some minor comments below.

spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

ACK, but see comments

spec/app/ics-721-nft-transfer/README.md Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
spec/app/ics-721-nft-transfer/README.md Outdated Show resolved Hide resolved
@aofengli
Copy link

How is the current progress? When will it be possible to merge?

@AdityaSripal
Copy link
Member

@haifengxi , this passes the requisite checks. Please check @cwgoes comments to see if there are any last changes you would like to incorporate. If not, then let me know that this is mergable and I will merge, thanks.

@haifengxi
Copy link
Contributor Author

haifengxi commented May 18, 2022

@haifengxi , this passes the requisite checks. Please check @cwgoes comments to see if there are any last changes you would like to incorporate. If not, then let me know that this is mergable and I will merge, thanks.

Added a paragraph clarifying NFT metadata immutability. I have nothing else to add to this spec for now. Thanks.

@AdityaSripal AdityaSripal merged commit bcd5f6d into cosmos:master May 23, 2022
@haifengxi haifengxi deleted the nft-transfer branch November 8, 2022 07:28
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.