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

Fix/minting-with-listing--rmrk2 #6156

Merged
merged 24 commits into from
Jun 21, 2023
Merged

Conversation

daiagi
Copy link
Contributor

@daiagi daiagi commented Jun 3, 2023

Thank you for your contribution to the KodaDot - One Stop Shop for Polkadot NFTs.

👇 __ Let's make a quick check before the contribution.

PR Type

  • Bugfix
  • Feature
  • Refactoring

Needs QA check

  • @kodadot/qa-guild please review

Context

Did your issue had any of the "$" label on it?

Screenshot 📸

  • My fix has changed UI

Testing

Test by mint+list something on RMRK2

Copilot Summary

🤖 Generated by Copilot at a76663f

This pull request adds support for the rmrk2 NFT format and improves the user experience of creating and listing NFTs for sale. It updates the CreateToken.vue component, the scheme.ts service, the GalleryItemDescription.vue component, and the types.ts composable to handle the new format and the useTransaction composable. It also fixes some path and naming issues.

🤖 Generated by Copilot at a76663f

To support the rmrk2 format
We had to update some code paths
We changed properties
To meta.attributes
And imported a new type from minimark

@kodabot
Copy link
Collaborator

kodabot commented Jun 3, 2023

SUCCESS @daiagi PR for issue #6149 which is assigned to you. Please wait for review and don't hesitate to grab another issue in the meantime!

@netlify
Copy link

netlify bot commented Jun 3, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit 6ac7787
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/6492f6fa09a9e7000808fd90
😎 Deploy Preview https://deploy-preview-6156--koda-canary.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@kodabot
Copy link
Collaborator

kodabot commented Jun 3, 2023

SUCCESS @daiagi PR for issue #6150 which is assigned to you. Please wait for review and don't hesitate to grab another issue in the meantime!

@reviewpad
Copy link
Contributor

reviewpad bot commented Jun 3, 2023

AI-Generated Summary: This pull request contains two patches:

  1. The first patch fixes a missing attribute issue in the GalleryItem by updating the object property path in GalleryItemDescription.vue. It changes nftMetadata.value?.properties to nftMetadata.value?.meta?.attributes.

  2. The second patch makes several changes related to the listing and redirect on RMRK2 NFTs in the CreateToken.vue component, including:

  • Updating the listForSale method to accept a single NFT item instead of an array, and switching to using the CreatedNFT or CreatedNFTV2 type from the minimark library, as well as watching the transaction processing status.
  • Updating the navigateToDetail method to accept an object with nftId and nftName instead of a full CreatedNFT object.
  • Adding new methods toNFTIdV1 and toNFTIdV2 in scheme.ts, with toNFTIdV2 handling RMRK2 NFTs.
  • Modifying the toNFTId method to call toNFTIdV1 and toNFTIdV2 depending on whether it's an RMRK 1 or RMRK 2 NFT.

@reviewpad reviewpad bot added the medium Pull request is medium label Jun 3, 2023
@daiagi daiagi marked this pull request as ready for review June 3, 2023 14:14
@daiagi daiagi requested a review from a team as a code owner June 3, 2023 14:14
@daiagi daiagi requested review from vikiival and Jarsen136 and removed request for a team June 3, 2023 14:14
@prury
Copy link
Member

prury commented Jun 3, 2023

Testing Process(working properly):

@daiagi thanks for adding a how to test section! ❤️

  • Signed two transactions, to mint and list.
  • Added four properties to the minted NFT:
  • listing ok:

Minted NFT: https://deploy-preview-6156--koda-canary.netlify.app/ksm/gallery/18200581-7EA1DCF47E98A25067-PNKTST-ZOMBIE_GIRL_PUNK-00000005

Extrinsics:
https://kusama.subscan.io/tx/0xcf4baa03b4e05fe7596066629d28943192b3ca9b3c382122980bdc2679062698
https://kusama.subscan.io/tx/0x22a1fbd32f6dbcef47006e9603e9614b1431dc37b88d664fb1c10f911601b990

@prury prury added the S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked label Jun 3, 2023
@roiLeo
Copy link
Contributor

roiLeo commented Jun 8, 2023

Testing Process(working properly):

Minted NFT: https://deploy-preview-6156--koda-canary.netlify.app/ksm/gallery/18200581-7EA1DCF47E98A25067-PNKTST-ZOMBIE_GIRL_PUNK-00000005

Shouldn't we have to show key & value instead of Section number?

@prury
Copy link
Member

prury commented Jun 8, 2023

Testing Process(working properly):

Minted NFT: https://deploy-preview-6156--koda-canary.netlify.app/ksm/gallery/18200581-7EA1DCF47E98A25067-PNKTST-ZOMBIE_GIRL_PUNK-00000005

Shouldn't we have to show key & value instead of Section number?

Makes sense! good catch!
@daiagi

@daiagi
Copy link
Contributor Author

daiagi commented Jun 9, 2023

@roiLeo @prury
resolved

KSM:

RMRK:

@prury
Copy link
Member

prury commented Jun 9, 2023

@daiagi really good, ty!

@daiagi
Copy link
Contributor Author

daiagi commented Jun 15, 2023

@vikiival please release new version of minimark

@vikiival
Copy link
Member

Screenshot 2023-06-15 at 12 39 06

@yangwao
Copy link
Member

yangwao commented Jun 15, 2023

I guess we should wait for this to upcoming release 👀

@exezbcz
Copy link
Member

exezbcz commented Jun 17, 2023

@daiagi oki copies, is it

@daiagi
Copy link
Contributor Author

daiagi commented Jun 18, 2023

/reviewpad summarize

@prury
Copy link
Member

prury commented Jun 19, 2023

@daiagi please lmk when i can check again

@vikiival
Copy link
Member

I think that you can test it

@prury
Copy link
Member

prury commented Jun 19, 2023

  • Its still showing editions instead of copies.

  • 5 editions test:
    image

https://kusama.subscan.io/tx/0xa576a85f53cc57a5bfbec7a4b7d2163129099e4c351753dba358469ddb760faf
https://kusama.subscan.io/tx/0x9dd8eb22997fa8b7c5a6efffffa4955c9d125d4f5adff9ba4cc7bfa89c4bca1a


and then I've tried to mint 10 copies of the first NFT (should be 9 max according to what viki wrote(3/3))
but it did mint the 10 copies, and then it disabled my ability to mint more NFTs in this collection(as expected).

  • listing occurred as expected

Extrinsics:
https://kusama.subscan.io/tx/0x00c9a44b9c197c705090ceb7bb4994861d169ba5f0662379da14c6cb6b3d0d8d
https://kusama.subscan.io/tx/0x8fc30ff58981ee51d9452d21694471f7e9a372cb6451edd5f15c25690d7528e1
https://kusama.subscan.io/tx/0xb317f9fc46ba65cc7513a25ed5e413ec6c6e9bcb714b8ace2bd4d0ddffb9370a

IMO, on the user side, we should have a input stating how many nfts/copies the user still has left.

@daiagi
Copy link
Contributor Author

daiagi commented Jun 20, 2023

ts still showing editions instead of copies.

done

Now testing if i can go above the limit:

fixed. it will now mint up to the maximum allowed in the collection

and then I've tried to mint 10 copies of the first NFT (should be 9 max according to what viki wrote(3/3))

i think you misunderstood. if the max of the collection is set to 3 you are suppose to able to mint only up to 3, not 9
#6156 (comment)

IMO, on the user side, we should have a input stating how many nfts/copies the user still has left.

True. but let's push it off to a new issue

@prury
Copy link
Member

prury commented Jun 20, 2023

ts still showing editions instead of copies.

done

Now testing if i can go above the limit:

fixed. it will now mint up to the maximum allowed in the collection

ty, will check.

and then I've tried to mint 10 copies of the first NFT (should be 9 max according to what viki wrote(3/3))

i think you misunderstood. if the max of the collection is set to 3 you are suppose to able to mint only up to 3, not 9 #6156 (comment)

oh, you are correct, i did misunderstood, its way simpler than what i have previously tough, thank you daiagi!

IMO, on the user side, we should have a input stating how many nfts/copies the user still has left.

True. but let's push it off to a new issue

will do!

@prury
Copy link
Member

prury commented Jun 20, 2023

@daiagi can you give me a hand and test it if you are being able to mint without listing with this deployed version? it won't work here for me, the transaction completes successfully, but no NFT is created.
similar to what is happening on #6245 (comment)

https://kusama.subscan.io/tx/0xdfa6a4a41f4adaaf6b23c3bad425aea45d0259fe6721366c0839fd728269c046

image

@roiLeo roiLeo mentioned this pull request Jun 20, 2023
@daiagi
Copy link
Contributor Author

daiagi commented Jun 20, 2023

@prury

Sure. I'll check

@daiagi
Copy link
Contributor Author

daiagi commented Jun 21, 2023

@prury
Mint w royalty/ w/o listing:

image

mint w/o royalty w/o listing:

image

mint 3 copies, w royalties, w properties w/o listing:

image

mint 2 copies, w list (list would have appeared in the next transaction, but i cancel the minting)

image

and your own successful mint without listing as well:
https://kusama.subscan.io/extrinsic/18439604-4
https://deploy-preview-6156--koda-canary.netlify.app/ksm/gallery/18439604-7EA1DCF47E98A25067-PNKTST-RADIOACTIVE_PUNK_1-00000006

@prury
Copy link
Member

prury commented Jun 21, 2023

Thank you for the many tests!

and your own successful mint without listing as well: https://kusama.subscan.io/extrinsic/18439604-4 https://deploy-preview-6156--koda-canary.netlify.app/ksm/gallery/18439604-7EA1DCF47E98A25067-PNKTST-RADIOACTIVE_PUNK_1-00000006

This one i minted on canary😛

did another one today and it went well:
https://kusama.subscan.io/tx/0xd5d1735f13d4aa784fe79fd642a49eedc4b3e3931e3a0e6fec7b12ae317dc76f

@daiagi last question, will page redirection after minting be done in another PR? otherwise works for me

@prury prury added the S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked label Jun 21, 2023
@vikiival
Copy link
Member

please resolve conflicts

@vikiival
Copy link
Member

@reviewpad reviewpad bot added large Pull request is large and removed medium Pull request is medium labels Jun 21, 2023
@codeclimate
Copy link

codeclimate bot commented Jun 21, 2023

Code Climate has analyzed commit 6ac7787 and detected 0 issues on this pull request.

View more on Code Climate.

@vikiival vikiival merged commit d814faa into main Jun 21, 2023
17 of 19 checks passed
@vikiival vikiival deleted the fix/minting-with-listing--rmrk2 branch June 21, 2023 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
large Pull request is large S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked waiting-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minting with Listing - not working properly Atributes/Traits not visible in Properties
8 participants