search

onflow / flow-nft

The non-fungible token standard on the Flow blockchain
https://onflow.org
The Unlicense
464 stars 166 forks source link

[standard v2] Remove `INFT` interface from `NonFungibleToken` #206

Closed SupunS closed 3 months ago

SupunS commented 3 months ago

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 https://github.com/onflow/flow-go/pull/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 commented 3 months ago

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 commented 3 months ago

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: https://github.com/onflow/cadence/pull/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 commented 3 months ago

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 https://github.com/onflow/flow-nft/blob/4c0c684baed98e2ffd7a8f9d27973c1db5dd1239/contracts/ExampleNFT.cdc#L39

to https://github.com/onflow/flow-nft/blob/3a595a3a674a8de2f760a1371570c4824fddf92d/contracts/ExampleNFT.cdc#L25

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

turbolent commented 3 months ago

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.

joshuahannan commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

I guess this issue can be closed now?

SupunS commented 3 months ago

Added in https://github.com/onflow/flow-nft/pull/205