Refactor gateway functions #186
Conversation
📝 WalkthroughWalkthroughThe changes involve significant modifications to several functions within the codebase, primarily focusing on restructuring their parameter handling. The functions now accept a unified Changes
Possibly related PRs
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (22)
packages/client/src/types.ts (3)
3-8: LGTM:revertOptionstype is well-defined.The
revertOptionstype covers essential options for handling transaction reverts with appropriate property types.Consider if any of these properties should be optional. For example,
revertMessagemight not always be necessary. If so, you could mark it as optional like this:revertMessage?: string;
10-13: LGTM:txOptionstype is well-defined, with room for potential enhancements.The
txOptionstype covers basic transaction options with appropriate types. Usingethers.BigNumberforgasPriceis a good choice for precise handling of large numbers.Consider the following enhancements:
- For consistency, you might want to use
ethers.BigNumberforgasLimitas well:gasLimit: ethers.BigNumber;
- You could include additional common transaction options, such as
nonceandvalue:export type txOptions = { gasLimit: ethers.BigNumber; gasPrice: ethers.BigNumber; nonce?: number; value?: ethers.BigNumber; };These additions would make the
txOptionstype more comprehensive and flexible for various transaction scenarios.
1-13: Consider adding JSDoc comments for better documentation.The introduction of
revertOptionsandtxOptionstypes is a positive step towards more structured and type-safe handling of transaction options and revert scenarios. This change likely improves code maintainability and reduces the likelihood of errors.To further enhance the usability of these types, consider adding JSDoc comments. This would provide inline documentation for developers using these types. For example:
/** * Options for handling transaction reverts. */ export type revertOptions = { /** Whether to call a function on revert. */ callOnRevert: boolean; /** Gas limit for the revert call. */ onRevertGasLimit: number; /** Address involved in the revert. */ revertAddress: string; /** Message associated with the revert. */ revertMessage: string; }; /** * Options for configuring transactions. */ export type txOptions = { /** Maximum gas units that can be used. */ gasLimit: number; /** Price of gas for the transaction. */ gasPrice: ethers.BigNumber; };This documentation will be especially helpful as these types are likely to be used across the codebase.
packages/tasks/src/evmCall.ts (1)
65-65: Approved: Updated example format for 'types' parameter.The change in the example format for the
typesparameter aligns well with the modification in theevmCallfunction wheretypesis now parsed from JSON. This provides a clearer example of how to input the types.Consider adding a brief comment or expanding the example to clarify that this is a JSON string representation of an array. For instance:
.addParam("types", `The types of the parameters as a JSON string (example: '["string", "uint256"]')`)This additional context could help prevent potential user errors when providing input.
packages/tasks/src/zetachainCall.ts (1)
Line range hint
1-82: Overall: Improved structure with minor enhancement neededThe refactoring of the
zetachainCallfunction and its associated task definition has significantly improved the code structure and usability. The introduction of nested objects for parameters and the updated task descriptions enhance readability and ease of use.However, there's one area that could benefit from additional attention:
- Error handling for JSON parsing of the
typesparameter should be implemented to ensure robustness.Addressing this point will further improve the reliability of the function.
packages/tasks/src/zetachainWithdrawAndCall.ts (2)
Line range hint
6-8: Consider using a more specific type for theargsparameter.Instead of using
anyfor theargsparameter, consider defining an interface that describes the expected structure of the arguments. This will improve type safety and make the function's requirements more explicit.Here's a suggested interface:
interface ZetachainWithdrawAndCallArgs { amount: string; function: string; gasLimit: number; gatewayZetaChain: string; receiver: string; callOnRevert: boolean; onRevertGasLimit: number; revertAddress: string; revertMessage: string; txOptionsGasLimit: number; txOptionsGasPrice: number; types: string; values: any[]; zrc20: string; }You can then update the function signature:
export const zetachainWithdrawAndCall = async ( args: ZetachainWithdrawAndCallArgs, hre: HardhatRuntimeEnvironment ) => { // ... }
Line range hint
34-38: Consider enhancing error handling and adding a timeout for transaction confirmation.While the current error handling catches and logs errors, it might be beneficial to provide more specific error messages or handle different types of errors separately. Additionally, consider adding a timeout for the
response.tx.wait()call to handle potential network issues.Here's a suggested improvement:
try { const receipt = await Promise.race([ response.tx.wait(), new Promise((_, reject) => setTimeout(() => reject(new Error('Transaction confirmation timeout')), 60000)) ]); console.log("Transaction hash:", receipt.transactionHash); } catch (e) { if (e instanceof Error) { console.error(`Transaction error: ${e.message}`); } else { console.error("An unknown error occurred during the transaction"); } // Optionally, re-throw the error if you want calling code to handle it throw e; }This adds a 60-second timeout for transaction confirmation and provides more specific error logging.
packages/client/src/evmCall.ts (2)
7-22: Documentation accurately reflects the new parameter structureThe updated documentation clearly describes the new
argsobject and its properties. However, there's a small discrepancy in the description of thetypesparameter.Consider updating line 15 to remove the reference to JSON:
- * @param {string} args.types - JSON string representing the types of the function parameters (e.g., ["uint256", "address"]). + * @param {string[]} args.types - Array of strings representing the types of the function parameters (e.g., ["uint256", "address"]).
Line range hint
1-81: Overall improvement in code structure and type safetyThe refactoring of the
evmCallfunction has significantly improved its structure and maintainability. Key improvements include:
- Introduction of TypeScript types for better type safety.
- Restructuring of parameters into logical groups (
revertOptionsandtxOptions).- Updated documentation reflecting the new structure.
- Simplified parameter parsing by using an array for types instead of a JSON string.
These changes align well with the PR objectives of improving the organization of gateway functions. The code is now more readable and easier to maintain.
Consider applying similar refactoring patterns to other gateway functions in the codebase for consistency. This could include:
- Using structured argument objects.
- Separating options into logical groups (e.g.,
txOptions,revertOptions).- Utilizing TypeScript types for improved type safety.
This consistent approach across all gateway functions will further enhance the overall codebase structure and maintainability.
packages/client/src/evmDeposit.ts (3)
8-23: LGTM: Function documentation updated correctly.The function documentation has been accurately updated to reflect the changes in the function signature. The new parameters
txOptionsandrevertOptionsare well-described, and obsolete parameter descriptions have been removed.Consider adding a brief explanation of what
txOptionsandrevertOptionsencompass in the main function description for better clarity at first glance.
41-48: LGTM: revertOptions object updated correctly.The revertOptions object has been successfully updated to use the new structure. The properties are correctly accessed from
args.revertOptions, and therevertMessageis properly converted to a hex string.Consider removing the unused
abortAddressproperty or add a TODO comment explaining why it's kept if it might be used in the future.
Line range hint
1-85: Overall: Excellent refactoring of the evmDeposit function.The changes made to the
evmDepositfunction significantly improve its structure and maintainability:
- The introduction of
revertOptionsandtxOptionstypes streamlines the function signature.- The documentation has been updated to reflect these changes accurately.
- The internal logic has been adjusted to use the new parameter structures consistently.
These modifications enhance the function's scalability and make it easier to maintain and extend in the future. The core functionality remains intact while improving the overall code quality.
Consider applying this pattern of using structured options to other similar functions in the codebase for consistency and improved maintainability across the project.
packages/client/src/evmDepositAndCall.ts (1)
8-25: LGTM: Updated function documentation.The function documentation has been accurately updated to reflect the new parameter structure, including
txOptionsandrevertOptions. The description for thetypesparameter has been improved for clarity.Consider adding a brief explanation of what
revertOptionsandtxOptionsencompass in the documentation for even better clarity.packages/client/src/zetachainWithdraw.ts (2)
6-6: Consider using PascalCase for type namesTypeScript conventions recommend using PascalCase for type and interface names. Consider renaming
revertOptionsandtxOptionstoRevertOptionsandTxOptionsfor consistency.Apply this diff to update the type imports:
-import type { revertOptions, txOptions } from "./types"; +import type { RevertOptions, TxOptions } from "./types";And update their usage throughout the code accordingly.
18-19: Align parameter documentation with function signatureThe order of parameters in the documentation does not match the function signature. In the documentation,
args.txOptionsis listed beforeargs.revertOptions, whereas in the function signature,revertOptionscomes beforetxOptions. For clarity, please update the documentation to match the parameter order in the function.Apply this diff to correct the documentation:
* @param {string} args.zrc20 - The address of the ZRC20 token contract from which the withdrawal will be made. - * @param {txOptions} args.txOptions - Transaction options such as gasPrice, nonce, etc. - * @param {revertOptions} args.revertOptions - Options to handle call reversion, including revert address and message. + * @param {revertOptions} args.revertOptions - Options to handle call reversion, including revert address and message. + * @param {txOptions} args.txOptions - Transaction options such as gasPrice, nonce, etc.packages/client/src/zetachainCall.ts (3)
17-17: Update documentation for 'args.types' parameterThe
args.typesparameter is now of typestring[], not a JSON string. Please update the documentation to reflect this change, including adjusting the example accordingly.
54-55: Clarify the use of 'abortAddress' in 'revertOptions'The
abortAddressfield inrevertOptionsis hardcoded to"0x0000000000000000000000000000000000000000"and marked as// not used. Consider removing it if it's unnecessary, or provide clarification on its purpose within the context.
Line range hint
66-77: Add error handling for unsupported parameter typesCurrently, the code handles
bool, integer types (int*,uint*), and defaults to returning the value as is for other types. To enhance robustness, consider adding error handling or validation for unsupported or unexpected types to prevent potential runtime issues.packages/client/src/zetachainWithdrawAndCall.ts (4)
19-19: Clarify the parameter type for 'args.values'For better type clarity, consider specifying the parameter type of
args.valuesasany[]instead ofArray. This aligns with TypeScript conventions and improves readability.Apply this diff:
- * @param {Array} args.values - The values to be passed to the function (should match the types). + * @param {any[]} args.values - The values to be passed to the function (should match the types).
22-23: Ensure parameter order consistency between documentation and function signatureIn the function signature,
revertOptionsprecedestxOptions, but in the documentation,txOptionsis documented beforerevertOptions. For consistency and better readability, consider aligning the parameter order in the documentation with the function signature.Apply this diff to reorder the documentation:
* @param {string} args.zrc20 - The address of the ZRC20 token contract used for the withdrawal and fee payment. * @param {number} args.gasLimit - The gas limit for the transaction. -* @param {txOptions} args.txOptions - Transaction options such as gasPrice, nonce, etc. -* @param {revertOptions} args.revertOptions - Options to handle call reversion, including revert address and message. +* @param {revertOptions} args.revertOptions - Options to handle call reversion, including revert address and message. +* @param {txOptions} args.txOptions - Transaction options such as gasPrice, nonce, etc.Also applies to: 39-40
Line range hint
68-75: Enhance boolean value parsing in 'valuesArray'The current logic for parsing boolean values uses
JSON.parse(value.toLowerCase()), which can lead to errors ifvalueis not a string or is already a boolean. Consider refining the parsing logic to handle different input types more robustly.Apply this diff to improve boolean parsing:
const type = args.types[index]; if (type === "bool") { - try { - return JSON.parse(value.toLowerCase()); - } catch (e) { - throw new Error(`Invalid boolean value: ${value}`); - } + if (typeof value === "boolean") { + return value; + } else if (typeof value === "string") { + if (value.toLowerCase() === "true") { + return true; + } else if (value.toLowerCase() === "false") { + return false; + } else { + throw new Error(`Invalid boolean string value: ${value}`); + } + } else { + throw new Error(`Invalid boolean value type: ${typeof value}`); + } } else if (type.startsWith("uint") || type.startsWith("int")) { return ethers.BigNumber.from(value); } else { return value; }
132-134: Consider handling the transaction receipt or emitted eventsAfter sending the transaction with
gateway[method](...), the function returns{ gasFee, gasZRC20, tx }. For improved usability, consider waiting for the transaction receipt and handling any emitted events or errors. This can provide upstream code with more detailed execution results.Example adjustment:
const tx = await gateway[method]( utils.hexlify(args.receiver), value, args.zrc20, message, args.gasLimit, revertOptions, args.txOptions ); +const receipt = await tx.wait(); +return { gasFee, gasZRC20, tx, receipt };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
- packages/client/src/evmCall.ts (3 hunks)
- packages/client/src/evmDeposit.ts (1 hunks)
- packages/client/src/evmDepositAndCall.ts (3 hunks)
- packages/client/src/types.ts (1 hunks)
- packages/client/src/zetachainCall.ts (5 hunks)
- packages/client/src/zetachainWithdraw.ts (4 hunks)
- packages/client/src/zetachainWithdrawAndCall.ts (6 hunks)
- packages/tasks/src/evmCall.ts (2 hunks)
- packages/tasks/src/evmDeposit.ts (1 hunks)
- packages/tasks/src/evmDepositAndCall.ts (2 hunks)
- packages/tasks/src/zetachainCall.ts (3 hunks)
- packages/tasks/src/zetachainWithdraw.ts (2 hunks)
- packages/tasks/src/zetachainWithdrawAndCall.ts (3 hunks)
🔇 Additional comments (33)
packages/client/src/types.ts (1)
1-1: LGTM: Import statement is correct and necessary.The import of
ethersis appropriate as it's used in thetxOptionstype definition.packages/tasks/src/evmCall.ts (1)
Line range hint
1-65: Verify impact on existing code and update documentation.The changes to the
evmCallfunction signature and internal structure, along with the updates to the task declaration, may have implications for existing code that uses this function or the "evm-call" task.Please run the following script to identify potential areas that may need updates:
- Update any code that directly calls
evmCallto use the new parameter structure.- Review and update any documentation or README files that describe the usage of the
evmCallfunction or the "evm-call" task.- Consider adding a migration guide or changelog entry to inform users of these changes and how to update their code accordingly.
- If this is a public API, consider versioning the changes and providing a deprecation period for the old function signature to allow for a smoother transition.
packages/tasks/src/evmDeposit.ts (1)
15-24: Approve refactoring of parameter handlingThe introduction of
revertOptionsandtxOptionsobjects improves code organization and readability. This structure groups related parameters together, making the code more maintainable and easier to extend in the future.To further improve this refactoring:
- Consider adding type definitions for the
argsparameter to enhance type safety. For example:interface EvmDepositArgs { amount: string; erc20?: string; gatewayEvm: string; receiver: string; revertOptions: { callOnRevert: boolean; onRevertGasLimit: number; revertAddress: string; revertMessage: string; }; txOptions: { gasLimit: number; gasPrice: number; }; } export const evmDeposit = async (args: EvmDepositArgs, hre: HardhatRuntimeEnvironment) => { // ... existing implementation }
- Ensure that all existing calls to
evmDepositin the codebase are updated to use this new structure.To verify backward compatibility and usage of the new structure, run the following script:
✅ Verification successful
Refactoring of Parameter Handling Verified Successfully
All instances of the
evmDepositfunction across the codebase have been updated to use the newrevertOptionsandtxOptionsstructures. This consistent refactoring enhances code organization, readability, and maintainability without introducing backward compatibility issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of evmDeposit function and verify if it's using the new structure # Search for evmDeposit function calls echo "Searching for evmDeposit function calls:" rg --type typescript "evmDeposit\(" -A 10 # Search for revertOptions and txOptions to verify new structure usage echo "Verifying usage of new structure (revertOptions and txOptions):" rg --type typescript "revertOptions:|txOptions:" -A 5Length of output: 397
Script:
#!/bin/bash # Description: Check for usage of evmDeposit function and verify if it's using the new structure by targeting .ts and .tsx files # Search for evmDeposit function calls in TypeScript files echo "Searching for evmDeposit function calls in .ts and .tsx files:" rg "evmDeposit\(" --glob "*.ts" --glob "*.tsx" -A 10 # Search for revertOptions and txOptions to verify new structure usage in TypeScript files echo "Verifying usage of new structure (revertOptions and txOptions) in .ts and .tsx files:" rg "revertOptions:|txOptions:" --glob "*.ts" --glob "*.tsx" -A 5Length of output: 8594
packages/tasks/src/zetachainWithdraw.ts (4)
13-26: Improved parameter handling and code organization.The refactoring of the
client.zetachainWithdrawcall withrevertOptionsandtxOptionsobjects enhances code readability and maintainability. This structure allows for better organization of related parameters and makes it easier to add or modify options in the future.
51-60: Consistent updates to task definition parameters.The task definition has been appropriately updated to reflect the changes in the function implementation. The renaming of
gasPricetotxOptionsGasPriceandgasLimittotxOptionsGasLimitmaintains consistency between the CLI interface and the function signature.
Line range hint
1-73: Well-executed refactoring for improved consistency and maintainability.The changes in this file are part of a larger refactoring effort to standardize parameter handling across gateway functions. The introduction of
revertOptionsandtxOptionsobjects, along with the updated function signature and consistent task definition changes, contributes to improved code organization, type safety, and maintainability.These changes align well with the PR objectives of refactoring gateway functions and should make the codebase more robust and easier to work with in the future.
30-30: Updated transaction receipt retrieval.The change from
tx.wait()toresponse.tx.wait()suggests an update in theZetaChainClientAPI. This modification appears to be correct based on the new structure.To ensure consistency across the codebase, let's verify the usage of this new pattern:
✅ Verification successful
Consistent usage of
response.tx.wait()confirmed across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of 'response.tx.wait()' pattern rg --type typescript 'response\.tx\.wait\(\)' .Length of output: 84
Script:
#!/bin/bash # Description: Check for consistent usage of 'response.tx.wait()' pattern in TypeScript files rg 'response\.tx\.wait\(\)' --glob '*.ts' --glob '*.tsx' .Length of output: 327
packages/tasks/src/evmDepositAndCall.ts (1)
75-75: Approved: Updated description fortypesparameterThe description for the
typesparameter has been updated to clearly indicate that it expects a JSON string. This change aligns well with the new implementation and provides clear guidance to users.packages/tasks/src/zetachainCall.ts (5)
13-31: Approve: Improved parameter structure and organizationThe restructuring of parameters into nested objects (
revertOptionsandtxOptions) enhances code readability and maintainability. This change aligns with best practices for handling complex function arguments.
28-28: Verify error handling for JSON parsingThe
typesparameter is now parsed from a JSON string. While this allows for more flexible input, it's important to ensure proper error handling for invalid JSON input.Consider adding a try-catch block around the JSON parsing to handle potential errors gracefully:
let parsedTypes; try { parsedTypes = JSON.parse(args.types); } catch (error) { console.error("Invalid JSON format for types:", error); // Handle the error appropriately (e.g., throw a custom error or use a default value) }
32-32: Approve: Improved transaction receipt handlingThe change to
await response.tx.wait()for obtaining the transaction receipt is more consistent with typical ethers.js usage and ensures the transaction is confirmed before proceeding.
53-65: Approve: Updated task parametersThe task parameters have been correctly updated to reflect the changes in the
zetachainCallfunction. The new parameter names (gasLimit,txOptionsGasPrice,txOptionsGasLimit) accurately represent their roles in the restructured function.
81-82: Approve: Improved parameter descriptionsThe addition of examples in the descriptions for the
functionandtypesparameters enhances the usability of the task. This change provides clear guidance on how to use these parameters correctly.packages/tasks/src/zetachainWithdrawAndCall.ts (2)
13-29: LGTM! Improved structure withrevertOptionsandtxOptions.The restructuring of parameters into
revertOptionsandtxOptionsobjects improves the code organization and makes it easier to manage related options. The JSON parsing of thetypesparameter is also a good addition for better type handling.
Line range hint
59-83: LGTM! Task definition updates align well with function changes.The updates to the task definition, including the parameter name changes and updated descriptions, accurately reflect the modifications made to the
zetachainWithdrawAndCallfunction. The new parameter names (txOptionsGasPrice,txOptionsGasLimit) and the updated descriptions forfunctionandtypesparameters provide clearer guidance for users of this task.packages/client/src/evmCall.ts (3)
5-5: LGTM: Improved type organizationThe addition of imported types from a separate file enhances code organization and type reusability.
29-31: LGTM: Improved function signatureThe updated function signature with a structured
argsobject enhances clarity and maintainability. It correctly uses the imported TypeScript types and aligns with the updated documentation.
40-40: LGTM: Simplified parameter parsingThe direct use of
args.typesas an array simplifies the code and aligns with the updated function signature. The removal of JSON parsing reduces potential errors and improves performance.Also applies to: 56-57
packages/client/src/evmDeposit.ts (3)
6-6: LGTM: New type imports added correctly.The import of
revertOptionsandtxOptionstypes is correctly added and aligns with the refactoring of the function parameters. This is a good practice as it improves type safety and code readability.
26-33: LGTM: Function signature refactored effectively.The function signature has been successfully refactored to use structured
revertOptionsandtxOptionsparameters. This change improves the function's scalability and maintainability by grouping related options together. The new signature aligns well with the updated documentation and the described changes in the AI-generated summary.
51-53: LGTM: txOptions object added correctly.The new txOptions object has been added successfully, correctly accessing
gasLimitandgasPricefromargs.txOptions. This change aligns well with the updated function signature and improves the organization of transaction-related parameters.packages/client/src/evmDepositAndCall.ts (6)
6-6: LGTM: Import statement for new types.The import statement for
revertOptionsandtxOptionstypes is correctly added. This change aligns well with the updated function signature and enhances type safety.
27-37: LGTM: Improved function signature.The function signature has been updated to use a more structured argument object, which improves type safety and organization of parameters. The changes to
revertOptions,txOptions, andtypesare consistent with the documentation and enhance the function's usability.
55-57: LGTM: Updated txOptions object.The
txOptionsobject is now correctly constructed using properties fromargs.txOptions. This change aligns with the new function signature and provides a clear structure for transaction-related options.
61-61: LGTM: Simplified parameter encoding.The direct use of
args.typesinstead of parsing a JSON string simplifies the code and reduces the potential for errors. This change aligns well with the updated function signature and improves type safety.Also applies to: 77-78
Line range hint
93-95: LGTM: Updated function call with new option objects.The function call now correctly uses the
revertOptionsandtxOptionsobjects, which is consistent with the updates to the function signature and internal logic. This provides a cleaner and more organized way to pass options to the contract method.Also applies to: 108-112
Line range hint
1-114: Overall assessment: Well-structured refactoring with improved type safety.The changes to
evmDepositAndCallfunction have significantly improved its structure and type safety. The introduction ofrevertOptionsandtxOptionsprovides a more organized approach to handling function parameters. The updates to the documentation, function signature, and internal logic are consistent and well-implemented.A few points to consider:
- The purpose of the unused
abortAddressinrevertOptionsshould be clarified or removed if unnecessary.- Consider adding brief explanations of
revertOptionsandtxOptionsin the function documentation for enhanced clarity.Overall, this refactoring aligns well with the PR objectives and improves the maintainability of the code.
packages/client/src/zetachainWithdraw.ts (3)
96-96: Return statement aligns with documented return structureThe return statement provides
{ gasFee, gasZRC20, tx }, which matches the documented return properties. Ensure consistency between the code and documentation.
94-94: Ensure method signature matches the gateway contractThe method name used in
gateway[method]is hard-coded. Verify that this method signature:"withdraw(bytes,uint256,address,(address,bool,address,bytes,uint256))"matches the actual method in the
GatewayZEVMcontract ABI to prevent runtime errors.Run the following script to confirm the method exists in the ABI:
#!/bin/bash # Description: Verify that the method signature exists in GatewayZEVM ABI # Expected: The method signature should be present in the ABI. jq '.abi[] | select(.name=="withdraw" and (.inputs | length == 4)) | select(.inputs[0].type=="bytes" and .inputs[1].type=="uint256" and .inputs[2].type=="address" and .inputs[3].type=="tuple")' ./abi/GatewayZEVM.sol/GatewayZEVM.json
65-65: Verify thatargs.txOptionscontains necessary transaction fieldsWhen passing
args.txOptionsto theapprovemethods, ensure that all required transaction options (e.g.,gasLimit,gasPrice,nonce) are appropriately set to avoid transaction failures.Run the following script to check the usage of
txOptions:Also applies to: 77-77, 83-83
packages/client/src/zetachainWithdrawAndCall.ts (3)
6-7: Imports are correctly addedThe import statement for
revertOptionsandtxOptionsfrom./typesis appropriate and ensures that the custom types are correctly referenced.
Line range hint
101-119: Verify that 'args.txOptions' does not override explicit parametersWhen passing
args.txOptionsto contract method calls, ensure that it doesn't overwrite important parameters likegasLimitorgasPricethat are explicitly set elsewhere in the code. It's important to confirm that the transaction options are merged correctly and that the explicit values take precedence when necessary.Run the following script to examine the usage of
args.txOptions:#!/bin/bash # Description: Check if 'args.txOptions' might override explicit transaction parameters. # Search for usages of 'args.txOptions' in method calls. rg --type typescript 'args\.txOptions' -A 3 -B 3Review the output to ensure that
args.txOptionsis appropriately used without unintended side effects.
57-62: Confirm the hardcoded 'abortAddress' or make it configurableThe
abortAddressis hardcoded to"0x0000000000000000000000000000000000000000"within therevertOptions. Please confirm if this is intentional. If there's a need for flexibility, consider addingabortAddresstoargs.revertOptionsto make it configurable.Run the following script to check the usage of
abortAddressacross the codebase:This will help determine if
abortAddressis always hardcoded or if there are cases where it is configurable.✅ Verification successful
Confirmed: The 'abortAddress' is consistently hardcoded to "0x0000000000000000000000000000000000000000" across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if 'abortAddress' is consistently hardcoded or if it should be configurable. # Search for 'abortAddress' assignments in TypeScript files. rg --type typescript 'abortAddress\s*:' -A 5Length of output: 179
Script:
#!/bin/bash # Description: Verify if 'abortAddress' is consistently hardcoded or if it should be configurable. # Search for 'abortAddress' assignments in TypeScript files with .ts and .tsx extensions. rg --glob '*.ts' --glob '*.tsx' 'abortAddress\s*:' -A 5Length of output: 3354
| 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), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improved parameter organization, but consider adding input validation.
The restructuring of parameters into revertOptions and txOptions objects enhances code organization and readability. However, there are a few points to consider:
- Parsing
typesfrom JSON allows for more flexible input, but it may introduce runtime errors if invalid JSON is provided. - The function now relies on the presence of specific properties in the
argsobject, which could lead to runtime errors if these properties are missing or undefined.
Consider adding input validation to ensure all required properties are present and correctly formatted. Here's an example of how you might implement this:
import { z } from 'zod';
const argsSchema = z.object({
gatewayEvm: z.string(),
receiver: z.string(),
callOnRevert: z.boolean(),
onRevertGasLimit: z.number().optional(),
revertAddress: z.string().optional(),
revertMessage: z.string().optional(),
gasLimit: z.number().optional(),
gasPrice: z.number().optional(),
types: z.string().refine(
(val) => {
try {
JSON.parse(val);
return true;
} catch {
return false;
}
},
{ message: "Invalid JSON string for 'types'" }
),
values: z.array(z.unknown()),
});
export const evmCall = async (args: unknown, hre: HardhatRuntimeEnvironment) => {
try {
const validatedArgs = argsSchema.parse(args);
// ... rest of the function using validatedArgs
} catch (e) {
if (e instanceof z.ZodError) {
console.error("Invalid input:", e.errors);
} else {
console.error("Transaction error:", e);
}
}
};This approach uses the zod library for runtime type checking and validation, ensuring that all required properties are present and correctly formatted before proceeding with the function execution.
| revertOptions: { | ||
| callOnRevert: args.callOnRevert, | ||
| onRevertGasLimit: args.onRevertGasLimit, | ||
| revertAddress: args.revertAddress, | ||
| revertMessage: args.revertMessage, | ||
| }, | ||
| txOptions: { | ||
| gasLimit: args.gasLimit, | ||
| gasPrice: args.gasPrice, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding type safety for the args parameter
The restructuring of the function parameters into revertOptions and txOptions objects improves code organization. However, the args parameter is typed as any, which loses type safety.
Consider defining an interface for the args parameter to ensure type safety and provide better documentation for function users. For example:
interface EvmDepositAndCallArgs {
amount: string;
erc20?: string;
gatewayEvm: string;
receiver: string;
revertOptions: {
callOnRevert: boolean;
onRevertGasLimit: number;
revertAddress: string;
revertMessage: string;
};
txOptions: {
gasLimit: number;
gasPrice: number;
};
types: string;
values: any[];
}
export const evmDepositAndCall = async (
args: EvmDepositAndCallArgs,
hre: HardhatRuntimeEnvironment
) => {
// ... rest of the function
}This change would provide better type checking and autocompletion for users of the function.
| gasLimit: args.gasLimit, | ||
| gasPrice: args.gasPrice, | ||
| }, | ||
| types: JSON.parse(args.types), |
There was a problem hiding this comment.
Add error handling for JSON parsing
The types parameter is now parsed from a JSON string, which introduces a potential point of failure.
Add error handling to catch and report JSON parsing errors. For example:
let parsedTypes;
try {
parsedTypes = JSON.parse(args.types);
} catch (error) {
console.error("Error parsing types:", error);
throw new Error("Invalid JSON string for types parameter");
}
// Use parsedTypes instead of JSON.parse(args.types) in the client callThis change will provide more informative error messages if the types parameter is not a valid JSON string.
| abortAddress: "0x0000000000000000000000000000000000000000", // not used | ||
| callOnRevert: args.revertOptions.callOnRevert, | ||
| onRevertGasLimit: args.revertOptions.onRevertGasLimit, | ||
| revertAddress: args.revertOptions.revertAddress, | ||
| revertMessage: utils.hexlify( | ||
| utils.toUtf8Bytes(args.revertOptions.revertMessage) | ||
| ), | ||
| }, | ||
| { | ||
| gasLimit: args.gasLimit, | ||
| gasPrice: args.gasPrice, | ||
| gasLimit: args.txOptions.gasLimit, | ||
| gasPrice: args.txOptions.gasPrice, |
There was a problem hiding this comment.
💡 Codebase verification
Unused abortAddress parameter
The abortAddress parameter is defined in the Revert.sol contract but does not appear to be used in any function implementations. If it's not required, consider removing it. Otherwise, if it's part of the contract interface, please add a comment explaining why it's set to the zero address.
🔗 Analysis chain
Improved structure for gateway call options
The use of structured revertOptions and txOptions enhances code readability and maintainability. However, there's a potential issue to address.
The abortAddress is hardcoded and marked as unused. Can we remove this parameter if it's not being used? If it's required by the contract interface but not used in this context, consider adding a comment explaining why it's set to the zero address.
To verify the usage of abortAddress, let's check the contract interface:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage of abortAddress in the GatewayEVM contract
# Test: Search for the function signature in the ABI
jq '.abi[] | select(.type == "function") | select(.name == "call") | .inputs' packages/client/src/abi/GatewayEVM.sol/GatewayEVM.json
# Test: Search for abortAddress usage in the contract source code
rg --type solidity 'abortAddress'
Length of output: 1045
Script:
#!/bin/bash
# Description: Retry searching for abortAddress in Solidity files using file extension
# Test: Search for abortAddress usage in the contract source code with correct file pattern
rg -g '*.sol' 'abortAddress'
Length of output: 157
| const revertOptions = { | ||
| abortAddress: "0x0000000000000000000000000000000000000000", | ||
| callOnRevert: args.callOnRevert, | ||
| onRevertGasLimit: args.onRevertGasLimit, | ||
| revertAddress: args.revertAddress, | ||
| abortAddress: "0x0000000000000000000000000000000000000000", // not used | ||
| callOnRevert: args.revertOptions.callOnRevert, | ||
| onRevertGasLimit: args.revertOptions.onRevertGasLimit, | ||
| revertAddress: args.revertOptions.revertAddress, | ||
| // not used | ||
| revertMessage: utils.hexlify(utils.toUtf8Bytes(args.revertMessage)), | ||
| revertMessage: utils.hexlify( | ||
| utils.toUtf8Bytes(args.revertOptions.revertMessage) | ||
| ), |
There was a problem hiding this comment.
💡 Codebase verification
Recommendation: Remove the unused abortAddress property
The abortAddress property is consistently set to "0x0000000000000000000000000000000000000000" across multiple files and is marked as not used. It would be beneficial to remove this property from the revertOptions object to simplify the codebase.
🔗 Analysis chain
LGTM with a question: Updated revertOptions object.
The revertOptions object is now correctly constructed using properties from args.revertOptions, which aligns with the new function signature. The encoding of revertMessage using utils.hexlify and utils.toUtf8Bytes is a good practice for blockchain interactions.
Could you clarify why abortAddress is included if it's not used? Consider removing it if it's unnecessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for uses of abortAddress in the codebase
rg --type typescript 'abortAddress'
Length of output: 70
Script:
#!/bin/bash
# Search for uses of abortAddress in TypeScript files
rg --glob "*.ts" --glob "*.tsx" 'abortAddress'
Length of output: 712
| callOnRevert: args.revertOptions.callOnRevert, | ||
| onRevertGasLimit: args.revertOptions.onRevertGasLimit, | ||
| revertAddress: args.revertOptions.revertAddress, | ||
| revertMessage: utils.hexlify( | ||
| utils.toUtf8Bytes(args.revertOptions.revertMessage) | ||
| ), |
There was a problem hiding this comment.
Handle optional revertMessage gracefully
If revertMessage can be optional or empty, consider adding a check to handle such cases to prevent potential errors during hex conversion.
Apply this diff to handle an optional revertMessage:
const revertOptions = {
abortAddress: args.revertOptions.abortAddress || "0x0000000000000000000000000000000000000000",
callOnRevert: args.revertOptions.callOnRevert,
onRevertGasLimit: args.revertOptions.onRevertGasLimit,
revertAddress: args.revertOptions.revertAddress,
- revertMessage: utils.hexlify(
- utils.toUtf8Bytes(args.revertOptions.revertMessage)
- ),
+ revertMessage: args.revertOptions.revertMessage
+ ? utils.hexlify(utils.toUtf8Bytes(args.revertOptions.revertMessage))
+ : "0x",
};This ensures that if revertMessage is not provided, a default empty value "0x" is used.
📝 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.
| callOnRevert: args.revertOptions.callOnRevert, | |
| onRevertGasLimit: args.revertOptions.onRevertGasLimit, | |
| revertAddress: args.revertOptions.revertAddress, | |
| revertMessage: utils.hexlify( | |
| utils.toUtf8Bytes(args.revertOptions.revertMessage) | |
| ), | |
| callOnRevert: args.revertOptions.callOnRevert, | |
| onRevertGasLimit: args.revertOptions.onRevertGasLimit, | |
| revertAddress: args.revertOptions.revertAddress, | |
| revertMessage: args.revertOptions.revertMessage | |
| ? utils.hexlify(utils.toUtf8Bytes(args.revertOptions.revertMessage)) | |
| : "0x", |
🛠️ Refactor suggestion
Make abortAddress configurable
Currently, abortAddress is hard-coded to "0x0000000000000000000000000000000000000000". Consider making abortAddress a configurable parameter within args.revertOptions to allow for greater flexibility.
Apply this diff to make abortAddress configurable:
const revertOptions = {
- abortAddress: "0x0000000000000000000000000000000000000000",
+ abortAddress: args.revertOptions.abortAddress || "0x0000000000000000000000000000000000000000",
callOnRevert: args.revertOptions.callOnRevert,
onRevertGasLimit: args.revertOptions.onRevertGasLimit,
revertAddress: args.revertOptions.revertAddress,
revertMessage: utils.hexlify(
utils.toUtf8Bytes(args.revertOptions.revertMessage)
),
};This change allows abortAddress to be specified when needed, while defaulting to "0x000...0" if not provided.
Committable suggestion was skipped due to low confidence.
| /** | ||
| * @function zetachainCall | ||
| * @description Calls a contract on a connected chain. | ||
| * | ||
| * @param {ZetaChainClient} this - The instance of the ZetaChain client that contains the signer information. | ||
| * @param {object} args - The function arguments. | ||
| * @param {string} args.function - The name of the function to be executed on the target contract. | ||
| * @param {string} args.gatewayZetaChain - The address of the ZetaChain gateway contract. | ||
| * @param {string} args.receiver - The address of the contract or account that will receive the call. | ||
| * @param {string} args.types - JSON string representing the types of the function parameters (e.g., ["uint256", "address"]). | ||
| * @param {Array} args.values - The values to be passed to the function (should match the types). | ||
| * @param {string} args.zrc20 - The address of the ZRC20 token contract used for paying gas fees. | ||
| * @param {number} args.gasLimit - The amount of gas to be used for the call. | ||
| * @param {txOptions} args.txOptions - Transaction options such as gasPrice, nonce, etc. | ||
| * @param {revertOptions} args.revertOptions - Options to handle call reversion, including revert address and message. | ||
| * | ||
| * @returns {object} - Returns an object containing the transaction, gas token, and gas fee. | ||
| * @property {object} tx - The transaction object for the cross-chain call. | ||
| * @property {string} gasZRC20 - The address of the ZRC20 gas token. | ||
| * @property {ethers.BigNumber} gasFee - The amount of gas fee paid for the transaction. | ||
| */ |
There was a problem hiding this comment.
💡 Codebase verification
Calls to zetachainCall in packages/tasks/src/zetachainCall.ts are missing txOptions and revertOptions parameters
- Location:
packages/tasks/src/zetachainCall.ts
🔗 Analysis chain
Ensure all calls to 'zetachainCall' are updated to match the new signature
The zetachainCall function signature has changed by incorporating txOptions and revertOptions. Please verify that all calls to this function within the codebase have been updated to reflect the new parameter structure.
Run the following script to locate and review all usages of zetachainCall:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all function calls to `zetachainCall` match the new signature.
# Search for function calls to 'zetachainCall' in TypeScript files
rg --type ts 'zetachainCall\(' -A 2 -B 2
Length of output: 458
| * @param {string} args.function - The name of the function to be called on the target contract. | ||
| * @param {string} args.gatewayZetaChain - The address of the ZetaChain gateway contract. | ||
| * @param {string} args.receiver - The address that will receive the withdrawn ZRC20 tokens or the contract to interact with. | ||
| * @param {string} args.types - JSON string representing the types of the function parameters (e.g., ["uint256", "address"]). |
There was a problem hiding this comment.
Update documentation to match parameter type change for 'args.types'
The args.types parameter has been changed from a JSON string to an array of strings (string[]), but the documentation still refers to it as a JSON string (e.g., ["uint256", "address"]). Please update the documentation to reflect that args.types is now an array of strings.
Apply this diff to correct the documentation:
- * @param {string} args.types - JSON string representing the types of the function parameters (e.g., ["uint256", "address"]).
+ * @param {string[]} args.types - An array of strings representing the types of the function parameters (e.g., ["uint256", "address"]).Also applies to: 41-41
|
@zeta-chain/fullstack @andresaiello please, review. |
Summary by CodeRabbit
New Features
revertOptionsandtxOptionsto streamline transaction and revert handling.Bug Fixes
Documentation