NFTStorefront v1 & v2 Cadence 1.0 Refactor #89

sisyphusSmiling · 2023-09-26T23:44:33Z

Closes: #87

This PR targets the existing stable-cadence branch, so includes some updates from main. The idea is to use stable-cadence as the working Stable Cadence branch moving forward.

Dependencies pulled from respective repo's stable-cadence branches:

ViewResolver
FungibleToken
NonFungibleToken
MultipleNFT
ExampleNFT

The changes against NFTStorefront and accompanying transactions & scripts have been made to conform to Cadence 1.0 language updates.

Note: It looks like the test suite is running the current version of Cadence as tests are not passing.

TODO:

Merge stable-cadence branch to this one? Picked up from up from Stable Cadence Feature Branch #74, so merge might be best way forward.
Update dependency repos
- flow-ft transactions & scripts on stable cadence feature branch
- flow-nft transactions & scripts on stable cadence feature branch
Get test cases to pass

* update go lib * ci: update emulator version the version specified wasn't released

Basic cleanups to set stage for more coming

* add functionality of ghost listing * add testing for the function * fix flow.json * minor fix * Typos hunter * change function name --------- Co-authored-by: Álvaro Lillo Igualada <[email protected]>

* add documentation related to the ghost listing * Update docs/documentation.md Co-authored-by: Joshua Hannan <[email protected]> * Update scripts/has_listing_become_ghosted.cdc Co-authored-by: Joshua Hannan <[email protected]> * Update scripts/has_listing_become_ghosted.cdc Co-authored-by: Joshua Hannan <[email protected]> * Update scripts/has_listing_become_ghosted.cdc Co-authored-by: Joshua Hannan <[email protected]> * Update docs/documentation.md Co-authored-by: j pimmel <[email protected]> * add a recomendation to use cleanup ghost listing * Update docs/documentation.md Co-authored-by: j pimmel <[email protected]> * Update docs/documentation.md Co-authored-by: j pimmel <[email protected]> * Update docs/documentation.md Co-authored-by: j pimmel <[email protected]> * minor fixes --------- Co-authored-by: Joshua Hannan <[email protected]> Co-authored-by: j pimmel <[email protected]>

* update the core contracts * fix test cases * minor fix

joshuahannan

Looks like v2 still needs some work, so I can come back to this review once you have the tests passing

contracts/NFTStorefrontV2.cdc

contracts/utility/NFTCatalog.cdc

sisyphusSmiling · 2023-09-29T23:14:02Z

Progress on this PR is blocked until flow-ft and flow-nft repos have fully functional stable cadence branches, especially the former.
It appears the transactions from flow-ft are utilized in the test suite and since flow-ft transactions aren't up to Cadence 1.0 in the stable cadence feature branch, they're failing in the NFTStorefront test suite.
Relevant discord discussion: https://discord.com/channels/613813861610684416/621847426201944074/1157425948405280858

…Also add new cadence testing framework for storefrontv2 testing.

joshuahannan

Looks pretty good! Just a few small comments about style and stuff. I only have reviewed the contract changes so far but I can get to the transactions and stuff later after you've addressed my initial comments.

Remember to update to all the latest versions of the emulator, cadence, and CLI so that we can be sure that we're testing with the most up to date versions. I left some comments where that would apply for CI and the Go tests, but I'm not sure where the dependencies are imported for the JS tests. You might need to update those too

.github/workflows/ci.yml

contracts/NFTStorefront.cdc

contracts/NFTStorefrontV2.cdc

docs/documentation.md

lib/go/test/go.mod

…v1. And update some documentation for cadence 1.0.

aishairzay · 2024-04-03T19:35:51Z

@joshuahannan thanks for the comments, newest update should address all of them.

I also removed the go testing entirely which makes the version issue not a problem. The previous go testing was very simple testing targeted only towards storefrontv1, and so I've replaced it with new cadence testing for storefrontv1.

highskore · 2024-04-15T08:18:49Z

I'm using the stable cadence version of NFTStorefront and borrowNFT is always reverting for me here:
assert(ref.isInstance(self.getDetails().nftType), message: "token has wrong type"), but creating the listing works fine, it would revert on create if type was actually invalid right?

Commenting this line makes it work obviously

aishairzay · 2024-04-15T15:15:19Z

I'm using the stable cadence version of NFTStorefront and borrowNFT is always reverting for me here: assert(ref.isInstance(self.getDetails().nftType), message: "token has wrong type"), but creating the listing works fine, it would revert on create if type was actually invalid right?

Commenting this line makes it work obviously

Taking a look at this - In create listing we're using the following code: assert(nft!.getType() == self.details.nftType, message: "token is not of specified type") which differs from the ref.isInstance(self.getDetails().nftType that's in borrowNFT. I'm not entirely sure why one would work and the other wouldn't, so looking into that.

highskore · 2024-04-15T15:37:45Z

I'm using the stable cadence version of NFTStorefront and borrowNFT is always reverting for me here: assert(ref.isInstance(self.getDetails().nftType), message: "token has wrong type"), but creating the listing works fine, it would revert on create if type was actually invalid right?
Commenting this line makes it work obviously

Taking a look at this - In create listing we're using the following code: assert(nft!.getType() == self.details.nftType, message: "token is not of specified type") which differs from the ref.isInstance(self.getDetails().nftType that's in borrowNFT. I'm not entirely sure why one would work and the other wouldn't, so looking into that.

I think a test covering borrowNFT would be sufficient

…ft type.

aishairzay · 2024-04-15T16:09:17Z

I'm using the stable cadence version of NFTStorefront and borrowNFT is always reverting for me here: assert(ref.isInstance(self.getDetails().nftType), message: "token has wrong type"), but creating the listing works fine, it would revert on create if type was actually invalid right?
Commenting this line makes it work obviously

Taking a look at this - In create listing we're using the following code: assert(nft!.getType() == self.details.nftType, message: "token is not of specified type") which differs from the ref.isInstance(self.getDetails().nftType that's in borrowNFT. I'm not entirely sure why one would work and the other wouldn't, so looking into that.

I think a test covering borrowNFT would be sufficient

Thanks for bringing this up.

I added a new script and test case to cover borrowNFT now, also updated the previewnet build to reflect the fix needed to properly check the nft type. If you run into any more issues please keep them coming here!

joshuahannan

Left a couple more comments. Basically just entitlement naming and some view functions. I'd really like to get all of these deployed on previewnet and staged on testnet soon. Right now it only looks like V2 is staged. Community members have been bugging us about it. Let me know when you think you'll be able to get to these and do the staging/deployments.

contracts/NFTStorefrontV2.cdc

joshuahannan · 2024-05-01T15:58:32Z

I updated all the dependencies, addressed all my previous comments on this PR, and got the cadence tests passing. NFTStorefront is staged on testnet, but NFTStorefrontV2 is not staged on testnet because we are still looking for the key information for the account.

Missing key information for previewnet also

Any more reviews here would be much appreciated

sisyphusSmiling

I left comments, but nothing that can't be addressed in a follow up - no reason to block. I can't leave an approval since I originally submitted the PR, but LGTM ✅

contracts/NFTStorefront.cdc

contracts/NFTStorefrontV2.cdc

flow.json

sisyphusSmiling · 2024-05-21T19:43:19Z

transactions/buy_item.cdc

-/// Buyer of the listing (,i.e. underling NFT) would authorize and sign the
-/// transaction and if purchase happens then transacted NFT would store in
-/// buyer's collection.
-
 transaction(listingResourceID: UInt64, storefrontAddress: Address, commissionRecipient: Address?) {


Since we're moving to a generalized interface for NFT & FT transactions, it'd be worth making this transaction compatible with any standard asset instead of the statically declared ExampleToken & ExampleNFT IMO

sisyphusSmiling · 2024-05-21T19:44:15Z

transactions/example-nft/burn_nft.cdc

+        // withdraw the NFT from the owner's collection
+        let nft <- self.collectionRef.withdraw(withdrawID: id)
+
+        destroy nft


It'd be nice to see Burner.burn used here

sisyphusSmiling · 2024-05-21T19:45:02Z

transactions/example-nft/mint_nft.cdc

+        // // Create the royalty details
+        // var count = 0
+        // var royalties: [MetadataViews.Royalty] = []
+        // while royaltyBeneficiaries.length > count {
+        //     let beneficiary = royaltyBeneficiaries[count]
+        //     let beneficiaryCapability = getAccount(beneficiary).capabilities.get<&{FungibleToken.Receiver}>(
+        //             MetadataViews.getRoyaltyReceiverPublicPath()
+        //         ) ?? panic("Beneficiary does not have Receiver configured at RoyaltyReceiverPublicPath")
+
+        //     royalties.append(
+        //         MetadataViews.Royalty(
+        //             receiver: beneficiaryCapability,
+        //             cut: cuts[count],
+        //             description: royaltyDescriptions[count]
+        //         )
+        //     )
+        //     count = count + 1
+        // }


Looks like this can be uncommented

sisyphusSmiling · 2024-05-21T19:46:19Z

transactions/sell_item.cdc

+transaction(
+    saleItemID: UInt64,
+    saleItemPrice: UFix64,
+    customID: String?,
+    commissionAmount: UFix64,
+    expiry: UInt64,
+    marketplacesAddress: [Address]
+) {


Same comment as above about generalizing the transaction interface for use with any NFT & FT

…kage

joshuahannan · 2024-05-21T21:43:14Z

@sisyphusSmiling I addressed your comments about the contracts and pushed my changes. I created an issue for the transaction changes that I am going to come back to later

poiati and others added 23 commits December 22, 2022 23:47

Update go lib (#76)

14b9020

* update go lib * ci: update emulator version the version specified wasn't released

Advise that version is no longer recommended

fb594cd

Move some documentation around and clear things up

2adb839

Content moved to homepage readme

10d6d16

Version disclaimer

be8a884

Just a brush up to prep for some broader changes

f7cb300

Merge pull request #80 from onflow/jp-update-structure

b594574

Basic cleanups to set stage for more coming

Add Ghost listing functionality (#79)

541f161

* add functionality of ghost listing * add testing for the function * fix flow.json * minor fix * Typos hunter * change function name --------- Co-authored-by: Álvaro Lillo Igualada <[email protected]>

Update the core contracts to their latest version (#84)

5d0c203

* update the core contracts * fix test cases * minor fix

update .gitignore

0885583

update ViewResolver for Cadence 1.0

b1cd04f

update FungibleToken for Cadence 1.0

f2728f3

update NonFungibleToken for Cadence 1.0

ffd20f2

update ExampleNFT for Cadence 1.0

d604d40

add MultipleNFT contract

266bccf

update NFTStorefront for Cadence 1.0

5d5759a

update NFTStorefront setup txn for Cadence 1.0

4647bc0

add updated ExampleNFT txns & scripts

16712da

update sell_item.cdc

c6abc55

update mint_nft.cdc

8585f7d

add FlowToken aliases to flow.json

20f7471

update v1 transactions & scripts

b305e85

sisyphusSmiling requested a review from a team September 26, 2023 23:44

sisyphusSmiling self-assigned this Sep 26, 2023

sisyphusSmiling changed the title ~~Stable cadence refactor~~ NFTStorefront Cadence 1.0 Refactor Sep 26, 2023

sisyphusSmiling added 2 commits September 27, 2023 15:50

Merge branch 'stable-cadence' into stable-cadence-refactor

f8c446f

bump ci flow-cli version

9b6248c

joshuahannan reviewed Sep 28, 2023

View reviewed changes

contracts/NFTStorefrontV2.cdc Outdated Show resolved Hide resolved

contracts/NFTStorefrontV2.cdc Outdated Show resolved Hide resolved

contracts/utility/NFTCatalog.cdc Outdated Show resolved Hide resolved

update go modules

ff62d07

update v2 sale transactions

54d6b65

sisyphusSmiling mentioned this pull request Sep 29, 2023

Stable Cadence preview release support onflow/flow-js-testing#224

Closed

sisyphusSmiling changed the title ~~NFTStorefront Cadence 1.0 Refactor~~ NFTStorefront v1 & v2 Cadence 1.0 Refactor Sep 29, 2023

new emulator api

e98b0db

aishairzay added 2 commits March 15, 2024 11:54

Use updated flow-nft and flow-ft standards, and updated cadence 1.0. …

26f9d7e

…Also add new cadence testing framework for storefrontv2 testing.

Make all scripts for storefrontv2 work with updated cadence 1.0.

db5860c

joshuahannan reviewed Apr 2, 2024

View reviewed changes

Remove previous go testing in favor of cadence testing for storefront…

bc78654

…v1. And update some documentation for cadence 1.0.

aishairzay marked this pull request as ready for review April 3, 2024 19:36

aishairzay added 2 commits April 10, 2024 11:45

Add flow previewnet deployment for storefrontv2.

731b0e4

Add previewnet to readme and add storefrontv1 to previewnet.

c0ed2d0

Add script to test borrowNFT and make borrowNFT correctly check the n…

51e56b2

…ft type.

joshuahannan reviewed Apr 22, 2024

View reviewed changes

contracts/NFTStorefrontV2.cdc Outdated Show resolved Hide resolved

contracts/NFTStorefrontV2.cdc Outdated Show resolved Hide resolved

joshuahannan added 2 commits April 26, 2024 12:45

use latest contracts and consolidate flow.json

46062ad

update provider capabilities

97e3cfa

joshuahannan approved these changes May 21, 2024

View reviewed changes

sisyphusSmiling commented May 21, 2024

View reviewed changes

address PR comments, update Cadence version, and add go contracts pac…

aa171af

…kage

joshuahannan merged commit a303f4a into stable-cadence May 21, 2024

joshuahannan deleted the stable-cadence-refactor branch May 21, 2024 21:45

joshuahannan restored the stable-cadence-refactor branch May 21, 2024 21:47

joshuahannan mentioned this pull request May 21, 2024

Merge Stable Cadence Changes to main #96

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NFTStorefront v1 & v2 Cadence 1.0 Refactor #89

NFTStorefront v1 & v2 Cadence 1.0 Refactor #89

sisyphusSmiling commented Sep 26, 2023 •

edited by aishairzay

Loading

joshuahannan left a comment

sisyphusSmiling commented Sep 29, 2023

joshuahannan left a comment

aishairzay commented Apr 3, 2024

highskore commented Apr 15, 2024

aishairzay commented Apr 15, 2024

highskore commented Apr 15, 2024

aishairzay commented Apr 15, 2024

joshuahannan left a comment •

edited

Loading

joshuahannan commented May 1, 2024 •

edited

Loading

sisyphusSmiling left a comment

sisyphusSmiling May 21, 2024

sisyphusSmiling May 21, 2024

sisyphusSmiling May 21, 2024

sisyphusSmiling May 21, 2024

joshuahannan commented May 21, 2024

NFTStorefront v1 & v2 Cadence 1.0 Refactor #89

NFTStorefront v1 & v2 Cadence 1.0 Refactor #89

Conversation

sisyphusSmiling commented Sep 26, 2023 • edited by aishairzay Loading

TODO:

joshuahannan left a comment

Choose a reason for hiding this comment

sisyphusSmiling commented Sep 29, 2023

joshuahannan left a comment

Choose a reason for hiding this comment

aishairzay commented Apr 3, 2024

highskore commented Apr 15, 2024

aishairzay commented Apr 15, 2024

highskore commented Apr 15, 2024

aishairzay commented Apr 15, 2024

joshuahannan left a comment • edited Loading

Choose a reason for hiding this comment

joshuahannan commented May 1, 2024 • edited Loading

sisyphusSmiling left a comment

Choose a reason for hiding this comment

sisyphusSmiling May 21, 2024

Choose a reason for hiding this comment

sisyphusSmiling May 21, 2024

Choose a reason for hiding this comment

sisyphusSmiling May 21, 2024

Choose a reason for hiding this comment

sisyphusSmiling May 21, 2024

Choose a reason for hiding this comment

joshuahannan commented May 21, 2024

sisyphusSmiling commented Sep 26, 2023 •

edited by aishairzay

Loading

joshuahannan left a comment •

edited

Loading

joshuahannan commented May 1, 2024 •

edited

Loading