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

Asset-hub-royalties #7816

Merged
merged 11 commits into from
Oct 26, 2023
Merged

Asset-hub-royalties #7816

merged 11 commits into from
Oct 26, 2023

Conversation

daiagi
Copy link
Contributor

@daiagi daiagi commented Oct 25, 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?

$

Related transacrions

  1. tx with set attributes (on chain royalty)
    https://assethub-kusama.subscan.io/extrinsic/5640724-2
  2. buy single item with royalty
    https://assethub-kusama.subscan.io/extrinsic/5640390-2
  3. buy 3 items with royalties (shopping cart)
    https://assethub-kusama.subscan.io/extrinsic/5645506-2

items minted with royalty:

/ahk/gallery/12-10
/ahk/gallery/12-11
/ahk/gallery/12-12

  • My fix has changed UI

Copilot Summary

🤖 Generated by Copilot at b6f31df

This pull request adds and improves the functionality for handling royalties on different chains and NFT standards. It enables minting and paying royalties on Statemine, toggles Rmrk-specific features based on the chain, and refactors the code for verifying and normalizing royalty data. It affects the files CreateNft.vue, transactionMintStatemine.ts, transactionBuy.ts, useIsChain.ts, support.ts, and utils.ts.

🤖 Generated by Copilot at b6f31df

Sing, O Muse, of the crafty developers who wrought
New features for the NFT marketplace, and sought
To honor the creators with royaltyFee and payTips
And to validate their data with verifyRoyalty and isRmrk scripts.

@daiagi daiagi requested a review from a team as a code owner October 25, 2023 02:54
@daiagi daiagi requested review from preschian and floyd-li and removed request for a team October 25, 2023 02:54
@kodabot
Copy link
Collaborator

kodabot commented Oct 25, 2023

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

@netlify
Copy link

netlify bot commented Oct 25, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit dc512ec
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/653a2a2337ee3c00083d9ff8
😎 Deploy Preview https://deploy-preview-7816--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 configuration.

@daiagi daiagi requested review from roiLeo and vikiival and removed request for floyd-li October 25, 2023 02:54
import { verifyRoyalty } from './utils'

function payRoyaltyAssetHub(
legacy,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function payRoyaltyAssetHub has 6 arguments (exceeds 4 allowed). Consider refactoring.

@reviewpad
Copy link
Contributor

reviewpad bot commented Oct 25, 2023

AI-Generated Summary: This pull request includes 3 patches.

  1. The first patch modifies the CreateNft.vue and useIsChain.ts components to show the royalty field for all chains except rmrk. The changes include adding a new computed value isRmrk in useIsChain, and using it to control the display of the royalty field in CreateNft.vue.

  2. The second patch affects the transactionMintStatemine.ts file to set royalty attributes during token minting. The minting function now checks whether the token has royalty enabled and properly set. If it does, two additional transaction methods for setting the royalty amount and recipient are included in the transaction execution.

  3. The third and final patch mainly affects the transactionBuy.ts component and introduces a new function payRoyaltyAssetHub. This function calculates and initiates a transaction to pay royalties when a token is bought. There are also updates to the execBuyRmrk, execBuyBasilisk and execBuyStatemine functions to include the royalty payment process in the buying operation.

On the whole, these changes thus deal with the implementation of royalty payments for NFTs across different chains.

@reviewpad reviewpad bot added medium Pull request is medium waiting-for-review labels Oct 25, 2023
Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise code lgtm

utils/support.ts Outdated Show resolved Hide resolved
Copy link
Member

@vikiival vikiival left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~

composables/useIsChain.ts Outdated Show resolved Hide resolved
Comment on lines +29 to +35
: api.tx.nfts.payTips([
{
collection: collectionId,
item: tokenId,
receiver: normalizedRoyalty.address,
amount: royaltyFee(price, normalizedRoyalty.amount),
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to utils?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't see the value in it
you will have a function with 4 params plus API

why not just use api.tx.nfts.payTips directly?

@daiagi
Copy link
Contributor Author

daiagi commented Oct 25, 2023

@prury pleae note that royalty can only be whole numbers (integers) until new squid is released
@vikiival

@vikiival
Copy link
Member

@prury pleae note that royalty can only be whole numbers (integers) until new squid is released
@vikiival

It is posible to do that automatized way

kodadot/stick@release-squid...main

@daiagi
Copy link
Contributor Author

daiagi commented Oct 25, 2023

It is posible to do that automatized way

thought so but didn't want to step in and do it without your consent

@vikiival
Copy link
Member

It is posible to do that automatized way

thought so but didn't want to step in and do it without your consent

Still can tag me ^-^

@prury
Copy link
Member

prury commented Oct 25, 2023

Ahk - custom royalties only
https://deploy-preview-7816--koda-canary.netlify.app/ahk/gallery/172-4
Ahp - custom royalty specifying address
https://deploy-preview-7816--koda-canary.netlify.app/ahp/gallery/14-3

Set @daiagi to receive 5% royalties, bought NFT from myself on AssetHub and value got sent to him
https://deploy-preview-7816--koda-canary.netlify.app/ahp/gallery/14-3

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

@vikiival what ? He is adding royalty to each nft unless of adding into the collection

@vikiival
Copy link
Member

@vikiival what ? He is adding royalty to each nft unless of adding into the collection

Sir I do not understand what you mean.
can you please rephrase?

@codeclimate
Copy link

codeclimate bot commented Oct 26, 2023

Code Climate has analyzed commit dc512ec and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 3

View more on Code Climate.

@sonarcloud
Copy link

sonarcloud bot commented Oct 26, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Comment on lines +71 to +74
// {
// linkName: 'Instagram',
// linkAddress: 'https://www.instagram.com/kodadot.xyz/',
// },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's fix it in another PR or maybe @prury has an idea?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's fix it in another PR or maybe @prury has an idea?

can we leave it like that and I'll apply a proper fix in another PR?

@yangwao
Copy link
Member

yangwao commented Oct 26, 2023

Thanks!
pay 50 usd

@yangwao yangwao merged commit ba7d603 into main Oct 26, 2023
13 of 14 checks passed
@yangwao yangwao deleted the asset-hub-royalties branch October 26, 2023 12:57
@yangwao
Copy link
Member

yangwao commented Oct 26, 2023

😍 Perfect, I’ve sent the payout
💵 $50 @ 4.34 USD/DOT ~ 11.521 $DOT
🧗 1cAsKZYNRb8dkSCpn4eVkEn6ycNZTGoDo5dGDgB8J1UUWK8
🔗 0x05c0694ad469c351f39e4db307e6bb0f1819d40bb35625e221b4fd6a41e24429

🪅 Let’s grab another issue and get rewarded!
🪄 github.com/kodadot/nft-gallery/issues

@yangwao yangwao added the paid pull-request has been paid label Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium Pull request is medium paid pull-request has been paid 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.

Royalties on Asset Hub
7 participants