Ping Pong example #204
Ping Pong example #204
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new event Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EchoContract as Echo
participant GatewayEVM
User->>Echo: call pingPong(receiver)
Echo->>GatewayEVM: call(receiver, contractAddress, RevertOptions)
GatewayEVM-->>Echo: response
Echo-->>User: return response
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
examples/hello/tasks/helloWithdrawAndCall.ts (1)
83-83: Consider enhancing the logging mechanism for improved debugging capabilities.The addition of this console log statement provides valuable visibility into the constructed message before it's passed to the contract method, which can be instrumental for debugging purposes. However, to maintain code quality and adhere to best practices, consider the following recommendations:
Implement a more robust logging mechanism that can be easily toggled between development and production environments. This approach allows for comprehensive debugging capabilities without compromising production code.
Utilize a logging library such as Winston or Pino, which offer more advanced features like log levels, timestamps, and structured logging. This would provide a more scalable and maintainable solution for debugging across your application.
If you decide to keep this log statement, ensure it's conditionally enabled only in non-production environments. This can be achieved by using environment variables or build flags.
Example implementation using environment variables:
if (process.env.NODE_ENV !== 'production') { console.log(`Debug - Message to be sent: ${message}`); }By implementing these suggestions, you can maintain the debugging benefits while ensuring your code remains production-ready and follows best practices for logging and error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- examples/hello/contracts/Echo.sol (2 hunks)
- examples/hello/contracts/Hello.sol (1 hunks)
- examples/hello/tasks/helloWithdrawAndCall.ts (1 hunks)
🧰 Additional context used
| @@ -9,6 +9,7 @@ contract Echo { | |||
|
|
|||
| event RevertEvent(string, RevertContext); | |||
| event HelloEvent(string, string); | |||
| event Foo(address); | |||
There was a problem hiding this comment.
Enhance event naming and documentation.
The event Foo lacks clarity and proper documentation. Consider the following improvements:
- Rename the event to a more descriptive name that reflects its purpose, e.g.,
ReceiverAddressLogged. - Add a NatSpec comment to explain when and why this event is emitted.
Proposed changes:
- event Foo(address);
+ /// @notice Emitted when a receiver address is logged
+ /// @param receiver The address of the receiver
+ event ReceiverAddressLogged(address indexed receiver);📝 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.
| event Foo(address); | |
| /// @notice Emitted when a receiver address is logged | |
| /// @param receiver The address of the receiver | |
| event ReceiverAddressLogged(address indexed receiver); |
| function pingPong(address receiver) external payable { | ||
| gateway.call( | ||
| receiver, | ||
| abi.encode(address(this)), | ||
| RevertOptions({ | ||
| revertAddress: address(0), | ||
| callOnRevert: false, | ||
| abortAddress: address(0), | ||
| revertMessage: "", | ||
| onRevertGasLimit: 0 | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Refactor function for clarity, completeness, and security.
The pingPong function requires several improvements:
- Rename the function to better describe its purpose, e.g.,
forwardCallToReceiver. - Emit the
Foo(or renamed) event to log the receiver address. - Consider passing
RevertOptionsas a parameter instead of hardcoding default values. - Implement checks for the payable amount if it's intended to be used.
Proposed refactoring:
- function pingPong(address receiver) external payable {
+ /// @notice Forwards a call to a receiver address through the gateway
+ /// @param receiver The address to receive the call
+ /// @param revertOptions Options for handling reverts
+ function forwardCallToReceiver(address receiver, RevertOptions memory revertOptions) external payable {
+ require(msg.value > 0, "Forwarded value must be greater than 0");
+ emit ReceiverAddressLogged(receiver);
gateway.call(
receiver,
abi.encode(address(this)),
- RevertOptions({
- revertAddress: address(0),
- callOnRevert: false,
- abortAddress: address(0),
- revertMessage: "",
- onRevertGasLimit: 0
- })
+ revertOptions
);
}This refactoring improves function naming, adds event emission, allows for flexible revert options, and includes a basic check for the payable amount.
Committable suggestion was skipped due to low confidence.
| gateway.call( | ||
| abi.encodePacked(receiver), | ||
| zrc20, | ||
| abi.encodeWithSelector(0xc8580dfc, address(this)), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Compute Function Selector Programmatically for Maintainability
Hardcoding the function selector 0xc8580dfc reduces code readability and can lead to errors if the function signature changes. It is advisable to compute the selector using Solidity's built-in functions to enhance maintainability and clarity.
Apply this diff to compute the function selector programmatically:
- abi.encodeWithSelector(0xc8580dfc, address(this)),
+ abi.encodeWithSelector(bytes4(keccak256("pingPong(address)")), address(this)),Alternatively, declare the function selector as a constant for better code organization:
bytes4 constant PINGPONG_SELECTOR = bytes4(keccak256("pingPong(address)"));And use it in your call:
abi.encodeWithSelector(PINGPONG_SELECTOR, address(this)),| // cast send 0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe "transfer(address,uint256)" 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E 2000000000000000000 --private-key 0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80 | ||
| // cast send 0xc3e53F4d16Ae77Db1c982e75a937B9f60FE63690 "pingPong(address)" 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E --value 10000000 --private-key 0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80 | ||
| // cast call 0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe "balanceOf(address)(uint256)" 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E |
There was a problem hiding this comment.
Remove Hardcoded Private Keys from Comments
Including private keys in code comments, even for testing purposes, poses a significant security risk and is considered a bad practice. It can lead to unintended exposure of sensitive information, especially if the code is shared publicly or moved to a production environment.
Please remove or obfuscate the private keys in the comments to prevent potential security vulnerabilities:
- // cast send 0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe "transfer(address,uint256)" 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E 2000000000000000000 --private-key 0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80
- // cast send 0xc3e53F4d16Ae77Db1c982e75a937B9f60FE63690 "pingPong(address)" 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E --value 10000000 --private-key 0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80
+ // cast send [Contract Address] "transfer(address,uint256)" [Recipient Address] [Amount] --private-key [PRIVATE KEY]
+ // cast send [Contract Address] "pingPong(address)" [Recipient Address] --value [Value] --private-key [PRIVATE KEY]📝 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.
| // cast send 0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe "transfer(address,uint256)" 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E 2000000000000000000 --private-key 0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80 | |
| // cast send 0xc3e53F4d16Ae77Db1c982e75a937B9f60FE63690 "pingPong(address)" 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E --value 10000000 --private-key 0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80 | |
| // cast call 0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe "balanceOf(address)(uint256)" 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E | |
| // cast send [Contract Address] "transfer(address,uint256)" [Recipient Address] [Amount] --private-key [PRIVATE KEY] | |
| // cast send [Contract Address] "pingPong(address)" [Recipient Address] --value [Value] --private-key [PRIVATE KEY] | |
| // cast call 0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe "balanceOf(address)(uint256)" 0xE6E340D132b5f46d1e472DebcD681B2aBc16e57E |
| address receiver = abi.decode(message, (address)); | ||
| (, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(700000); | ||
| IZRC20(zrc20).approve(address(gateway), gasFee); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameterize the Gas Limit for Flexibility
Hardcoding the gas limit to 700,000 reduces the flexibility and adaptability of the contract. Consider defining the gas limit as a constant or allowing it to be passed as a parameter to enhance maintainability and ease future adjustments.
Apply this diff to define the gas limit as a constant:
+ uint256 constant GAS_LIMIT = 700_000;
function onCrossChainCall(
zContext calldata context,
address zrc20,
uint256 amount,
bytes calldata message
) external override {
address receiver = abi.decode(message, (address));
- (, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(700000);
+ (, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(GAS_LIMIT);
IZRC20(zrc20).approve(address(gateway), gasFee);Committable suggestion was skipped due to low confidence.
| @@ -30,6 +31,20 @@ contract Echo { | |||
| gateway.call(receiver, message, revertOptions); | |||
| } | |||
|
|
|||
| function pingPong(address receiver) external payable { | |||
There was a problem hiding this comment.
nit: instead of pingPong I guess should be ping because the pong it's on the other side
✅ Calling a contract directly works:
❌ Making a call from ZetaChain fails:
❌ Calling ZetaChain gateway directly also fails:
Summary by CodeRabbit
Summary by CodeRabbit
pingPongfunction in theEchocontract, enabling interaction with a specified address and improving contract functionality.Hellocontract by integrating gas fee management and more complex interactions with the gateway.messagevariable in thehelloWithdrawAndCalltask.