fix: remove tx option defaults #193
Conversation
📝 WalkthroughWalkthroughThe changes in the pull request focus on the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EVMTask
User->>EVMTask: Call evmDepositAndCall(gasPrice, gasLimit, ...)
EVMTask-->>User: Execute transaction with provided parameters
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/tasks/src/evmDepositAndCall.ts (1)
Line range hint
8-31: Improve error handling and type validation.The current error handling could be enhanced to provide better debugging information and prevent potential issues:
- The generic error handling masks specific issues
- JSON parsing of
typescould fail silently- No validation between parsed types and provided values
Consider this improvement:
export const evmDepositAndCall = async ( args: any, hre: HardhatRuntimeEnvironment ) => { + let parsedTypes; try { + parsedTypes = JSON.parse(args.types); + if (!Array.isArray(parsedTypes) || parsedTypes.length !== args.values.length) { + throw new Error("Types array length must match values array length"); + } const [signer] = await hre.ethers.getSigners(); const client = new ZetaChainClient({ network: "testnet", signer }); const tx = await client.evmDepositAndCall({ amount: args.amount, erc20: args.erc20, gatewayEvm: args.gatewayEvm, receiver: args.receiver, revertOptions: { callOnRevert: args.callOnRevert, onRevertGasLimit: args.onRevertGasLimit, revertAddress: args.revertAddress, revertMessage: args.revertMessage, }, txOptions: { gasLimit: args.gasLimit, gasPrice: args.gasPrice, }, - types: JSON.parse(args.types), + types: parsedTypes, values: args.values, }); if (tx) { const receipt = await tx.wait(); console.log("Transaction hash:", receipt.transactionHash); } } catch (e) { - console.error("Transaction error:", e); + if (e instanceof SyntaxError) { + console.error("Invalid JSON format for types:", e.message); + } else { + console.error("Transaction failed:", e instanceof Error ? e.message : String(e)); + } + process.exit(1); } };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/tasks/src/evmDepositAndCall.ts(1 hunks)
🔇 Additional comments (1)
packages/tasks/src/evmDepositAndCall.ts (1)
54-55: LGTM! Removing default gas values is a good practice.
Removing hardcoded defaults for gasPrice and gasLimit is the right approach as these values should be network-specific and determined at runtime. This change helps prevent potential issues with inappropriate gas values across different networks.
Let's verify the error handling when these values are not provided:
Consider adding JSDoc comments to these parameters to document:
- That these values are required
- The expected format (e.g., wei for gasPrice)
- Recommendations for obtaining appropriate values
✅ Verification successful
The previous search showed that gas parameters are used across the codebase, but we need to verify specifically how the evmDepositAndCall function handles missing gas values to ensure removing defaults is safe.
We need to verify how transaction options are validated before being used in the client implementation to ensure removing defaults is safe.
We need to understand how the transaction options are defined and validated in the client implementation to ensure removing defaults is safe.
Removing default gas values is safe and recommended
The code inspection confirms that:
- Gas values are passed through from task arguments to the client implementation without modification
- The client implementation expects these values in the
txOptionstype and uses them directly - There's no fallback logic or default values in the client implementation
This validates that removing hardcoded defaults from the task is the correct approach as it:
- Forces users to provide appropriate network-specific gas values
- Aligns with the client implementation's expectations
- Prevents potential issues from using inappropriate default values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there's proper error handling or documentation for missing gas values
# Test 1: Look for error handling related to missing gas values
rg -A 5 "gasLimit|gasPrice" --type ts
# Test 2: Check for related documentation
fd -e md -e txt . -x grep -l "gasLimit|gasPrice"
Length of output: 53598
Script:
#!/bin/bash
# Let's check how the gas values are handled in the evmDepositAndCall task implementation
# Check the implementation of evmDepositAndCall function in the client
rg -A 10 "export const evmDepositAndCall" packages/client/src/evmDepositAndCall.ts
# Check if there's error handling for missing gas values
rg -A 5 "if \(!.*gas.*\)" packages/client/src/
Length of output: 403
Script:
#!/bin/bash
# Let's check how the client validates and handles transaction options
# Check the txOptions type definition and usage
rg -A 5 "txOptions.*{" packages/client/src/
# Check if there's validation in the evmDepositAndCall implementation
ast-grep --pattern 'export const evmDepositAndCall = async function ($_, $_) {
$$$
const tx = await $$$
$$$
}'
Length of output: 252
Script:
#!/bin/bash
# Let's check the type definitions and validation
# Check type definitions
rg "type txOptions" packages/client/src/
# Check how evmDepositAndCall is implemented
rg -l "evmDepositAndCall" packages/client/src/
rg -A 20 "evmDepositAndCall.*=" packages/client/src/evmDepositAndCall.ts
# Check if there's any validation before using txOptions
rg -B 5 -A 10 "txOptions\." packages/client/src/evmDepositAndCall.ts
Length of output: 1458
Each chain has its own gas limit and price and having defaults prevents hardhat from inheriting these values from config. This resulted in tx broadcasting being stuck.
Summary by CodeRabbit
New Features
evmDepositAndCalltask to allow for more flexible gas settings by removing default values forgasPriceandgasLimit.Bug Fixes