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

refactor: Deprecation of RMRK #10686

Merged
merged 15 commits into from
Sep 19, 2024
Merged

refactor: Deprecation of RMRK #10686

merged 15 commits into from
Sep 19, 2024

Conversation

Jarsen136
Copy link
Contributor

@Jarsen136 Jarsen136 commented Jul 24, 2024 •
edited
Loading

Thank you for your contribution to the Koda - Generative Art Marketplace.

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

PR Type

  • 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 something on UI; a screenshot is best to understand changes for others.

@Jarsen136 Jarsen136 requested a review from a team as a code owner July 24, 2024 21:08
@Jarsen136 Jarsen136 requested review from preschian and vikiival and removed request for a team July 24, 2024 21:08
@netlify Netlify
Copy link

netlify bot commented Jul 24, 2024 •
edited
Loading

✅ Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit 4141440
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/66ebeea148f0b10008d74450
😎 Deploy Preview https://deploy-preview-10686--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.

codeclimate[bot]
codeclimate bot reviewed Jul 24, 2024
View reviewed changes
utils/chain.ts Show resolved Hide resolved
@Jarsen136 Jarsen136 mentioned this pull request Jul 24, 2024
Closed
6 tasks
@vikiival
Copy link
Member

search endpoint:

Can you please expand this idea?

Polysearch is in Workers btw

@Jarsen136
Copy link
Contributor Author

search endpoint:

Can you please expand this idea?

Polysearch is in Workers btw

Polysearch search endpoint would return nfts in any chain including rmrk\ksm, which should not be shown on the website.
For now, I add the chain params so it would only search nft from the current supported chain.

If we want to keep mutilple chain search, there are two solutions on the worker side.

  1. delete ksm\rmrk items in the database table.
  2. manually exclude items from ksm\rmrk chain when executing the search SQL

@vikiival
Copy link
Member

delete ksm\rmrk items in the database table.

This. Database will be wiped and reindexed again 🩷

@Jarsen136
Copy link
Contributor Author

delete ksm\rmrk items in the database table.

This. Database will be wiped and reindexed again 🩷

Okay, then could you please let me know when you remove it? I will change back the search params on the frontend.

@codeclimate CodeClimate
Copy link

codeclimate bot commented Jul 25, 2024

Code Climate has analyzed commit 410aa21 and detected 4 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 4

View more on Code Climate.

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jul 30, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@vikiival
Copy link
Member

As agreed on metahours, RMRK v1 will be deprecated instantly.
RMRK 2 will be decomissioned after discussion

@vikiival vikiival closed this Aug 14, 2024
@vikiival vikiival reopened this Sep 11, 2024
@Jarsen136
Copy link
Contributor Author

As agreed on metahours, RMRK v1 will be deprecated instantly. RMRK 2 will be decomissioned after discussion

So what should I do to continue with this PR? Should RMRK2 be deprecated? @vikiival

@vikiival
Copy link
Member

o what should I do to continue with this PR? Should RMRK2 be deprecated

yes

We can safely deprecate both

vikiival
vikiival reviewed Sep 16, 2024
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.

Otherwise ok

components/create/CreateNft.vue Show resolved Hide resolved
components/search/SearchSuggestion.vue Show resolved Hide resolved
components/stats/StatsHeader.vue Show resolved Hide resolved
@vikiival
Copy link
Member

@kodadot/internal-dev can I please ask you for one more check ?

@vikiival
Copy link
Member

cc @Jarsen136 you have some conflicts here

@Jarsen136
Copy link
Contributor Author

cc @Jarsen136 you have some conflicts here

done

@vikiival
Copy link
Member

Tests are failing sir

@Jarsen136
Copy link
Contributor Author

Tests are failing sir

The failed tests are related to #10702 https://github.com/kodadot/nft-gallery/actions/runs/10880638038/job/30187898925

@vikiival
Copy link
Member

PLZ @hassnian
@preschian

hassnian
hassnian approved these changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@hassnian hassnian left a comment

Choose a reason for hiding this comment

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

otherwise LGTM

components/carousel/utils/useCarouselEvents.ts Outdated Show resolved Hide resolved
components/items/ItemsGrid/useItemsGrid.ts Outdated Show resolved Hide resolved
composables/massmint/useMassMint.ts Outdated Show resolved Hide resolved
@preschian
Copy link
Member

  • keep teleport \ transfer \ migrate

@vikiival @Jarsen136 is it safe to delete migrate page/components? or, we already discuss this somewhere before?

@Jarsen136
Copy link
Contributor Author

  • keep teleport \ transfer \ migrate

@vikiival @Jarsen136 is it safe to delete migrate page/components? or, we already discuss this somewhere before?

Here is the context about migrate #10667 (comment)

@vikiival
Copy link
Member

@JustLuuuu @exezbcz can you please test?

@JustLuuuu
Copy link
Member

wfm

@exezbcz
Copy link
Member

exezbcz commented Sep 18, 2024

works, minted drop without any issues

good job

@vikiival
Copy link
Member

Thanks 🥰

@Jarsen136
Copy link
Contributor Author

@vikiival Could we merge it?

@vikiival vikiival added this pull request to the merge queue Sep 19, 2024
Merged via the queue into kodadot:main with commit 87cada1 Sep 19, 2024
13 of 16 checks passed
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Sep 19, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@hassnian hassnian mentioned this pull request Oct 22, 2024
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeclimate codeclimate[bot] codeclimate[bot] left review comments

@vikiival vikiival vikiival left review comments

@hassnian hassnian hassnian approved these changes

@preschian preschian preschian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecation of RMRK
6 participants
@Jarsen136 @vikiival @preschian @JustLuuuu @exezbcz @hassnian