-
-
Notifications
You must be signed in to change notification settings - Fork 347
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
feat: multi chain balances #6019
Conversation
✅ Deploy Preview for koda-canary ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
AI-Generated Summary: This pull request introduces a new feature for displaying multi-chain balances, adds a new In the |
Reviewpad Report
|
hi @yangwao @exezbcz @JustLuuuu, need your feedback about multi-chain balances in this PR. Is it better like this? I change the text from |
if it would be stable I'm okay with it 😉 |
Todo
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't work for me.
On kusama: should show only availableBalance
How are you going to handle /snek
balance?
I'd be happy to help you, since this morning I've been banging my head with #5838 as I can't manage to have identityStore.auth.balance
reactive (won't update on page)
could you give me your address with
for now, snek is still unchanged, still using the old component. I think we need to distinguish
sure, since I'm actually still new interact directly with chain/polkadot API stuff |
alright |
oohh thanks, will update that 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be possible to store (or fetch) balance values in pinia auth.balance
state? So it would be easier to access between components
let me check, didn't check the store yet for balance stuff |
Love it! |
I just modified a little bit on |
I've already tried that but it didn't worked for me, idk why but know it work wtf. |
viki did meanwhile, if something can be reused? |
more of a feature request, but can we highlight / make bold the token used for paying? (on Basilisk) overall great stuff. love it |
check out nft-gallery/components/bsx/Create/utils.ts Lines 29 to 49 in 0ea86c3
|
async function getBalance(chainName, token = 'KSM', tokenId = 0) { | ||
const currentAddress = accountId.value | ||
const prefix = mapToPrefix[chainName] | ||
const chain = CHAINS[prefix] | ||
|
||
const publicKey = decodeAddress(currentAddress) | ||
const prefixAddress = encodeAddress(publicKey, chain.ss58Format) | ||
const wsProvider = new WsProvider(ENDPOINT_MAP[prefix]) | ||
|
||
const api = await ApiPromise.create({ | ||
provider: wsProvider, | ||
}) | ||
|
||
let currentBalance | ||
|
||
if (tokenId) { | ||
const balance = await api.query.tokens.accounts(prefixAddress, tokenId) | ||
currentBalance = format( | ||
(balance as PalletBalancesAccountData).free, | ||
chain.tokenDecimals, | ||
false | ||
) | ||
} else { | ||
const balance = await balanceOf(api, prefixAddress) | ||
currentBalance = format(balance, chain.tokenDecimals, false) | ||
} | ||
|
||
const balance = delimiter(currentBalance) | ||
const usd = calculateUsd(balance, token) | ||
|
||
availableChains[chainName].push({ | ||
token, | ||
balance, | ||
usd, | ||
}) | ||
totalUsd.value.push(usd) | ||
|
||
await wsProvider.disconnect() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to leverage the identity store imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, on the Todo section, I'm planning to store them in the store to improve the skeleton. the question is, should I make a new state or use the existing state balance
? because the existing balance
is a kinda different format with multi-chain assets. need feedback btw cc @vikiival @roiLeo @Jarsen136 @daiagi
in the existing balance
state, is using prefix: balance
format
so, it will store like this
ksm: 1,
rmrk: 1,
bsx: 1,
stmn: 1,
total: 4
in the multi-chain assets, because of ksm (rmrk2)
and rmrk
using the same chain (Kusama). I'm thinking to store them like this
kusama: 1,
basilisk: 1,
statemime: 1,
total: 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do as you like we will apply new format where it's needed.
Keep in mind we'll need something simple to access in order to check if account has sufficient funds to buy/makeOffer on item (it was pretty easy with urlPrefix)
But I would like to know if we will loose snek
balance with this changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I would like to know if we will loose snek balance with this changes?
identity store and snek are so far still untouched in this PR, it should show the balance with existing components. ref: https://github.com/kodadot/nft-gallery/pull/6019/files#diff-88e8e60a0e3064ffe92b5025e2fc8fda186fbaa66718474a9142cfd1af489262R17-R18
but, lmk if you found something strange on snek
wallet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leverage more identity store
seems like we need to ask the @kodadot/design-squad how to deliver this, wdyt? to make it clear |
@preschian, just to be sure, do you mean asset to pay the transactions or buying nfts in general? |
@exezbcz the asset used for paying for transactions on Basilisk (ksm or bsx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@daiagi Okay, I am not 100% sure whether we should highlight it somehow. If it isn't explicitly mentioned, the user could be confused about what that means. I can think of some possible solutions, but it's against the concept of a smart and simple asset view. It would require additional tooltips etc.; that is not what we want. I could mention this in the signing modal. I can create a follow-up issue with p5, and I may come up with another, more innovative solution. |
Code Climate has analyzed commit 32f443e and detected 0 issues on this pull request. View more on Code Climate. |
I would try but https://deploy-preview-6019--koda-canary.netlify.app/ |
pay 80 usd I like fresh take and all others contributing to discussion! probably continue in |
😍 Perfect, I’ve sent the payout 🪅 Let’s grab another issue and get rewarded! |
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
Context
Had issue bounty label?
Todo
Copilot Summary
🤖 Generated by Copilot at 18988f4
This pull request adds a new feature to show the balances of different tokens and chains for the connected wallet. It introduces a new component
MultipleBalances.vue
that queries and displays the balances using the Polkadot API and the fiat store. It also updates the existing componentWalletAsset.vue
to use the new component and to hide the total value when not on thesnek
prefix.🤖 Generated by Copilot at 18988f4