kodadot / nft-gallery

Generative Art Marketplace
https://kodadot.xyz
MIT License
605 stars 347 forks source link

Composable-transactionMint #5276

Closed daiagi closed 1 year ago

daiagi commented 1 year ago

Thank you for your contribution to the KodaDot NFT gallery.

πŸ‘‡ _ Let's make a quick check before the contribution.

PR Type

Context

Before submitting pull request, please make sure:

Optional

Had issue bounty label?

Community participation

Screenshot πŸ“Έ

kodabot commented 1 year ago

WARNING @daiagi PR for issue #5034 which isn't assigned to you. Please be warned that this PR may get rejected if there's another assignee for issue #5034

netlify[bot] commented 1 year ago

Deploy Preview for koda-nuxt ready!

Name Link
Latest commit 2740e9f6d0554168766b8172db94d9da641757e4
Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/64187d73b99a7b00084abbe7
Deploy Preview https://deploy-preview-5276--koda-nuxt.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.

daiagi commented 1 year ago

this PR covers minting Token composable Minting collection will follow in another PR

vikiival commented 1 year ago

Minting collection will follow in another PR

I would rather do this first

vikiival commented 1 year ago

Yeah sorry, closing that was a bit harsh. Sorry for that I just overreacted

I just rather do the collections first, it is much easier and 80% of pinning logic can be extracted away.

The reasoning is this is hard to review. Contains a lot of dupictated code.

daiagi commented 1 year ago

Yeah, it was challenging to write as well

I'll do the collection minting, sure

But as for this one, how about I split the files to make it easier read?

And there are actually no duplications In fact I've removed duplicate code

vikiival commented 1 year ago

But as for this one, how about I split the files to make it easier read?

Yes please decompose pinning from the file

I'll do the collection minting, sure

Yeah I think it's easier to do.

Sorry one more time for closing that πŸ™ˆ

and thanks @roiLeo for reopening that

roiLeo commented 1 year ago

Hey! I don't want to interfere but I've just tested on snek (/snek/gallery/331660682-9) and for me it works very well. It's already a good start and it allows to get the logic out of the components (better readability). And that is what I asked when I've opened #5034. In any case it will be useful when we'll rewrite minting experence (and maybe massmint). Nice work, keep it up! πŸ˜‰

daiagi commented 1 year ago

Hey! I don't want to interfere but I've just tested on snek (/snek/gallery/331660682-9) and for me it works very well. It's already a good start and it allows to get the logic out of the components (better readability). And that is what I asked when I've open #5034. In any case it will be useful when we'll rewrite minting experence (and maybe massmint). Nice work, keep it up! wink

Thanks @roiLeo Appreciate the positive feedback :smile:

vikiival commented 1 year ago

Wondering why are you refactoring SimpleMint instead of rmrk/CreateToken.vue ?

Screenshot 2023-03-15 at 14 48 16
vikiival commented 1 year ago

But so far really good job πŸ€—

daiagi commented 1 year ago

Wondering why are you refactoring SimpleMint instead of rmrk/CreateToken.vue ?

I was working on CreateToken

simpleMint just one line change due to moving utils outside of rmrk folder

image

vikiival commented 1 year ago

image

vikiival commented 1 year ago

Looks like some failed checks

daiagi commented 1 year ago

Update: created composables for minting collection and use them in CreateCollection components

@roiLeo have a look once again please

daiagi commented 1 year ago

Looks like some failed checks

@vikiival they can be ignored because 1. image

both these function will return for chains bsx, snek, rmrk, rmrk2, as needed, DeepSource complains that there isn't a return value outside of an if, and there shouldn't be

2. image

this is about the signature of executeTransaction \ howAboutToExecute. I'm not gonna mess with it in this PR

roiLeo commented 1 year ago

Update: created composables for minting collection and use them in CreateCollection components

@roiLeo have a look once again please

Now same issue as #5266 (collectionId: 3124270296)

Screenshot 2023-03-16 at 13-21-58 KodaDot Low fees and low carbon minting KodaDot - NFT Market Explorer

I saw somewhere that [[ Rare Waifu ]] was default fallback metadata

daiagi commented 1 year ago

Now same issue as #5266 (collectionId: 3124270296)

What chain? I'll double check.

roiLeo commented 1 year ago

Now same issue as #5266 (collectionId: 3124270296)

What chain? I'll double check.

/snek BLOCK 1827618

daiagi commented 1 year ago

ok so...

  1. it happens when minting collection on snek in beta as well
  2. I've tested the minting collection in this PR in RMRK and BSX, both works

so the issue does not stem from this PR

damskyftw commented 1 year ago

It currently doesn’t let me mint with KSM fee asset. Says "Insufficient funds" even tho i have enough :)

daiagi commented 1 year ago

It currently doesn’t let me mint with KSM fee asset. Says "Insufficient funds" even tho i have enough :)

Hey @helloitsdamsky Do I understand correctly that you tried on kusama network? Did you try minting a collection or NFT? does it work for you on beta?

Lastly, what account did you try with?

Thank you Possibly related with #5284

update: I've just tested minting collection and NFT on Kusama and Basilisk. I've encountered no issues

daiagi commented 1 year ago

tested again by minting collection and NFT on BSX and RMRK

image image

vikiival commented 1 year ago

Can please @JustLuuuu or @helloitsdamsky test?

daiagi commented 1 year ago

/reviewpad summarize

JustLuuuu commented 1 year ago

Can please @JustLuuuu or @helloitsdamsky test?

If Im using the correct Deploy link for testing, it's still not working for me:

Screenshot 2023-03-20 at 09 58 08

I have created a new wallet especially to test this while I was writing an article about minting on Basilisk. This account has exactly 0 bsx tokens. Only KSM (bridged). I believe even if you set KSM as a fee, it still somehow requires bsx. Test it with a wallet where you have 0 bsx.

When I tried to test it with a wallet where I had some bsx tokens. It worked.

reviewpad[bot] commented 1 year ago

This pull request includes various changes across multiple files. Changes involve new functions, types, and modifications to existing functions related to minting NFTs and collections. The execMintToken, execMintBasilisk, execMintCollection, execMintCollectionRmrk, useNewCollectionId, constructMeta, and useTransaction functions were added or modified. Various files such as CreateToken.vue, transactionMintCollection.ts, utils.ts, and notification.ts were also updated. Other changes include updates to import statements and regular expressions.

daiagi commented 1 year ago

I have created a new wallet especially to test this while I was writing an article about minting on Basilisk. This account has exactly 0 bsx tokens. Only KSM (bridged). I believe even if you set KSM as a fee, it still somehow requires bsx. Test it with a wallet where you have 0 bsx.

When I tried to test it with a wallet where I had some bsx tokens. It worked.

strongly suspect it has to do with this:

daiagi commented 1 year ago

I had a chat with @JustLuuuu who confirmed minting on Kusama works I have also created PR #5305 to fix the minting with bridged KSM on Basilisk issue

vikiival commented 1 year ago

More like suggestions otherwise LGTM

vikiival commented 1 year ago

pay 100 usd

yangwao commented 1 year ago

😍 Perfect, I’ve sent the payout πŸ’΅ $100 @ 35.66 USD/KSM ~ 2.804 $KSM πŸ§— EfmnRhHaQqfT3phm4cUCHCU3gFVDoSPR1U9WXzMRQBMqZ4L πŸ”— 0xc516fcda2389cb05b71352d71bcc3885de14deface541810ad216fbb1a4dc5ab

πŸͺ… Let’s grab another issue and get rewarded! πŸͺ„ github.com/kodadot/nft-gallery/issues

codeclimate[bot] commented 1 year ago

Code Climate has analyzed commit 2740e9f6 and detected 14 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 6
Duplication 8

View more on Code Climate.