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.

Sign up for GitHub

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

Jump to bottom

🔥 withdraw on burn item #4811

Merged
merged 8 commits into from
Jan 29, 2023
Merged

🔥 withdraw on burn item #4811

merged 8 commits into from
Jan 29, 2023

Conversation

roiLeo
Copy link
Contributor

@roiLeo roiLeo commented Jan 23, 2023

Smelly code :/ need some refactoring in transactionConsume watcher

PR Type

  • Feature

Context

Before submitting pull request, please make sure:

  • My contribution builds clean without any errors or warnings
  • I've merged recent default branch -- main and I've no conflicts
  • I've tried to respect high code quality standards
  • I've didn't break any original functionality
  • I've posted a screenshot of demonstrated change in this PR

Optional

  • I've tested it at /snek/gallery/659233203-2

Had issue bounty label?

  • Fill up your KSM address: Payout

Screenshot 📸

Enregistrement.de.l.ecran.2023-01-23.a.2.55.39.PM.mov

@roiLeo roiLeo requested a review from a team as a code owner January 23, 2023 14:02
@roiLeo roiLeo requested review from vikiival and Jarsen136 and removed request for a team January 23, 2023 14:02
@netlify
Copy link

netlify bot commented Jan 23, 2023 •
edited

✅ Deploy Preview for koda-nuxt ready!

Name Link
🔨 Latest commit f8a9592
🔍 Latest deploy log https://app.netlify.com/sites/koda-nuxt/deploys/63d3e1bbc2483200083a6669
😎 Deploy Preview https://deploy-preview-4811--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.

@roiLeo roiLeo mentioned this pull request Jan 23, 2023
Closed
13 tasks
codeclimate[bot]
codeclimate bot reviewed Jan 23, 2023
View reviewed changes
composables/transaction/transactionConsume.ts Outdated
import { bsxParamResolver, getApiCall } from '@/utils/gallery/abstractCalls'
import type { ActionConsume } from './types'

export function exectConsumeTx(item: ActionConsume, api, executeTransaction) {
Copy link

Choose a reason for hiding this comment

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

Function exectConsumeTx has 46 lines of code (exceeds 25 allowed). Consider refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aight!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sloth

@vikiival
Copy link
Member

vikiival commented Jan 23, 2023 •
edited

Am I blind or where is the burn button pleas? 🥺

@roiLeo
Copy link
Contributor Author

roiLeo commented Jan 23, 2023

Am I blind or where is the burn button?

#4763 (comment)

"More actions" > "Burn"

@roiLeo roiLeo added the A-basilisk issues related to basilisk parachain label Jan 23, 2023
@yangwao yangwao mentioned this pull request Jan 26, 2023
Closed
vikiival
vikiival reviewed Jan 26, 2023
View reviewed changes
composables/transaction/transactionBurn.ts Outdated
Comment on lines 43 to 52
const arg = [
[...offerWithdrawArgs, api.tx.nft.burn(collectionId, tokenId)],
]

executeTransaction({
cb: cb,
arg: arg,
successMessage: item.successMessage,
errorMessage: item.errorMessage,
})
Copy link
Member

Choose a reason for hiding this comment

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

Missing arg: bsxParamResolver(item.nftId, Interaction.CONSUME, ''),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missing arg: bsxParamResolver(item.nftId, Interaction.CONSUME, ''),

same thing as api.tx.nft.burn(collectionId, tokenId)

Copy link
Member

Choose a reason for hiding this comment

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

Not the same sir

vikiival
vikiival requested changes Jan 26, 2023
View reviewed changes
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.

please clarify

vikiival
vikiival requested changes Jan 27, 2023
View reviewed changes
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.

TO make it work with current impl

composables/transaction/transactionBurn.ts Outdated
Comment on lines 42 to 44
const cb = hasOffers.value
? api.tx.utility.batchAll
: getApiCall(api, item.urlPrefix, Interaction.CONSUME)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const cb = hasOffers.value
? api.tx.utility.batchAll
: getApiCall(api, item.urlPrefix, Interaction.CONSUME)
const cb = api.tx.utility.batchAll

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops my bad I've missed it

@codeclimate
Copy link

codeclimate bot commented Jan 27, 2023

Code Climate has analyzed commit f8a9592 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

@yangwao yangwao added the S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved label Jan 29, 2023
@yangwao
Copy link
Member

yangwao commented Jan 29, 2023

pay 50 usd

@yangwao yangwao merged commit 063db68 into kodadot:main Jan 29, 2023
@yangwao
Copy link
Member

yangwao commented Jan 29, 2023

😍 Perfect, I’ve sent the payout
💵 $50 @ 36.74 USD/KSM ~ 1.361 $KSM
🧗 HguGafrfk8nRP9a4tgJf7hLvSSFie8GtkDjyWFHQ2kDUAMc
🔗 0xd035fe74d9622fd5a55f7396b85a7ac490516f4c75bd6944f64197e4a69f1df9

🪅 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 Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeclimate codeclimate[bot] codeclimate left review comments

@vikiival vikiival vikiival approved these changes

@Jarsen136 Jarsen136 Awaiting requested review from Jarsen136 Jarsen136 is a code owner automatically assigned from kodadot/code-review-guild

Assignees
No one assigned
Labels
A-basilisk issues related to basilisk parachain paid pull-request has been paid S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Burning NFT should cancel Offers
3 participants
@roiLeo @vikiival @yangwao