Solana deposit and call #203
Conversation
|
Warning Rate limit exceeded@fadeev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several changes to the Changes
Suggested reviewers
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];| } 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
}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/client/src/solanaDepositAndCall.ts (4)
2-3: Consolidate imports from "@solana/web3.js"Combine multiple imports from the same module into a single statement for clarity.
Apply this change:
- import { TransactionMessage, VersionedTransaction } from "@solana/web3.js"; - import { Transaction } from "@solana/web3.js"; + import { Transaction, TransactionMessage, VersionedTransaction } from "@solana/web3.js";
34-36: Unnecessary optional chaining insignAllTransactionsmethodSince
this.solanaAdapteris confirmed to be defined, you can remove the optional chaining?.for clarity.Apply this change:
- if (!this.solanaAdapter?.signAllTransactions) { + if (!this.solanaAdapter.signAllTransactions) {
42-43: Unnecessary optional chaining insignTransactionmethod
this.solanaAdapteris already verified to exist, so the optional chaining?.is unnecessary.Apply this change:
- if (!this.solanaAdapter?.signTransaction) { + if (!this.solanaAdapter.signTransaction) {
83-83: Validateargs.recipientbefore processingEnsure that
args.recipientis a valid Ethereum address to prevent errors during processing.Add validation:
if (!ethers.utils.isAddress(args.recipient)) { throw new Error("Invalid recipient address"); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/client/src/solanaDepositAndCall.ts(1 hunks)
🔇 Additional comments (2)
packages/client/src/solanaDepositAndCall.ts (2)
25-25: Avoid casting types unnecessarily in getEndpoints call
Casting "solana" as any reduces type safety. Use the appropriate type or adjust the function definition to accept "solana" without casting.
Apply this change:
- 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.
138-140: Re-throw errors after logging in catch block
Errors are logged but not re-thrown, which may lead to silent failures. Re-throw the error after logging to allow upstream handling.
Apply this change:
} catch (error) {
console.error("Transaction failed:", error);
+ throw error; // Re-throw the error to be handled by the caller
}
|
@zeta-chain/fullstack please, review again. |
depositanddepositAndCallfunctions.Summary by CodeRabbit
New Features
Bug Fixes
Chores