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

[standard v2] Remove INFT interface from NonFungibleToken #206

Closed
SupunS opened this issue Mar 25, 2024 · 8 comments
Closed

[standard v2] Remove INFT interface from NonFungibleToken #206

SupunS opened this issue Mar 25, 2024 · 8 comments

Comments

@SupunS
Copy link
Member

SupunS commented Mar 25, 2024

Issue To Be Solved

I guess the reason for keeping that was to make the NonFungibleToken backward compatible, and to make the life easier for the migrations.

Now that the migrations and contract update validator support changing the type from NonFungibleToken.INFT to NonFungibleToken.NFT (added in onflow/flow-go#5564), we could remove the old INFT interface, and ask developers to start using the new NFT.

In-fact now it is also unsafe from migrations point of view, to have both the INFT and the NFT around, because it could theoretically cause "type confusion" for certain migrated values. So removing the INFT interface, and requiring people to start using NFT instead seems the best option.

@joshuahannan
Copy link
Member

I'm hesitant to make any more breaking changes to the standard because we said that it was finalized. Is this something we really need to do?

@SupunS
Copy link
Member Author

SupunS commented Mar 26, 2024

So the problem for the migrations is that developers now have two options to go with (could either use INFT or NFT), and that will create ambiguity for the migrations.

I opened this PR: onflow/cadence#3193, to add a restriction to the contract update validation, such that it will enforce the developers to use the new NFT when updating conformance in their contracts. So removing it here physically would not be required, but people will have to switch to start using the new one when updating their code.

Would that going to be OK? Or do you think we should still let them use the old one?

@SupunS
Copy link
Member Author

SupunS commented Mar 26, 2024

Just to be clear, as an alternative solution, we can let people use both INFT and NFT for conformances. But if we take this path, then anyone who wants to change their conformance from the old INFT to the new NFT would not be able to do so.
i.e: converting

pub resource NFT: NonFungibleToken.INFT, MetadataViews.Resolver {

to

access(all) resource NFT: NonFungibleToken.NFT, ViewResolver.Resolver {

will not be supported. They would have to keep using the NFT: NonFungibleToken.INFT conformance as is.

@turbolent
Copy link
Member

turbolent commented Mar 26, 2024

Even though this change is not required, it would be great to still get it in.

It would improve the situation for existing code, which currently may and will have to keep using INFT, while new code will likely use NFT. Old code can currently not switch to using NFT. This leaves a confusing situation where some code uses INFT and other code uses NFT. We might want to make this cleaner.

The proposed change, the removal of the INFT type, was already previously agreed on in the FLIP, and it was only re-introduced as a work-around to keep the migration simple.

@turbolent turbolent mentioned this issue Mar 26, 2024
@joshuahannan
Copy link
Member

It isn't about whether or not I agree with the change, it is that we told everyone that the standards were finalized and that we wouldn't be making any more breaking changes to them and I'm trying to figure out if it is okay to go back on that.

@joshuahannan
Copy link
Member

After thinking a little bit, I believe it is probably okay. I made the change in supun's branch. Nobody has staged any updates yet, right?

@turbolent
Copy link
Member

I guess this issue can be closed now?

@SupunS
Copy link
Member Author

SupunS commented Mar 28, 2024

Added in #205

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

No branches or pull requests

3 participants