Skip to content

Conversation

@fadeev
Copy link
Member

@fadeev fadeev commented Oct 3, 2024

  • Renamed "Revert" contract to "Echo", because it's no longer just a revert contract
  • Added echoCall, helloCall, helloWithdrawAndCall
  • withdrawAndCall
  • Single deployment task for both contracts

Depends on zeta-chain/localnet#45

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced the Echo contract for enhanced interaction with the ZetaChain and EVM.
    • Added new Hardhat tasks (echo-call, hello-call, hello-withdraw-and-call) for calling contracts on both EVM and ZetaChain.
    • Updated README to provide clearer examples for deploying and interacting with contracts, including detailed command usage.
  • Bug Fixes

    • Improved deployment script to clarify gateway address usage and ensure successful execution.
  • Chores

    • Updated dependencies in the project configuration for better compatibility.
    • Removed deprecated deployment task to streamline processes.

@fadeev fadeev requested a review from andresaiello as a code owner October 3, 2024 09:13
@fadeev fadeev marked this pull request as draft October 3, 2024 09:13
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 3, 2024

📝 Walkthrough
## Walkthrough
The changes in this pull request involve significant updates to the `examples/hello` directory, including a restructuring of the README file to focus on a new "Hello Example" format. A new `Echo` contract is introduced, which integrates with the `GatewayEVM` and includes a function for external calls. Additionally, new Hardhat tasks for interacting with contracts on EVM and ZetaChain are created, while some existing tasks are modified or removed. The `package.json` files are updated to include new dependencies and adjust deployment scripts.

## Changes

| File Path                                | Change Summary                                                                                                                |
|------------------------------------------|------------------------------------------------------------------------------------------------------------------------------|
| examples/hello/README.md                 | Restructured to focus on "Hello Example"; added command examples for EVM and ZetaChain interactions.                         |
| examples/hello/contracts/Echo.sol        | Introduced `Echo` contract with `GatewayEVM`, added `hello`, `onRevert`, and `call` functions, and emitted events.           |
| examples/hello/contracts/Hello.sol       | Changed `gateway` variable visibility from `public` to `public immutable`; added `withdrawAndCall` function and error type.  |
| examples/hello/hardhat.config.ts         | Modified imports: removed `deployRevert` and `gatewayCall`, added `helloCall`, `echoCall`, and `helloWithdrawAndCall`.     |
| examples/hello/package.json               | Updated `deploy` script to specify contract name `Echo` and associated gateway address; updated dependency versions.          |
| examples/hello/tasks/echoCall.ts         | Added new Hardhat task `echo-call` for invoking functions on deployed contracts with various parameters.                     |
| examples/hello/tasks/helloCall.ts        | Updated task name from `gateway-call` to `hello-call`; modified logic for parameter validation and function calls.           |
| examples/hello/tasks/helloWithdrawAndCall.ts | Introduced new task `hello-withdraw-and-call` for withdrawing ZRC-20 tokens and calling specified functions.               |

## Possibly related PRs
- **#200**: The addition of the `gatewayCall` function in the `Hello` contract directly relates to the changes made in the main PR, which also involves enhancing interactions with the ZetaChain gateway.

## Suggested reviewers
- andresaiello

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 87c868b and 16391e6.

⛔ Files ignored due to path filters (1)
  • examples/hello/yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (1)
  • examples/hello/package.json (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • examples/hello/package.json

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.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

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

🧹 Outside diff range and nitpick comments (9)
examples/hello/tasks/deploy.ts (1)

34-34: Parameter description update is informative, consider minor enhancement.

The updated description "Gateway address (default: ZetaChain Gateway)" provides valuable context about the default value, enhancing the task's documentation and usability.

Consider further improving the description by including the default address:

-    "Gateway address (default: ZetaChain Gateway)",
+    "Gateway address (default: ZetaChain Gateway at 0xA51c1fc2f0D1a1b8494Ed1FE312d7C3a78Ed91C0)",

This addition would provide users with immediate visibility of the default address without needing to inspect the code.

examples/hello/README.md (4)

1-5: Enhance the introductory section with context and proper formatting.

The title effectively communicates the purpose of the example. However, consider the following improvements:

  1. Add a brief introductory paragraph explaining the purpose of this example and what users can expect to learn.
  2. Provide context for the yarn deploy command, explaining what it does and any prerequisites.
  3. Add a language specifier to the code block for proper syntax highlighting.

Apply this diff to implement these suggestions:

 # Hello Example

+This example demonstrates how to interact with contracts on both EVM and ZetaChain networks. It provides a simple "Hello" use case to illustrate cross-chain communication.
+
+## Deployment
+
+To deploy the necessary contracts, run the following command:
+
-```
+```bash
 yarn deploy

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 Markdownlint</summary><blockquote>

3-3: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

</blockquote></details>

</details>

---

`7-24`: **Enhance the EVM section with improved formatting and explanations.**

The EVM section provides valuable examples for different scenarios. To improve clarity and usability, consider the following suggestions:

1. Add brief explanations before each example to clarify the expected outcome.
2. Include language specifiers in all code blocks for proper syntax highlighting.
3. Consider adding placeholders or comments for values that users might need to replace.


Apply this diff to implement these suggestions:

```diff
 ## EVM

-Successful call:
+### Successful call
+This example demonstrates a successful call to the contract:

-```
+```bash
 npx hardhat call-from-evm --contract 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E --receiver 0x67d269191c92Caf3cD7723F116c85e6E9bf55933 --network localhost --types '["string"]' hello

-Failed call:
+### Failed call
+This example shows a failed call due to incorrect parameter type:

- +bash
npx hardhat call-from-evm --contract 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E --receiver 0x67d269191c92Caf3cD7723F116c85e6E9bf55933 --network localhost --types '["uint256"]' 42


-Failed call with handled revert:
+### Failed call with handled revert
+This example demonstrates how to handle a revert in the contract:

-```
+```bash
npx hardhat call-from-evm --contract 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E --receiver 0x67d269191c92Caf3cD7723F116c85e6E9bf55933 --network localhost --revert-address 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E --revert-message 0x --call-on-revert --types '["uint256"]' 42

+Note: Replace the contract and receiver addresses with your actual deployed contract addresses.


<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 Markdownlint</summary><blockquote>

11-11: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

17-17: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

23-23: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

</blockquote></details>

</details>

---

`27-45`: **Enhance the ZetaChain section with improved formatting and explanations.**

The ZetaChain section follows a similar structure to the EVM section, which is excellent for consistency. To further improve clarity and usability, consider the following suggestions:

1. Add brief explanations before each example to clarify the expected outcome.
2. Include language specifiers in all code blocks for proper syntax highlighting.
3. Consider adding placeholders or comments for values that users might need to replace.
4. Provide a brief explanation of the ZRC20 token and its role in these transactions.


Apply this diff to implement these suggestions:

```diff
 ## ZetaChain

-Successful call:
+### Successful call
+This example demonstrates a successful call to the contract on ZetaChain:

-```
+```bash
 npx hardhat call-from-zetachain --contract 0x67d269191c92Caf3cD7723F116c85e6E9bf55933 --receiver 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E --zrc20 0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe --function "hello(string)" --network localhost --types '["string"]' hello

-Failed call:
+### Failed call
+This example shows a failed call due to incorrect parameter type:

- +bash
npx hardhat call-from-zetachain --contract 0x67d269191c92Caf3cD7723F116c85e6E9bf55933 --receiver 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E --zrc20 0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe --function "hello(string)" --network localhost --types '["uint256"]' 42


-Failed call with handled revert:
+### Failed call with handled revert
+This example demonstrates how to handle a revert in the contract on ZetaChain:

-```
+```bash
npx hardhat call-from-zetachain --contract 0x67d269191c92Caf3cD7723F116c85e6E9bf55933 --receiver 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E --zrc20 0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe --function "hello(string)" --network localhost --revert-address 0x67d269191c92Caf3cD7723F116c85e6E9bf55933 --revert-message 0x --call-on-revert  --types '["uint256"]' 42

+Note: Replace the contract, receiver, and ZRC20 addresses with your actual deployed contract and token addresses. The ZRC20 token is a standard for fungible tokens on ZetaChain, similar to ERC20 on Ethereum.


<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 Markdownlint</summary><blockquote>

31-31: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

37-37: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

43-43: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

</blockquote></details>

</details>

---

`45-45`: **Add a conclusion section to summarize key points and provide next steps.**

The document effectively demonstrates interactions with both EVM and ZetaChain networks. To further enhance its educational value, consider adding a conclusion section that:

1. Summarizes the key differences between EVM and ZetaChain interactions.
2. Highlights the importance of proper error handling in cross-chain communications.
3. Provides next steps or resources for users to learn more about ZetaChain development.


Add the following content at the end of the document:

```markdown
## Conclusion

This example demonstrates the similarities and differences between interacting with contracts on EVM-compatible chains and ZetaChain. Key takeaways include:

1. The command structure is similar for both EVM and ZetaChain interactions, with some ZetaChain-specific parameters (e.g., ZRC20 token address).
2. Proper error handling is crucial in cross-chain communications, as demonstrated by the "Failed call with handled revert" examples.
3. ZetaChain introduces the concept of ZRC20 tokens, which are analogous to ERC20 tokens on Ethereum.

To continue your journey with ZetaChain development:

- Explore the [ZetaChain documentation](https://www.zetachain.com/docs/) for more in-depth information.
- Join the [ZetaChain Discord community](https://discord.gg/zetachain) to connect with other developers and get support.
- Check out more examples and tutorials in the [ZetaChain GitHub repository](https://github.com/zeta-chain/zetachain).
examples/swap/package.json (1)

62-62: Address minor formatting issue: Add newline at end of file.

The file is missing a newline character at the end. While this doesn't affect functionality, it's a common best practice to include one.

Apply this change to resolve the issue:

 }
+
examples/hello/tasks/callFromZetachain.ts (2)

81-82: Approval: Improved task naming and description.

The modifications to the task name and description are appropriate and enhance clarity. The new name "call-from-zetachain" is more descriptive and accurately represents the task's functionality.

Consider further refining the description for optimal clarity:

-  "Calls the gatewayCall function on a contract on ZetaChain",
+  "Executes the gatewayCall function on a specified contract deployed on ZetaChain",

This minor adjustment emphasizes that the contract is user-specified and already deployed on ZetaChain.


123-123: Approval: New parameter enhances task flexibility.

The addition of the "name" parameter is a judicious enhancement, allowing users to specify the target contract dynamically. This change harmonizes with the modified contract instantiation logic.

Consider refining the parameter description for improved clarity:

-  .addParam("name", "The name of the contract", "Hello")
+  .addParam("name", "The name of the contract to be called (default: 'Hello')")

This adjustment explicitly states that it's the name of the contract to be called and indicates the default value in the description.

examples/hello/tasks/callFromEVM.ts (1)

66-66: Correct the typo in the parameter description.

The description contains a duplicated word:

  • Original: "Gas limit for for a cross-chain call"
  • Corrected: "Gas limit for a cross-chain call"
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 2f1b130 and cc57caa.

📒 Files selected for processing (9)
  • examples/hello/README.md (1 hunks)
  • examples/hello/contracts/Echo.sol (2 hunks)
  • examples/hello/hardhat.config.ts (1 hunks)
  • examples/hello/package.json (2 hunks)
  • examples/hello/tasks/callFromEVM.ts (1 hunks)
  • examples/hello/tasks/callFromZetachain.ts (3 hunks)
  • examples/hello/tasks/deploy.ts (2 hunks)
  • examples/hello/tasks/deployRevert.ts (0 hunks)
  • examples/swap/package.json (1 hunks)
💤 Files with no reviewable changes (1)
  • examples/hello/tasks/deployRevert.ts
🧰 Additional context used
🪛 Markdownlint
examples/hello/README.md

3-3: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


11-11: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


17-17: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


23-23: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


31-31: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


37-37: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


43-43: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🪛 GitHub Check: Slither
examples/hello/contracts/Echo.sol

[warning] 8-8: State variables that could be declared immutable
Echo.gateway (contracts/Echo.sol#8) should be immutable

🔇 Additional comments (4)
examples/hello/tasks/deploy.ts (1)

15-15: Deployment argument update is correct and consistent.

The modification to use args.gateway instead of args.gatewayZetaChain aligns with the parameter renaming in the task definition, ensuring the correct argument is passed to the contract deployment.

examples/hello/README.md (1)

1-45: Approve changes with suggested improvements.

The restructured README provides a comprehensive guide for the "Hello Example" project, demonstrating interactions with both EVM and ZetaChain networks. The content is well-organized and covers various scenarios, including successful calls, failed calls, and error handling.

With the suggested improvements implemented, including:

  1. Enhanced introductory section with context
  2. Improved formatting and explanations for both EVM and ZetaChain sections
  3. Addition of a conclusion section

This document will serve as an excellent resource for developers looking to understand the similarities and differences between EVM and ZetaChain interactions.

🧰 Tools
🪛 Markdownlint

3-3: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


11-11: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


17-17: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


23-23: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


31-31: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


37-37: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


43-43: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

examples/hello/package.json (1)

10-10: 🛠️ Refactor suggestion

Enhance deploy script with explicit contract deployment and gateway specification.

The modifications to the deploy script introduce more precise control over the deployment process:

  1. The script now explicitly specifies the contract name as Echo.
  2. A gateway address 0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0 has been added to the deployment parameters.

These changes align with the PR objective "Call from EVM" and provide a more targeted deployment approach. However, there are a few considerations:

  1. The hardcoded gateway address may limit flexibility across different environments.
  2. The script combines multiple operations, which may complicate debugging if issues arise.

Consider the following improvements:

  1. Utilize environment variables for the gateway address to enhance flexibility:

    "deploy": "npx hardhat compile --force && npx hardhat deploy --network localhost && npx hardhat deploy --name Echo --network localhost --gateway $GATEWAY_ADDRESS"
  2. Split the script into separate commands for better granularity and easier debugging:

    "scripts": {
      "compile": "npx hardhat compile --force",
      "deploy:base": "npx hardhat deploy --network localhost",
      "deploy:echo": "npx hardhat deploy --name Echo --network localhost --gateway $GATEWAY_ADDRESS",
      "deploy:all": "npm run compile && npm run deploy:base && npm run deploy:echo"
    }

These suggestions aim to improve maintainability and flexibility while preserving the intended functionality.

To ensure the gateway address is valid and accessible, you may want to run the following verification:

This script will help identify other occurrences of the gateway address in the codebase and verify its format.

✅ Verification successful

Gateway address is correctly formatted and properly used in the deploy script.

The gateway address 0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0 is correctly formatted as a valid Ethereum address and is exclusively used within the deploy script. No issues were found regarding its usage.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the gateway address exists in the codebase and is accessible

# Search for the gateway address in the codebase
echo "Searching for gateway address usage:"
rg --type-add 'config:*.{json,js,ts}' --type config '0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0'

# Check if the address is a valid Ethereum address
if [[ "0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0" =~ ^0x[a-fA-F0-9]{40}$ ]]; then
    echo "The gateway address is a valid Ethereum address format."
else
    echo "Warning: The gateway address does not conform to the standard Ethereum address format."
fi

Length of output: 595

examples/hello/tasks/callFromZetachain.ts (1)

63-63: Approval: Enhanced flexibility in contract instantiation.

The modification to use args.name for contract instantiation is a commendable improvement. It augments the task's versatility, allowing interaction with various contracts without code alterations.

import { task, types } from "hardhat/config";
import type { HardhatRuntimeEnvironment } from "hardhat/types";

const main = async (args: any, hre: HardhatRuntimeEnvironment) => {
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

Define explicit types for 'args' to enhance type safety.

The use of args: any weakens type safety and may lead to runtime errors. Defining a specific interface for args improves code reliability and maintainability.

@fadeev fadeev marked this pull request as ready for review October 4, 2024 08:12
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: 7

🧹 Outside diff range and nitpick comments (2)
examples/swap/tasks/deploy.ts (1)

41-43: Task parameter update approved with a minor suggestion.

The renaming of the parameter from "gatewayZetaChain" to "gateway" and the updated description enhance the clarity of the code. This change aligns well with the modification in the deployment logic.

To further improve clarity, consider updating the description to:

"Gateway contract address (default: ZetaChain Gateway)"

This minor adjustment explicitly states that the parameter expects a contract address.

examples/hello/contracts/Hello.sol (1)

Line range hint 1-52: Consider optimizing gatewayCall and commendation on Solidity version.

  1. Optimization suggestion for gatewayCall:
    The current implementation of gatewayCall approves the exact amount of tokens needed for each call. While this is secure, it may lead to higher gas costs for frequent calls. Consider implementing an approval strategy that grants a larger allowance to the gateway, reducing the number of approval transactions. However, ensure to implement proper allowance management to maintain security.

  2. Commendation on Solidity version:
    The use of Solidity version 0.8.26 is commendable. This recent version includes important safety features and optimizations, contributing to the overall security and efficiency of the contract.

Would you like assistance in implementing an optimized approval strategy for the gatewayCall function?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between cc57caa and e223861.

📒 Files selected for processing (5)
  • examples/hello/contracts/Echo.sol (2 hunks)
  • examples/hello/contracts/Hello.sol (1 hunks)
  • examples/hello/tasks/callFromEVM.ts (1 hunks)
  • examples/hello/tasks/callFromZetachain.ts (5 hunks)
  • examples/swap/tasks/deploy.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • examples/hello/contracts/Echo.sol
🔇 Additional comments (7)
examples/swap/tasks/deploy.ts (1)

17-17: Deployment logic update approved.

The modification to use args.gateway instead of args.gatewayZetaChain in the contract deployment is consistent with the parameter renaming. This change maintains coherence throughout the file and improves code clarity.

examples/hello/contracts/Hello.sol (1)

10-10: Excellent enhancement to the gateway variable declaration.

The addition of the immutable keyword to the gateway variable declaration is a commendable improvement. This modification ensures that the gateway address can only be set once during contract construction, thereby enhancing the contract's security and immutability. It effectively prevents any accidental or malicious modifications to the gateway address post-deployment, aligning with best practices for declaring critical contract dependencies.

examples/hello/tasks/callFromEVM.ts (3)

31-49: Value processing and encoding implementation is correct.

The implementation for processing and encoding values based on their types is well-structured and handles different scenarios appropriately. It correctly parses boolean values, converts numeric types to BigNumber, and uses the default ABI coder for encoding. This approach ensures proper handling of various parameter types.


51-63: Contract interaction implementation is correct and follows best practices.

The code correctly obtains the contract factory, attaches it to the specified address, and connects it to the signer. The gatewayCall function is invoked with the properly prepared parameters, including the receiver address, encoded parameters, revert options, and transaction options. The transaction hash is logged, and the code waits for the transaction to be confirmed before logging a success message. This implementation follows best practices for contract interaction.


66-101: Task definition is comprehensive and well-structured.

The task definition for "call-from-evm" is thorough and provides a wide range of options to customize the gateway call. It includes essential parameters such as the contract address and receiver, as well as optional parameters for gas prices, gas limits, and revert options. The use of addOptionalParam and addFlag functions allows for flexible invocation of the gateway call. The task definition is well-aligned with the implementation in the main function.

examples/hello/tasks/callFromZetachain.ts (2)

87-88: Task renaming enhances clarity

Renaming the task to "call-from-zetachain" and updating the description improves the clarity of its purpose, making it more intuitive for users.


123-123: Ensure default contract name aligns with common usage

Adding the name parameter with a default value of "Hello" allows flexibility in specifying the contract. Verify that "Hello" is the most appropriate default for common use cases. If another contract is more frequently used, consider updating the default value accordingly.

@fadeev fadeev marked this pull request as draft October 4, 2024 08:49
Comment on lines +7 to +36
contract Echo {
GatewayEVM public immutable gateway;

event RevertEvent(string, RevertContext);
event HelloEvent(string, string);

constructor(address payable gatewayAddress) {
gateway = GatewayEVM(gatewayAddress);
}

function hello(string memory message) external payable {
emit HelloEvent("Hello on EVM", message);
}

function onRevert(RevertContext calldata revertContext) external {
emit RevertEvent("Revert on EVM", revertContext);
}

function call(
address receiver,
bytes calldata message,
RevertOptions memory revertOptions
) external {
gateway.call(receiver, message, revertOptions);
}

receive() external payable {}

fallback() external payable {}
}

Check warning

Code scanning / Slither

Contracts that lock Ether Medium

Contract locking ether found:
Contract Echo has payable functions:
- Echo.hello(string)
- Echo.receive()
- Echo.fallback()
But does not have a function to withdraw the ether
Comment on lines 45 to 69

Check warning

Code scanning / Slither

Unused return Medium

Comment on lines 45 to 69

Check warning

Code scanning / Slither

Unused return Medium

@fadeev fadeev marked this pull request as ready for review October 8, 2024 09:17
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: 18

🧹 Outside diff range and nitpick comments (10)
examples/hello/README.md (4)

1-5: Enhance code block readability with language specification.

The introduction and deployment instructions are concise and clear. However, to improve readability and adhere to Markdown best practices, please specify the language for the code block.

Apply this change to the deployment code block:

-```
+```bash
 yarn deploy

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 Markdownlint</summary><blockquote>

3-3: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

</blockquote></details>

</details>

---

`7-24`: **Enhance EVM section readability and formatting.**

The EVM section provides comprehensive examples for both successful and failed calls, which is commendable. To further improve this section:

1. Add language specifiers to all code blocks for syntax highlighting.
2. Consider breaking long commands into multiple lines for improved readability.


Apply these changes to the EVM section:

```diff
 ## EVM

 Successful call:

-```
+```bash
-npx hardhat echo-call --contract 0xc3e53F4d16Ae77Db1c982e75a937B9f60FE63690 --receiver 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E --network localhost --types '["string"]' hello
+npx hardhat echo-call \
+  --contract 0xc3e53F4d16Ae77Db1c982e75a937B9f60FE63690 \
+  --receiver 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E \
+  --network localhost \
+  --types '["string"]' \
+  hello

Failed call:

- +bash
-npx hardhat echo-call --contract 0xc3e53F4d16Ae77Db1c982e75a937B9f60FE63690 --receiver 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E --network localhost --types '["uint256"]' 42
+npx hardhat echo-call \

  • --contract 0xc3e53F4d16Ae77Db1c982e75a937B9f60FE63690 \
  • --receiver 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E \
  • --network localhost \
  • --types '["uint256"]' \
  • 42

Failed call with handled revert:

-```
+```bash
-npx hardhat echo-call --contract 0xc3e53F4d16Ae77Db1c982e75a937B9f60FE63690 --receiver 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E --network localhost --revert-address 0xc3e53F4d16Ae77Db1c982e75a937B9f60FE63690 --revert-message 0x --call-on-revert --types '["uint256"]' 42
+npx hardhat echo-call \
+  --contract 0xc3e53F4d16Ae77Db1c982e75a937B9f60FE63690 \
+  --receiver 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E \
+  --network localhost \
+  --revert-address 0xc3e53F4d16Ae77Db1c982e75a937B9f60FE63690 \
+  --revert-message 0x \
+  --call-on-revert \
+  --types '["uint256"]' \
+  42

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 Markdownlint</summary><blockquote>

11-11: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

17-17: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

23-23: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

</blockquote></details>

</details>

---

`27-45`: **Enhance ZetaChain section readability and formatting.**

The ZetaChain section maintains consistency with the EVM section, which is excellent for user comprehension. To further improve this section:

1. Add language specifiers to all code blocks for syntax highlighting.
2. Break long commands into multiple lines for improved readability.


Apply these changes to the ZetaChain section:

```diff
 ## ZetaChain

 Successful call:

-```
+```bash
-npx hardhat hello-call --contract 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E --receiver 0xc3e53F4d16Ae77Db1c982e75a937B9f60FE63690 --zrc20 0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe --function "hello(string)" --network localhost --types '["string"]' hello
+npx hardhat hello-call \
+  --contract 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E \
+  --receiver 0xc3e53F4d16Ae77Db1c982e75a937B9f60FE63690 \
+  --zrc20 0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe \
+  --function "hello(string)" \
+  --network localhost \
+  --types '["string"]' \
+  hello

Failed call:

- +bash
-npx hardhat hello-call --contract 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E --receiver 0xc3e53F4d16Ae77Db1c982e75a937B9f60FE63690 --zrc20 0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe --function "hello(string)" --network localhost --types '["uint256"]' 42
+npx hardhat hello-call \

  • --contract 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E \
  • --receiver 0xc3e53F4d16Ae77Db1c982e75a937B9f60FE63690 \
  • --zrc20 0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe \
  • --function "hello(string)" \
  • --network localhost \
  • --types '["uint256"]' \
  • 42

Failed call with handled revert:

-```
+```bash
-npx hardhat hello-call --contract 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E --receiver 0xc3e53F4d16Ae77Db1c982e75a937B9f60FE63690 --zrc20 0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe --function "hello(string)" --network localhost --revert-address 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E --revert-message 0x --call-on-revert  --types '["uint256"]' 42
+npx hardhat hello-call \
+  --contract 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E \
+  --receiver 0xc3e53F4d16Ae77Db1c982e75a937B9f60FE63690 \
+  --zrc20 0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe \
+  --function "hello(string)" \
+  --network localhost \
+  --revert-address 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E \
+  --revert-message 0x \
+  --call-on-revert \
+  --types '["uint256"]' \
+  42

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 Markdownlint</summary><blockquote>

31-31: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

37-37: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

43-43: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

</blockquote></details>

</details>

---

`1-45`: **Enhance README with introduction and setup instructions.**

The README provides comprehensive examples for both EVM and ZetaChain interactions, which is commendable. However, to improve its usefulness, especially for new users, consider adding the following sections:

1. A brief introduction explaining the purpose of the "Hello Example" project.
2. Prerequisites or setup instructions required before running the provided commands.


Consider adding the following sections at the beginning of the README:

```markdown
# Hello Example

This project demonstrates how to interact with smart contracts on both EVM-compatible chains and ZetaChain using Hardhat tasks.

## Prerequisites

Before running the commands in this README, ensure you have the following installed:

- Node.js (version X.X.X or higher)
- Yarn (version X.X.X or higher)
- Hardhat (version X.X.X or higher)

## Setup

1. Clone this repository
2. Run `yarn install` to install dependencies
3. Configure your `.env` file with necessary environment variables (if applicable)

## Deployment

Please adjust the versions and setup instructions according to your project's specific requirements.

🧰 Tools
🪛 Markdownlint

3-3: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


11-11: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


17-17: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


23-23: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


31-31: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


37-37: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


43-43: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

examples/hello/contracts/Hello.sol (1)

41-41: Suggestion: Handle or explicitly ignore unused return value.

The withdrawGasFeeWithGasLimit function call returns two values, but only the second one (gasFee) is used. To improve code clarity and avoid potential issues, it's recommended to either use or explicitly ignore all returned values.

Apply this diff to address the issue:

- (, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(gasLimit);
+ (address returnedGasZRC20, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(gasLimit);
+ assert(returnedGasZRC20 == zrc20);

This change captures both return values and adds an assertion to ensure that the returned gas ZRC20 address matches the expected one. If you're certain that this check is unnecessary, you can use the following alternative:

- (, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(gasLimit);
+ (/* address gasZRC20 */, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(gasLimit);

This explicitly ignores the first return value, making it clear that it's intentionally unused.

Also applies to: 56-57

examples/hello/tasks/echoCall.ts (2)

46-49: Add error handling for parameter encoding.

Encoding parameters using ethers.utils.defaultAbiCoder.encode may fail if there's a mismatch between types and values. Including a try-catch block ensures that encoding errors are handled gracefully.

Wrap the encoding process:

-const encodedParameters = ethers.utils.defaultAbiCoder.encode(
-  types,
-  valuesArray
-);
+let encodedParameters: string;
+try {
+  encodedParameters = ethers.utils.defaultAbiCoder.encode(types, valuesArray);
+} catch (error) {
+  throw new Error(`Error encoding parameters: ${error.message}`);
+}

62-63: Handle errors when waiting for transaction confirmation.

Waiting for the transaction to be mined may fail due to network issues or transaction reversion. Including error handling ensures that such cases are managed.

Wrap tx.wait() in a try-catch block:

-await tx.wait();
+try {
+  await tx.wait();
+  console.log("gatewayCall executed successfully");
+} catch (error) {
+  throw new Error(`Transaction confirmation failed: ${error.message}`);
+}
examples/hello/tasks/helloCall.ts (1)

119-119: Consider making name an optional parameter

Since a default value "Hello" is provided for name, it would be appropriate to use .addOptionalParam instead of .addParam to reflect that name is optional.

Apply this diff to adjust the parameter definition:

-.addParam("name", "The name of the contract", "Hello")
+.addOptionalParam("name", "The name of the contract", "Hello")
examples/hello/tasks/helloWithdrawAndCall.ts (2)

15-15: Consider removing unused 'abortAddress' from 'revertOptions'

The abortAddress is set to "0x0000000000000000000000000000000000000000" and marked as not used. To improve code clarity, consider removing it from the revertOptions object if it's unnecessary.

Apply this diff to remove the unused property:

 const revertOptions = {
-  abortAddress: "0x0000000000000000000000000000000000000000", // not used
   callOnRevert: args.callOnRevert,
   onRevertGasLimit: args.onRevertGasLimit,
   revertAddress: args.revertAddress,
   revertMessage: ethers.utils.hexlify(
     ethers.utils.toUtf8Bytes(args.revertMessage)
   ),
 };

137-139: Clarify parameter descriptions and examples for better usability

The descriptions for the types and values parameters could be more detailed to aid users in providing correct input.

Enhance the parameter descriptions:

 .addParam("amount", "Amount of ZRC-20 to withdraw")
-.addParam("types", `The types of the parameters (example: '["string"]')`)
-.addVariadicPositionalParam("values", "The values of the parameters");
+.addParam("types", `JSON array of parameter types (e.g., '["string","uint256"]')`)
+.addVariadicPositionalParam("values", "Values for the parameters, matching the types provided");
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 923f041 and 87c868b.

⛔ Files ignored due to path filters (1)
  • examples/hello/yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (8)
  • examples/hello/README.md (1 hunks)
  • examples/hello/contracts/Echo.sol (1 hunks)
  • examples/hello/contracts/Hello.sol (2 hunks)
  • examples/hello/hardhat.config.ts (1 hunks)
  • examples/hello/package.json (2 hunks)
  • examples/hello/tasks/echoCall.ts (1 hunks)
  • examples/hello/tasks/helloCall.ts (4 hunks)
  • examples/hello/tasks/helloWithdrawAndCall.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • examples/hello/hardhat.config.ts
  • examples/hello/package.json
🧰 Additional context used
🪛 Markdownlint
examples/hello/README.md

3-3: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


11-11: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


17-17: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


23-23: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


31-31: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


37-37: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


43-43: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🪛 GitHub Check: Slither
examples/hello/contracts/Echo.sol

[warning] 7-36: Contracts that lock Ether
Contract locking ether found:
Contract Echo (contracts/Echo.sol#7-36) has payable functions:
- Echo.hello(string) (contracts/Echo.sol#17-19)
- Echo.receive() (contracts/Echo.sol#33)
- Echo.fallback() (contracts/Echo.sol#35)
But does not have a function to withdraw the ether

examples/hello/contracts/Hello.sol

[warning] 34-46: Unused return
Hello.call(bytes,address,bytes,uint256,RevertOptions) (contracts/Hello.sol#34-46) ignores return value by IZRC20(zrc20).approve(address(gateway),gasFee) (contracts/Hello.sol#44)


[warning] 34-46: Unused return
Hello.call(bytes,address,bytes,uint256,RevertOptions) (contracts/Hello.sol#34-46) ignores return value by (None,gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(gasLimit) (contracts/Hello.sol#41)


[warning] 48-80: Unused return
Hello.withdrawAndCall(bytes,uint256,address,bytes,uint256,RevertOptions) (contracts/Hello.sol#48-80) ignores return value by IZRC20(gasZRC20).approve(address(gateway),gasFee) (contracts/Hello.sol#70)


[warning] 48-80: Unused return
Hello.withdrawAndCall(bytes,uint256,address,bytes,uint256,RevertOptions) (contracts/Hello.sol#48-80) ignores return value by IZRC20(zrc20).approve(address(gateway),target) (contracts/Hello.sol#61)

🔇 Additional comments (7)
examples/hello/contracts/Echo.sol (4)

1-7: Contract declaration and imports are appropriate.

The contract uses a recent and secure Solidity version (0.8.26). The imports from "@zetachain/protocol-contracts" are relevant to the contract's functionality. The contract name "Echo" is concise and descriptive.


8-11: State variables and events are well-defined.

The gateway variable is correctly declared as immutable, addressing the previous gas optimization suggestion. The events RevertEvent and HelloEvent are appropriately defined for logging contract actions.


13-15: Constructor correctly initializes the gateway.

The constructor properly initializes the gateway variable with the provided gatewayAddress.


21-23: The onRevert function handles revert contexts appropriately.

This function correctly emits a RevertEvent with the provided revert context.

examples/hello/contracts/Hello.sol (2)

10-10: Approval: Immutable gateway variable.

The change of the gateway variable to public immutable is a commendable improvement. This modification ensures that the gateway address cannot be altered post-deployment, enhancing the contract's security and gas efficiency.


14-14: Approval: Addition of TransferFailed error.

The introduction of the TransferFailed error is a positive addition. It provides a specific, gas-efficient way to handle transfer failures, improving error reporting and debugging capabilities.

examples/hello/tasks/helloCall.ts (1)

Line range hint 68-75: Verify that the contract implements the call method

Ensure that the contract specified by args.name actually implements the call method being invoked. This prevents runtime errors due to undefined method calls.

Run the following script to verify that the call method exists in the contract:

Comment on lines +17 to +19
function hello(string memory message) external payable {
emit HelloEvent("Hello on EVM", message);
}
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

Consider the implications of the payable hello function.

The hello function is marked as payable but doesn't use the sent Ether. This could lead to locked Ether in the contract.

Consider one of the following options:

  1. Remove the payable keyword if Ether is not needed.
  2. Implement a withdrawal mechanism for the received Ether.
  3. Document the intended use of the received Ether if it's by design.

Comment on lines +25 to +31
function call(
address receiver,
bytes calldata message,
RevertOptions memory revertOptions
) external {
gateway.call(receiver, message, revertOptions);
}
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

The call function allows external calls to the gateway.

This function correctly forwards the call to the gateway. However, consider adding access control to prevent unauthorized usage.

Consider implementing the Ownable pattern from OpenZeppelin and adding the onlyOwner modifier to this function:

import "@openzeppelin/contracts/access/Ownable.sol";

contract Echo is Ownable {
    // ...

    function call(
        address receiver,
        bytes calldata message,
        RevertOptions memory revertOptions
    ) external onlyOwner {
        gateway.call(receiver, message, revertOptions);
    }

    // ...
}

Comment on lines +33 to +35
receive() external payable {}

fallback() external payable {}
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

Implement withdrawal mechanism for received Ether.

The receive() and fallback() functions allow the contract to receive Ether, but there's no mechanism to withdraw it. This could lead to permanently locked Ether in the contract.

Consider implementing a withdrawal function:

function withdraw() external onlyOwner {
    uint256 balance = address(this).balance;
    payable(owner()).transfer(balance);
}

Also, document the intended use of received Ether in the contract.

Comment on lines +48 to +80
function withdrawAndCall(
bytes memory receiver,
uint256 amount,
address zrc20,
bytes calldata message,
uint256 gasLimit,
RevertOptions memory revertOptions
) external {
(address gasZRC20, uint256 gasFee) = IZRC20(zrc20)
.withdrawGasFeeWithGasLimit(gasLimit);
uint256 target = zrc20 == gasZRC20 ? amount + gasFee : amount;
if (!IZRC20(zrc20).transferFrom(msg.sender, address(this), target))
revert TransferFailed();
IZRC20(zrc20).approve(address(gateway), target);
if (zrc20 != gasZRC20) {
if (
!IZRC20(gasZRC20).transferFrom(
msg.sender,
address(this),
gasFee
)
) revert TransferFailed();
IZRC20(gasZRC20).approve(address(gateway), gasFee);
}
gateway.withdrawAndCall(
receiver,
amount,
zrc20,
message,
gasLimit,
revertOptions
);
}
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

⚠️ Potential issue

Approval with suggestions: New withdrawAndCall function.

The addition of the withdrawAndCall function is a valuable enhancement, providing a convenient way to perform withdrawals and calls in a single transaction. The implementation includes proper checks for transfer success and handles different ZRC20 scenarios effectively.

However, there are opportunities for further improvement:

  1. The return values of the approve function calls are not checked. Consider handling the approval results to ensure the operations' success.

Apply these diffs to address the issues:

- IZRC20(zrc20).approve(address(gateway), target);
+ if (!IZRC20(zrc20).approve(address(gateway), target))
+     revert TransferFailed();

- IZRC20(gasZRC20).approve(address(gateway), gasFee);
+ if (!IZRC20(gasZRC20).approve(address(gateway), gasFee))
+     revert TransferFailed();

These changes ensure that all approval operations are successful before proceeding with the gateway call.

  1. Consider extracting the common logic for transferring and approving into a separate internal function to reduce code duplication and improve maintainability.
function _transferAndApprove(address token, address from, uint256 amount) internal {
    if (!IZRC20(token).transferFrom(from, address(this), amount))
        revert TransferFailed();
    if (!IZRC20(token).approve(address(gateway), amount))
        revert TransferFailed();
}

This refactored function can then be used in both call and withdrawAndCall, reducing duplication and improving the contract's overall structure.

🧰 Tools
🪛 GitHub Check: Slither

[warning] 48-80: Unused return
Hello.withdrawAndCall(bytes,uint256,address,bytes,uint256,RevertOptions) (contracts/Hello.sol#48-80) ignores return value by IZRC20(gasZRC20).approve(address(gateway),gasFee) (contracts/Hello.sol#70)


[warning] 48-80: Unused return
Hello.withdrawAndCall(bytes,uint256,address,bytes,uint256,RevertOptions) (contracts/Hello.sol#48-80) ignores return value by IZRC20(zrc20).approve(address(gateway),target) (contracts/Hello.sol#61)

Comment on lines +34 to 46
function call(
bytes memory receiver,
address zrc20,
bytes calldata message,
uint256 gasLimit,
RevertOptions memory revertOptions
) external {
(, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(gasLimit);
if (!IZRC20(zrc20).transferFrom(msg.sender, address(this), gasFee))
revert TransferFailed();
IZRC20(zrc20).approve(address(gateway), gasFee);
gateway.call(receiver, zrc20, message, gasLimit, revertOptions);
}
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

Approval with suggestion: Improved call function with transfer check.

The renaming of gatewayCall to call and the addition of a transfer success check are commendable improvements. The use of the custom TransferFailed error enhances gas efficiency and error reporting.

However, there's an opportunity for further improvement:

The return value of the approve function call is not checked. Consider handling the approval result to ensure the operation's success.

Apply this diff to address the issue:

- IZRC20(zrc20).approve(address(gateway), gasFee);
+ if (!IZRC20(zrc20).approve(address(gateway), gasFee))
+     revert TransferFailed();

This change ensures that the approval operation is successful before proceeding with the gateway call.

📝 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
function call(
bytes memory receiver,
address zrc20,
bytes calldata message,
uint256 gasLimit,
RevertOptions memory revertOptions
) external {
(, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(gasLimit);
if (!IZRC20(zrc20).transferFrom(msg.sender, address(this), gasFee))
revert TransferFailed();
IZRC20(zrc20).approve(address(gateway), gasFee);
gateway.call(receiver, zrc20, message, gasLimit, revertOptions);
}
function call(
bytes memory receiver,
address zrc20,
bytes calldata message,
uint256 gasLimit,
RevertOptions memory revertOptions
) external {
(, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(gasLimit);
if (!IZRC20(zrc20).transferFrom(msg.sender, address(this), gasFee))
revert TransferFailed();
if (!IZRC20(zrc20).approve(address(gateway), gasFee))
revert TransferFailed();
gateway.call(receiver, zrc20, message, gasLimit, revertOptions);
}
🧰 Tools
🪛 GitHub Check: Slither

[warning] 34-46: Unused return
Hello.call(bytes,address,bytes,uint256,RevertOptions) (contracts/Hello.sol#34-46) ignores return value by IZRC20(zrc20).approve(address(gateway),gasFee) (contracts/Hello.sol#44)


[warning] 34-46: Unused return
Hello.call(bytes,address,bytes,uint256,RevertOptions) (contracts/Hello.sol#34-46) ignores return value by (None,gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(gasLimit) (contracts/Hello.sol#41)


const functionSignature = ethers.utils.id(args.function).slice(0, 10);

const types = JSON.parse(args.types);
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

Avoid shadowing imported 'types' by renaming the local variable

The variable types imported from "hardhat/config" is being shadowed by the local types variable declared on line 26. This can lead to confusion and potential bugs.

Rename the local types variable to something like paramTypes:

- const types = JSON.parse(args.types);
+ const paramTypes = JSON.parse(args.types);

Update all subsequent references to use paramTypes instead of types.

📝 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 types = JSON.parse(args.types);
const paramTypes = JSON.parse(args.types);

Comment on lines +84 to +93
const tx = await contract.withdrawAndCall(
ethers.utils.hexlify(args.receiver),
amount,
args.zrc20,
message,
gasLimit,
revertOptions,
txOptions
);

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 robust error handling for the 'withdrawAndCall' transaction

The withdrawAndCall function call may fail due to various reasons. To prevent the script from crashing without informative feedback, wrap the call in a try-catch block.

Implement error capture:

- const tx = await contract.withdrawAndCall(
-   ethers.utils.hexlify(args.receiver),
-   amount,
-   args.zrc20,
-   message,
-   gasLimit,
-   revertOptions,
-   txOptions
- );
- console.log(`Transaction hash: ${tx.hash}`);
- await tx.wait();
+ try {
+   const tx = await contract.withdrawAndCall(
+     ethers.utils.hexlify(args.receiver),
+     amount,
+     args.zrc20,
+     message,
+     gasLimit,
+     revertOptions,
+     txOptions
+   );
+   console.log(`Transaction hash: ${tx.hash}`);
+   await tx.wait();
+   console.log("gatewayCall executed successfully");
+ } catch (error) {
+   throw new Error(`withdrawAndCall failed: ${error.message}`);
+ }
📝 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 tx = await contract.withdrawAndCall(
ethers.utils.hexlify(args.receiver),
amount,
args.zrc20,
message,
gasLimit,
revertOptions,
txOptions
);
try {
const tx = await contract.withdrawAndCall(
ethers.utils.hexlify(args.receiver),
amount,
args.zrc20,
message,
gasLimit,
revertOptions,
txOptions
);
console.log(`Transaction hash: ${tx.hash}`);
await tx.wait();
console.log("gatewayCall executed successfully");
} catch (error) {
throw new Error(`withdrawAndCall failed: ${error.message}`);
}

Comment on lines +28 to +32
if (types.length !== args.values.length) {
throw new Error(
`The number of types (${types.length}) does not match the number of values (${args.values.length}).`
);
}
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

Add validation for 'paramTypes' to ensure it's an array

After parsing args.types, it's prudent to verify that paramTypes is indeed an array to prevent runtime errors.

Include an array check:

 const paramTypes = JSON.parse(args.types);
+ if (!Array.isArray(paramTypes)) {
+   throw new Error("The 'types' parameter should be a JSON array.");
+ }

Committable suggestion was skipped due to low confidence.

Comment on lines +62 to +80
const zrc20 = new ethers.Contract(args.zrc20, ZRC20ABI.abi, signer);
const [gasZRC20, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit);
const gasZRC20Contract = new ethers.Contract(gasZRC20, ZRC20ABI.abi, signer);
const gasFeeApprove = await gasZRC20Contract.approve(
args.contract,
gasZRC20 == args.zrc20 ? gasFee.add(amount) : gasFee,
txOptions
);
await gasFeeApprove.wait();

if (gasZRC20 !== args.zrc20) {
const targetTokenApprove = await zrc20.approve(
args.contract,
gasFee.add(amount),
txOptions
);
await targetTokenApprove.wait();
}

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

Include error handling for contract interactions

When calling contract methods like withdrawGasFeeWithGasLimit, approve, and wait, exceptions can occur. Wrap these calls in try-catch blocks to provide informative error messages.

Modify the code to handle exceptions:

- const [gasZRC20, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit);
+ let gasZRC20, gasFee;
+ try {
+   [gasZRC20, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit);
+ } catch (error) {
+   throw new Error(`Error fetching gas fee: ${error.message}`);
+ }

...

- const gasFeeApprove = await gasZRC20Contract.approve(
-   args.contract,
-   gasZRC20 == args.zrc20 ? gasFee.add(amount) : gasFee,
-   txOptions
- );
- await gasFeeApprove.wait();
+ try {
+   const gasFeeApprove = await gasZRC20Contract.approve(
+     args.contract,
+     gasZRC20 === args.zrc20 ? gasFee.add(amount) : gasFee,
+     txOptions
+   );
+   await gasFeeApprove.wait();
+ } catch (error) {
+   throw new Error(`Error approving gas fee: ${error.message}`);
+ }

...

- await targetTokenApprove.wait();
+ } catch (error) {
+   throw new Error(`Error approving target token: ${error.message}`);
+ }
📝 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 zrc20 = new ethers.Contract(args.zrc20, ZRC20ABI.abi, signer);
const [gasZRC20, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit);
const gasZRC20Contract = new ethers.Contract(gasZRC20, ZRC20ABI.abi, signer);
const gasFeeApprove = await gasZRC20Contract.approve(
args.contract,
gasZRC20 == args.zrc20 ? gasFee.add(amount) : gasFee,
txOptions
);
await gasFeeApprove.wait();
if (gasZRC20 !== args.zrc20) {
const targetTokenApprove = await zrc20.approve(
args.contract,
gasFee.add(amount),
txOptions
);
await targetTokenApprove.wait();
}
const zrc20 = new ethers.Contract(args.zrc20, ZRC20ABI.abi, signer);
let gasZRC20, gasFee;
try {
[gasZRC20, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit);
} catch (error) {
throw new Error(`Error fetching gas fee: ${error.message}`);
}
const gasZRC20Contract = new ethers.Contract(gasZRC20, ZRC20ABI.abi, signer);
try {
const gasFeeApprove = await gasZRC20Contract.approve(
args.contract,
gasZRC20 === args.zrc20 ? gasFee.add(amount) : gasFee,
txOptions
);
await gasFeeApprove.wait();
} catch (error) {
throw new Error(`Error approving gas fee: ${error.message}`);
}
if (gasZRC20 !== args.zrc20) {
try {
const targetTokenApprove = await zrc20.approve(
args.contract,
gasFee.add(amount),
txOptions
);
await targetTokenApprove.wait();
} catch (error) {
throw new Error(`Error approving target token: ${error.message}`);
}
}

Comment on lines +34 to +48
const valuesArray = args.values.map((value: any, index: number) => {
const type = types[index];

if (type === "bool") {
try {
return JSON.parse(value.toLowerCase());
} catch (e) {
throw new Error(`Invalid boolean value: ${value}`);
}
} else if (type.startsWith("uint") || type.startsWith("int")) {
return ethers.BigNumber.from(value);
} else {
return value;
}
});
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

Enhance type handling and input validation for parameters

The current implementation might not handle all possible Solidity types and lacks comprehensive input validation.

Improve the parameter processing logic:

 const valuesArray = args.values.map((value: any, index: number) => {
   const type = paramTypes[index];

   if (type === "bool") {
     const lowerValue = value.toLowerCase();
     if (lowerValue === "true" || lowerValue === "false") {
       return lowerValue === "true";
     } else {
       throw new Error(`Invalid boolean value: ${value}`);
     }
   } else if (type.startsWith("uint") || type.startsWith("int")) {
     if (!/^-?\d+$/.test(value)) {
       throw new Error(`Invalid numeric value: ${value}`);
     }
     return ethers.BigNumber.from(value);
   } else if (type === "address") {
     if (!ethers.utils.isAddress(value)) {
       throw new Error(`Invalid address: ${value}`);
     }
     return value;
   } else if (type === "string") {
     return value;
   } else if (type.endsWith("[]")) {
     // Handle array types if necessary
     // ...
     throw new Error(`Array types are not supported yet: ${type}`);
   } else {
     throw new Error(`Unsupported type: ${type}`);
   }
 });
📝 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 valuesArray = args.values.map((value: any, index: number) => {
const type = types[index];
if (type === "bool") {
try {
return JSON.parse(value.toLowerCase());
} catch (e) {
throw new Error(`Invalid boolean value: ${value}`);
}
} else if (type.startsWith("uint") || type.startsWith("int")) {
return ethers.BigNumber.from(value);
} else {
return value;
}
});
const valuesArray = args.values.map((value: any, index: number) => {
const type = paramTypes[index];
if (type === "bool") {
const lowerValue = value.toLowerCase();
if (lowerValue === "true" || lowerValue === "false") {
return lowerValue === "true";
} else {
throw new Error(`Invalid boolean value: ${value}`);
}
} else if (type.startsWith("uint") || type.startsWith("int")) {
if (!/^-?\d+$/.test(value)) {
throw new Error(`Invalid numeric value: ${value}`);
}
return ethers.BigNumber.from(value);
} else if (type === "address") {
if (!ethers.utils.isAddress(value)) {
throw new Error(`Invalid address: ${value}`);
}
return value;
} else if (type === "string") {
return value;
} else if (type.endsWith("[]")) {
// Handle array types if necessary
// ...
throw new Error(`Array types are not supported yet: ${type}`);
} else {
throw new Error(`Unsupported type: ${type}`);
}
});

@fadeev
Copy link
Member Author

fadeev commented Oct 8, 2024

@andresaiello please, review.

@fadeev fadeev merged commit 893d2bf into main Oct 16, 2024
7 checks passed
@fadeev fadeev deleted the call-from-evm branch October 16, 2024 06:24
@coderabbitai coderabbitai bot mentioned this pull request Oct 16, 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.

3 participants