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

FLIP 258: NFT Storage Requirement #258

Merged
merged 4 commits into from
Apr 17, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 119 additions & 0 deletions application/20240411-nft-storage.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
---
status: Implemented
flip: 258
authors: Joshua Hannan ([email protected])
Cody Bouche ([email protected])
sponsor: Joshua Hannan ([email protected])
updated: 2024-03-11
---

# Non-Fungible Token Storage Requirement

## Objective

Enforce that NFT projects store their NFTs in their `Collection`
in an `access(contract)` dictionary field and provide a default function
for iterating over the NFT IDs in the collection.

## Motivation

The original NFT standard enforced that `Collection`s had a field for storing NFTs:
```cadence
pub var ownedNFTs: @{UInt64: NonFungibleToken.NFT}
```

In the V2 NFT Standard, it was decided that we wanted to give projects more flexibility
for how they store their NFTs, so this requirement was removed.
We didn't realize at the time that this requirement actually had benefits like
allowing more sophisticated and standardized scripts that interacted with collections,
like iterating through IDs in a collection without having
to load all of them into memory at once.

## Design Proposal

[The conversation that triggered this](https://discord.com/channels/613813861610684416/621847426201944074/1227401046926692443)
is in Discord.
A [pull request with the suggested code changes](https://github.com/onflow/flow-nft/pull/211)
is in the flow-nft github repository.


This FLIP proposes adding the `ownedNFTs` field back to the standard:

```cadence
access(contract) var ownedNFTs: @{UInt64: {NonFungibleToken.NFT}}
```

It also proposes adding a function to `NonFungibleToken.Collection`, `forEachID()`,
that allows iterating through the IDs in a collection without having
to load all of them into memory first:
```cadence
/// Allows a given function to iterate through the list
/// of owned NFT IDs in a collection without first
/// having to load the entire list into memory
access(all) fun forEachID(_ f: fun (UInt64): Bool): Void {
self.ownedNFTs.forEachKey(f)
}
```

The function specification provides a default implementation so that projects
don't have to implement the function themselves.

## User Benefit

With a standard specification for how to store NFTs, developers
can have more confidence in understanding any given NFT project's storage.

Without this storage specification, there would be no way to get all the IDs
in a large collection without hitting the computation limit.

This also allows the standard to define more sophisticated functionality that
interacts with stored NFTs, like we have done with adding the `forEachID()` function.
In the future, there may be other functionality that is enabled
by having standardized storage.

## Drawbacks

This could make things a little more awkward for developers who do want to define
a more sophisticated system for storing NFTs
because they will still have to have the `ownedNFTs` field,
but it doesn't prevent them at all because they can simply ignore the field requirement
and do whatever else they want.

## Alternatives Considered

1. Keep the standard the same:
* This would make some projects like evaluate not be able to function properly who rely on the iterator feature for their app.
2. Only introduce the `forEachID()` function with a default implementation but without the storage requirement
* This would not potentially be a breaking change but would leave it up
to the projects to implement the function, which is not reliable.
We are far enough from the upgrade that this change should be acceptable.
3. Introduce a Cadence feature that allows getting slices from the IDs array
* There isn't time to do this before the Cadence 1.0 upgrade.

## Performance Implications

The `forEachID()` also has a limit (200k) on how many IDs it can access,
but that is much larger than almost any collection on Flow will ever be.

## Dependencies


### Engineering Impact

* Build and test time will stay the same. they are relatively small changes.

### Best Practices


### Tutorials and Examples

* Check out the Cadence 1.0 migration guide for instructions on how to update contracts to the new standards:
* https://cadence-lang.org/docs/cadence_migration_guide/

### Compatibility

* FCL, emulator, and other such tools should not be affected.

### User Impact

* The upgrade will go out on the networks at the same time as Cadence 1.0 (Crescendo) if approved.