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

ref(transfer token switch): Refactor for token switching at transfer #6895

Merged
merged 44 commits into from
Aug 28, 2023

Conversation

hassnian
Copy link
Contributor

@hassnian hassnian commented Aug 23, 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

Context

These two new files in general.

utils/object.ts is very verbose, and requires a lot of data manipulation.

composables/useToken.ts is currently bound to 3 tokens (chains) Therefore my best bet is that at this point both AssetHubs do not work

And with current impl I am unable to implement USDT

  • KSM token on Basilisk is not implementable too
  • refactored both files
  • works on all chains with KSM token on Basilisk
  • it is really confusing that I am on the KSM prefix and I want to send DOT

in this point we can be able to either use route replace or

Remove transfer from prefix (therefore user has to specify chain)

  • kept url prefix

Did your issue had any of the "$" label on it?

  • Fill up your DOT address: Payout

Community participation

Screenshot 📸

  • My fix has changed something on UI;

Copilot Summary

🤖 Generated by Copilot at b2fdb6b

This pull request adds support for multiple balances and tokens across different chains for the NFT gallery. It refactors and enhances several components and composables to use chain and token data and utilities from external modules, and to handle the token selection and validation logic. It also removes some unused code and improves code reusability and consistency. The main files affected are MultipleBalances.vue, Money.vue, Transfer.vue, useToken.ts, useBalance.ts, chain.ts, and identity.ts.

🤖 Generated by Copilot at b2fdb6b

useBalance switch
Chains and tokens everywhere
Autumn of refactor

@hassnian hassnian requested a review from a team as a code owner August 23, 2023 03:39
@hassnian hassnian requested review from preschian and Jarsen136 and removed request for a team August 23, 2023 03:39
@kodabot
Copy link
Collaborator

kodabot commented Aug 23, 2023

SUCCESS @hassnian PR for issue #6857 which is assigned to you. Please wait for review and don't hesitate to grab another issue in the meantime!

@netlify
Copy link

netlify bot commented Aug 23, 2023

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit cdc383a
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/64ec5b9316c9c800082a62a0
😎 Deploy Preview https://deploy-preview-6895--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.

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

reviewpad bot commented Aug 23, 2023

AI-Generated Summary: This pull request consists of three patches.

The first patch refactors the handling of tokens and crosschain tokens in multiple components and utilities, mainly focusing on improving the calculation and formatting of balances. Key changes include moving the networkToPrefix object from MultipleBalances.vue to chain.ts, adding a new computed decimals property in Money.vue and Transfer.vue to dynamically determine decimal places based on chain details, and simplifying the calculation of token chains and validity checks in useToken.ts.

The second patch fixes balance issues within the Transfer.vue component and useBalance.ts composable. Notably, the computed property balance within Transfer.vue is replaced with a newly introduced getBalance method in useBalance.ts. This method calculates the relevant balance based on token and chain.

The third patch refines previous changes and removes unneeded variables. Transfer.vue is further simplified by using the renamed isTokenValidForChain method from useToken.ts and removing the unneeded identityStore.

@hassnian hassnian changed the title ref(token switch): Refactor for token switching at transfer ref(transfer token switch): Refactor for token switching at transfer Aug 23, 2023
utils/chain.ts Outdated Show resolved Hide resolved
composables/useBalance.ts Outdated Show resolved Hide resolved
composables/useToken.ts Outdated Show resolved Hide resolved
@hassnian hassnian requested a review from Jarsen136 August 23, 2023 06:10
Copy link
Contributor

@Jarsen136 Jarsen136 left a comment

Choose a reason for hiding this comment

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

small stuff

composables/useBalance.ts Outdated Show resolved Hide resolved
@Jarsen136 Jarsen136 requested a review from vikiival August 23, 2023 10:56
composables/useToken.ts Outdated Show resolved Hide resolved
@prury
Copy link
Member

prury commented Aug 23, 2023

sometimes the page gets refreshed, sometimes it doesn't, most times the selected asset gets to be the first in order, but not all the time(check KSM behavior)

position.mp4

@prury prury added the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label Aug 23, 2023
@hassnian
Copy link
Contributor Author

hassnian commented Aug 23, 2023

sometimes the page gets refreshed, sometimes it doesn't, most times the selected asset gets to be the first in order, but not all the time(check KSM behavior)

position.mp4

@prury
that would be the expected behavior in this case since when it refreshes it changing networks see pr context section @vikiival 's comment

to remove the refresh as mentioned we have to get rid of the prefix inside the url , the whole state will rerender on route change , tabs included , that's why sometimes is first (route change), is not first just changing token on the same network

where is not changing networks is when a token is available also inside that chain like KSM on bsx

  • have you tested ksm on bsx? does that work properly

thanks

waiting on feedback @Jarsen136 @vikiival

@reviewpad reviewpad bot added medium Pull request is medium and removed large Pull request is large labels Aug 27, 2023
@exezbcz
Copy link
Member

exezbcz commented Aug 27, 2023

@prury could you have a look, please? 👀

@hassnian hassnian marked this pull request as ready for review August 27, 2023 14:55
@hassnian
Copy link
Contributor Author

ok here we go

@Jarsen136 removed all traces of code that is not used (basically all related to mutlichain tokens), pls review last one 🤞

@prury pls verify that all works

@preschian need a review :D

@prury
Copy link
Member

prury commented Aug 27, 2023

what a journey eh?

only thing i've found so far(BSX balances):
seems to be getting my KSM on bsx balance

image

happens after i chose BSX then go to Kusama(could be a one time only bug), but its getting my bsx balance

image

once its fixed I'll make some transfers

@codeclimate
Copy link

codeclimate bot commented Aug 28, 2023

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

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

@sonarqubecloud
Copy link

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

@hassnian
Copy link
Contributor Author

@prury
yep a loooong journey :d

try now

@prury prury added S-works-for-me-✅ qa-guild has tested PR from end user perspective and functionality worked and removed S-changes-requested-🤞 PR is almost good to go, just some fine tunning labels Aug 28, 2023
@Jarsen136 Jarsen136 added the S-code-lgtm-✅ code review guild has reviewed this PR and it's code is approved label Aug 28, 2023
@yangwao
Copy link
Member

yangwao commented Aug 28, 2023

pay 50 usd

@yangwao yangwao merged commit e4df2a8 into kodadot:main Aug 28, 2023
@yangwao
Copy link
Member

yangwao commented Aug 28, 2023

😍 Perfect, I’ve sent the payout
💵 $50 @ 4.56 USD/DOT ~ 10.965 $DOT
🧗 13QUj3pZyFNfYj4AM336hRdyLQbevj5H3sR4PKmLEXLdwZhh
🔗 0xe59543bc8b680512daa96da5225d5acd4dfecc87d9e8605188a2e270048e537c

🪅 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 Aug 28, 2023
@hassnian
Copy link
Contributor Author

@yangwao payout failed

@yangwao yangwao removed the paid pull-request has been paid label Aug 30, 2023
@yangwao
Copy link
Member

yangwao commented Aug 30, 2023

pay 50 usd

@yangwao
Copy link
Member

yangwao commented Aug 30, 2023

😍 Perfect, I’ve sent the payout
💵 $50 @ 4.59 USD/DOT ~ 10.893 $DOT
🧗 13QUj3pZyFNfYj4AM336hRdyLQbevj5H3sR4PKmLEXLdwZhh
🔗 0x5d93ec54c5617bef1c62282316527c17e4d740ce492ff050dd1f81da4b624af1

🪅 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 Aug 30, 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-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.

Refactor for token switching at transfer
7 participants