Support query solana balances #185
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes primarily involve updates to the Changes
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Solana CLI stores keys in |
|
Also, please, add a way to specify a custom Solana address to query. If address is provided, don't derive it from private key. Maybe, |
Given that we currently have tokens on three heterogeneous chains, and may expand to more in the future, I'd like to clarify a few points:
|
No, I don't think it's mandatory. |
I think we can. This is basically "watch mode" in a wallet, where you provide an address, can view balances, but of course can't sign txs. |
Yes, I think it's a good decision! |
npx hardhat balances --solana HEgoKVmb6yohTatYM3D7nXFojo7uYEmitz2qj1255KL2 --evm 0x6093537Aa6C8C8bf4705bda40aC9193977208B39 --bitcoin tb1quy77dhfutn7dndevv7vff5he7g0d2j2hj2jwrv
EVM: 0x6093537Aa6C8C8bf4705bda40aC9193977208B39
Bitcoin: tb1quy77dhfutn7dndevv7vff5he7g0d2j2hj2jwrv
Solana: HEgoKVmb6yohTatYM3D7nXFojo7uYEmitz2qj1255KL2
┌─────────┬───────────────────┬───────────────────────────────────┬─────────┬─────────────┐
│ (index) │ Chain │ Token │ Type │ Amount │
├─────────┼───────────────────┼───────────────────────────────────┼─────────┼─────────────┤
│ 0 │ 'amoy_testnet' │ 'WZETA' │ 'ERC20' │ '0.000000' │
│ 1 │ 'amoy_testnet' │ 'MATIC.AMOY' │ 'Gas' │ '0.650769' │
│ 2 │ 'base_sepolia' │ 'ETH.BASESEPOLIA' │ 'Gas' │ '0.000000' │
│ 3 │ 'bsc_testnet' │ 'USDC' │ 'ERC20' │ '0.001689' │
│ 4 │ 'bsc_testnet' │ 'WZETA' │ 'ERC20' │ '0.000000' │
│ 5 │ 'bsc_testnet' │ 'tBNB' │ 'Gas' │ '0.795821' │
│ 6 │ 'btc_testnet' │ 'tBTC' │ 'Gas' │ '0.000112' │
│ 7 │ 'sepolia_testnet' │ 'USDC.SEPOLIA' │ 'ERC20' │ '0.000000' │
│ 8 │ 'sepolia_testnet' │ 'WZETA' │ 'ERC20' │ '0.000000' │
│ 9 │ 'sepolia_testnet' │ 'sETH.SEPOLIA' │ 'Gas' │ '0.241025' │
│ 10 │ 'solana_devnet' │ 'SOL.SOLANA' │ 'Gas' │ '3.189423' │
│ 11 │ 'zeta_testnet' │ 'sETH.SEPOLIA' │ 'ZRC20' │ '0.031614' │
│ 12 │ 'zeta_testnet' │ 'USDC-goerli_testnet' │ 'ZRC20' │ '0.000000' │
│ 13 │ 'zeta_testnet' │ 'gETH' │ 'ZRC20' │ '0.000000' │
│ 14 │ 'zeta_testnet' │ 'ETH.BASESEPOLIA' │ 'ZRC20' │ '0.000000' │
│ 15 │ 'zeta_testnet' │ 'tMATIC' │ 'ZRC20' │ '0.000000' │
│ 16 │ 'zeta_testnet' │ 'tBTC' │ 'ZRC20' │ '0.000022' │
│ 17 │ 'zeta_testnet' │ 'MATIC.AMOY' │ 'ZRC20' │ '0.108915' │
│ 18 │ 'zeta_testnet' │ 'USDC-bsc_testnet' │ 'ZRC20' │ '0.000000' │
│ 19 │ 'zeta_testnet' │ 'USDC-mumbai_testnet' │ 'ZRC20' │ '0.000000' │
│ 20 │ 'zeta_testnet' │ 'SOL.SOLANA' │ 'ZRC20' │ '0.000000' │
│ 21 │ 'zeta_testnet' │ 'ZetaChain ZRC20 USDC on SEPOLIA' │ 'ZRC20' │ '0.116826' │
│ 22 │ 'zeta_testnet' │ 'tBNB' │ 'ZRC20' │ '0.000000' │
│ 23 │ 'zeta_testnet' │ 'WZETA' │ 'ERC20' │ '0.042220' │
│ 24 │ 'zeta_testnet' │ 'ZETA' │ 'Gas' │ '70.274601' │
└─────────┴───────────────────┴───────────────────────────────────┴─────────┴─────────────┘npx hardhat balances
EVM: 0x6093537Aa6C8C8bf4705bda40aC9193977208B39
Bitcoin: tb1quy77dhfutn7dndevv7vff5he7g0d2j2hj2jwrv
Solana: HEgoKVmb6yohTatYM3D7nXFojo7uYEmitz2qj1255KL2
┌─────────┬───────────────────┬───────────────────────────────────┬─────────┬─────────────┐
│ (index) │ Chain │ Token │ Type │ Amount │
├─────────┼───────────────────┼───────────────────────────────────┼─────────┼─────────────┤
│ 0 │ 'amoy_testnet' │ 'WZETA' │ 'ERC20' │ '0.000000' │
│ 1 │ 'amoy_testnet' │ 'MATIC.AMOY' │ 'Gas' │ '0.650769' │
│ 2 │ 'base_sepolia' │ 'ETH.BASESEPOLIA' │ 'Gas' │ '0.000000' │
│ 3 │ 'bsc_testnet' │ 'USDC' │ 'ERC20' │ '0.001689' │
│ 4 │ 'bsc_testnet' │ 'WZETA' │ 'ERC20' │ '0.000000' │
│ 5 │ 'bsc_testnet' │ 'tBNB' │ 'Gas' │ '0.795821' │
│ 6 │ 'btc_testnet' │ 'tBTC' │ 'Gas' │ '0.000112' │
│ 7 │ 'sepolia_testnet' │ 'USDC.SEPOLIA' │ 'ERC20' │ '0.000000' │
│ 8 │ 'sepolia_testnet' │ 'WZETA' │ 'ERC20' │ '0.000000' │
│ 9 │ 'sepolia_testnet' │ 'sETH.SEPOLIA' │ 'Gas' │ '0.241025' │
│ 10 │ 'solana_devnet' │ 'SOL.SOLANA' │ 'Gas' │ '3.189423' │
│ 11 │ 'zeta_testnet' │ 'sETH.SEPOLIA' │ 'ZRC20' │ '0.031614' │
│ 12 │ 'zeta_testnet' │ 'USDC-goerli_testnet' │ 'ZRC20' │ '0.000000' │
│ 13 │ 'zeta_testnet' │ 'gETH' │ 'ZRC20' │ '0.000000' │
│ 14 │ 'zeta_testnet' │ 'ETH.BASESEPOLIA' │ 'ZRC20' │ '0.000000' │
│ 15 │ 'zeta_testnet' │ 'tMATIC' │ 'ZRC20' │ '0.000000' │
│ 16 │ 'zeta_testnet' │ 'tBTC' │ 'ZRC20' │ '0.000022' │
│ 17 │ 'zeta_testnet' │ 'MATIC.AMOY' │ 'ZRC20' │ '0.108915' │
│ 18 │ 'zeta_testnet' │ 'USDC-bsc_testnet' │ 'ZRC20' │ '0.000000' │
│ 19 │ 'zeta_testnet' │ 'USDC-mumbai_testnet' │ 'ZRC20' │ '0.000000' │
│ 20 │ 'zeta_testnet' │ 'SOL.SOLANA' │ 'ZRC20' │ '0.000000' │
│ 21 │ 'zeta_testnet' │ 'ZetaChain ZRC20 USDC on SEPOLIA' │ 'ZRC20' │ '0.116826' │
│ 22 │ 'zeta_testnet' │ 'tBNB' │ 'ZRC20' │ '0.000000' │
│ 23 │ 'zeta_testnet' │ 'WZETA' │ 'ERC20' │ '0.042220' │
│ 24 │ 'zeta_testnet' │ 'ZETA' │ 'Gas' │ '70.274601' │
└─────────┴───────────────────┴───────────────────────────────────┴─────────┴─────────────┘
|
lukema95
requested review from
andresaiello,
fadeev and
a team
as code owners
|
@fadeev pls review. Looks like we need to update |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (1)
packages/client/src/getBalances.ts (1)
Line range hint
51-60: Update JSDoc to includesolanaAddressparameterThe JSDoc comment for the
getBalancesfunction is missing the description for the newsolanaAddressparameter. Please update the documentation to include this parameter for clarity and completeness.Here is the suggested update:
/** * Get token balances of all tokens on all chains connected to ZetaChain. * * @param this - ZetaChainClient instance. * @param options.evmAddress EVM address + * @param options.solanaAddress Solana address * @param options.btcAddress Bitcoin address * @returns */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
🔇 Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
- package.json (2 hunks)
- packages/client/src/getBalances.ts (3 hunks)
- packages/tasks/src/balances.ts (5 hunks)
🧰 Additional context used
Biome
packages/tasks/src/balances.ts
[error] 81-86: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.(lint/complexity/noUselessLoneBlockStatements)
[error] 92-97: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments not posted (3)
package.json (3)
97-97: Verify the intention behind using a release candidate version.The dependency
@zetachain/networkshas been updated from^10.0.0to10.0.0-rc1. While this allows testing with a specific release candidate, it's important to consider the potential instability in production code.Please confirm if this change is intentional and if there are plans to update to a stable version before the next production release.
103-103: LGTM! Consider updating documentation if necessary.The addition of the
bs58dependency (version^6.0.0) is appropriate for the new Solana balance querying feature. This library is commonly used for Base58 encoding/decoding in cryptocurrency applications.If this new dependency introduces significant new functionality, consider updating the project's documentation to reflect its usage and purpose.
97-103: Changes align well with PR objectives.The modifications to
package.jsonare minimal and focused:
- Updating
@zetachain/networksto a specific release candidate version.- Adding
bs58for Base58 encoding/decoding.These changes support the PR objective of adding Solana balance querying functionality. The focused nature of these changes is good for maintainability.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
packages/client/src/getBalances.ts (4)
56-60: LGTM! Consider updating the function documentation.The updated function signature correctly includes the
solanaAddressparameter and makesevmAddressoptional, aligning with the PR objectives. This change enhances the flexibility of the function to query balances across different blockchain networks.Consider updating the function documentation above the
getBalancesfunction to include information about the newsolanaAddressparameter and the optional nature ofevmAddress.
223-249: LGTM! Consider extracting the chain name list for better maintainability.The changes correctly handle gas token balances for EVM chains based on the availability of
evmAddress. The exclusion of Solana chain names is appropriate.Consider extracting the list of excluded chain names into a constant at the top of the file for better maintainability. For example:
const NON_EVM_CHAIN_NAMES = [ "btc_testnet", "btc_mainnet", "solana_mainnet", "solana_testnet", "solana_devnet", ]; // Then use it in the filter: .filter( (token) => token.coin_type === "Gas" && !NON_EVM_CHAIN_NAMES.includes(token.chain_name) )This would make it easier to update the list of excluded chains in the future.
277-302: LGTM! Consider enhancing error handling and balance calculation for Solana.The new implementation for querying Solana balances aligns well with the PR objectives. The check for
solanaAddressensures that Solana balances are only queried when a Solana address is provided.Consider the following improvements:
- Enhance error handling:
if (!response.ok) { console.error(`Failed to get balance for Solana: ${response.statusText}`); return; } const r = await response.json(); if (!r.result || typeof r.result.value !== 'number') { console.error('Invalid response format for Solana balance'); return; }
- Use
BigIntfor precise balance calculation:const balance = BigInt(r.result.value) / BigInt(10 ** 9); balances.push({ ...token, balance: balance.toString() });These changes will improve the robustness and precision of the Solana balance querying.
Line range hint
56-302: Consider improving error handling consistency and logging across all network types.The
getBalancesfunction now effectively handles multiple blockchain networks based on provided addresses. However, there's an opportunity to enhance the consistency of error handling and logging across different network types.Consider the following improvements:
- Implement a consistent error handling approach for all API calls and data processing steps.
- Use a logging library or create a custom logging function to ensure consistent log format and level across the function.
- Consider wrapping each network-specific balance fetching logic in try-catch blocks to prevent a failure in one network from affecting others.
Example of a consistent error handling and logging approach:
const logError = (message: string, error?: any) => { console.error(`[getBalances] ${message}`, error); }; // Inside each network-specific section: try { // Network-specific balance fetching logic } catch (error) { logError(`Failed to fetch balance for ${networkName}`, error); // Decide whether to continue with other networks or return partial results }Implementing these suggestions will improve the overall robustness and maintainability of the
getBalancesfunction.packages/tasks/src/balances.ts (3)
23-24: Clarify the note regardingBTC_PRIVATE_KEYformatThe note "(without the 0x prefix)" may be unnecessary for
BTC_PRIVATE_KEYsince Bitcoin private keys typically do not have a0xprefix. This could confuse users who might think they need to remove a prefix that isn't there.Apply this diff to update the instructions:
EVM_PRIVATE_KEY=123... (without the 0x prefix) - BTC_PRIVATE_KEY=123... (without the 0x prefix) + BTC_PRIVATE_KEY=123... SOLANA_PRIVATE_KEY=123.. (base58 encoded or json array)
76-91: Consider supporting Solana CLI keypair fileIt was suggested to optionally use the Solana CLI configuration file at
~/.config/solana/id.jsonwhenSOLANA_PRIVATE_KEYis not set. Implementing this feature would simplify key management for users who have already configured their Solana environment.I can help implement this functionality if you'd like. Do you want me to provide code to read the Solana key from the CLI configuration file?
115-117: Improve address output formatting for better readabilityThe current address output concatenates strings, which may result in inconsistent formatting. Consider using an array to construct the output for better readability and maintainability.
Apply this diff to refactor the output:
spinner.stop(); - console.log(` - EVM: ${evmAddress} ${btcAddress ? `\nBitcoin: ${btcAddress}` : ""} ${ - solanaAddress ? `\nSolana: ${solanaAddress}` : "" - } - `); + const addresses = []; + if (evmAddress) addresses.push(`EVM: ${evmAddress}`); + if (btcAddress) addresses.push(`Bitcoin: ${btcAddress}`); + if (solanaAddress) addresses.push(`Solana: ${solanaAddress}`); + console.log(addresses.join('\n'));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/client/src/getBalances.ts (3 hunks)
- packages/tasks/src/balances.ts (5 hunks)
🧰 Additional context used
Biome
packages/tasks/src/balances.ts
[error] 96-101: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments not posted (3)
packages/client/src/getBalances.ts (2)
147-221: LGTM! Proper handling of ERC20 and ZRC20 tokens based onevmAddressavailability.The changes ensure that ERC20 and ZRC20 token balances are only queried when an EVM address is provided. This aligns well with the updated function signature where
evmAddressis now optional.
256-268: LGTM! Consider adding error handling for the Bitcoin balance query.The changes correctly handle Bitcoin balance queries based on the availability of
btcAddress. The balance calculation usingfunded_txo_sumandspent_txo_sumis accurate.To ensure robust error handling, consider adding checks for the API response and the presence of required properties. Here's a script to verify the current implementation:
If the script doesn't show any error handling, consider adding it as follows:
const response = await fetch(`${API}/address/${btcAddress}`); if (!response.ok) { console.error(`Failed to fetch Bitcoin balance: ${response.statusText}`); return; } const r = await response.json(); if (!r.chain_stats || typeof r.chain_stats.funded_txo_sum === 'undefined' || typeof r.chain_stats.spent_txo_sum === 'undefined') { console.error('Invalid response format for Bitcoin balance'); return; } const { funded_txo_sum, spent_txo_sum } = r.chain_stats;packages/tasks/src/balances.ts (1)
96-101: Remove unnecessary bracesThe block starting at line 96 is unnecessary and can be removed to simplify the code.
This issue was previously noted, and the past review comment is still valid.
🧰 Tools
Biome
[error] 96-101: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.(lint/complexity/noUselessLoneBlockStatements)
There was a problem hiding this comment.
Looks good! I think XYZ_PRIVATE_KEY is definitely the way to go, but can you please add PRIVATE_KEY as an alias to EVM_PRIVATE_KEY for backwards compatibility. We don't want to break the experience for no good reason.
So, the logic is for EVM if exists use EVM_PRIVATE_KEY (higher priority), if not use PRIVATE_KEY (lower priority).
Done |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/tasks/src/balances.ts (1)
24-24: Improve clarity in theSOLANA_PRIVATE_KEYsetup instructionsTo help users correctly set up the
SOLANA_PRIVATE_KEY, consider providing examples that illustrate both acceptable formats: the base58 encoded string and the JSON array fromid.json.Apply this diff to update the instructions:
SOLANA_PRIVATE_KEY=123.. (base58 encoded or json array) + # Examples: + # SOLANA_PRIVATE_KEY=Base58EncodedString + # or + # SOLANA_PRIVATE_KEY=[12,34,...] (contents of id.json)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/tasks/src/balances.ts (5 hunks)
🧰 Additional context used
🪛 Biome
packages/tasks/src/balances.ts
[error] 96-101: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.(lint/complexity/noUselessLoneBlockStatements)
| return process.exit(1); | ||
|
|
||
| const evmKey = process.env.EVM_PRIVATE_KEY || process.env.PRIVATE_KEY; | ||
| const solanaKey = process.env.SOLANA_PRIVATE_KEY; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement key retrieval from the Solana CLI configuration
The PR objectives mention a suggestion to optionally use the Solana key stored in ~/.config/solana/id.json if SOLANA_PRIVATE_KEY is not set. Implementing this functionality would enhance user experience by simplifying key management for those who have the Solana CLI installed.
You might modify the code to attempt reading the private key from the Solana CLI configuration file when the environment variable is not set:
import fs from 'fs';
import os from 'os';
import path from 'path';
// ... existing imports ...
const solanaConfigPath = path.join(os.homedir(), '.config', 'solana', 'id.json');
let solanaKey: string | undefined = process.env.SOLANA_PRIVATE_KEY;
if (!solanaKey && fs.existsSync(solanaConfigPath)) {
solanaKey = fs.readFileSync(solanaConfigPath, 'utf-8');
}
// ... existing logic for handling solanaKey ...Also applies to: 75-91
Relevant issue: #180
Summary by CodeRabbit
New Features
Bug Fixes
Documentation