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

Refactor: Gateway functions to get Gateway address dynamically #205

Merged
merged 4 commits into from
Dec 26, 2024

Conversation

lukema95
Copy link
Collaborator

@lukema95 lukema95 commented Dec 20, 2024

issue: #201

Update:

  1. Make the gateway contract parameter optional in the task command line.
  2. Extend ZetaChainClient to support reading gateway contract addresses from the contract configuration file. Add a 'contracts' field to store the contract information read from the file.
  3. If no gateway parameter is provided, ZetaChainClient will use the Gateway contract address from the configuration file. For localhost/localnet environments, the gateway parameter must be passed either in the task command line or through the contracts field in ZetaChainClient.

Summary by CodeRabbit

  • New Features

    • Enhanced flexibility in various functions by making the gatewayEvm and gatewayZetaChain parameters optional.
    • Introduced new interfaces for address management in the ZetaChainClient.
  • Bug Fixes

    • Improved error handling for gateway address retrieval across multiple functions.
  • Chores

    • Updated dependency versions in the project configuration to reflect recent changes.
  • Documentation

    • Updated task definitions to remove default values for certain parameters, requiring explicit user input.
    • Modified task configurations to dynamically use the current network context.

Copy link
Contributor

coderabbitai bot commented Dec 20, 2024

📝 Walkthrough

Walkthrough

This pull request introduces modifications to the ZetaChain toolkit, focusing on enhancing the flexibility of gateway address handling across multiple functions. The changes primarily involve making gateway addresses optional in various client and task functions, introducing a dynamic method to retrieve gateway addresses using getGatewayAddress(). Additionally, the package.json file has been updated to reflect changes in dependency versions.

Changes

File Change Summary
package.json Updated @zetachain/networks from 10.0.0 to v10.0.0-rc1, @zetachain/protocol-contracts remains at 11.0.0-rc3.
packages/client/src/client.ts Added getGatewayAddress() method, new address interfaces, updated client constructor to handle optional contracts.
packages/client/src/evmCall.ts, evmDeposit.ts, evmDepositAndCall.ts Made gatewayEvm parameter optional, updated logic to use getGatewayAddress().
packages/client/src/zetachainCall.ts, zetachainWithdraw.ts, zetachainWithdrawAndCall.ts Made gatewayZetaChain parameter optional, updated logic to use getGatewayAddress().
packages/tasks/src/* Updated tasks to dynamically use network from Hardhat runtime environment and removed default values for gatewayEvm and gatewayZetaChain.

Possibly related issues

Possibly related PRs

Suggested labels

dependencies

Suggested reviewers

  • fadeev
  • andresaiello

Tip

CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command @coderabbitai generate docstrings to have CodeRabbit automatically generate docstrings for your pull request. We would love to hear your feedback on Discord.


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 generate docstrings to generate docstrings for this PR. (Beta)
  • @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.

@lukema95
Copy link
Collaborator Author

@fadeev pls review

@fadeev
Copy link
Member

fadeev commented Dec 20, 2024

Looks good so far 👍

@fadeev
Copy link
Member

fadeev commented Dec 20, 2024

@coderabbitai review

Copy link
Contributor

coderabbitai bot commented Dec 20, 2024

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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: 4

🔭 Outside diff range comments (7)
packages/tasks/src/evmDepositAndCall.ts (1)

Line range hint 14-33: Add validation for optional gateway parameter

While the gateway parameter has been made optional in the task definition, there's no visible validation or fallback logic in the function implementation. This could lead to runtime errors if the gateway address is not provided.

Consider adding validation like this:

 const tx = await client.evmDepositAndCall({
   amount: args.amount,
   erc20: args.erc20,
-  gatewayEvm: args.gatewayEvm,
+  gatewayEvm: args.gatewayEvm ?? await client.getGatewayAddress(),
   receiver: args.receiver,
   revertOptions: {
packages/tasks/src/zetachainCall.ts (1)

Line range hint 20-37: Add validation for optional gateway parameter

Similar to evmDepositAndCall, the gateway parameter needs validation logic to handle cases where it's not provided.

Consider adding validation:

 const response = await client.zetachainCall({
   callOptions,
   function: args.function,
-  gatewayZetaChain: args.gatewayZetaChain,
+  gatewayZetaChain: args.gatewayZetaChain ?? await client.getGatewayAddress(),
   receiver: args.receiver,
   revertOptions: {
packages/tasks/src/zetachainWithdrawAndCall.ts (1)

Line range hint 20-37: Add validation for optional gateway parameter and enhance error handling

The withdraw and call operation is more complex and needs careful handling of the optional gateway parameter.

Consider these improvements:

 const response = await client.zetachainWithdrawAndCall({
   amount: args.amount,
   callOptions,
   function: args.function,
-  gatewayZetaChain: args.gatewayZetaChain,
+  gatewayZetaChain: args.gatewayZetaChain ?? await client.getGatewayAddress(),
   receiver: args.receiver,
   revertOptions: {

Also, consider adding specific error handling for gateway-related failures:

 } catch (e) {
-  console.error("Transaction error:", e);
+  if (e.message?.includes('gateway')) {
+    console.error("Gateway error: Please ensure gateway address is valid or provided in config");
+  } else {
+    console.error("Transaction error:", e);
+  }
 }
packages/client/src/evmDeposit.ts (1)

Line range hint 71-71: Critical: Fix ERC20 approval to use resolved gateway address

The ERC20 approval is still using args.gatewayEvm directly instead of the resolved gatewayEvmAddress. This could cause the approval to fail when the gateway address is provided through the fallback mechanism.

- await erc20Contract.connect(signer).approve(args.gatewayEvm, value);
+ await erc20Contract.connect(signer).approve(gatewayEvmAddress, value);
packages/client/src/zetachainWithdraw.ts (1)

Line range hint 82-83: Critical: Fix multiple gateway address usages in token approvals

There are multiple instances where the original args.gatewayZetaChain is used instead of the resolved gatewayZetaChainAddress. This could cause approvals to fail when the gateway address is provided through the fallback mechanism.

- args.gatewayZetaChain,
+ gatewayZetaChainAddress,

This fix needs to be applied to all three approve calls:

  1. Line 82: approveGasAndWithdraw
  2. Line 93: gasZRC20Contract approval
  3. Line 98: zrc20 approval

Also applies to: 93-94, 98-99

packages/client/src/zetachainCall.ts (1)

Line range hint 89-93: Critical: Update approve() to use gatewayZetaChainAddress

The approve() call still uses args.gatewayZetaChain directly, which could be undefined. This needs to be updated to use the gatewayZetaChainAddress variable.

Apply this fix:

  const approve = await gasZRC20Contract.approve(
-   args.gatewayZetaChain,
+   gatewayZetaChainAddress,
    gasFee,
    args.txOptions
  );
packages/client/src/evmDepositAndCall.ts (1)

Line range hint 95-95: Critical: Update approve() to use gatewayEvmAddress

Similar to zetachainCall.ts, the approve() call uses args.gatewayEvm directly, which could be undefined.

Apply this fix:

-   await erc20Contract.connect(signer).approve(args.gatewayEvm, value);
+   await erc20Contract.connect(signer).approve(gatewayEvmAddress, value);
🧹 Nitpick comments (3)
packages/tasks/src/zetachainWithdrawAndCall.ts (1)

Line range hint 1-93: Consider adding documentation for gateway configuration

Since the gateway parameters are now optional across multiple files, it would be helpful to add documentation about how the gateway addresses are resolved when not provided.

Consider adding a comment block at the top of each file explaining:

  • When gateway parameters are required vs optional
  • How default gateway addresses are resolved
  • Any environment-specific considerations (e.g., localhost requirements mentioned in PR objectives)
packages/client/src/evmCall.ts (1)

Line range hint 8-19: Update JSDoc to reflect optional parameter

The @param description for args.gatewayEvm should indicate that it's now optional and mention the fallback behavior using getGatewayAddress().

- * @param {string} args.gatewayEvm - The address of the EVM gateway contract.
+ * @param {string} [args.gatewayEvm] - The address of the EVM gateway contract. If not provided, falls back to the address from getGatewayAddress().
packages/client/src/zetachainWithdrawAndCall.ts (1)

37-37: Update JSDoc to reflect optional parameter

The gatewayZetaChain parameter is now optional, but the JSDoc comment still shows it as required. Please update the documentation to reflect this change.

- * @param {string} args.gatewayZetaChain - The address of the ZetaChain gateway contract.
+ * @param {string} [args.gatewayZetaChain] - The address of the ZetaChain gateway contract. If not provided, defaults to the address from configuration.
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5e088f9 and 27b099b.

⛔ Files ignored due to path filters (2)
  • typechain-types/factories/contracts/SwapHelperLib__factory.ts is excluded by !typechain-types/**
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (14)
  • package.json (1 hunks)
  • packages/client/src/client.ts (5 hunks)
  • packages/client/src/evmCall.ts (2 hunks)
  • packages/client/src/evmDeposit.ts (1 hunks)
  • packages/client/src/evmDepositAndCall.ts (2 hunks)
  • packages/client/src/zetachainCall.ts (2 hunks)
  • packages/client/src/zetachainWithdraw.ts (2 hunks)
  • packages/client/src/zetachainWithdrawAndCall.ts (2 hunks)
  • packages/tasks/src/evmCall.ts (2 hunks)
  • packages/tasks/src/evmDeposit.ts (2 hunks)
  • packages/tasks/src/evmDepositAndCall.ts (2 hunks)
  • packages/tasks/src/zetachainCall.ts (2 hunks)
  • packages/tasks/src/zetachainWithdraw.ts (2 hunks)
  • packages/tasks/src/zetachainWithdrawAndCall.ts (2 hunks)
🔇 Additional comments (20)
packages/client/src/client.ts (5)

5-6: Imports for Mainnet and Testnet Addresses Look Good

Bringing in distinct address configurations for mainnet and testnet from "@zetachain/protocol-contracts" helps you avoid scattering address data across the codebase. This modular approach is more maintainable.


36-36: Optional Contracts Parameter

Introducing the optional "contracts" property in the base interface effectively supports both local and mainnet/testnet configurations. Good addition for future expansions.


74-86: Clearly-Defined Interfaces for Localnet and Mainnet/Testnet

Defining separate interfaces "MainnetTestnetAddress" and "LocalnetAddress" simplifies type handling across different network environments.


95-95: Storing Contracts in a Private Property

Having a private property "contracts" confines the address management logic within the class, preventing accidental external modification.


157-165: Localnet Contracts Enforcement vs. Default Fallback

  1. Throwing an error when localnet or localhost is specified without providing contracts is a strong safeguard.
  2. Falling back to testnet or mainnet addresses otherwise is a good design for user convenience.

No issues found here.

packages/tasks/src/evmCall.ts (2)

9-10: Dynamically Using Network Name

Using "hre.network.name" to configure the network dynamically, rather than hardcoding "testnet", is more flexible and allows the script to work across multiple environments seamlessly.


36-36: Optional GatewayEvm Parameter

Removing the default address and making it optional clarifies execution requirements, forcing users to supply an explicit gateway address when needed. This is consistent with your new approach in other tasks.

packages/tasks/src/evmDeposit.ts (2)

9-10: ** Leveraging hre.network.name Improves Extensibility**

Same reasoning as evmCall: automatically picking the correct environment is a cleaner approach.


38-38: Optional GatewayEvm Parameter

In line with the new pattern, making "gatewayEvm" optional encourages explicit user configuration or a fallback logic handled in the ZetaChainClient.

packages/tasks/src/zetachainWithdraw.ts (2)

12-13: Dynamic Network Selection

Again, picking up "hre.network.name" to specify the network is consistent with your revised approach across tasks, ensuring maintainability and reusability.


41-41: Optional GatewayZetaChain Parameter

Allowing an optional "gatewayZetaChain" parameter makes sense given that you’re retrieving a default address inside the client when it’s not provided. This approach improves overall adaptability.

packages/tasks/src/evmDepositAndCall.ts (1)

12-13: LGTM: Network configuration change looks good

The dynamic network configuration using hre.network.name aligns with the PR objectives and is consistent with changes across other files.

packages/tasks/src/zetachainCall.ts (1)

17-18: LGTM: Network configuration change is consistent

The dynamic network configuration using hre.network.name is properly implemented and consistent with other files.

packages/tasks/src/zetachainWithdrawAndCall.ts (1)

17-18: LGTM: Network configuration change is consistent

The dynamic network configuration using hre.network.name is properly implemented and consistent with other files.

packages/client/src/evmCall.ts (1)

37-42: Verify gateway address resolution

The fallback logic looks good, but we should verify that getGatewayAddress() never returns undefined or invalid addresses.

✅ Verification successful

Gateway address resolution is properly validated

The implementation of getGatewayAddress() has proper validation:

  • For localnet/localhost, it validates the gateway address exists in the contracts configuration and throws an error if not found
  • For other networks (mainnet/testnet), it uses predefined addresses from mainnet/testnet configurations
  • The gateway address is required during client initialization, either through:
    • Explicit contracts configuration parameter
    • Built-in mainnet/testnet addresses
    • For localnet, contracts configuration is mandatory and throws if not provided

The fallback logic in the original code is safe as getGatewayAddress() will always return a valid address or throw an error if one cannot be found.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for getGatewayAddress implementation and its error handling
ast-grep --pattern 'getGatewayAddress() {
  $$$
}'

# Search for tests validating gateway address resolution
rg -l "getGatewayAddress.*test" -g "*.test.ts"

Length of output: 101


Script:

#!/bin/bash
# Search for getGatewayAddress method with more flexible pattern
ast-grep --pattern 'getGatewayAddress'

# Search for gateway address related tests with broader pattern
rg "getGatewayAddress" -g "*.test.ts" -A 5

# Search for gateway address validation or error handling
rg "gatewayEvmAddress|gatewayEvm" -g "*.ts" -A 3

Length of output: 6282


Script:

#!/bin/bash
# Search for the base class implementation to find getGatewayAddress
rg "class.*extends" -g "*.ts" -A 10

# Search for gateway address initialization or configuration
rg "constructor.*gateway" -g "*.ts" -A 10

# Look for any constant or configuration files that might contain gateway addresses
fd -e json -e ts -x rg -l "gateway.*address"

Length of output: 18851


Script:

#!/bin/bash
# Search for getGatewayAddress implementation in client.ts
rg "getGatewayAddress" ./packages/client/src/client.ts -A 5

# Look for gateway address initialization in client constructor
rg "constructor" ./packages/client/src/client.ts -A 10

# Search for any gateway address validation
rg "gatewayEvmAddress|getGatewayAddress" ./packages/client/src/client.ts -B 5 -A 5

Length of output: 1222


Script:

#!/bin/bash
# Search for the complete implementation of getGatewayAddress
rg "getGatewayAddress" ./packages/client/src/client.ts -B 5 -A 15

# Look for contract addresses configuration
rg "contracts.*=" ./packages/client/src/client.ts -A 5

# Search for LocalnetAddress type definition
rg "LocalnetAddress" -g "*.ts" -A 5

Length of output: 2416

packages/client/src/zetachainWithdraw.ts (1)

41-47: Verify consistent gateway address resolution

The gateway address resolution follows the same pattern as other files. Let's verify the consistency of gateway address handling across all files.

✅ Verification successful

Based on the search results, I can now generate the final response:

Gateway address resolution is consistent across the codebase

The codebase consistently follows the pattern of using an optional gateway address parameter with a fallback to getGatewayAddress(). This is implemented uniformly across all relevant files:

  • zetachainWithdraw.ts, zetachainWithdrawAndCall.ts, zetachainCall.ts: Uses args.gatewayZetaChain || this.getGatewayAddress()
  • evmDeposit.ts, evmDepositAndCall.ts, evmCall.ts: Uses args.gatewayEvm || this.getGatewayAddress()
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for the pattern of gateway address resolution
rg "gatewayZetaChainAddress.*=.*getGatewayAddress" -A 5

# Search for any remaining direct uses of gateway parameters
rg "args\.gateway(Evm|ZetaChain)" -g "*.ts"

Length of output: 2731

packages/client/src/zetachainCall.ts (1)

35-35: LGTM: Gateway parameter made optional with proper fallback

The implementation correctly makes the gateway parameter optional and provides a fallback using getGatewayAddress(), which aligns with the PR objectives.

Also applies to: 46-47, 49-49

packages/client/src/evmDepositAndCall.ts (1)

32-32: LGTM: Consistent implementation of optional gateway parameter

The implementation follows the same pattern as zetachainCall.ts, making the gateway parameter optional with proper fallback to getGatewayAddress().

Also applies to: 42-47

package.json (1)

103-104: Verify compatibility between RC versions

The update to @zetachain/networks v10.0.0-rc1 while using @zetachain/protocol-contracts 11.0.0-rc3 needs verification to ensure compatibility.

✅ Verification successful

No compatibility issues found between RC versions

Based on the analysis of the codebase:

  1. The package uses @zetachain/networks v10.0.0-rc1 and @zetachain/protocol-contracts 11.0.0-rc3 as direct dependencies
  2. The imports and usage patterns show standard integration with these packages through their stable interfaces (ABIs, types, and network configurations)
  3. Both packages are release candidates from consecutive major versions (10 and 11) which typically maintain compatibility during the RC phase
  4. The extensive usage across the codebase (client, tasks, templates) shows these versions are being actively used together
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any breaking changes or compatibility issues in the changelog/release notes

# Check for version compatibility mentions in README files
fd -t f "README.md" -X grep -l "compatibility"

# Look for version constraints or compatibility matrices in documentation
fd -t f -e md -e txt | xargs grep -l "version.*compatibility"

# Search for potential version-related issues in recent commits
git log --since="1 month ago" --grep="breaking\|compatibility\|version"

Length of output: 108


Script:

#!/bin/bash
# Let's check the dependencies in package.json and look for version patterns
cat package.json

# Check if there are any specific version requirements or constraints mentioned
fd -t f -e json -e yaml -e yml | xargs grep -l "@zetachain"

# Look for import statements or usage of these packages
rg "@zetachain/(networks|protocol-contracts)" -A 2

Length of output: 21067

packages/client/src/zetachainWithdrawAndCall.ts (1)

Line range hint 37-52: Verify impact on existing integrations

The gateway parameter is now optional, which could affect existing integrations that rely on this function. Please ensure that:

  1. All existing calls to this function are reviewed
  2. Integration tests cover both cases (with and without gateway parameter)
  3. Documentation is updated to reflect this change
✅ Verification successful

Optional gateway parameter change is safe and properly handled

The verification shows that the change is safe and well-integrated:

  1. The function is only used in two main contexts:

    • As part of the client API (packages/client/src/client.ts)
    • In a Hardhat task (packages/tasks/src/zetachainWithdrawAndCall.ts)
  2. The task implementation already handles the optional gateway parameter correctly, using it as an optional parameter in its interface.

  3. The client implementation maintains backward compatibility by falling back to this.getGatewayAddress() when the gateway parameter is not provided.

No breaking changes or integration issues were found. The optional parameter pattern is consistently implemented across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for direct calls to zetachainWithdrawAndCall
rg -A 5 "zetachainWithdrawAndCall" --type typescript

# Look for potential test files
fd -e test.ts -e spec.ts | xargs rg "zetachainWithdrawAndCall"

Length of output: 150


Script:

#!/bin/bash
# Search for direct calls to zetachainWithdrawAndCall using ripgrep
rg -A 5 "zetachainWithdrawAndCall"

# Look for test files and their content
find . -type f \( -name "*test*.ts" -o -name "*spec*.ts" \) -exec grep -l "zetachainWithdrawAndCall" {} \;

# Search for function definition and its usage pattern
ast-grep --pattern 'function zetachainWithdrawAndCall'

Length of output: 3618

packages/client/src/evmDeposit.ts Show resolved Hide resolved
@lukema95 lukema95 marked this pull request as ready for review December 24, 2024 06:10
@lukema95 lukema95 requested review from andresaiello, fadeev and a team as code owners December 24, 2024 06:10
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

🧹 Nitpick comments (1)
packages/client/src/evmDeposit.ts (1)

30-30: Update JSDoc to reflect optional gateway parameter

The gatewayEvm parameter is now optional in the function signature, but the JSDoc comment still describes it as required. Update the documentation to indicate that this parameter is optional and explain the fallback behavior when it's not provided.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 27b099b and 0d29d59.

📒 Files selected for processing (5)
  • packages/client/src/evmDeposit.ts (2 hunks)
  • packages/client/src/evmDepositAndCall.ts (3 hunks)
  • packages/client/src/zetachainCall.ts (3 hunks)
  • packages/client/src/zetachainWithdraw.ts (4 hunks)
  • packages/client/src/zetachainWithdrawAndCall.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • packages/client/src/zetachainCall.ts
  • packages/client/src/zetachainWithdraw.ts
  • packages/client/src/zetachainWithdrawAndCall.ts
  • packages/client/src/evmDepositAndCall.ts
🔇 Additional comments (3)
packages/client/src/evmDeposit.ts (3)

38-43: LGTM: Gateway address resolution looks good

The implementation correctly handles the optional gateway parameter by falling back to this.getGatewayAddress() when not provided. The gateway contract instantiation properly uses the resolved address.


69-69: Fixed: Consistent usage of resolved gateway address

The ERC20 approve call now correctly uses the resolved gatewayEvmAddress instead of args.gatewayEvm, addressing the inconsistency noted in previous reviews.


38-38: Verify getGatewayAddress implementation

Let's verify the implementation of getGatewayAddress() and ensure there are no remaining direct uses of args.gatewayEvm.

✅ Verification successful

Let me gather more information about the implementation of getGatewayAddress since it wasn't found in the first search.


Let me check one more thing to understand the network and contracts configuration.


Implementation of getGatewayAddress is properly implemented and used consistently

The verification shows that:

  • getGatewayAddress() is properly implemented in the client class, handling both localnet and non-localnet cases
  • The pattern args.gatewayEvm || this.getGatewayAddress() is consistently used across all relevant files (evmDeposit, evmCall, evmDepositAndCall)
  • The implementation follows a proper fallback pattern where custom gateway addresses can be provided, defaulting to the client's gateway address if not specified
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for getGatewayAddress implementation
ast-grep --pattern 'getGatewayAddress() {
  $$$
}'

# Check for any remaining direct uses of args.gatewayEvm
rg 'args\.gatewayEvm' --type ts

Length of output: 1037


Script:

#!/bin/bash
# Search for class that might contain getGatewayAddress
ast-grep --pattern 'class $_ {
  $$$
  getGatewayAddress$$$
  $$$
}'

# Also search for any potential interface definitions
ast-grep --pattern 'interface $_ {
  $$$
  getGatewayAddress$$$
  $$$
}'

# Search for any file containing getGatewayAddress with context
rg "getGatewayAddress" -A 5 --type ts

Length of output: 2995


Script:

#!/bin/bash
# Check for network and contracts configuration
rg "LocalnetAddress" -A 5 --type ts

# Look for contract addresses configuration
fd "addresses.ts|addresses.json" --type f --exec cat {}

Length of output: 1413

@fadeev
Copy link
Member

fadeev commented Dec 25, 2024

@lukema95 ready to merge once the conflicts are resolved.

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

♻️ Duplicate comments (1)
packages/client/src/client.ts (1)

193-208: ⚠️ Potential issue

Fix async usage in Array.find

The async callback in Array.find won't work as expected since Array.find doesn't await the promises.

This is a duplicate of a previous review comment. Please implement one of the suggested fixes:

  1. Get chainId first, then use it in a synchronous find
  2. Use a for-of loop with await
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0d29d59 and 7af8ced.

⛔ 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 (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • package.json
🔇 Additional comments (2)
packages/client/src/client.ts (2)

5-6: LGTM: Well-structured type definitions and imports

The new interfaces and imports provide a clear and type-safe way to handle different network configurations. The separation between MainnetTestnetAddress and LocalnetAddress interfaces appropriately models the different requirements for each environment.

Also applies to: 37-37, 75-87


158-166: LGTM: Clear initialization logic with proper error handling

The constructor properly handles contract initialization with appropriate error handling for localnet environments. The fallback to testnet/mainnet addresses is well-implemented.

Comment on lines +179 to +220
public getGatewayAddress(): string {
if (this.network === "localnet" || this.network === "localhost") {
const gateway = (this.contracts as LocalnetAddress[]).find(
(item) => item.type === "gatewayZEVM"
);

if (!gateway) {
throw new Error("Gateway address not found in localnet configuration");
}

return gateway.address;
} else {
let gateway;
if (this.wallet) {
try {
gateway = (this.contracts as MainnetTestnetAddress[]).find(
async (item) =>
(await this.wallet!.getChainId()) === item.chain_id &&
item.type === "gateway"
);
} catch (error) {
throw new Error("Failed to get gateway address: " + error);
}
} else {
try {
gateway = (this.contracts as MainnetTestnetAddress[]).find(
async (item) =>
(await this.signer!.getChainId()) === item.chain_id &&
item.type === "gateway"
);
} catch (error) {
throw new Error("Failed to get gateway address: " + error);
}
}

if (!gateway) {
throw new Error(`Gateway address not found in signer or wallet`);
}

return gateway.address;
}
}
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

Refactor to reduce duplication and improve error handling

The current implementation has duplicate try-catch blocks and could be simplified.

Consider this refactored version:

 public getGatewayAddress(): string {
   if (this.network === "localnet" || this.network === "localhost") {
     const gateway = (this.contracts as LocalnetAddress[]).find(
       (item) => item.type === "gatewayZEVM"
     );

     if (!gateway) {
       throw new Error("Gateway address not found in localnet configuration");
     }

     return gateway.address;
   }
   
-  let gateway;
-  if (this.wallet) {
-    try {
-      gateway = (this.contracts as MainnetTestnetAddress[]).find(
-        async (item) =>
-          (await this.wallet!.getChainId()) === item.chain_id &&
-          item.type === "gateway"
-      );
-    } catch (error) {
-      throw new Error("Failed to get gateway address: " + error);
-    }
-  } else {
-    try {
-      gateway = (this.contracts as MainnetTestnetAddress[]).find(
-        async (item) =>
-          (await this.signer!.getChainId()) === item.chain_id &&
-          item.type === "gateway"
-      );
-    } catch (error) {
-      throw new Error("Failed to get gateway address: " + error);
-    }
-  }
+  try {
+    const provider = this.wallet || this.signer;
+    if (!provider) {
+      throw new Error("No wallet or signer available");
+    }
+    
+    const chainId = await provider.getChainId();
+    const gateway = (this.contracts as MainnetTestnetAddress[]).find(
+      (item) => item.chain_id === chainId && item.type === "gateway"
+    );
+    
+    if (!gateway) {
+      throw new Error(`Gateway address not found for chain ID ${chainId}`);
+    }
+    
+    return gateway.address;
+  } catch (error) {
+    throw new Error(`Failed to get gateway address: ${error}`);
+  }
-  if (!gateway) {
-    throw new Error(`Gateway address not found in signer or wallet`);
-  }
-
-  return gateway.address;
 }

This refactored version:

  1. Eliminates duplicate code by using a common provider
  2. Fixes the async/await issue in Array.find
  3. Provides more specific error messages
  4. Has better null checking
📝 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 getGatewayAddress(): string {
if (this.network === "localnet" || this.network === "localhost") {
const gateway = (this.contracts as LocalnetAddress[]).find(
(item) => item.type === "gatewayZEVM"
);
if (!gateway) {
throw new Error("Gateway address not found in localnet configuration");
}
return gateway.address;
} else {
let gateway;
if (this.wallet) {
try {
gateway = (this.contracts as MainnetTestnetAddress[]).find(
async (item) =>
(await this.wallet!.getChainId()) === item.chain_id &&
item.type === "gateway"
);
} catch (error) {
throw new Error("Failed to get gateway address: " + error);
}
} else {
try {
gateway = (this.contracts as MainnetTestnetAddress[]).find(
async (item) =>
(await this.signer!.getChainId()) === item.chain_id &&
item.type === "gateway"
);
} catch (error) {
throw new Error("Failed to get gateway address: " + error);
}
}
if (!gateway) {
throw new Error(`Gateway address not found in signer or wallet`);
}
return gateway.address;
}
}
public getGatewayAddress(): string {
if (this.network === "localnet" || this.network === "localhost") {
const gateway = (this.contracts as LocalnetAddress[]).find(
(item) => item.type === "gatewayZEVM"
);
if (!gateway) {
throw new Error("Gateway address not found in localnet configuration");
}
return gateway.address;
}
try {
const provider = this.wallet || this.signer;
if (!provider) {
throw new Error("No wallet or signer available");
}
const chainId = await provider.getChainId();
const gateway = (this.contracts as MainnetTestnetAddress[]).find(
(item) => item.chain_id === chainId && item.type === "gateway"
);
if (!gateway) {
throw new Error(`Gateway address not found for chain ID ${chainId}`);
}
return gateway.address;
} catch (error) {
throw new Error(`Failed to get gateway address: ${error}`);
}
}

@lukema95
Copy link
Collaborator Author

@lukema95 ready to merge once the conflicts are resolved.

done

@fadeev fadeev merged commit c537397 into zeta-chain:main Dec 26, 2024
5 checks passed
@fadeev
Copy link
Member

fadeev commented Dec 26, 2024

@lukema95 I've made a pre-release with these changes to make it easier to test changes to the UniversalKit:

https://github.com/zeta-chain/toolkit/releases/tag/v13.0.0-rc9

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.

2 participants