Solana deposit and call #203
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several changes to the Changes
Possibly related PRs
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
|
This comment was marked as outdated.
This comment was marked as outdated.
|
Deposit works: |
|
Deposit and call: It's failing, but that's expected, because currently Solana routes calls to |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (2)
packages/client/src/solanaDeposit.ts (1)
Line range hint
121-123: Enhance error handling with specific error typesThe current error handling catches all errors and logs them generically. Consider:
- Adding specific error types for common failures
- Including error codes for better debugging
- Structuring the error response for better client handling
} catch (error) { - console.error("Transaction failed:", error); + const errorMessage = error instanceof Error ? error.message : String(error); + console.error("Solana deposit transaction failed:", { + error: errorMessage, + code: error?.code, + type: "SOLANA_DEPOSIT_ERROR" + }); + throw new Error(`Solana deposit failed: ${errorMessage}`); }packages/tasks/src/solanaDepositAndCall.ts (1)
32-35: Remove unused variable 'idPath' from destructuring assignmentThe variable
idPathis extracted fromargsbut not used in the function, which can lead to confusion.Update the code to remove
idPath:- const { amount, idPath } = args; + const { amount } = args;
📜 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 (9)
package.json(3 hunks)packages/client/src/client.ts(2 hunks)packages/client/src/idl/gateway.json(0 hunks)packages/client/src/index.ts(1 hunks)packages/client/src/solanaDeposit.ts(2 hunks)packages/client/src/solanaDepositAndCall.ts(1 hunks)packages/tasks/src/index.ts(1 hunks)packages/tasks/src/solanaDeposit.ts(2 hunks)packages/tasks/src/solanaDepositAndCall.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/client/src/idl/gateway.json
🔇 Additional comments (7)
packages/client/src/solanaDeposit.ts (2)
5-5: LGTM! Import IDL from npm package
Importing Gateway_IDL from npm package improves maintainability and ensures consistency across projects.
82-84: Verify recipient format validation
The recipient handling has been simplified to directly use ethers.utils.arrayify. While this is more efficient, ensure that:
- The input format is validated before conversion
- The recipient buffer length matches the contract's expectations
packages/tasks/src/solanaDeposit.ts (1)
67-71: LGTM! Task registration is well-defined
The task parameters are clearly defined with appropriate descriptions and default values.
packages/client/src/client.ts (2)
26-26: LGTM! Clean import addition.
The new import follows the existing pattern and maintains alphabetical order.
180-180: LGTM! Method assignment follows existing pattern.
The new method assignment is consistent with the class structure and existing patterns.
Let's verify that the imported function exists:
✅ Verification successful
Function exists and is properly implemented
The solanaDepositAndCall function is correctly defined in both packages/client/src/solanaDepositAndCall.ts and packages/tasks/src/solanaDepositAndCall.ts. The assignment in client.ts is valid as it references the function from the client implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the solanaDepositAndCall function exists in the source files
# Search for the solanaDepositAndCall function definition
ast-grep --pattern 'export const solanaDepositAndCall = $_' || \
ast-grep --pattern 'export function solanaDepositAndCall($_)'
Length of output: 11334
packages/client/src/index.ts (1)
18-18: Approved: Export of 'solanaDepositAndCall' is correctly added
The new export statement properly includes solanaDepositAndCall in the module exports.
packages/tasks/src/index.ts (1)
15-15: Approved: Export of 'solanaDepositAndCall' is correctly added
The new export statement properly includes solanaDepositAndCall in the module exports.
| @@ -56,6 +56,7 @@ | |||
| "@typescript-eslint/eslint-plugin": "^5.59.9", | |||
| "@typescript-eslint/parser": "^5.59.9", | |||
| "@uniswap/v2-core": "^1.0.1", | |||
| "@zetachain/protocol-contracts-solana": "2.0.0-rc1", | |||
There was a problem hiding this comment.
💡 Codebase verification
Update dependencies to latest stable versions
Both packages have stable versions available that are newer than the release candidates:
@zetachain/protocol-contracts-solana: Use version2.0.0instead of2.0.0-rc1@zetachain/networks: Use version10.0.0instead of10.0.0-rc1
🔗 Analysis chain
Verify stability of release candidate dependencies
The new dependency @zetachain/[email protected] and updated @zetachain/[email protected] are both release candidates. While this aligns with the PR objective of importing IDL from npm, ensure these versions are stable enough for production use.
Also applies to: 104-104
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for newer versions and release notes
echo "Checking @zetachain/protocol-contracts-solana versions:"
npm view @zetachain/protocol-contracts-solana versions --json
echo "Checking @zetachain/networks versions:"
npm view @zetachain/networks versions --json
Length of output: 848
| @@ -30,18 +30,9 @@ export const solanaDeposit = async ( | |||
| recipient = args.recipient; | |||
| } | |||
| const { amount, idPath } = args; | |||
| const params = [JSON.parse(args.types), args.values]; | |||
| await client.solanaDeposit({ amount, params, recipient }); | |||
| await client.solanaDeposit({ amount, recipient }); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add amount validation
The amount parameter is passed directly to the client without validation. Consider adding checks for:
- Negative values
- Maximum deposit limits
- Sufficient balance
- await client.solanaDeposit({ amount, recipient });
+ const parsedAmount = parseFloat(amount);
+ if (isNaN(parsedAmount) || parsedAmount <= 0) {
+ throw new Error("Invalid deposit amount: must be a positive number");
+ }
+ const balance = await provider.connection.getBalance(wallet.publicKey);
+ if (balance < parsedAmount * anchor.web3.LAMPORTS_PER_SOL) {
+ throw new Error("Insufficient balance for deposit");
+ }
+ await client.solanaDeposit({ amount: parsedAmount, recipient });Committable suggestion skipped: line range outside the PR's diff.
| export const solanaDepositAndCall = async ( | ||
| args: any, | ||
| hre: HardhatRuntimeEnvironment | ||
| ) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify a more precise type for 'args' parameter in 'solanaDepositAndCall' function
Using any for args reduces type safety and can lead to runtime errors. Define a specific interface or type for args to improve code reliability and maintainability.
Consider defining an interface for args:
interface SolanaDepositAndCallArgs {
idPath: string;
solanaNetwork: string;
recipient: string;
types: string;
values: any[];
amount: number;
}Then update the function signature:
export const solanaDepositAndCall = async (
args: SolanaDepositAndCallArgs,
hre: HardhatRuntimeEnvironment
) => {
// function body
};| let recipient; | ||
| try { | ||
| if ((bech32 as any).decode(args.recipient)) { | ||
| recipient = utils.solidityPack( | ||
| ["bytes"], | ||
| [utils.toUtf8Bytes(args.recipient)] | ||
| ); | ||
| } | ||
| } catch (e) { | ||
| recipient = args.recipient; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify and clarify recipient address decoding logic
The current logic attempts to decode the recipient address using bech32.decode within a try-catch block, and uses an if statement that is always truthy if no exception is thrown. This can be simplified for clarity.
Refactor the code as follows:
let recipient;
try {
bech32.decode(args.recipient);
recipient = utils.solidityPack(
["bytes"],
[utils.toUtf8Bytes(args.recipient)]
);
} catch (e) {
recipient = args.recipient;
}This removes the unnecessary if condition since if bech32.decode doesn't throw an error, the recipient is assumed to be in Bech32 format.
| recipient = args.recipient; | ||
| } | ||
| const { amount, idPath } = args; | ||
| const params = [JSON.parse(args.types), args.values]; |
There was a problem hiding this comment.
Add error handling for JSON parsing of 'args.types'
Parsing args.types without error handling can result in unhandled exceptions if args.types is not valid JSON.
Add a try-catch block to handle parsing errors:
let paramTypes;
try {
paramTypes = JSON.parse(args.types);
} catch (error) {
throw new Error(`Invalid JSON in 'types' parameter: ${error.message}`);
}
const params = [paramTypes, args.values];| const message = Buffer.from( | ||
| ethers.utils.arrayify( | ||
| ethers.utils.defaultAbiCoder.encode(args.params[0], args.params[1]) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Validate 'args.params' before encoding
To prevent runtime errors, ensure that args.params[0] and args.params[1] are valid before encoding.
Add validation for args.params:
if (!Array.isArray(args.params[0]) || !Array.isArray(args.params[1])) {
throw new Error("Invalid 'params' format. Expected arrays of types and values.");
}| } catch (error) { | ||
| console.error("Transaction failed:", error); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Re-throw errors after logging in catch block
Currently, errors are logged but not re-thrown, which could lead to silent failures. Re-throw the error after logging to allow upstream handling.
Update the catch block:
} catch (error) {
console.error("Transaction failed:", error);
throw error; // Re-throw the error to be handled by the caller
}| const network = "solana_" + this.network; | ||
| const api = getEndpoints("solana" as any, network); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure correct type when calling 'getEndpoints'
Casting "solana" as any reduces type safety. Use the appropriate type or adjust the function definition to accept a string.
Modify the code to avoid casting:
- const api = getEndpoints("solana" as any, network);
+ const api = getEndpoints("solana", network);If getEndpoints requires a specific type, consider updating its type definitions to accept "solana" as a valid option.
Committable suggestion skipped: line range outside the PR's diff.
depositanddepositAndCallfunctions.Summary by CodeRabbit
New Features
solanaDepositAndCallfor enhanced Solana transaction capabilities.solana-deposit-and-callfor executing deposit and call operations from the command line.Bug Fixes
solanaDepositfunction by removing unnecessary parameters for a simpler interface.Chores