search

onflow / flips

Flow Improvement Proposals
25 stars 23 forks source link

FLIP 258: NFT Storage Requirement #258

Closed joshuahannan closed 5 months ago

joshuahannan commented 5 months ago

Adds a FLIP that proposes adding access(contract) var ownedNFTs: @{UInt64: {NonFungibleToken.NFT}} to the NFT standard Collection

PR: https://github.com/onflow/flow-nft/pull/211 Discord Discussion

joshuahannan commented 5 months ago

After looking at the currently staged contracts on testnet, it looks like there are a good amount that still have ownedNFTs as access(all). Not sure how much of a problem this is. We would need to tell them all to re-stage their contracts

cody-evaluate commented 5 months ago

After looking at the currently staged contracts on testnet, it looks like there are a good amount that still have ownedNFTs as access(all). Not sure how much of a problem this is. We would need to tell them all to re-stage their contracts

fwiw, it dont think it will break anything because access(all) is a more derived/loose access level than the interface signature but Im not sure if the access(contract) on the Collection interface definition would gate access regardless of the access(all) concrete implementation or not (maybe someone could just force cast to the concrete and withdraw but im not 100% sure).

joshuahannan commented 5 months ago

fwiw, it dont think it will break anything because access(all) is a more derived/loose access level than the interface signature but Im not sure if the access(contract) on the Collection interface definition would gate access regardless of the access(all) concrete implementation or not (maybe someone could just force cast to the concrete and withdraw but im not 100% sure).

Why is that true? I just changed the access modifier in ExampleNFT to access(all) but kept the one in NonFungibleToken as access(contract) and it looked like it worked. It seems like an interface specifying an access level should need to be followed and having a looser access level should not be allowed.

@cody-evaluate

joshuahannan commented 5 months ago

I think you're right actually. So this won't be a breaking change. That is great news! I'll update the FLIP

cody-evaluate commented 5 months ago

fwiw, it dont think it will break anything because access(all) is a more derived/loose access level than the interface signature but Im not sure if the access(contract) on the Collection interface definition would gate access regardless of the access(all) concrete implementation or not (maybe someone could just force cast to the concrete and withdraw but im not 100% sure).

Why is that true? I just changed the access modifier in ExampleNFT to access(all) but kept the one in NonFungibleToken as access(contract) and it looked like it worked. It seems like an interface specifying an access level should need to be followed and having a looser access level should not be allowed.

@cody-evaluate

i 100% agree, im just saying that defining ownedNFTs in the concrete implementation as access(all) despite being defined as access(contract) in the interface wont break at compile time but there just might be security issues (due to force casting)

basically just saying access modifiers are covariant.