Update NBA contracts & txs to Cadence 1.0 #248

bshahid331 · 2024-03-13T17:19:22Z

No description provided.

sisyphusSmiling

Only had a chance to look through FastBreakV1, but leaving initial comments (some nits)

sisyphusSmiling · 2024-03-13T18:02:39Z

contracts/FastBreakV1.cdc

@@ -179,7 +183,7 @@ pub contract FastBreakV1: NonFungibleToken {

    /// Get a Fast Break Run by Id
    ///
-    pub fun getFastBreakRun(id: String): FastBreakV1.FastBreakRun? {
+    view access(all) fun getFastBreakRun(id: String): FastBreakV1.FastBreakRun? {


I don't think there's an issue with the ordering here, but recommending the following format for consistency with core contracts and language docs

Suggested change

view access(all) fun getFastBreakRun(id: String): FastBreakV1.FastBreakRun? {

access(all) view fun getFastBreakRun(id: String): FastBreakV1.FastBreakRun? {

sisyphusSmiling · 2024-03-13T18:03:03Z

contracts/FastBreakV1.cdc

@@ -232,7 +236,7 @@ pub contract FastBreakV1: NonFungibleToken {

        /// Get a account's active Fast Break Submission
        ///
-        pub fun getFastBreakSubmissionByPlayerId(playerId: UInt64): FastBreakV1.FastBreakSubmission? {
+        view access(all) fun getFastBreakSubmissionByPlayerId(playerId: UInt64): FastBreakV1.FastBreakSubmission? {


Same as above, and throughout the rest of the contract

Suggested change

view access(all) fun getFastBreakSubmissionByPlayerId(playerId: UInt64): FastBreakV1.FastBreakSubmission? {

access(all) view fun getFastBreakSubmissionByPlayerId(playerId: UInt64): FastBreakV1.FastBreakSubmission? {

sisyphusSmiling · 2024-03-13T18:05:58Z

contracts/FastBreakV1.cdc

@@ -382,11 +386,11 @@ pub contract FastBreakV1: NonFungibleToken {
    /// Resource for playing Fast Break
    /// The Fast Break Player plays the game & mints game tokens
    ///
-    pub resource Player: FastBreakPlayer, NonFungibleToken.INFT {
+    access(all) resource Player: FastBreakPlayer {


Did you mean to remove INFT conformance here or should FastBreakPlayer implement INFT?

We're updating the standard to remove INFT, so that shouldn't be here, but NonFungibleToken.NFT should be here

@bshahid331 This needs to implement NonFungibleToken.NFT

sisyphusSmiling · 2024-03-13T18:18:50Z

contracts/FastBreakV1.cdc

-        pub fun borrowNFT(id: UInt64): &NonFungibleToken.NFT
-        pub fun borrowNFTSafe(id: UInt64): &NonFungibleToken.NFT?
-        pub fun borrowFastBreakNFT(id: UInt64): &FastBreakV1.NFT? {
+    access(all) resource interface FastBreakNFTCollectionPublic {


Since interfaces can inherent interfaces, you could have this interface implement NFT.CollectionPublic and include additional methods specific for FastBreak

sisyphusSmiling · 2024-03-13T18:20:26Z

contracts/FastBreakV1.cdc

+        access(all) fun getIDs(): [UInt64]
+        access(all) fun borrowNFT(_ id: UInt64): &{NonFungibleToken.NFT}?


I believe you'll want these to be view methods or they'll conflict with NFT.CollectionPublic methods. I'd also consider removing borrowNFTSafe below since borrowNFT now returns an optional

…ublic interface

joshuahannan

Great start! I only reviewed the contracts and left comments on them. You'll need to find someone on the studio team to review the rest of the code

joshuahannan · 2024-03-27T19:18:14Z

contracts/MarketTopShotOldVersion.cdc

+access(all) contract Market {
+
+    access(all) entitlement Create
+    access(all) entitlement Withdraw


I would probably recommend removing Withdraw here and just using NonFungibleToken.Withdraw instead

joshuahannan · 2024-03-27T19:20:27Z

contracts/TopShot.cdc


-pub contract TopShot: NonFungibleToken {
+access(all) contract TopShot: NonFungibleToken, ViewResolver {


There is no need to add ViewResolver here since it is already inherited with NonFungibleToken.

joshuahannan · 2024-03-27T19:21:32Z

contracts/TopShot.cdc

    // The network the contract is deployed on
-    pub fun Network() : String { return ${NETWORK} }
+    view access(all) fun Network() : String { return ${NETWORK} }


This is just a style thing, but the common place for putting view in a function declaration is after the access specification, so access(all) view fun Network...

joshuahannan · 2024-03-27T19:23:33Z

contracts/TopShot.cdc

@@ -369,7 +372,7 @@ pub contract TopShot: NonFungibleToken {
        //
        // Returns: The NFT that was minted
        // 
-        pub fun mintMoment(playID: UInt32): @NFT {
+        access(NFTMinter) fun mintMoment(playID: UInt32): @NFT {


I'm a little confused why you have a minter entitlement but don't have entitlements for any of the other methods? This never has a public capability, right? So why the need for a single entitlement here?

joshuahannan · 2024-03-27T19:24:04Z

contracts/TopShot.cdc

@@ -399,7 +402,7 @@ pub contract TopShot: NonFungibleToken {
        //
        // Returns: Collection object that contains all the Moments that were minted
        //
-        pub fun batchMintMoment(playID: UInt32, quantity: UInt64): @Collection {
+        access(all) fun batchMintMoment(playID: UInt32, quantity: UInt64): @Collection {


If you're including the NFTMinter entitlement, it should be here too, right?

NFTMinter entitlement?

joshuahannan · 2024-03-27T19:43:23Z

contracts/FastBreakV1.cdc

@@ -382,11 +386,11 @@ pub contract FastBreakV1: NonFungibleToken {
    /// Resource for playing Fast Break
    /// The Fast Break Player plays the game & mints game tokens
    ///
-    pub resource Player: FastBreakPlayer, NonFungibleToken.INFT {
+    access(all) resource Player: FastBreakPlayer {


We're updating the standard to remove INFT, so that shouldn't be here, but NonFungibleToken.NFT should be here

joshuahannan · 2024-03-27T19:47:01Z

contracts/FastBreakV1.cdc

+    access(all) resource Collection:
        NonFungibleToken.Provider,
        NonFungibleToken.Receiver,
-        NonFungibleToken.CollectionPublic,
+        NonFungibleToken.Collection,


This only needs to be NonFungibleToken.Collection

contracts/FastBreakV1.cdc

joshuahannan · 2024-03-27T19:48:05Z

contracts/FastBreakV1.cdc

+        view access(all) fun borrowNFTSafe(id: UInt64): &{NonFungibleToken.NFT}? {
+            return &self.ownedNFTs[id]


This can be removed

joshuahannan · 2024-03-27T19:48:58Z

contracts/FastBreakV1.cdc

+    view access(all) fun getContractViews(resourceType: Type?): [Type] {
+        return []
+    }
+
+    view access(all) fun resolveContractView(resourceType: Type?, viewType: Type): AnyStruct? {
+        return nil
+    }


I would think that there would be some views that this contract would implement. You should figure out which ones can be included so Top Shot can set a good example

joshuahannan

Followed up on a few of my comments that were never addressed and left a few more. The changes will require the contracts to be re-staged on testnet after you address them. Let me know before you want to re-stage so I can verify that everything has been addressed

joshuahannan · 2024-04-22T20:06:12Z

contracts/FastBreakV1.cdc

    /// Contract events
    ///
-    pub event ContractInitialized()
+    access(all) event ContractInitialized()


This can be removed

joshuahannan · 2024-04-22T20:06:42Z

contracts/FastBreakV1.cdc

+    access(all) event Withdraw(id: UInt64, from: Address?)

-    pub event Deposit(id: UInt64, to: Address?)
+    access(all) event Deposit(id: UInt64, to: Address?)


I'd recommend removing these because the default NonFungibleToken events are much better

joshuahannan · 2024-04-22T20:07:33Z

contracts/FastBreakV1.cdc

@@ -382,11 +386,11 @@ pub contract FastBreakV1: NonFungibleToken {
    /// Resource for playing Fast Break
    /// The Fast Break Player plays the game & mints game tokens
    ///
-    pub resource Player: FastBreakPlayer, NonFungibleToken.INFT {
+    access(all) resource Player: FastBreakPlayer {


@bshahid331 This needs to implement NonFungibleToken.NFT

joshuahannan · 2024-04-22T20:08:19Z

contracts/FastBreakV1.cdc

        FastBreakNFTCollectionPublic
    {

-        pub var ownedNFTs: @{UInt64: NonFungibleToken.NFT}
+        access(contract) var ownedNFTs: @{UInt64: {NonFungibleToken.NFT}}


Update to access(all)

joshuahannan · 2024-04-22T20:09:00Z

contracts/FastBreakV1.cdc

+        access(all) view fun getLength(): Int {
+            return self.ownedNFTs.keys.length


Suggested change

access(all) view fun getLength(): Int {

return self.ownedNFTs.keys.length

access(all) view fun getLength(): Int {

return self.ownedNFTs.length

joshuahannan · 2024-04-22T20:12:19Z

contracts/TopShot.cdc

@@ -421,15 +422,15 @@ pub contract TopShot: NonFungibleToken {
        //
        // Returns: The NFT that was minted
        //
-        pub fun mintMomentWithSubedition(playID: UInt32, subeditionID: UInt32): @NFT {
+        access(all) fun mintMomentWithSubedition(playID: UInt32, subeditionID: UInt32): @NFT {


NFTMinter entitlement?

joshuahannan · 2024-04-22T20:12:44Z

contracts/TopShot.cdc

@@ -463,7 +464,7 @@ pub contract TopShot: NonFungibleToken {
        //
        // Returns: Collection object that contains all the Moments that were minted
        //
-         pub fun batchMintMomentWithSubedition(playID: UInt32, quantity: UInt64, subeditionID: UInt32): @Collection {
+         access(all) fun batchMintMomentWithSubedition(playID: UInt32, quantity: UInt64, subeditionID: UInt32): @Collection {


NFTMinter entitlement?

joshuahannan · 2024-04-22T20:13:52Z

contracts/TopShot.cdc

        // Dictionary of Moment conforming tokens
        // NFT is a resource type with a UInt64 ID field
-        pub var ownedNFTs: @{UInt64: NonFungibleToken.NFT}
+        access(contract) var ownedNFTs: @{UInt64: {NonFungibleToken.NFT}}


Suggested change

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

access(all) var ownedNFTs: @{UInt64: {NonFungibleToken.NFT}}

This needs to be access(all) now based on recent FLIPs and standard changes

joshuahannan · 2024-04-22T20:14:26Z

contracts/TopShot.cdc

@@ -1191,7 +1224,7 @@ pub contract TopShot: NonFungibleToken {

        // lock takes a token id and a duration in seconds and locks
        // the moment for that duration
-        pub fun lock(id: UInt64, duration: UFix64) {
+        access(NonFungibleToken.Update | NonFungibleToken.Owner) fun lock(id: UInt64, duration: UFix64) {


Can you include this event emission here?

contracts/TopShot.cdc

* cadence 1 upgrade * fix tests * token updated

first version of cadence 1.0

30006da

bshahid331 requested a review from rrrkren March 13, 2024 17:19

bshahid331 requested a review from a team as a code owner March 13, 2024 17:19

sisyphusSmiling reviewed Mar 13, 2024

View reviewed changes

bshahid331 added 2 commits March 20, 2024 11:47

Add minter entitlement and inherity NFT public interface in TopShot p…

1743167

…ublic interface

update go version

52cadab

joshuahannan reviewed Mar 27, 2024

View reviewed changes

bshahid331 added 3 commits April 3, 2024 12:34

Swith view and access positions

4ed8b2e

Adjust smart contracts for cadence 1.0

3d8761e

fix contracts test

269f08b

joshuahannan reviewed Apr 22, 2024

View reviewed changes

rrrkren added 2 commits May 7, 2024 15:03

nft v1.0.0 upgrade (#252)

255fec3

* cadence 1 upgrade * fix tests * token updated

sharded collection conforming to NFT provider and deposit

523ab99

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update NBA contracts & txs to Cadence 1.0 #248

Update NBA contracts & txs to Cadence 1.0 #248

bshahid331 commented Mar 13, 2024

sisyphusSmiling left a comment

sisyphusSmiling Mar 13, 2024

sisyphusSmiling Mar 13, 2024

sisyphusSmiling Mar 13, 2024

joshuahannan Mar 27, 2024

joshuahannan Apr 22, 2024

sisyphusSmiling Mar 13, 2024

sisyphusSmiling Mar 13, 2024

joshuahannan left a comment

joshuahannan Mar 27, 2024

joshuahannan Mar 27, 2024

joshuahannan Mar 27, 2024

joshuahannan Mar 27, 2024

joshuahannan Mar 27, 2024

joshuahannan Apr 22, 2024

joshuahannan Mar 27, 2024

joshuahannan Mar 27, 2024

joshuahannan Mar 27, 2024

joshuahannan Mar 27, 2024

joshuahannan left a comment

joshuahannan Apr 22, 2024

joshuahannan Apr 22, 2024

joshuahannan Apr 22, 2024

joshuahannan Apr 22, 2024

joshuahannan Apr 22, 2024

joshuahannan Apr 22, 2024

joshuahannan Apr 22, 2024

joshuahannan Apr 22, 2024

joshuahannan Apr 22, 2024

	view access(all) fun getFastBreakRun(id: String): FastBreakV1.FastBreakRun? {
	access(all) view fun getFastBreakRun(id: String): FastBreakV1.FastBreakRun? {

	view access(all) fun getFastBreakSubmissionByPlayerId(playerId: UInt64): FastBreakV1.FastBreakSubmission? {
	access(all) view fun getFastBreakSubmissionByPlayerId(playerId: UInt64): FastBreakV1.FastBreakSubmission? {

		access(all) fun getIDs(): [UInt64]
		access(all) fun borrowNFT(_ id: UInt64): &{NonFungibleToken.NFT}?


		pub contract TopShot: NonFungibleToken {
		access(all) contract TopShot: NonFungibleToken, ViewResolver {

		view access(all) fun borrowNFTSafe(id: UInt64): &{NonFungibleToken.NFT}? {
		return &self.ownedNFTs[id]

		access(all) view fun getLength(): Int {
		return self.ownedNFTs.keys.length

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

Update NBA contracts & txs to Cadence 1.0 #248

Are you sure you want to change the base?

Update NBA contracts & txs to Cadence 1.0 #248

Conversation

bshahid331 commented Mar 13, 2024

sisyphusSmiling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuahannan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuahannan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment