nft-storefront icon indicating copy to clipboard operation
nft-storefront copied to clipboard

NFTStorefront v1 & v2 Cadence 1.0 Refactor

Open sisyphusSmiling opened this issue 2 years ago • 7 comments

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:

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

sisyphusSmiling avatar Sep 26 '23 23:09 sisyphusSmiling

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

sisyphusSmiling avatar Sep 29 '23 23:09 sisyphusSmiling

@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.

aishairzay avatar Apr 03 '24 19:04 aishairzay

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

highskore avatar Apr 15 '24 08:04 highskore

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.

aishairzay avatar Apr 15 '24 15:04 aishairzay

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

highskore avatar Apr 15 '24 15:04 highskore

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!

aishairzay avatar Apr 15 '24 16:04 aishairzay

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

joshuahannan avatar May 01 '24 15:05 joshuahannan

@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

joshuahannan avatar May 21 '24 21:05 joshuahannan