Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Solana support to ZetaChain client and browser deposits #199

Merged
merged 3 commits into from
Nov 29, 2024

Conversation

lukema95
Copy link
Collaborator

@lukema95 lukema95 commented Nov 22, 2024

  1. Add Solana Wallet to ZetaChainClient
  2. Update solanaDeposit to support browser

Related issue #182

Summary by CodeRabbit

Release Notes

  • New Features

    • Added support for Solana wallets alongside existing Ethereum wallet capabilities.
    • Introduced new methods to check if a Solana wallet is connected and retrieve public keys.
    • Enhanced the solanaDeposit function for improved transaction handling and error reporting.
    • New function to retrieve and parse keypair from a specified file path.
  • Bug Fixes

    • Improved error handling for wallet capabilities and transaction failures.
  • Chores

    • Updated project dependencies to include the Solana wallet adapter and modified existing dependency versions.

@lukema95 lukema95 requested review from andresaiello, fadeev and a team as code owners November 22, 2024 15:33
Copy link
Contributor

coderabbitai bot commented Nov 22, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

The pull request introduces several enhancements to the @zetachain/toolkit project, primarily focusing on integrating Solana wallet support. The package.json file is updated to include a new dependency for Solana wallet management. The ZetaChainClient class is modified to accommodate Solana wallets, including new properties and methods for wallet management. Additionally, the solanaDeposit function is significantly updated to handle Solana transactions and wallet interactions, while a new utility function for keypair retrieval is added in a related task file.

Changes

File Change Summary
package.json - Added dependency: "@solana/wallet-adapter-react": "^0.15.35" in dependencies section.
- Updated version of "@zetachain/networks" from "10.0.0" to "v10.0.0-rc1".
packages/client/src/client.ts - Updated ZetaChainClientParams to include solanaAdapter and solanaWallet.
- Added properties: public solanaWallet and public solanaAdapter in ZetaChainClient.
- Added methods: isSolanaWalletConnected and getSolanaPublicKey.
packages/client/src/solanaDeposit.ts - Updated solanaDeposit function to check for connected Solana wallet and handle transactions using solanaAdapter or solanaWallet.
- Removed idPath parameter from function arguments.
packages/tasks/src/solanaDeposit.ts - Added getKeypairFromFile function for keypair retrieval.
- Modified solanaDeposit function to accept solanaWallet and update recipient handling.

Possibly related PRs

  • Support query solana balances #185: The changes in this PR involve updating the @zetachain/networks dependency version in package.json to 10.0.0-rc1, which directly relates to the version update from 10.0.0 to v10.0.0-rc1 in the main PR.
  • Update hardhat account, support solana #190: This PR adds support for Solana wallets in the hardhat account command, which is relevant to the main PR's enhancements for Solana wallet integration in the ZetaChainClient class and related functionalities.

Suggested reviewers

  • andresaiello

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Outside diff range and nitpick comments (6)
package.json (1)

95-95: Consider pinning the dependency version.

The caret (^) in the version allows minor updates which could introduce breaking changes. Consider pinning the version for better dependency management:

-    "@solana/wallet-adapter-react": "^0.15.35",
+    "@solana/wallet-adapter-react": "0.15.35",
packages/tasks/src/solanaDeposit.ts (3)

14-16: Ensure 'idPath' is correctly provided and handled

The idPath parameter is used to retrieve the keypair from the file system. Ensure that args.idPath is correctly provided or defaults to a valid path. Consider adding validation to handle cases where the keypair file may not exist or idPath is invalid.


45-45: Optimize module imports for better code clarity

Instead of dynamically importing the path module within the function, consider importing it at the top of the file. This improves code readability and avoids unnecessary dynamic imports.

Apply this diff to import path at the top:

+import path from "path";
...
-export const getKeypairFromFile = async (filepath: string) => {
-  const path = await import("path");
+export const getKeypairFromFile = async (filepath: string) => {

64-70: Simplify error handling for JSON parsing

The current error handling checks for a specific error message when parsing JSON. Instead, consider catching all parsing errors and providing a clear error message. This simplifies the code and ensures all JSON parsing errors are appropriately handled.

Apply this diff:

     try {
       parsedFileContents = Uint8Array.from(JSON.parse(fileContents));
     } catch (thrownObject) {
-      const error: any = thrownObject;
-      if (!error.message.includes("Unexpected token")) {
-        throw error;
-      }
-      throw new Error(`Invalid secret key file at '${filepath}'!`);
+      throw new Error(`Invalid secret key file at '${filepath}': ${thrownObject.message}`);
     }
packages/client/src/client.ts (2)

41-70: Consider simplifying the ZetaChainClientParams type definition

The current union of interfaces with never and undefined properties can be complex and hard to maintain. Using a discriminated union with a common walletType property can make the code more readable and easier to extend.

Here's a suggested refactor:

 export type ZetaChainClientParams = ZetaChainClientParamsBase &
-  (
-    | {
-        signer: Signer;
-        solanaAdapter?: never;
-        solanaWallet?: never;
-        wallet?: never;
-      }
-    | {
-        signer?: never;
-        solanaAdapter: WalletContextState;
-        solanaWallet?: never;
-        wallet?: never;
-      }
-    | {
-        signer?: never;
-        solanaAdapter?: never;
-        solanaWallet: SolanaWallet;
-        wallet?: never;
-      }
-    | {
-        signer?: never;
-        solanaAdapter?: never;
-        solanaWallet?: never;
-        wallet: Wallet;
-      }
-    | {
-        signer?: undefined;
-        solanaAdapter?: undefined;
-        solanaWallet?: undefined;
-        wallet?: undefined;
-      }
-  );
+  (
+    | { walletType: 'signer'; signer: Signer }
+    | { walletType: 'solanaAdapter'; solanaAdapter: WalletContextState }
+    | { walletType: 'solanaWallet'; solanaWallet: SolanaWallet }
+    | { walletType: 'wallet'; wallet: Wallet }
+    | { walletType?: undefined }
+  );

This approach introduces a walletType discriminator, making it clearer which wallet is being used and simplifying type checks.


160-164: Handle null values explicitly in getSolanaPublicKey

When both solanaAdapter and solanaWallet are undefined, the method currently returns null, which is appropriate. However, if publicKey is null or undefined within solanaAdapter or solanaWallet, it might be clearer to handle these cases explicitly.

Consider updating the method to make the intent explicit:

 public getSolanaPublicKey(): PublicKey | null {
   return (
-    this.solanaAdapter?.publicKey || this.solanaWallet?.publicKey || null
+    (this.solanaAdapter && this.solanaAdapter.publicKey) ||
+    (this.solanaWallet && this.solanaWallet.publicKey) ||
+    null
   );
 }

This makes it clear that the method returns a PublicKey if available, or null otherwise.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 5d9873c and 126bed5.

⛔ Files ignored due to path filters (27)
  • typechain-types/@openzeppelin/contracts/index.ts is excluded by !typechain-types/**
  • typechain-types/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/IERC1155Errors.ts is excluded by !typechain-types/**
  • typechain-types/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/IERC20Errors.ts is excluded by !typechain-types/**
  • typechain-types/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/IERC721Errors.ts is excluded by !typechain-types/**
  • typechain-types/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/index.ts is excluded by !typechain-types/**
  • typechain-types/@openzeppelin/contracts/interfaces/index.ts is excluded by !typechain-types/**
  • typechain-types/@openzeppelin/contracts/token/ERC20/ERC20.ts is excluded by !typechain-types/**
  • typechain-types/@openzeppelin/contracts/token/ERC20/IERC20.ts is excluded by !typechain-types/**
  • typechain-types/@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.ts is excluded by !typechain-types/**
  • typechain-types/contracts/EthZetaMock.sol/ZetaEthMock.ts is excluded by !typechain-types/**
  • typechain-types/contracts/TestZRC20.ts is excluded by !typechain-types/**
  • typechain-types/contracts/shared/MockZRC20.ts is excluded by !typechain-types/**
  • typechain-types/factories/@openzeppelin/contracts/index.ts is excluded by !typechain-types/**
  • typechain-types/factories/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/IERC1155Errors__factory.ts is excluded by !typechain-types/**
  • typechain-types/factories/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/IERC20Errors__factory.ts is excluded by !typechain-types/**
  • typechain-types/factories/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/IERC721Errors__factory.ts is excluded by !typechain-types/**
  • typechain-types/factories/@openzeppelin/contracts/interfaces/draft-IERC6093.sol/index.ts is excluded by !typechain-types/**
  • typechain-types/factories/@openzeppelin/contracts/interfaces/index.ts is excluded by !typechain-types/**
  • typechain-types/factories/@openzeppelin/contracts/token/ERC20/ERC20__factory.ts is excluded by !typechain-types/**
  • typechain-types/factories/@openzeppelin/contracts/token/ERC20/IERC20__factory.ts is excluded by !typechain-types/**
  • typechain-types/factories/@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata__factory.ts is excluded by !typechain-types/**
  • typechain-types/factories/contracts/EthZetaMock.sol/ZetaEthMock__factory.ts is excluded by !typechain-types/**
  • typechain-types/factories/contracts/TestZRC20__factory.ts is excluded by !typechain-types/**
  • typechain-types/factories/contracts/shared/MockZRC20__factory.ts is excluded by !typechain-types/**
  • typechain-types/hardhat.d.ts is excluded by !typechain-types/**
  • typechain-types/index.ts is excluded by !typechain-types/**
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (4)
  • package.json (1 hunks)
  • packages/client/src/client.ts (4 hunks)
  • packages/client/src/solanaDeposit.ts (3 hunks)
  • packages/tasks/src/solanaDeposit.ts (4 hunks)
🔇 Additional comments (4)
package.json (1)

95-95: Verify Solana wallet adapter version compatibility and security.

Let's ensure the chosen version is secure and compatible with other Solana dependencies.

✅ Verification successful

Let me gather more information about the existing Solana dependencies in the project to verify compatibility.


Version 0.15.35 is compatible and secure

The chosen version of @solana/wallet-adapter-react (0.15.35) is:

  • The latest available version from npm
  • Compatible with the project's @solana/web3.js (^1.95.3), meeting the peer dependency requirement (^1.77.3)
  • Already in use in the codebase (packages/client/src/client.ts)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check latest version and security advisories for @solana/wallet-adapter-react

# Get latest version from npm
echo "Latest version from npm:"
npm view @solana/wallet-adapter-react version

# Check for known vulnerabilities
echo -e "\nChecking for vulnerabilities:"
npm audit --json @solana/wallet-adapter-react@0.15.35 | jq '.advisories'

# Check compatibility with existing Solana dependencies
echo -e "\nChecking peer dependencies:"
npm info @solana/wallet-adapter-react@0.15.35 peerDependencies

Length of output: 797


Script:

#!/bin/bash
# Check existing Solana dependencies and their versions
echo "Checking Solana dependencies in package.json:"
rg '"@solana/.*":|"@coral-xyz/.*":' package.json

# Check if there are any direct imports of the wallet adapter
echo -e "\nChecking usage of wallet adapter:"
rg -l "@solana/wallet-adapter-react"

Length of output: 442

packages/tasks/src/solanaDeposit.ts (3)

1-2: Imports are appropriate for Solana wallet integration

The imports of Wallet from @coral-xyz/anchor and Keypair from @solana/web3.js are necessary for the Solana wallet functionality.


17-19: Initialization of ZetaChainClient with 'solanaWallet' is appropriate

The client is correctly instantiated with the solanaWallet, aligning with the updated client requirements. This change enables Solana wallet integration within the ZetaChainClient.


33-33: Updated 'client.solanaDeposit' method invocation aligns with new signature

The call to client.solanaDeposit without the idPath parameter reflects the updated method signature in the client interface. This ensures compatibility with the modifications made to the ZetaChainClient.

Comment on lines +47 to +49
const home = process.env.HOME || null;
if (home) {
filepath = path.join(home, filepath.slice(1));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use 'os.homedir()' for cross-platform home directory resolution

Using process.env.HOME may not be reliable across all operating systems (e.g., it may not be defined on Windows). Instead, use os.homedir() from Node.js's os module to get the home directory in a cross-platform manner.

Apply this diff:

+import os from "os";
...
-    const home = process.env.HOME || null;
+    const home = os.homedir();

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines 129 to 131
} catch (error) {
throw new Error(`Could not read keypair from file at '${filepath}'`);
}
// Parse contents of file
let parsedFileContents;
try {
parsedFileContents = Uint8Array.from(JSON.parse(fileContents));
} catch (thrownObject) {
const error: any = thrownObject;
if (!error.message.includes("Unexpected token")) {
throw error;
}
throw new Error(`Invalid secret key file at '${filepath}'!`);
console.error("Transaction failed:", error);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Properly handle errors to prevent unexpected behavior.

Currently, the function logs the error but does not rethrow it or return a value, which could lead to inconsistent behavior for callers expecting a transaction signature or an error.

Modify the catch block to rethrow the error:

  console.error("Transaction failed:", error);
+ throw error;

Alternatively, ensure the function returns a consistent result or handles the error appropriately.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
} catch (error) {
throw new Error(`Could not read keypair from file at '${filepath}'`);
}
// Parse contents of file
let parsedFileContents;
try {
parsedFileContents = Uint8Array.from(JSON.parse(fileContents));
} catch (thrownObject) {
const error: any = thrownObject;
if (!error.message.includes("Unexpected token")) {
throw error;
}
throw new Error(`Invalid secret key file at '${filepath}'!`);
console.error("Transaction failed:", error);
}
} catch (error) {
console.error("Transaction failed:", error);
throw error;
}

Comment on lines +103 to +118
if (this.solanaAdapter) {
const { blockhash, lastValidBlockHeight } =
await connection.getLatestBlockhash();
const messageLegacy = new TransactionMessage({
instructions: tx.instructions,
payerKey: this.solanaAdapter.publicKey!,
recentBlockhash: blockhash,
}).compileToV0Message();

console.log("Transaction signature:", txSignature);
} catch (error) {
console.error("Transaction failed:", error);
}
};
const versionedTransaction = new VersionedTransaction(messageLegacy);

const getKeypairFromFile = async (filepath: string) => {
const path = await import("path");
if (filepath[0] === "~") {
const home = process.env.HOME || null;
if (home) {
filepath = path.join(home, filepath.slice(1));
txSignature = await this.solanaAdapter.sendTransaction(
versionedTransaction,
connection
);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Utilize lastValidBlockHeight when sending the transaction.

You retrieve lastValidBlockHeight but do not use it. Including it in the send options ensures that the transaction is valid within the expected block height range and can prevent transactions from failing due to blockhash expiration.

Apply this change to include lastValidBlockHeight:

  txSignature = await this.solanaAdapter.sendTransaction(
    versionedTransaction,
    connection,
+   {
+     minContextSlot: lastValidBlockHeight,
+   }
  );

This ensures that the transaction remains valid and provides additional safeguards against blockhash-related issues.

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +46 to +49
provider = new anchor.AnchorProvider(
connection,
walletAdapter as any,
anchor.AnchorProvider.defaultOptions()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid using the any type when casting walletAdapter.

Casting walletAdapter to any defeats TypeScript's type safety. It's better to define a proper interface for walletAdapter to ensure type safety and catch potential errors at compile time.

You can define an interface and use it instead of any:

+ interface WalletAdapter {
+   publicKey: anchor.web3.PublicKey;
+   signAllTransactions: (txs: Transaction[]) => Promise<Transaction[]>;
+   signTransaction: (tx: Transaction) => Promise<Transaction>;
+ }
  
  provider = new anchor.AnchorProvider(
    connection,
-   walletAdapter as any,
+   walletAdapter as WalletAdapter,
    anchor.AnchorProvider.defaultOptions()
  );

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +156 to +158
public isSolanaWalletConnected(): boolean {
return this.solanaAdapter?.connected || this.solanaWallet !== undefined;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure consistent return type in isSolanaWalletConnected

The method currently returns undefined or boolean due to the use of optional chaining. This can lead to unexpected behavior when the result is used in conditional statements.

Modify the method to ensure it always returns a boolean:

 public isSolanaWalletConnected(): boolean {
-  return this.solanaAdapter?.connected || this.solanaWallet !== undefined;
+  return (
+    (this.solanaAdapter ? this.solanaAdapter.connected : false) ||
+    (this.solanaWallet !== undefined)
+  );
 }

This change guarantees that the method returns true or false, avoiding potential issues with undefined.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public isSolanaWalletConnected(): boolean {
return this.solanaAdapter?.connected || this.solanaWallet !== undefined;
}
public isSolanaWalletConnected(): boolean {
return (
(this.solanaAdapter ? this.solanaAdapter.connected : false) ||
(this.solanaWallet !== undefined)
);
}

Comment on lines +133 to +136
} else if (params.solanaWallet) {
this.solanaWallet = params.solanaWallet;
} else if (params.solanaAdapter) {
this.solanaAdapter = params.solanaAdapter;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add checks for multiple wallet parameters in the constructor

Currently, the constructor checks for both wallet and signer being provided but doesn't handle cases where multiple Solana wallet parameters are supplied together. To prevent conflicting wallet parameters, ensure that only one wallet type is provided at a time.

You can update the constructor to include these checks:

 if (params.wallet && params.signer) {
   throw new Error("You can only provide a wallet or a signer, not both.");
+} else if (
+  [params.wallet, params.signer, params.solanaWallet, params.solanaAdapter].filter(Boolean).length > 1
+) {
+  throw new Error("You can only provide one wallet parameter.");
 } else if (params.wallet) {
   this.wallet = params.wallet;
 } else if (params.signer) {
   this.signer = params.signer;
 } else if (params.solanaWallet) {
   this.solanaWallet = params.solanaWallet;
 } else if (params.solanaAdapter) {
   this.solanaAdapter = params.solanaAdapter;
 }

This ensures that if more than one wallet parameter is provided, an error is thrown.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
} else if (params.solanaWallet) {
this.solanaWallet = params.solanaWallet;
} else if (params.solanaAdapter) {
this.solanaAdapter = params.solanaAdapter;
if (params.wallet && params.signer) {
throw new Error("You can only provide a wallet or a signer, not both.");
} else if (
[params.wallet, params.signer, params.solanaWallet, params.solanaAdapter].filter(Boolean).length > 1
) {
throw new Error("You can only provide one wallet parameter.");
} else if (params.wallet) {
this.wallet = params.wallet;
} else if (params.signer) {
this.signer = params.signer;
} else if (params.solanaWallet) {
this.solanaWallet = params.solanaWallet;
} else if (params.solanaAdapter) {
this.solanaAdapter = params.solanaAdapter;

@lukema95
Copy link
Collaborator Author

For Cli:

npx hardhat solana-deposit --amount 0.1 --recipient 0x1b02C3079c039DF9E20DC68427cb87bf821BffeF --types '["address", "bytes", "bool"]' 0x05ba149a7bd6dc1f937fa9046a9e05c05f3b18b0 0x4955a3F38ff86ae92A914445099caa8eA2B9bA32 true

For Browser:
I wrote a front-end test case here.
image
Final call result: 4CX34eMBpKcSPSjWfcuVa8whPsG1bk7vtrnN5PqvgrM5AZQa9UCTNF6gzKx5KuYfetNSwbhMRw3Gg69Q31Wis3Q7
Note: If you use the Phantom Wallet, you need to set it to developer mode.

@lukema95
Copy link
Collaborator Author

@fadeev pls review

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
packages/client/src/solanaDeposit.ts (1)

Line range hint 12-19: Consider splitting browser and programmatic wallet handling.

The function currently handles both browser wallet adapter and programmatic wallet scenarios, which increases complexity. Consider:

  1. Splitting into separate functions for better separation of concerns
  2. Adding input validation for amount and recipient
  3. Using TypeScript types for params instead of any[]
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 126bed5 and 8d133a0.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (3)
  • package.json (1 hunks)
  • packages/client/src/solanaDeposit.ts (3 hunks)
  • packages/tasks/src/solanaDeposit.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • package.json
🔇 Additional comments (10)
packages/tasks/src/solanaDeposit.ts (4)

40-41: LGTM! Well-documented optional parameters with sensible defaults.

The task parameters are properly documented and include reasonable default values:

  • solanaNetwork defaults to "devnet"
  • idPath defaults to the standard Solana CLI config location

48-50: Use 'os.homedir()' for cross-platform home directory resolution

Using process.env.HOME may not be reliable across all operating systems (e.g., it may not be defined on Windows).


54-72: LGTM! Excellent error handling practices.

The error handling is well-implemented:

  • Specific error messages for file reading failures
  • Proper JSON parsing error detection
  • Clear error messages that help with troubleshooting

14-20: LGTM! API parameter has been removed as requested.

The changes look good:

  • Proper wallet initialization using keypair
  • Simplified parameter structure
  • Removed API parameter as suggested in the previous review

Let's verify that all API parameters have been removed from the codebase:

Also applies to: 32-34

✅ Verification successful

API parameter has been successfully removed from solanaDeposit task

The verification confirms that:

  • The solanaDeposit.ts task file no longer uses any API parameters
  • The remaining API references found in client files (packages/client/src/solanaDeposit.ts and packages/client/src/getBalances.ts) are part of the internal client implementation and not related to the task's parameter structure
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining API parameters in Solana-related code
rg -i 'solana.*api|api.*solana' 

Length of output: 225


Script:

#!/bin/bash
# Let's check the context of these API usages to understand if they're related to our changes
rg -B 5 -A 5 'const api = getEndpoints\("solana"' packages/client/src/solanaDeposit.ts

# Also check the other file
rg -B 5 -A 5 'API = this.getEndpoint\("solana"' packages/client/src/getBalances.ts

# Let's also verify if there are any references to these endpoints in our task file
rg -l "getEndpoints|getEndpoint" packages/tasks/src/solanaDeposit.ts

Length of output: 877


Script:

#!/bin/bash
# Let's check the actual implementation in our task file to confirm it's not using any API endpoints
cat packages/tasks/src/solanaDeposit.ts

# Also check if there are any other files in the tasks directory that might be using these endpoints
rg -l "getEndpoints|getEndpoint" packages/tasks/

Length of output: 2561

packages/client/src/solanaDeposit.ts (6)

20-22: LGTM! Good defensive programming practice.

The wallet connection check prevents undefined behavior by failing fast when prerequisites aren't met.


31-47: LGTM! Well-structured wallet adapter with proper error handling.

The implementation properly handles different wallet capabilities with clear error messages.


49-52: Avoid using any type when casting walletAdapter.

This is a type safety issue that was previously identified.


105-120: Utilize lastValidBlockHeight in transaction options.

The lastValidBlockHeight is retrieved but not used in the transaction options.


95-97: LGTM! Proper null checking on public key access.

The conditional access to public key with null checking is well implemented.


131-134: Properly handle errors to prevent unexpected behavior.

The function should not silently fail and potentially return undefined.

Comment on lines +24 to +25
const network = "solana_" + this.network;
const api = getEndpoints("solana" as any, network);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove unsafe type assertion in getEndpoints call.

The as any type assertion bypasses TypeScript's type checking and could hide potential errors.

-const api = getEndpoints("solana" as any, network);
+const api = getEndpoints("solana", network);

Consider updating the getEndpoints function's type definition if needed.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const network = "solana_" + this.network;
const api = getEndpoints("solana" as any, network);
const network = "solana_" + this.network;
const api = getEndpoints("solana", network);

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
packages/client/src/client.ts (1)

39-68: Consider refactoring the type definition for better maintainability.

The discriminated union type effectively prevents multiple wallet configurations, but consider extracting common properties to a base interface for better maintainability:

+interface WalletConfig {
+  signer?: never;
+  solanaAdapter?: never;
+  solanaWallet?: never;
+  wallet?: never;
+}

+interface SignerConfig extends WalletConfig {
+  signer: Signer;
+}

+interface SolanaAdapterConfig extends WalletConfig {
+  solanaAdapter: WalletContextState;
+}

// ... similar interfaces for other configs

export type ZetaChainClientParams = ZetaChainClientParamsBase &
  (SignerConfig | SolanaAdapterConfig | /* other configs */);
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 8d133a0 and 2d29b88.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (2)
  • package.json (1 hunks)
  • packages/client/src/client.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • package.json
🔇 Additional comments (4)
packages/client/src/client.ts (4)

76-77: LGTM! Properties are well-typed and consistent.

The new Solana wallet properties follow the established pattern and are properly typed.


158-162: LGTM! Well-implemented public key retrieval.

The method properly handles null cases and uses appropriate nullish coalescing for fallbacks.


131-134: ⚠️ Potential issue

Add validation for multiple wallet parameters.

The constructor should validate that only one wallet parameter is provided at a time.

As suggested in the previous review, add comprehensive validation:

 if (params.wallet && params.signer) {
   throw new Error("You can only provide a wallet or a signer, not both.");
+} else if (
+  [params.wallet, params.signer, params.solanaWallet, params.solanaAdapter].filter(Boolean).length > 1
+) {
+  throw new Error("You can only provide one wallet parameter.");
 } else if (params.wallet) {

154-156: ⚠️ Potential issue

Ensure consistent boolean return type.

The method should always return a boolean value, not undefined.

As suggested in the previous review:

 public isSolanaWalletConnected(): boolean {
-  return this.solanaAdapter?.connected || this.solanaWallet !== undefined;
+  return (
+    (this.solanaAdapter ? this.solanaAdapter.connected : false) ||
+    (this.solanaWallet !== undefined)
+  );
 }

@fadeev fadeev self-requested a review November 29, 2024 13:36
@fadeev fadeev merged commit 856177b into zeta-chain:main Nov 29, 2024
3 checks passed
@coderabbitai coderabbitai bot mentioned this pull request Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants