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

Remove giant squid from the code #10299

Merged
merged 16 commits into from
May 22, 2024
Merged

Remove giant squid from the code #10299

merged 16 commits into from
May 22, 2024

Conversation

Jarsen136
Copy link
Contributor

@Jarsen136 Jarsen136 commented May 14, 2024

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

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

PR Type

  • Bugfix
  • Refactoring

Needs QA check

  • @kodadot/qa-guild please review

Context

  1. replace on-chain identity with profile data
  2. remove giant squid request
  3. remove the identity setting page completely
  4. performance: batch request several profiles data in one request https://github.com/kodadot/private-workers/pull/156
  5. Clean the console log for profile request https://github.com/kodadot/private-workers/pull/159
  6. Introduce vue-query to better manage the request cache #Better data-fetching library for nuxt #10329
  7. cache single profile request/profiles/:address` cache profile calls #10323
  8. cache profiles list request/profiles?ids=string[] cache profile calls #10323

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.

image

image

Copy link

netlify bot commented May 14, 2024

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit f757d47
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/664e01061b096f0008b58b9a
😎 Deploy Preview https://deploy-preview-10299--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.

@Jarsen136 Jarsen136 changed the title WIP: Remove giant squid from the code Remove giant squid from the code May 17, 2024
@Jarsen136 Jarsen136 marked this pull request as ready for review May 17, 2024 13:44
@Jarsen136 Jarsen136 requested a review from a team as a code owner May 17, 2024 13:44
@Jarsen136 Jarsen136 requested review from vikiival and daiagi and removed request for a team May 17, 2024 13:44
@Jarsen136
Copy link
Contributor Author

Code Climate has analyzed commit bcce23e and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2
View more on Code Climate.

It's the wrong analysis from codeclimate. Let's ignore it.

@prury
Copy link
Member

prury commented May 17, 2024

tests can be ignored as well, twitter changed itself to x

@prury
Copy link
Member

prury commented May 17, 2024

i'm checking the other errors

@prury
Copy link
Member

prury commented May 17, 2024

getting some different errors now:

image

image

@daiagi
Copy link
Contributor

daiagi commented May 17, 2024

getting some different errors now:

not sure errors is the right word here

these profiles simply don't exist

@Jarsen136
Copy link
Contributor Author

Jarsen136 commented May 17, 2024

getting some different errors now:

not sure errors is the right word here

these profiles simply don't exist

Yup. It's the expected response result with a 404 status code, so it's not a real "error".

However, if we would like to make the console look more clean without showing any request 404 code, I will simply change the 404 code to 200 and return null value. Only applied for API of /profile/:address because it should be called lots of times. In this way, we could "hide" the 404 request error from the console.

https://github.com/kodadot/private-workers/pull/159

@Jarsen136
Copy link
Contributor Author

getting some different errors now:

not sure errors is the right word here
these profiles simply don't exist

Yup. It's the expected response result with a 404 status code, so it's not a real "error".

However, if we would like to make the console look more clean without showing any request 404 code, I will simply change the 404 code to 200 and return null value. Only applied for API of /profile/:address because it should be called lots of times. In this way, we could "hide" the 404 request error from the console.

kodadot/private-workers#159

https://github.com/kodadot/private-workers/pull/159 Deployed and also removed the error log from the console

Copy link
Contributor

@daiagi daiagi left a comment

Choose a reason for hiding this comment

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

can benefit from caching

rest is kinda minor

components/collection/activity/events/Events.vue Outdated Show resolved Hide resolved
components/collection/activity/events/Events.vue Outdated Show resolved Hide resolved
components/identity/module/IdentityPopoverHeader.vue Outdated Show resolved Hide resolved
composables/useIdentity.ts Outdated Show resolved Hide resolved
composables/useIdentity.ts Outdated Show resolved Hide resolved
@Jarsen136
Copy link
Contributor Author

can benefit from caching

rest is kinda minor

Thanks! I have updated them.

@Jarsen136 Jarsen136 requested a review from daiagi May 19, 2024 22:40
components/collection/activity/events/Events.vue Outdated Show resolved Hide resolved
@daiagi daiagi mentioned this pull request May 20, 2024
@daiagi
Copy link
Contributor

daiagi commented May 20, 2024

@Jarsen136
you may want to look at #10323

@Jarsen136
Copy link
Contributor Author

@Jarsen136 you may want to look at #10323

#10324 It could be done first.

@prury prury added the S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked label May 20, 2024
@Jarsen136
Copy link
Contributor Author

@vikiival How about merging it ?

@vikiival
Copy link
Member

Is it somehow cached?

@Jarsen136
Copy link
Contributor Author

Jarsen136 commented May 21, 2024

Is it somehow cached?

image

It's already cached but not ideal. Ideally, It needs a better fetching library with cache. Should I implement it at the current PR also?
#10329

@Jarsen136
Copy link
Contributor Author

✅ Cache Handler

image

image

Copy link

socket-security bot commented May 22, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/eslint-config-prettier@9.1.0 None 0 20.8 kB lydell
npm/eslint-plugin-prettier@5.1.3 Transitive: environment +5 243 kB jounqin
npm/lodash@4.17.21 None 0 1.41 MB bnjmnt4n
npm/prettier@3.2.5 environment, filesystem, unsafe 0 8.39 MB prettier-bot
npm/prismjs@1.29.0 None 0 2.05 MB rundevelopment
npm/three@0.164.1 None 0 21.9 MB mrdoob
npm/typescript@5.4.5 None 0 32.4 MB typescript-bot

🚮 Removed packages: npm/@dargmuesli/nuxt-cookie-control@7.5.1, npm/@farcaster/auth-client@0.1.1, npm/@fortawesome/fontawesome-svg-core@6.5.2, npm/@fortawesome/vue-fontawesome@3.0.6, npm/@google/model-viewer@3.5.0, npm/@histoire/plugin-vue@0.17.6, npm/@kodadot1/hyperdata@0.0.1-rc.4, npm/@kodadot1/minimark@0.1.14-rc.0, npm/@kodadot1/minipfs@0.4.3-rc.1, npm/@kodadot1/sub-api@0.3.1-rc.0, npm/@nuxt/content@2.12.1, npm/@nuxt/image@1.7.0, npm/@nuxt/types@2.17.3, npm/@nuxtjs/apollo@5.0.0-alpha.6, npm/@nuxtjs/color-mode@3.4.1, npm/@nuxtjs/device@3.1.1, npm/@nuxtjs/google-fonts@3.2.0, npm/@nuxtjs/i18n@8.3.1, npm/@nuxtjs/sitemap@5.1.5, npm/@oruga-ui/oruga-next@0.7.0, npm/@paraspell/sdk@5.2.1, npm/@pinia/nuxt@0.5.1, npm/@polkadot/api-base@10.13.1, npm/@polkadot/api@10.13.1, npm/@polkadot/apps-config@0.135.1, npm/@polkadot/extension-dapp@0.47.3, npm/@polkadot/extension-inject@0.47.3, npm/@polkadot/types@10.13.1, npm/@polkadot/ui-keyring@3.6.6, npm/@polkadot/ui-settings@3.6.6, npm/@polkadot/util-crypto@12.6.2, npm/@polkadot/util@12.6.2, npm/@polkadot/vue-identicon@3.6.6, npm/@ramp-network/ramp-instant-sdk@4.0.5, npm/@transak/transak-sdk@1.4.1, npm/@types/jest@27.5.2, npm/@types/lodash@4.17.1, npm/@types/markdown-it@13.0.8, npm/@types/node@20.12.11, npm/@typescript-eslint/eslint-plugin@6.21.0, npm/@typescript-eslint/parser@6.21.0, npm/@vite-pwa/nuxt@0.7.0, npm/@vitejs/plugin-vue@4.6.2, npm/@vitejs/plugin-vue@5.0.4, npm/@vitest/coverage-c8@0.33.0, npm/@vitest/coverage-istanbul@0.34.6, npm/@vueuse/core@9.13.0, npm/@vueuse/nuxt@9.13.0, npm/autoprefixer@10.4.19, npm/bulma@0.9.4, npm/changelogen@0.5.5, npm/chart.js@4.4.2, npm/chartjs-adapter-date-fns@3.0.0, npm/chartjs-plugin-zoom@2.0.1

View full report↗︎

Copy link

socket-security bot commented May 22, 2024

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert Package NoteSource
Install scripts npm/es5-ext@0.10.64
  • Install script: postinstall
  • Source: node -e "try{require('./_postinstall')}catch(e){}" || exit 0
  • orphan: npm/es5-ext@0.10.64
Protestware/Troll package npm/es5-ext@0.10.64
  • Note: This package prints a protestware console message on install regarding Ukraine for users with Russian language locale
  • orphan: npm/es5-ext@0.10.64
Install scripts npm/esbuild@0.20.2
  • orphan: npm/esbuild@0.20.2

View full report↗︎

Next steps

What is an install script?

Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts.

Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead.

What is protestware?

This package is a joke, parody, or includes undocumented or hidden behavior unrelated to its primary function.

Consider that consuming this package my come along with functionality unrelated to its primary purpose.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/foo@1.0.0 or ignore all packages with @SocketSecurity ignore-all

  • @SocketSecurity ignore npm/es5-ext@0.10.64
  • @SocketSecurity ignore npm/esbuild@0.20.2

@prury
Copy link
Member

prury commented May 22, 2024

@Jarsen136 can you check socket security issues?

@Jarsen136
Copy link
Contributor Author

@Jarsen136 can you check socket security issues?

The issues it reported are not from this PR. I have no idea why it's warning here. Let's ignore it for now.

image

package in the code:
image
image

@prury
Copy link
Member

prury commented May 22, 2024

@Jarsen136 can you check socket security issues?

The issues it reported are not from this PR. I have no idea why it's warning here. Let's ignore it for now.

strange indeed, thank you checking!

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.

nice!

Copy link

codeclimate bot commented May 22, 2024

Code Climate has analyzed commit f757d47 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

View more on Code Climate.

Copy link

sonarcloud bot commented May 22, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@Jarsen136 Jarsen136 added this pull request to the merge queue May 22, 2024
Merged via the queue into kodadot:main with commit a81f58e May 22, 2024
17 of 19 checks passed
@Jarsen136 Jarsen136 deleted the issue-10268 branch May 22, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cache profile calls Remove giant squid from the code
4 participants