feat: add Chain.getBalance() for token balance queries #99
Conversation
- Add getBalance() method to Chain class for native and token balances - Implement for EVM (native + ERC-20), Solana (SOL + SPL/Token-2022), Aptos (APT + Fungible Assets) - Add resolveATA() utility for Solana ATA derivation with automatic token program detection - Add CLI 'token <network> <address> [token]' command for balance queries - Sui and TON throw CCIPNotImplementedError (partial support)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
👋 aelmanaa, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
Coverage Report |
PabloMansanet
left a comment
PabloMansanet
left a comment
There was a problem hiding this comment.
Looks good and heading in the right direction. There are some issues that need to be addressed though, mostly around error propagation and validation. Main issues blocking merge IMO:
- Error propagation needs work as RPC errors are swallowed and translated to "balance 0" in ways that could be very opaque to the library user.
- Solana in particular has some validation problems around ATA derivation, accepting invalid mint accounts.
| ) | ||
| return | ||
| case Format.log: | ||
| logger.log(`Balance: ${balance}`) |
There was a problem hiding this comment.
Maybe this one could specify the token too?
| export async function resolveATA( | ||
| connection: Connection, | ||
| mint: PublicKey, | ||
| owner: PublicKey, | ||
| ): Promise<ResolvedATA | null> { | ||
| const mintInfo = await connection.getAccountInfo(mint) | ||
| if (!mintInfo) { | ||
| return null | ||
| } | ||
| const ata = getAssociatedTokenAddressSync(mint, owner, undefined, mintInfo.owner) | ||
| return { | ||
| ata, | ||
| tokenProgram: mintInfo.owner, | ||
| mintInfo, | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this could do with more validation: the promises here are pretty brittle.
For example, this will accept any account as mint as long as it has an owner. There's no validation done here that the owner is any of the two token programs, and it's not done in getAssociatedTokenAddressSync either (it will just perform address derivation with no checks).
This needs to check that mintInfo.owner matches one of the two token programs (SPL token or SPL token 22) to avoid false positives or mixups when switching the order of arguments.
| if (!mintInfo) { | ||
| return null | ||
| } | ||
| const ata = getAssociatedTokenAddressSync(mint, owner, undefined, mintInfo.owner) |
There was a problem hiding this comment.
We're calling with allowOwnerOffCurve undefined, which defaults to false, and that means we can't resolve ATAs for PDAs, only for EOAs. There's significant value in retrieving the balance of PDAs in CCIP in particular (CCIPReceiver contracts often use PDAs as vaults). Unless there's a reason to avoid it, I think we should be passing true (or at least exposing it as an option).
| export type GetBalanceOpts = { | ||
| /** Token address. Use null/undefined for native token balance. */ | ||
| token?: string | null | ||
| /** Address to query balance for. */ | ||
| address: string | ||
| } |
There was a problem hiding this comment.
NIT: Do we need an Opts struct here? With only two parameters that seem globally valid, maybe passing them directly to getBalance is more readable.
Feel free to ignore me here though, might be unfamiliarity with TS practices.
| }) | ||
| return BigInt(balance) | ||
| } catch { | ||
| return 0n |
There was a problem hiding this comment.
Hmm, returning 0 balance with a blanket catch-all seems wrong to me. I think we need to bubble up an error instead.
| const accountInfo = await this.connection.getTokenAccountBalance(resolved.ata) | ||
| return BigInt(accountInfo.value.amount) | ||
| } catch { | ||
| return 0n // Account doesn't exist = 0 balance |
There was a problem hiding this comment.
I'm really not sure about using 0 balance as the universal return when there's any error. This catch could've been hit by many reasons, such as connectivity problems.
Also, ironically, on Solana you can have a non-existent account with a positive balance 😆 a pattern that sometimes happens is that people will fund an address they know is derived from their programID, but not initialize it so it remains owned by the SystemProgram and can be used to pay for other things, so this comment wouldn't be accurate either.
Logging doesn't really help since this is a library, so I think we should have error propagation for the user to decide what to do when retrieving balance fails.
| mintInfo.owner, | ||
| ) | ||
| const resolved = await resolveATA(connection, token, owner) | ||
| if (!resolved) throw new CCIPTokenMintNotFoundError(token.toBase58()) |
There was a problem hiding this comment.
This error conversion seems to assume too much; ATA resolution could fail for other reasons (currently, for example, if owner is a PDA)
3 participants
This pull request introduces a unified token balance query feature across the SDK and CLI for EVM, Solana, and Aptos chains. It adds a new
Chain.getBalance()method to the SDK, a CLI command for querying balances, and supporting utilities for Solana token account resolution. The documentation is updated with usage examples for developers.SDK: Token Balance Query API
getBalance()method to theChainclass, with implementations for EVM, Solana, and Aptos chains, enabling native and token balance queries using a unified API. [1] [2] [3] [4]GetBalanceOptstype for balance query options, exported from the SDK entrypoint. [1] [2]CLI: Token Balance Command
token <network> <address> [token]command to the CLI for querying balances, supporting native and token assets on EVM, Solana, and Aptos.Solana: ATA Resolution Utility
resolveATA()utility to automatically derive the correct Associated Token Account (ATA) and detect SPL Token vs Token-2022 programs, used in both SDK and CLI. [1] [2] [3] [4]SDK: Chain Implementations and Error Handling
getBalance()in test chains, and added stub methods for chains where balance queries are not supported (Sui, TON). [1] [2] [3] [4]Documentation