feat(solana): deposit command #317
Conversation
📝 WalkthroughWalkthroughThe updates include raising the Node.js version from 18 to 21 in all GitHub Actions workflows, upgrading the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (depositCommand)
participant KeypairHelper
participant SolanaProvider
participant GatewayProgram
User->>CLI (depositCommand): Run deposit command with options
CLI (depositCommand)->>KeypairHelper: Derive Keypair (from mnemonic or private key)
KeypairHelper-->>CLI (depositCommand): Return Keypair
CLI (depositCommand)->>SolanaProvider: Connect to selected network
SolanaProvider-->>CLI (depositCommand): Return provider
CLI (depositCommand)->>GatewayProgram: Initialize with IDL and provider
CLI (depositCommand)->>GatewayProgram: Call deposit/depositSplToken with params
GatewayProgram-->>CLI (depositCommand): Return transaction hash
CLI (depositCommand)->>User: Output transaction hash
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/commands/src/solana/deposit.ts (2)
124-124: Purpose of null parameter is unclear.Both
depositSplTokenanddepositmethods have anullparameter, but there's no comment explaining what this parameter is for.Add comments to explain the purpose of this parameter:
- null + null // Message parameter - null means no additional message dataAlso applies to: 141-141
161-163: Consider adding a default network.The network option doesn't have a default value, which means it must be explicitly provided. Consider setting a default, such as "devnet" for ease of use.
.addOption( - new Option("--network <network>", "Solana network").choices(networks) + new Option("--network <network>", "Solana network").choices(networks).default("devnet") )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (29)
typechain-types/@uniswap/index.tsis excluded by!typechain-types/**typechain-types/@uniswap/v3-core/contracts/index.tsis excluded by!typechain-types/**typechain-types/@uniswap/v3-core/contracts/interfaces/callback/IUniswapV3SwapCallback.tsis excluded by!typechain-types/**typechain-types/@uniswap/v3-core/contracts/interfaces/callback/index.tsis excluded by!typechain-types/**typechain-types/@uniswap/v3-core/contracts/interfaces/index.tsis excluded by!typechain-types/**typechain-types/@uniswap/v3-core/index.tsis excluded by!typechain-types/**typechain-types/@uniswap/v3-periphery/contracts/index.tsis excluded by!typechain-types/**typechain-types/@uniswap/v3-periphery/contracts/interfaces/ISwapRouter.tsis excluded by!typechain-types/**typechain-types/@uniswap/v3-periphery/contracts/interfaces/index.tsis excluded by!typechain-types/**typechain-types/@uniswap/v3-periphery/index.tsis excluded by!typechain-types/**typechain-types/contracts/SwapHelpers.sol/SwapLibrary.tsis excluded by!typechain-types/**typechain-types/contracts/SwapHelpers.sol/index.tsis excluded by!typechain-types/**typechain-types/contracts/index.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/index.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/v3-core/contracts/index.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/v3-core/contracts/interfaces/callback/IUniswapV3SwapCallback__factory.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/v3-core/contracts/interfaces/callback/index.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/v3-core/contracts/interfaces/index.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/v3-core/index.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/v3-periphery/contracts/index.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/v3-periphery/contracts/interfaces/ISwapRouter__factory.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/v3-periphery/contracts/interfaces/index.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/v3-periphery/index.tsis excluded by!typechain-types/**typechain-types/factories/contracts/SwapHelpers.sol/SwapLibrary__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/SwapHelpers.sol/index.tsis excluded by!typechain-types/**typechain-types/factories/contracts/index.tsis excluded by!typechain-types/**typechain-types/hardhat.d.tsis excluded by!typechain-types/**typechain-types/index.tsis excluded by!typechain-types/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
.github/workflows/build.yaml(1 hunks).github/workflows/lint.yaml(1 hunks).github/workflows/test.yaml(1 hunks)package.json(2 hunks)packages/client/src/solanaDeposit.ts(1 hunks)packages/client/src/solanaDepositAndCall.ts(1 hunks)packages/commands/src/solana/deposit.ts(1 hunks)packages/commands/src/solana/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/commands/src/solana/deposit.ts (1)
packages/tasks/src/account.ts (1)
main(121-169)
🔇 Additional comments (14)
package.json (1)
121-121: Dependency bump for @zetachain/protocol-contracts-solana
Updated the Solana protocol-contracts dependency from2.0.0-rc1to3.0.2-rc2. Ensure compatibility with the new Gateway IDL and that theyarn.lockhas been updated and committed to avoid version mismatches..github/workflows/lint.yaml (1)
18-18: Upgrade Node.js runtime to 21
Aligns the lint workflow environment with build and test pipelines.actions/setup-node@v3supports Node.js 21 out of the box..github/workflows/build.yaml (1)
18-18: Upgrade Node.js runtime to 21
Consistent Node.js version across CI ensures uniform behavior. No additional changes needed..github/workflows/test.yaml (1)
18-18: Upgrade Node.js runtime to 21
Brings the test workflow in line with build and lint jobs—good for consistency.packages/client/src/solanaDeposit.ts (1)
8-8:✅ Verification successful
Switch to development Gateway IDL path
The import was changed to@zetachain/protocol-contracts-solana/dev/idl/gateway.json. Confirm that this path exists in version3.0.2-rc2of the package and that any reference insolanaDepositAndCall.tsmatches.To verify, you can run:
🏁 Script executed:
#!/usr/bin/env bash # Verify that the development IDL exists in the installed package if [ ! -f "node_modules/@zetachain/protocol-contracts-solana/dev/idl/gateway.json" ]; then echo "ERROR: dev/idl/gateway.json not found in protocol-contracts-solana" exit 1 else echo "OK: Gateway IDL file found" fiLength of output: 150
🏁 Script executed:
#!/usr/bin/env bash # Verify that solanaDepositAndCall.ts uses the same development IDL path rg -n 'from\s+".*@zetachain/protocol-contracts-solana/dev/idl/gateway.json"' packages/client/src/solanaDepositAndCall.ts || \ echo "WARNING: No matching import of gateway.json found in solanaDepositAndCall.ts"Length of output: 209
Development Gateway IDL path verified
The import path@zetachain/protocol-contracts-solana/dev/idl/gateway.jsonexists in version 3.0.2-rc2 and is consistently used in both files; no changes required.• packages/client/src/solanaDeposit.ts — line 8
• packages/client/src/solanaDepositAndCall.ts — line 8packages/client/src/solanaDepositAndCall.ts (1)
8-8: Updated Gateway IDL import path to use development version.The import path for the Gateway IDL has been updated to use the development version from the
/devdirectory. This change aligns with the dependency upgrade to@zetachain/[email protected]and supports the new deposit functionality.packages/commands/src/solana/index.ts (1)
3-3: New deposit command successfully integrated.The deposit command import and registration look good. This properly exposes the new Solana deposit functionality to the CLI.
Also applies to: 11-11
packages/commands/src/solana/deposit.ts (7)
1-15: Import statements look good.All necessary dependencies are imported for Solana interaction, token handling, and CLI commands. Both development and production Gateway IDLs are properly imported.
18-28: DepositOptions interface is well-defined.The interface provides all necessary parameters for deposit operations, supporting both native SOL and SPL token deposits.
30-47: Key derivation functions implemented correctly.The helper functions properly implement keypair derivation from both mnemonics and base58-encoded private keys with appropriate error handling.
54-61: Wallet initialization looks good.Properly handles keypair initialization from either private key or mnemonic with appropriate error handling.
63-68: API endpoint selection is correct.The code properly selects the appropriate Solana API endpoint based on the network parameter.
149-164: Command definition looks good.The CLI command is well-defined with appropriate required and optional parameters. The usage of Option with choices for network is a good practice.
51-52:❌ Incorrect review comment
Consider environment-specific IDL selection.
The code selects the Gateway IDL based on network, but uses the same IDL for both mainnet and devnet. Ensure this approach is correct as mainnet may require a stable IDL version.
🏁 Script executed:
#!/bin/bash # Check if there are references to different IDL usage for mainnet vs devnet rg -A 5 -B 5 "mainnet.*IDL|devnet.*IDL" --type tsLength of output: 744
🏁 Script executed:
#!/bin/bash # Locate where GATEWAY_DEV_IDL and GATEWAY_PROD_IDL are defined to verify if a separate devnet IDL exists rg -n "GATEWAY_DEV_IDL" --type ts rg -n "GATEWAY_PROD_IDL" --type tsLength of output: 565
No separate devnet IDL exists
The@zetachain/protocol-contracts-solanapackage only provides two IDLs—
dev/idl/gateway.json(used forlocalnet)prod/idl/gateway.json(used for everything else, including both devnet and mainnet)As such, using
GATEWAY_PROD_IDLfor both devnet and mainnet is intentional and correct. No changes are required unless you plan to introduce a distinct devnet IDL.• File: packages/commands/src/solana/deposit.ts
Lines: 51–52Likely an incorrect or invalid review comment.
|
Modified |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
typechain-types/factories/contracts/EthZetaMock.sol/ZetaEthMock__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/SwapHelpers.sol/SwapLibrary__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/TestZRC20__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/shared/MockZRC20__factory.tsis excluded by!typechain-types/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
package.json(1 hunks)packages/client/src/solanaDeposit.ts(2 hunks)packages/client/src/solanaDepositAndCall.ts(2 hunks)packages/commands/src/solana/deposit.ts(1 hunks)packages/commands/src/solana/index.ts(1 hunks)packages/tasks/src/solanaDeposit.ts(1 hunks)packages/tasks/src/solanaDepositAndCall.ts(1 hunks)utils/accounts.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/tasks/src/solanaDeposit.ts
- packages/tasks/src/solanaDepositAndCall.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- package.json
- packages/client/src/solanaDeposit.ts
- packages/commands/src/solana/index.ts
- packages/client/src/solanaDepositAndCall.ts
- packages/commands/src/solana/deposit.ts
🔇 Additional comments (3)
utils/accounts.ts (3)
5-5: Added bs58 library for base58 encoding/decodingGood choice to add the bs58 library to handle base58 encoding/decoding for Solana private keys. This is standard practice when working with Solana keypairs.
83-105: Enhanced flexibility for handling Solana private keysThe improved private key handling now supports both base58 and hex formats, which aligns well with the new Solana deposit command requirements. The error handling is comprehensive with helpful error messages.
108-108:❓ Verification inconclusive
Potential backward compatibility issue
Changing the private key encoding from hex to base58 might break backward compatibility with code that expects the private key to be in hex format. Consider verifying that all consuming code can handle this change.
🏁 Script executed:
#!/bin/bash # Search for uses of SolanaAccountData that might be affected by the encoding change echo "Searching for places where SolanaAccountData private keys are used..." rg -A 5 "SolanaAccountData" --type typescript echo "Searching for places where Solana private keys might be accessed..." rg -A 5 "\.privateKey.*solana" --type typescriptLength of output: 460
🏁 Script executed:
#!/bin/bash # Search for references without relying on --type filter echo "Searching for uses of SolanaAccountData in .ts files..." rg -A5 "SolanaAccountData" -g '*.ts' echo "Searching for privateKey fields in .ts files..." rg -A5 "\.privateKey" -g '*.ts'Length of output: 7950
Verify backward compatibility of Solana privateKey encoding
Changing the Solana private key output from hex to base58 will break any code or persisted data that still expects a hex string. Please confirm that all consumers have been updated (or that legacy hex keys are still supported), including:
• utils/accounts.ts – createSolanaAccount now does
privateKey: bs58.encode(keypair.secretKey)• utils/addressResolver.ts – code loading SolanaAccountData from disk
• packages/commands/src/solana/deposit.ts – keypairFromPrivateKey(...) must decode base58
• Any other CLI commands or helpers that read account.privateKey (e.g. in bitcoin, evm, sui)
• tests or migrations for existing user‐stored keysIf any consumers still expect hex, either restore hex support or add a migration/dual-format parser.
| @@ -15,7 +15,7 @@ jobs: | |||
| - name: Setup Node.js | |||
| uses: actions/setup-node@v3 | |||
| with: | |||
| node-version: "18" | |||
| node-version: "21" | |||
There was a problem hiding this comment.
Ideally, we should address these types of changes in their own separate PR.
There was a problem hiding this comment.
I agree, but the test were failing because of the node version. So either we update node in this PR or merge with failing tests.
| @@ -1,5 +1,5 @@ | |||
| import { Wallet } from "@coral-xyz/anchor"; | |||
| import { Keypair } from "@solana/web3.js"; | |||
| import { Keypair } from "@coral-xyz/anchor/node_modules/@solana/web3.js"; | |||
| .addOption( | ||
| new Option("--network <network>", "Solana network").choices(networks) | ||
| ) | ||
| .action(main); |
There was a problem hiding this comment.
Let's please use a Zod schema to validate params here.
| } else if (options.mnemonic) { | ||
| keypair = await keypairFromMnemonic(options.mnemonic); | ||
| } else { | ||
| throw new Error("Either privateKey or mnemonic must be provided"); |
There was a problem hiding this comment.
This shouldn't be necessary if we validate params with a solid Zod schema.
There was a problem hiding this comment.
Right now we're duplicating logic: optionality, default values, conflicts are handled both in commander and zod.
There was a problem hiding this comment.
I can see advantages of zod for exported functions, but less so for commands that already use declarative syntax to do validation.
For example,
.refine((data) => !(data.mnemonic && data.privateKey), {
message: "Only one of mnemonic or privateKey can be provided, not both",
});And:
.addOption(
new Option("--name <name>", "Account name")
.conflicts(["private-key"])
)
.addOption(
new Option("--private-key <key>", "Private key for signing transactions")
.conflicts(["name"])
)We don't need both.
There was a problem hiding this comment.
Maybe we do types in zod like .string(), but default values and validation in commander.
There was a problem hiding this comment.
Default values and enum values should definitely be in commander, because they show up in --help.
There was a problem hiding this comment.
Added zod validation: a4f049f
| const provider = new anchor.AnchorProvider( | ||
| connection, | ||
| new Wallet(keypair), | ||
| {} |
There was a problem hiding this comment.
Do we really need this? I would imagine this will work the same without this empty object.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
utils/solana.commands.helpers.ts (3)
5-14: Consider adding stricter validation for field formatsThe schema efficiently defines the required fields and their types, but could benefit from more specific format validation:
amountshould validate that it's a valid number stringrecipientshould validate as a valid Solana address (base58 encoded public key)mintshould validate as a valid Solana address when providednetworkshould restrict to supported networks (devnet, localnet, mainnet)export const solanaDepositOptionsSchema = z .object({ - amount: z.string(), + amount: z.string().refine((val) => !isNaN(parseFloat(val)), { + message: "Amount must be a valid number", + }), mint: z.string().optional(), mnemonic: z.string().optional(), - network: z.string(), + network: z.enum(["devnet", "localnet", "mainnet"]), privateKey: z.string().optional(), - recipient: z.string(), + recipient: z.string().regex(/^[1-9A-HJ-NP-Za-km-z]{32,44}$/, { + message: "Recipient must be a valid Solana address", + }), tokenProgram: z.string().default(SOLANA_TOKEN_PROGRAM), })
15-17: Consider enforcing at least one authentication methodThe refinement correctly ensures that both authentication methods aren't provided simultaneously, but doesn't enforce that at least one is provided.
.refine((data) => !(data.mnemonic && data.privateKey), { message: "Only one of mnemonic or privateKey can be provided, not both", }); + .refine((data) => !!(data.mnemonic || data.privateKey), { + message: "Either mnemonic or privateKey must be provided", + });
1-18: Add JSDoc comments for better documentationConsider adding JSDoc comments to explain the purpose of the schema and document each field's requirements. This would improve developer experience and make the code more maintainable.
+/** + * Schema for validating Solana deposit command options. + * Used to ensure all required fields are present and correctly formatted. + */ export const solanaDepositOptionsSchema = z .object({ + /** + * Amount to deposit, must be a valid number as string + */ amount: z.string(), + /** + * Optional SPL token mint address for token deposits. + * Required when depositing SPL tokens, not used for SOL deposits. + */ mint: z.string().optional(), + /** + * Optional BIP39 mnemonic phrase for wallet derivation. + * Mutually exclusive with privateKey. + */ mnemonic: z.string().optional(), + /** + * Solana network to use (devnet, localnet, mainnet) + */ network: z.string(), + /** + * Optional base58-encoded private key for wallet. + * Mutually exclusive with mnemonic. + */ privateKey: z.string().optional(), + /** + * Destination Solana address for the deposit + */ recipient: z.string(), + /** + * Token program address, defaults to standard Solana token program + */ tokenProgram: z.string().default(SOLANA_TOKEN_PROGRAM), })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/commands/src/solana/deposit.ts(1 hunks)utils/solana.commands.helpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/commands/src/solana/deposit.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
utils/solana.commands.helpers.ts (1)
types/shared.constants.ts (1)
SOLANA_TOKEN_PROGRAM(2-3)
🔇 Additional comments (1)
utils/solana.commands.helpers.ts (1)
1-4: Nice clean imports!The imports are well-organized with clear separation between external libraries (zod) and internal modules.
Deposit SOL
✅ Devnet
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/3Tw2S8HPbFAN7xtMase8oLa23d1f9etacTfhuREbGnccuSHTkREbtM9sB3H2H5pYbEhpWMs74wA3QS5wxhSnGsSM
✅ Localnet
Deposit SPL
✅ Devnet
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/5LjZVoeCScPqmVBkPGrY4LWX3ukDE2f8z28vjbEbDwJPtmmUPKG1o6D8T3uc61h78NKisxQyABhYMmnVi2secydr
✅ Localnet
Right now it's a bit confusing, because by default localnet airdrops SPL tokens only to the account defined in
~/.config/solana/id.json, and not to the address associated with the default mnemonic below.Summary by CodeRabbit
New Features
Chores
Improvements