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

✨ migrate Identity store to pinia #5400

Merged
merged 36 commits into from
May 12, 2023
Merged

Conversation

roiLeo
Copy link
Contributor

@roiLeo roiLeo commented Mar 28, 2023

This one need to be tested (doesn't work yet)
edit: Tested new minting on /snek/gallery/3124270296-1

TODO

  • onChange account fetchBalance is triggered but new value not diplayed
  • onLoad app doesn't show WalletName
  • [Vue warn]: Write operation failed: computed value is readonly
  • issue with subapi (connect to bsx on rmrk chain???)
  • > After connect, if realod, it doesn't stay connected (the wallet) Balance is not loaded properly of compare with beta
  • rmrk2 not loading on first open wallet sidebar

PR Type

  • Refactoring

Context

Screenshot 📸

updated:

Screenshot 2023-04-12 at 14-24-12 KodaDot - NFT Market Explorer

Copilot philosophy

🤖 Generated by Copilot at 23b3c6a

Summary

Walkthrough

stores/identity.ts Outdated Show resolved Hide resolved
@netlify
Copy link

netlify bot commented Mar 28, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit 9460a83
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/645e6249aa122f00088b5071
😎 Deploy Preview https://deploy-preview-5400--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 settings.

stores/identity.ts Outdated Show resolved Hide resolved
@reviewpad reviewpad bot mentioned this pull request Apr 5, 2023
@roiLeo
Copy link
Contributor Author

roiLeo commented Apr 5, 2023

Screenshot 2023-04-05 at 17-23-35 ✨ migrate Identity store to pinia by roiLeo · Pull Request #5400 · kodadot_nft-gallery

wassup

@reviewpad reviewpad bot added the large Pull request is large label Apr 5, 2023
@reviewpad
Copy link
Contributor

reviewpad bot commented Apr 5, 2023

Reviewpad Report

⚠️ Warnings

  • Please link an issue to the pull request

@reviewpad
Copy link
Contributor

reviewpad bot commented Apr 7, 2023

AI-Generated Summary: This pull request involves updates to several components and store files, with the primary focus on refactoring the handling of user identities and account balances. The code now utilizes a new identity store instead of the previous Vuex store structure. Major changes include importing useIdentityStore from '@/stores/identity', replacing instances of $store with identityStore, and updating getter methods and computed properties to use the new identityStore.

In addition, the nuxt.config.js file has been updated with the removal of the userBalance plugin, and the entire identity.ts Vuex store file has been deleted. A new file stores/identity.ts has been added, implementing an improved store for managing user identities and balances.

Overall, these changes aim to enhance code maintainability and readability, and leverage the new identity store for more effective account management.

@yangwao
Copy link
Member

yangwao commented Apr 9, 2023

wassup

@reviewpad
Copy link
Contributor

reviewpad bot commented Apr 12, 2023

AI-Generated Summary: This pull request includes changes to various components, stores, and utilities of the application to improve authentication state management and modularization. The changes primarily involve replacing previous usage of Vuex stores with a new identity store, useIdentityStore, which is imported from '@/stores/identity'. This identity store is now used to handle authentication, balances, and identities more effectively. Additionally, the userBalance plugin and the entire store/identity.ts file have been deleted, and several components, such as Navbar.vue and AccountBalance.vue have been updated with the new identity store implementation. The changes made to the emitAccountChange method of some components involve emitting an account object instead of just the address string. Overall, the changes lead to a more streamlined and modular codebase, easier maintenance, and better separation of concerns within the application.

@roiLeo roiLeo marked this pull request as ready for review April 12, 2023 12:27
@roiLeo roiLeo requested a review from a team as a code owner April 12, 2023 12:27
@roiLeo roiLeo requested review from vikiival and daiagi and removed request for a team April 12, 2023 12:27
@reviewpad
Copy link
Contributor

reviewpad bot commented Apr 12, 2023

AI-Generated Summary: This pull request includes changes to various components, initially focusing on the implementation of a new useIdentityStore from '@/stores/identity' to manage identity and authentication states. The changes also refactor components to use this new store instead of directly accessing the Vuex store or other variables. The main updates involve the replacement of $store getters and actions with corresponding methods and properties from useIdentityStore, making the components more maintainable and easier to test. Additionally, some components have had their methods updated, such as the conversion of the balanceOf method from a getter to a regular method. Some functions and plugins related to user balance and address formatting have been commented out or removed, possibly for testing or refactoring purposes. Overall, the pull request aims to improve code structure, organization, and consistency in handling identity and authentication states across various components.

@vikiival
Copy link
Member

Screenshot 2023-04-05 at 17-23-35 ✨ migrate Identity store to pinia by roiLeo · Pull Request #5400 · kodadot_nft-gallery

wassup

Clicked this magic button

Screenshot 2023-04-12 at 19 43 28

stores/identity.ts Outdated Show resolved Hide resolved
store/index.ts Outdated Show resolved Hide resolved
pages/transfer.vue Outdated Show resolved Hide resolved
composables/useAuth.ts Show resolved Hide resolved
@vikiival
Copy link
Member

vikiival commented May 3, 2023

@kodadot/qa-guild pls

Need to test if we have correct token amounts on chain and correct identities

@vikiival
Copy link
Member

vikiival commented May 3, 2023

cc @helloitsdamsky

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.

wfm

stores/identity.ts Show resolved Hide resolved
@daiagi daiagi added the S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked label May 4, 2023
@yangwao
Copy link
Member

yangwao commented May 8, 2023

identity should work, right?
bc I don't see it working

also balance bug found prob not related to this pr

Screen.Recording.2023-05-08.at.21.50.43.mov

@yangwao
Copy link
Member

yangwao commented May 8, 2023

on Mar 28

I would merge bc it's open quite long here

@roiLeo
Copy link
Contributor Author

roiLeo commented May 9, 2023

I would merge bc it's open quite long here

Feel free but I'll let you know this could break some part of app.

IDK why but on RMRK2 sometimes it's fetching balance from BSX rpc endpoint.

edit: now doesn't work on rmrk2... 😮‍💨

@daiagi
Copy link
Contributor

daiagi commented May 9, 2023

I would merge bc it's open quite long here

Feel free but I'll let you know this could break some part of app.

IDK why but on RMRK2 sometimes it's fetching balance from BSX rpc endpoint.

edit: now doesn't work on rmrk2... face_exhaling

seems related with

@roiLeo
Copy link
Contributor Author

roiLeo commented May 9, 2023

seems related with

* [Balance bug on rmrk2 #5838](https://github.com/kodadot/nft-gallery/issues/5838)

Maybe something wrong with sub-api package as I don't have the good apiInstance when ksm (rmrk2) prefix

@yangwao
Copy link
Member

yangwao commented May 10, 2023

Feel free but I'll let you know this could break some part of app.

okay can wait probably, yet we want to deploy rmrk2 to production

@yangwao
Copy link
Member

yangwao commented May 10, 2023

hmm, did not help apparently

@yangwao
Copy link
Member

yangwao commented May 10, 2023

@prury
Copy link
Member

prury commented May 11, 2023

testing on this one

* https://deploy-preview-5400--koda-canary.netlify.app/ksm/collection/22708b368d163c8007-CITY/activity
  • Loading times are longer than beta.kodadot.xyz

  • if i select the BSX network, check my balances and then change to RMRK without closing the modal it still shows my BSX balances:
    showing BSX Assets on Kusama

Balances from Kusama won't load, only if change to another network and try again, or refresh the page, also, taking lots of times to fetch balances:
kusama wont load

@prury prury added the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label May 11, 2023
@yangwao
Copy link
Member

yangwao commented May 11, 2023

  • if i select the BSX network, check my balances and then change to RMRK without closing the modal it still shows my BSX balances:

this was might fixed in

May be related too PR above

@codeclimate
Copy link

codeclimate bot commented May 12, 2023

Code Climate has analyzed commit 9460a83 and detected 0 issues on this pull request.

View more on Code Climate.

@yangwao
Copy link
Member

yangwao commented May 12, 2023

  • if i select the BSX network, check my balances and then change to RMRK without closing the modal it still shows my BSX balances:

I would propose to close sidebar on network change till now, I've made follow up in #5942 cc @prury

Balances from Kusama won't load, only if change to another network and try again, or refresh the page, also, taking lots of times to fetch balances:

image

worked for me

@yangwao
Copy link
Member

yangwao commented May 12, 2023

prepare for bugs

@yangwao yangwao merged commit 665fd17 into kodadot:main May 12, 2023
12 of 15 checks passed
@roiLeo roiLeo mentioned this pull request May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
large Pull request is large S-changes-requested-🤞 PR is almost good to go, just some fine tunning S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved 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.

None yet

7 participants