What I’ve learned in the year since I wrote the NBA Top Shot smart contracts
Hello! My name is Josh, and I write smart contracts at Dapper Labs for the Flow blockchain.
If you’re new here, welcome! This is my bi-weekly blog about Cadence, Flow’s new state-of-the-art language for smart contracts. I recommend starting out with my first post about beginner materials before reading this, because I’ll be assuming readers already have a basic understanding of Cadence.
NBA Top Shot
If you are at all interested in learning about Flow or Cadence, you have probably heard of NBA Top Shot, the officially licensed digital collectible experience from Dapper Labs. Top Shot is a large project with many moving parts, but the record of Moment ownership and value transfer is handled by a few smart contracts on the Flow blockchain, which I was in charge of writing. (with the valuable help of many others on the Flow and Top Shot teams!)
Yes, that is my Top Shot collection and yes, I am also an Indiana Pacers fan. You would probably be able to guess that from some of the comments in the Top Shot smart contract.
I started working for Dapper Labs in September of 2019 and while I had years of experience with Solidity and Ethereum, I was still a complete newcomer to Cadence. I started writing the Top Shot contracts in December and finished the first drafts in January, after which we spent a few months tweaking and testing before launching in June.
Over a year has passed since I wrote the smart contracts, and my skills as a software developer, and particularly a Cadence developer, have improved significantly. While I am proud of what we built and am extremely confident in the security and reliability of the Top Shot smart contracts, there are many things I would have done differently in hindsight to have more consistent design, readability, and usability, kind of like some NBA teams feel about their Jersey choices.
In contrast to certain teams’ jersey choices, NBA Top Shot has been very successful to say the least, which has inspired many new teams in the space to want to build similar experiences on Flow. Many of these projects simply copy and paste the Top Shot smart contract code and just change some names without putting much thought into it.
I totally understand the urge to do this because it requires the least amount of effort on the part of the smart contract developer, but I hope that more newcomers can learn from the decisions of projects that have come before them so they can make meaningful improvements to the state of the art and not be stuck with a sub-optimal way of doing things.
Lets look at TopShot.cdc
The Core Top Shot Contract
I would recommend checking out the Top Shot smart contract repo if you haven’t already.
There is a lot of documentation for the contracts there to help you get comfortable, but I’ll give a brief overview here.
TopShot.cdc is a NFT contract (implements the NonFungibleToken interface) that defines the categorization, creation, and ownership of Top Shot Moment NFTs.
Each Top Shot Moment NFT represents a play from a game in the NBA season. Plays are grouped into sets which usually have some overarching theme, like rarity or the type of the play.
A set can have one or more plays in it and the same play can exist in multiple sets, but the combination of a play and a set, otherwise known as an edition, is unique and is what classifies an individual Moment.
Multiple Moments can be minted from the same edition and each receives a serial number that indicates where in the edition it was minted.
Funny anecdote: Top Shot used to have a completely different architecture where we had NFT Molds that minted new moments, but that is a story for another day. Good times.
As you can see, the main structure is fairly simple. You add plays to sets, then mint moments from those combinations. It works well for us and for the users, but there are three main improvements I would like to have made to it and that I think others who are making similar experiences should consider.
These suggestions apply to the original version of the Top Shot contracts. Some of the improvements have already been implemented by the Top Shot team and included in an upgrade, but some of them are not able be upgraded using the traditional methods, so they still remain. (the ones that remain aren’t vulnerabilities, just non-ideal designs) I’ll include a FIXED flag for each one that has been fixed in the update.
First Improvement: Make Dictionary and Array Fields Private
FIXED
This is a mistake that I see Cadence developers make all the time. In Cadence, fields that are public mean that they are publicly readable but not publicly writable. This only applies to the field itself, but if the field is a dictionary or array, the members of that dictionary or array are still able to be assigned to. See our best practices document for more info:
Any Cadence developer should default to making all dictionary and array fields private by default, by which I mean access(contract) or access(self) . They should also define explicit getter and setter functions for those that make it extremely clear what type of access the developer wants to allow.
In the case of the Top Shot contract, this means that these fields should be made access(contract):
// SHOULD ALL BE `access(contract)`pub var plays: [UInt32]pub var retired: {UInt32: Bool}pub var numberMintedPerPlay: {UInt32: UInt32}
If they are public, this means that the admin would have the ability to modify the retired status or number minted for specific moment plays, which is not an ability that the admin should have. The Top Shot team is fixing these issues in our contract promptly.
Second Improvement: More thoughtful metadata
The Play struct currently has two fields:
pub let playID: UInt32pub let metadata: {String: String}
Metadata for Top Shot NFTs is currently simply a String to String mapping, so for example, a mapping in the metadata field might be "FullName": "Domantas Sabonis", or "Golden State Warriors": “Blew a 3–1 lead in the 2016 NBA Finals".
Each Moment has a bunch of fields. This is a decent way to manage it, but it doesn’t allow for much wiggle room. If we ever want to add more complex metadata to Top Shot moments, working with this construct will be difficult.
If I had to redo this, I would have thought more deeply about what potential metadata my project might need to store. It could be a json data structure, an image, or many other things. Every project needs to think deeply about what makes their metadata unique and figure out a way to include it in the smart contract code.
See this issue in the NFT standard repo for a discussion about on-chain metadata if you’d like to contribute!
Third Improvement: Unified Set resource and Set metadata struct
Partially Fixed: We added a unified set metadata struct, but could not unify the set resource
Currently, Sets in Top Shot are represented by two different data structures:
- A
SetDatastruct, which records the id, name, and series of the set. - A
Setresource, which records other information about the set, including plays that are in it, editions, and retired statuses. It also acts as a authorization resource for the admin to create editions, mint moments, retire plays, and more.
We originally thought it might be useful to have them separate to keep static metadata about a set separate from the dynamic data that the admin controls, but in practice, this isn’t really that useful and just makes the code a little bit harder to understand and harder to query.
How could we have improved it?
If I could do it over again, I would have put all the SetData fields in the Set resource like this:
pub resource Set { pub let setID: UInt32 pub let name: String pub let series: UInt32 pub var plays: [UInt32] pub var retired: {UInt32: Bool} pub var locked: Bool pub var numberMintedPerPlay: {UInt32: UInt32} ...}
This way, we have a single unified Set object that the admin uses to manage set data. Then, I would have also removed the setDatas field from the smart contract, because those are stored in the Set resource now, so the field isn’t necessary.
access(self) var setDatas: {UInt32: SetData} // REMOVEI also would change theSetData struct definition to have all of the same fields as the Set resource. You might be wondering, “If you removed the SetData field, why does the SetData struct still need to exist?”
// New SetData struct with all the same fields as Set
//pub struct SetData { pub let setID: UInt32 pub let name: String pub let series: UInt32 pub var plays: [UInt32] pub var retired: {UInt32: Bool} pub var locked: Bool pub var numberMintedPerPlay: {UInt32: UInt32} init(setID: UInt32) {
self.setID = TopShot.setDatas[setID].setID
self.name = TopShot.setDatas[setID].name //... and so on
}
In the original version of the Top Shot contract, when someone wants to query information about a Set or Play in Top Shot, they have to call individual getter functions for each piece of data. This is very cumbersome and I have discovered in my time since writing the contract that it is almost always better to group related data together for queries.
Since SetData no longer needs to be the source of truth for important Set metadata, it can be used for a better purpose, an easily queryable source of all the current information about a Set. Now, if someone wants a piece or all of the information about a Set, they can simply find all the information in one place by instantiating a new SetData struct for the set they want to know about, like this!
pub fun main(id: UInt32): SetData {
let set = TopShot.SetData(setID: id)
log(set.name)
log(set.locked)
... return set}
They could parse it however they want within the script, or within the application code that sent the script. It gives the developer a lot more flexibility than the sufficient, but restrictive getters that the Top Shot contract currently provides.
Fourth Improvement: Perform state changing operations in admin resources, not in public structs
FIXED
In the original version of the Top Shot contract, when a new play or set is created, the ID tracker for the play or the set would be incremented to make sure that the next play or set created would have a unique ID. It would also emit an event indicating what the new play or set is. Seems fine on the surface, but the problem arises from where these operations happen.
These operations were performed in the initializer for the Play and Set structs:
pub struct Play { init() {
self.playID = TopShot.nextPlayID // Increment the ID so that it isn't used again
TopShot.nextPlayID = TopShot.nextPlayID + UInt32(1)
emit PlayCreated(id: self.playID, metadata: metadata)
}
}
At the time, Cadence did not, and still doesn’t, support private struct definitions. This means that anyone could create an instance of this Play struct or the SetData struct whenever they want. Every initialization would increment the id counter and emit an event, even though these sets and plays that were created were not real sets or plays. Eventually, the field would reach the limit for UInt32 and overflow, which could prevent new plays or sets from being created. This would be quite expensive to exploit, but is still a serious vulnerability that should always be avoided.
This references an important security aspect of smart contract development that should always be considered, regardless of the contract you are working on. If you have any operations that change important state in the contract, you should always default those operations to being hidden in an admin resource that restricts that functionality to only those who should be able to do it. There are obviously exceptions to this, but they should be carefully considered before committing to.
Fifth Improvement: Borrow references to resources instead of loading them from storage
FIXED
The Top Shot contract stores an important resource in a contract field, the Set resource. There are certain functions, including the queryable set metadata functionality that I described above, that need to get information about one of these sets that is stored in the contract. The solution that we originally used for accessing these sets was to load the whole set from storage, read its fields, and then put it back where it came from.
if let setToRead <- TopShot.sets.remove(key: setID) {
// See if the Play is retired from this Set
let retired = setToRead.retired[playID]
// Put the Set back in the contract storage
TopShot.sets[setID] <-! setToRead
// Return the retired status
return retired
}This works fine, but it is pretty inefficient. It takes a lot of computational resources to load an object from storage and save it again.
This coding pattern also caused a problem with the TopShotShardedCollection.cdc smart contract because when you load a resource from storage, its owner field is set to nil . If you try to access its owner field expecting it to be a certain value (like when you are emitting a Deposit event, for example), it will be nil , even if you plan to put it right back into storage right after withdrawing it like we did in the sharded collection contract.
The correct solution for this pattern is to borrow a reference to the resource instead. Borrowing a reference only uses only line of code instead of two and is a much more efficient operation:
let collectionRef = &self.collections[bucket] as! &TopShot.CollectionConclusion
I hope these suggestions have been useful for some of you! I would like to further reiterate to those of you who want to build NFT projects on Flow to think deeply about what makes your NFTs unique and try to reflect that with unique features in your NFT smart contract. A simple copy and paste of the Top Shot contract will not set you apart from the rest of the pack.
If you have any comments about improvements you’d make to the Top Shot contract, or just want to talk smack about the Pacers, please share here or in our Discord! There may be some I am missing and we might even be able to include them in a later version.
If you have any questions, the entire Flow team, Top Shot team, and community is here to support you! Please do not hesitate to reach out via our Discord server, the Flow Forum, or via an issue in the Top Shot Github repo.
Are there any other topics or interesting projects that you know would useful to newcomers or that you would like me to write a blog post about? Feel free to comment with your ideas and I might include them in a future post!
Flow Discord: https://discord.gg/flow
Flow Forum: https://forum.onflow.org
Flow Github: https://github.com/onflow/flow
See you next week! 👋
