-
Notifications
You must be signed in to change notification settings - Fork 133
Added updated balance visualization in Connect modal #2680
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
Conversation
🦋 Changeset detectedLatest commit: 9f7c956 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
| { | ||
| symbol: "FLOW", | ||
| name: "Flow Token", | ||
| vaultIdentifier: `A.${chainId === "testnet" ? "7e60df042a9c0868" : "1654653399040a61"}.FlowToken.Vault`, |
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 we need emulator?
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.
Added!
chasefleming
left a comment
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.
Can we just add commas to the numbers like 1748.261 FLOW otherwise otherwise lgtm?
| { | ||
| symbol: "USDC", | ||
| name: "USD Coin", | ||
| vaultIdentifier: `A.${chainId === "testnet" ? "b7ace0a920d2c37d" : "1e4aa0b87d10b141"}.USDCFlow.Vault`, |
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 we need emulator here support here?
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've hidden them because they don't have default addresses on the emulator and are just there to show some values mainly for testnet/mainnet
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.
Pull Request Overview
This PR enhances the Connect component to support multi-token balance visualization. Users can now view balances for multiple configured tokens (like USDC, USDT) through a dropdown selector in the Connect modal, replacing the previous single-token FLOW display.
Key Changes:
- Renamed
totalbalance type tocombinedin the cross-VM token balance hook to better reflect its purpose - Added
balanceTokensprop to Connect component enabling multi-token support with dropdown selection - Enhanced Connect modal UI with Flowscan integration, improved balance formatting, and Cadence/EVM breakdown display
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react-sdk/src/hooks/useCrossVmTokenBalance.ts | Renamed balance property from total to combined |
| packages/react-sdk/src/hooks/useCrossVmTokenBalance.test.ts | Updated test to reflect renamed balance property |
| packages/react-sdk/src/components/Connect.tsx | Added multi-token support with dropdown selector, Flowscan link, and enhanced balance display |
| packages/demo/src/components/component-cards/connect-card.tsx | Added interactive demo controls for multi-token mode and balance type selection |
| .changeset/young-terms-agree.md | Added changeset documenting the enhancement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| </div> | ||
| </div> | ||
|
|
||
| {showBreakdown && balanceType !== "combined" && ( |
Copilot
AI
Oct 31, 2025
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.
The breakdown should be displayed when balanceType === 'combined' rather than hidden. When users select 'combined' balance type, they would expect to see how that total is composed of Cadence and EVM balances.
| {showBreakdown && balanceType !== "combined" && ( | |
| {showBreakdown && balanceType === "combined" && ( |
| { | ||
| symbol: "USDT", | ||
| name: "Tether USD", | ||
| vaultIdentifier: "A.1e4aa0b87d10b141.USDTFlow.Vault", | ||
| erc20Address: "0x674843C06FF83502ddb4D37c2E09C01cdA38cbc8", | ||
| }, | ||
| ] |
Copilot
AI
Oct 31, 2025
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.
The USDT token configuration is missing the testnet conditional logic that USDC has. This token will fail on testnet since it uses a mainnet-only address and ERC20 contract. Either add testnet support or conditionally exclude it from the list when on testnet.
| { | |
| symbol: "USDT", | |
| name: "Tether USD", | |
| vaultIdentifier: "A.1e4aa0b87d10b141.USDTFlow.Vault", | |
| erc20Address: "0x674843C06FF83502ddb4D37c2E09C01cdA38cbc8", | |
| }, | |
| ] | |
| // Only include USDT on mainnet | |
| ...(chainId === "mainnet" | |
| ? [ | |
| { | |
| symbol: "USDT", | |
| name: "Tether USD", | |
| vaultIdentifier: "A.1e4aa0b87d10b141.USDTFlow.Vault", | |
| erc20Address: "0x674843C06FF83502ddb4D37c2E09C01cdA38cbc8", | |
| }, | |
| ] | |
| : []), |
| if (!chainId) return [{symbol: "FLOW", name: "Flow Token"}] | ||
|
|
||
| const address = | ||
| chainId === "testnet" ? "7e60df042a9c0868" : "1654653399040a61" |
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.
same q re. networks
| { | ||
| symbol: "USDT", | ||
| name: "Tether USD", | ||
| vaultIdentifier: "A.1e4aa0b87d10b141.USDTFlow.Vault", |
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.
This identifier doesn't exist
| : "0xF1815bd50389c46847f0Bda824eC8da914045D14", | ||
| }, | ||
| { | ||
| symbol: "USDT", |
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.
Is USDT canonical? I have not seen it used before
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've replaced it with USDF (PYUSD) 👍
| symbol: "USDT", | ||
| name: "Tether USD", | ||
| vaultIdentifier: "A.1e4aa0b87d10b141.USDTFlow.Vault", | ||
| erc20Address: "0x674843C06FF83502ddb4D37c2E09C01cdA38cbc8", |
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.
what happens on other networks?
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.
They need to put a tokenConfig that is appropriated for the selected network, so in case it's testnet the tokenconfig should have testnet addresses and same for mainnet. consider that usually this data once is set never changes because except for rare cases like the playground or dashboards/explorers you will never change network in a dapp, but even in that case it would still work, i've tried in the playground
| export interface TokenConfig { | ||
| symbol: string | ||
| name: string | ||
| vaultIdentifier?: string | ||
| erc20Address?: string | ||
| } |
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 don't think symbol is needed here, it should be implicit (optional override is OK)
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.
What do you mean by implicit? To me having both is useful because the symbol it is a direct property of a token and the actual ticker, while the name is more descriptive friendly label. An example is:
stFlow (Increment Staked FLOW)
symbol: stFlow
name: stFlow (Increment Staked FLOW)
or
USDF (PYUSD)
symbol: USDF
name: USDF (PYUSD)
| // Get Flowscan URL based on network | ||
| const getFlowscanUrl = () => { | ||
| if (!user?.addr || !chainId) return null | ||
| if (chainId === "emulator" || chainId === "local") return null | ||
|
|
||
| const baseUrl = | ||
| chainId === "testnet" | ||
| ? "https://testnet.flowscan.io" | ||
| : "https://flowscan.io" | ||
|
|
||
| return `${baseUrl}/account/${user.addr}` | ||
| } |
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 feel like this already exists somewhere (we have the transaction link component)
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've added a utility file with flowscan utils so that we can reuse it everywhere, and refactored where it was used
| vaultIdentifier: "A.1e4aa0b87d10b141.USDCFlow.Vault", | ||
| erc20Address: "0xF1815bd50389c46847f0Bda824eC8da914045D14", |
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.
Only one one or the other should be provided, and this should be enforced otherwise it's going to lead to issues (e.g. in the example currently there is a bad token address, but the UI itself seems ostensibly fine)
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.
Yepp, I added a strict typecheck changing the tokenconfig interface so that you are required to provide one or the other and i updated the example 🤝
| hasData && balanceData[balanceType] | ||
| ? formatBalance(balanceData[balanceType].formatted) | ||
| : "0" |
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.
Why did this change from the locale string?
Enhanced Connect component with multi-token support and improved UX. This PR adds multi-token balance visualization to the Connect component through a new balanceTokens prop, allowing users to view balances for any configured token via a dropdown selector.