feat: Gateway methods and Solana deposit #179
Conversation
WalkthroughWalkthroughThe recent changes encompass updates to various smart contracts, libraries, and configuration files within the ZetaChain protocol. Key modifications include the introduction of new functionalities for handling deposits and withdrawals across Solana and Ethereum blockchains, enhancements to contract interfaces, and updates to bytecode across multiple factories. Additionally, new dependencies were added to the project, and the TypeScript configuration was adjusted for improved type management. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ZetaChainClient
participant SolanaNetwork
participant EVMNetwork
User->>ZetaChainClient: Call solanaDeposit(amount, recipient)
ZetaChainClient->>SolanaNetwork: Initiate deposit transaction
SolanaNetwork-->>ZetaChainClient: Transaction success
ZetaChainClient-->>User: Confirm deposit
sequenceDiagram
participant User
participant EVMNetwork
participant GatewayContract
User->>EVMNetwork: Call evmDeposit(amount, recipient)
EVMNetwork->>GatewayContract: Initiate deposit transaction
GatewayContract-->>EVMNetwork: Transaction success
EVMNetwork-->>User: Confirm deposit
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 (
|
fadeev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Outside diff range, codebase verification and nitpick comments (4)
tsconfig.json (1)
15-15: Potential Issue: Verify Type Resolution Due totypeRootsRemovalThe removal of
typeRootscombined with the absence of thenode_modules/@typesdirectory could lead to issues with type resolution. Ensure that all necessary type definitions are explicitly included in the project to avoid potential type-related errors. Consider verifying the project's type dependencies and their resolution paths.
- Only one type definition file found:
typechain-types/hardhat.d.ts.node_modules/@typesdirectory is missing, which might affect access to common DefinitelyTyped definitions.Analysis chain
Approved: Strict mode enabled. Verify type resolution.
Enabling
strictmode is a positive change for enhancing type safety. However, the removal oftypeRootsneeds careful verification to ensure that all necessary types are resolved correctly.Run the following script to verify type resolution:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all necessary types are resolved correctly. # Test: Check TypeScript compilation for errors. Expect: No errors related to types. npx tsc --noEmitLength of output: 89
Script:
#!/bin/bash # Description: Search for type definition files in the project to ensure necessary types are available. # Find all type definition files in the project fd --extension d.ts # Check for the presence of a node_modules/@types directory if [ -d "node_modules/@types" ]; then echo "node_modules/@types directory exists." else echo "node_modules/@types directory does not exist." fiLength of output: 184
packages/tasks/src/solanaDeposit.ts (1)
29-35: Approve task registration with suggestions for improvement.The task registration for
solana-depositis well-structured with appropriate parameters. However, consider adding more descriptive texts for each parameter to enhance clarity and usability for end-users. For example:- .addParam("amount", "Amount of SOL to deposit") + .addParam("amount", "Amount of SOL to deposit. Specify the amount in lamports.")This small addition can significantly improve the user experience by providing clearer instructions.
typechain-types/factories/@zetachain/protocol-contracts/contracts/zevm/interfaces/IZRC20.sol/IZRC20__factory.ts (1)
Line range hint
10-185: Evaluate Impact of ABI Changes and RenamingThe changes in the ABI, including the removal of events like
Approval,Deposit, andTransfer, and the renaming ofPROTOCOL_FEEtoPROTOCOL_FLAT_FEE, indicate a significant shift in the contract's functionality. It is crucial to evaluate the impact of these changes on existing integrations and user interactions.Consider updating the documentation to reflect these changes and provide guidance on how to handle the new contract interface. Additionally, review whether the removal of these functions and events aligns with the overall design goals of the contract.
typechain-types/factories/contracts/SwapHelperLib__factory.ts (1)
Line range hint
1-1: General review of structural elements and static typing.The file structure, including imports, ABI declarations, and class definitions, follows the typical pattern for Ethereum smart contract TypeScript factories. However, there are some general improvements and corrections needed:
- Ensure consistent use of TypeScript types to avoid any potential type errors during compilation or runtime.
- The use of
{}as a type should be replaced with more specific type definitions to enhance code clarity and maintainability.- events: {}; + events: Record<string, unknown>;This change will help in defining the object shape explicitly, avoiding the use of the banned
{}type which is too generic.Also applies to: 3-3, 5-24, 26-26, 44-44, 62-62, 114-114, 141-141, 144-144, 170-170, 231-231, 290-290, 351-351, 414-414
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
Files selected for processing (38)
- contracts/SwapHelperLib.sol (4 hunks)
- package.json (1 hunks)
- packages/client/src/client.ts (2 hunks)
- packages/client/src/idl/gateway.json (1 hunks)
- packages/client/src/index.ts (1 hunks)
- packages/client/src/solanaDeposit.ts (1 hunks)
- packages/tasks/src/evmCall.ts (1 hunks)
- packages/tasks/src/evmDeposit.ts (1 hunks)
- packages/tasks/src/evmDepositAndCall.ts (1 hunks)
- packages/tasks/src/index.ts (1 hunks)
- packages/tasks/src/solanaDeposit.ts (1 hunks)
- packages/tasks/src/zetachainCall.ts (1 hunks)
- packages/tasks/src/zetachainWithdraw.ts (1 hunks)
- packages/tasks/src/zetachainWithdrawAndCall.ts (1 hunks)
- tsconfig.json (1 hunks)
- typechain-types/@zetachain/protocol-contracts/contracts/zevm/interfaces/IZRC20.sol/IZRC20.ts (21 hunks)
- typechain-types/@zetachain/protocol-contracts/contracts/zevm/interfaces/IZRC20.sol/IZRC20Metadata.ts (1 hunks)
- typechain-types/@zetachain/protocol-contracts/contracts/zevm/interfaces/IZRC20.sol/ZRC20Events.ts (1 hunks)
- typechain-types/@zetachain/protocol-contracts/contracts/zevm/interfaces/IZRC20.sol/index.ts (1 hunks)
- typechain-types/@zetachain/protocol-contracts/contracts/zevm/interfaces/index.ts (1 hunks)
- typechain-types/@zetachain/protocol-contracts/contracts/zevm/interfaces/zContract.sol/UniversalContract.ts (1 hunks)
- typechain-types/@zetachain/protocol-contracts/contracts/zevm/interfaces/zContract.sol/ZContract.ts (1 hunks)
- typechain-types/@zetachain/protocol-contracts/contracts/zevm/interfaces/zContract.sol/index.ts (1 hunks)
- typechain-types/factories/@zetachain/protocol-contracts/contracts/zevm/SystemContract.sol/SystemContract__factory.ts (1 hunks)
- typechain-types/factories/@zetachain/protocol-contracts/contracts/zevm/interfaces/IZRC20.sol/IZRC20Metadata__factory.ts (1 hunks)
- typechain-types/factories/@zetachain/protocol-contracts/contracts/zevm/interfaces/IZRC20.sol/IZRC20__factory.ts (4 hunks)
- typechain-types/factories/@zetachain/protocol-contracts/contracts/zevm/interfaces/IZRC20.sol/ZRC20Events__factory.ts (1 hunks)
- typechain-types/factories/@zetachain/protocol-contracts/contracts/zevm/interfaces/IZRC20.sol/index.ts (1 hunks)
- typechain-types/factories/@zetachain/protocol-contracts/contracts/zevm/interfaces/index.ts (1 hunks)
- typechain-types/factories/@zetachain/protocol-contracts/contracts/zevm/interfaces/zContract.sol/UniversalContract__factory.ts (1 hunks)
- typechain-types/factories/@zetachain/protocol-contracts/contracts/zevm/interfaces/zContract.sol/ZContract__factory.ts (1 hunks)
- typechain-types/factories/@zetachain/protocol-contracts/contracts/zevm/interfaces/zContract.sol/index.ts (1 hunks)
- typechain-types/factories/contracts/OnlySystem__factory.ts (1 hunks)
- typechain-types/factories/contracts/SwapHelperLib__factory.ts (1 hunks)
- typechain-types/factories/contracts/TestSystemContract__factory.ts (1 hunks)
- typechain-types/factories/contracts/shared/MockSystemContract.sol/MockSystemContract__factory.ts (1 hunks)
- typechain-types/hardhat.d.ts (2 hunks)
- typechain-types/index.ts (1 hunks)
Files skipped from review due to trivial changes (9)
- contracts/SwapHelperLib.sol
- typechain-types/@zetachain/protocol-contracts/contracts/zevm/interfaces/IZRC20.sol/index.ts
- typechain-types/@zetachain/protocol-contracts/contracts/zevm/interfaces/index.ts
- typechain-types/@zetachain/protocol-contracts/contracts/zevm/interfaces/zContract.sol/ZContract.ts
- typechain-types/@zetachain/protocol-contracts/contracts/zevm/interfaces/zContract.sol/index.ts
- typechain-types/factories/@zetachain/protocol-contracts/contracts/zevm/interfaces/IZRC20.sol/index.ts
- typechain-types/factories/@zetachain/protocol-contracts/contracts/zevm/interfaces/index.ts
- typechain-types/factories/@zetachain/protocol-contracts/contracts/zevm/interfaces/zContract.sol/ZContract__factory.ts
- typechain-types/factories/@zetachain/protocol-contracts/contracts/zevm/interfaces/zContract.sol/index.ts
Additional context used
Biome
typechain-types/factories/@zetachain/protocol-contracts/contracts/zevm/interfaces/zContract.sol/UniversalContract__factory.ts
[error] 105-116: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
typechain-types/factories/@zetachain/protocol-contracts/contracts/zevm/interfaces/IZRC20.sol/ZRC20Events__factory.ts
[error] 166-177: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
typechain-types/@zetachain/protocol-contracts/contracts/zevm/interfaces/zContract.sol/UniversalContract.ts
[error] 85-85: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 166-166: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
typechain-types/factories/@zetachain/protocol-contracts/contracts/zevm/interfaces/IZRC20.sol/IZRC20Metadata__factory.ts
[error] 285-296: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
typechain-types/@zetachain/protocol-contracts/contracts/zevm/interfaces/IZRC20.sol/ZRC20Events.ts
[error] 16-16: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 145-145: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 147-147: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 216-216: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 218-218: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
typechain-types/@zetachain/protocol-contracts/contracts/zevm/interfaces/IZRC20.sol/IZRC20.ts
[error] 129-129: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 321-321: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
typechain-types/@zetachain/protocol-contracts/contracts/zevm/interfaces/IZRC20.sol/IZRC20Metadata.ts
[error] 141-141: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 351-351: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
GitHub Check: build
packages/client/src/solanaDeposit.ts
[warning] 10-10:
Prefer using arrow functions over plain functions
Additional comments not posted (38)
packages/client/src/index.ts (1)
14-14: Approved: New export forsolanaDeposit.The addition of
solanaDepositto the exports is consistent with the existing structure and aligns with the PR's objectives to enhance Solana functionalities.packages/tasks/src/index.ts (7)
13-13: Approved: New export forevmCall.The addition of
evmCallto the exports is consistent with the existing structure and aligns with the PR's objectives to expand EVM functionalities.
14-14: Approved: New export forevmDeposit.The addition of
evmDepositto the exports is consistent with the existing structure and aligns with the PR's objectives to expand EVM functionalities.
15-15: Approved: New export forevmDepositAndCall.The addition of
evmDepositAndCallto the exports is consistent with the existing structure and aligns with the PR's objectives to expand EVM functionalities.
16-16: Approved: New export forsolanaDeposit.The addition of
solanaDepositto the exports is consistent with the existing structure and aligns with the PR's objectives to expand Solana functionalities.
19-19: Approved: New export forzetachainCall.The addition of
zetachainCallto the exports is consistent with the existing structure and aligns with the PR's objectives to expand ZetaChain functionalities.
20-20: Approved: New export forzetachainWithdraw.The addition of
zetachainWithdrawto the exports is consistent with the existing structure and aligns with the PR's objectives to expand ZetaChain functionalities.
21-21: Approved: New export forzetachainWithdrawAndCall.The addition of
zetachainWithdrawAndCallto the exports is consistent with the existing structure and aligns with the PR's objectives to expand ZetaChain functionalities.packages/client/src/client.ts (1)
20-20: Approve the addition of thesolanaDepositmethod.The integration of the
solanaDepositmethod into theZetaChainClientclass is seamless and enhances its functionality by allowing it to handle Solana-specific deposits. The method is correctly imported and assigned within the class structure.Also applies to: 130-130
packages/client/src/solanaDeposit.ts (2)
10-78: Enhance error handling and security in blockchain transactions.The function
solanaDepositperforms critical operations involving blockchain transactions. Here are a few suggestions to enhance its robustness and security:
Error Handling: The current implementation logs errors to the console. Consider rethrowing the error or handling it in a way that the calling function can react appropriately, especially in a production environment where logging might not be sufficient.
Environmental Variables: The function uses
process.env.HOMEdirectly. Ensure that there is a fallback or default value if this environmental variable is not set, to prevent runtime errors.Input Validation: Add checks to validate the
argsobject to ensure all required properties are present and correctly formatted before proceeding with the transaction.Security: Review the security implications of reading files and handling private keys within the application. Ensure that the key management practices align with best security practices, such as using secure storage and handling mechanisms.
Tools
GitHub Check: build
[warning] 10-10:
Prefer using arrow functions over plain functions
81-110: Refine file handling and error management ingetKeypairFromFile.This function handles sensitive operations involving file paths and key management. Consider the following improvements:
Dynamic Imports: The use of dynamic imports for
pathandfs/promisesmodules is unconventional. Consider importing these modules statically at the top of the file for better performance and clarity.Error Messages: The error messages could be more descriptive to aid in debugging. For example, include the type of error caught when parsing the file contents.
Path Handling: The handling of the
~symbol for home directory paths is a good feature. Ensure that this functionality is tested across different operating systems to avoid path resolution issues.Security: Since this function deals with private keys, ensure that the file access permissions are checked and that the keys are never logged or improperly exposed.
packages/tasks/src/zetachainWithdraw.ts (1)
7-81: Improve error handling and transaction management inzetachainWithdraw.The function
zetachainWithdrawis crucial for handling token withdrawals. Consider the following enhancements to improve its reliability and security:
Error Handling: The current implementation catches errors and logs them. Consider enhancing this by adding more detailed error information and possibly rethrowing the error or handling it in a way that allows the calling function to react appropriately.
Transaction Management: The function performs several blockchain operations that could benefit from more detailed logging or monitoring, especially in a production environment where tracking transaction statuses is crucial.
Input Validation: Ensure that all inputs, especially those related to contract addresses and amounts, are validated before use to prevent errors and potential security vulnerabilities.
Security Practices: Review the security practices around handling private keys and interacting with smart contracts to ensure that they are in line with best practices.
packages/tasks/src/evmDepositAndCall.ts (1)
7-80: Enhance error handling and transaction management inevmDepositAndCall.The function
evmDepositAndCallis critical for handling deposits and calls on the EVM. Consider the following enhancements to improve its reliability and security:
Error Handling: The current implementation catches errors and logs them. Consider enhancing this by adding more detailed error information and possibly rethrowing the error or handling it in a way that allows the calling function to react appropriately.
Transaction Management: The function performs several blockchain operations that could benefit from more detailed logging or monitoring, especially in a production environment where tracking transaction statuses is crucial.
Input Validation: Ensure that all inputs, especially those related to contract addresses and amounts, are validated before use to prevent errors and potential security vulnerabilities.
Security Practices: Review the security practices around handling private keys and interacting with smart contracts to ensure that they are in line with best practices.
packages/tasks/src/zetachainCall.ts (1)
1-2: Review of imports and task configuration
- The imports are well-organized and relevant to the functionality being implemented.
- The task configuration is comprehensive, covering various parameters needed for blockchain interactions.
Also applies to: 4-5, 78-122
package.json (1)
90-95: Review of new dependencies
- Correctness: The versions specified for
@coral-xyz/anchorand@solana/web3.jsare appropriate and use semantic versioning correctly.- Security: Ensure that these libraries are from trusted sources and are widely used in the community to avoid potential security risks.
- Best Practices: Regularly update these dependencies to keep up with new features and security patches.
packages/tasks/src/zetachainWithdrawAndCall.ts (1)
96-139: Review Task Parameter ConfigurationsThe task
zetachain-withdraw-and-callis well-configured with several parameters to control its operation. However, ensure that the default values and types for these parameters are appropriate for all expected use cases. For example, the default gas price and gas limit at lines 114-123 are critical for transaction cost and execution feasibility.Consider reviewing these settings with the team to ensure they align with the current network conditions and expected transaction complexity.
typechain-types/index.ts (1)
49-58: Good organization and clarity in updates.The updates to paths and the addition of new types and factories enhance the clarity and modularity of the contract interfaces. This organization aids in maintaining a clear structure within the project, which is beneficial for both development and maintenance.
packages/client/src/idl/gateway.json (5)
1-8: Verify program metadata and address.Ensure that the
addressand metadata fields such asname,version, anddescriptionaccurately reflect the deployed program and its purpose. This is crucial for maintainability and user understanding.
9-398: Review instruction definitions for correctness and security.Verify the uniqueness of each instruction's
discriminatorto ensure correct operation routing. Additionally, check that account permissions (writable,signer) and argument types are correctly set according to the intended functionality, which is critical for the security and proper execution of the program.Verification successful
Verification Successful: Unique Discriminators and Correct Account Permissions
The discriminators for each instruction are unique, ensuring proper routing of operations. Additionally, the account permissions and argument types are correctly configured, aligning with the intended functionality and security requirements.
- Discriminators are unique across all instructions.
- Account permissions (
writable,signer) and argument types are correctly set.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the uniqueness of discriminators and correctness of account permissions and argument types. # Test: Check discriminators for uniqueness and review account permissions and argument types. echo "Review discriminators for uniqueness in the instruction set." echo "Check account permissions and argument types for each instruction against the program specification."Length of output: 341
Script:
#!/bin/bash # Extract discriminators and check for uniqueness jq '.instructions[].discriminator' packages/client/src/idl/gateway.json | sort | uniq -d # Verify account permissions and argument types jq '.instructions[] | {name: .name, accounts: .accounts, args: .args}' packages/client/src/idl/gateway.jsonLength of output: 4740
415-460: Review error definitions for uniqueness and clarity.Verify that each error code is unique and that the messages clearly describe the conditions under which the errors occur. This is essential for debugging and user feedback.
Verification successful
All error codes and messages are unique and distinct.
The verification process confirms that each error code in the
gateway.jsonfile is unique, and all error messages are distinct. This ensures clarity and effectiveness in debugging and user feedback. No further action is required.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the uniqueness of error codes and clarity of error messages. # Test: Check error codes for uniqueness and review error messages for clarity. echo "Review error codes for uniqueness and error messages for clarity in the program configuration."Length of output: 198
Script:
#!/bin/bash # Extract error codes from the JSON file and check for duplicates. # Use jq to parse the JSON and extract error codes. error_codes=$(jq -r '.errors[].code' packages/client/src/idl/gateway.json) # Check for duplicate error codes. duplicate_codes=$(echo "$error_codes" | sort | uniq -d) # Output the duplicate error codes, if any. if [ -n "$duplicate_codes" ]; then echo "Duplicate error codes found:" echo "$duplicate_codes" else echo "All error codes are unique." fi # Extract error messages and check for clarity (distinctiveness). error_messages=$(jq -r '.errors[].msg' packages/client/src/idl/gateway.json) # Check for duplicate error messages. duplicate_messages=$(echo "$error_messages" | sort | uniq -d) # Output the duplicate error messages, if any. if [ -n "$duplicate_messages" ]; then echo "Duplicate error messages found:" echo "$duplicate_messages" else echo "All error messages are distinct." fiLength of output: 870
462-496: Verify thePdatype definition and field types.Ensure that the
Pdatype definition and its fields (nonce,tss_address,authority,chain_id,deposit_paused) are correctly defined according to the program's requirements. This is crucial for the integrity and functionality of the program.
400-413: Verify thePdaaccount discriminator.Ensure that the discriminator for the
Pdaaccount is unique and correctly configured to prevent any potential conflicts or misrouting within the program.typechain-types/hardhat.d.ts (6)
99-102: Approved: NewgetContractFactorymethod forIZRC20Metadata.The addition follows existing patterns and enhances the Hardhat environment's functionality by allowing the creation of
IZRC20Metadatacontract factories.
104-106: Approved: NewgetContractFactorymethod forZRC20Events.The addition follows existing patterns and enhances the Hardhat environment's functionality by allowing the creation of
ZRC20Eventscontract factories.
108-110: Approved: NewgetContractFactorymethod forUniversalContract.The addition follows existing patterns and enhances the Hardhat environment's functionality by allowing the creation of
UniversalContractcontract factories.
293-297: Approved: NewgetContractAtmethod forIZRC20Metadata.The addition follows existing patterns and enhances the Hardhat environment's functionality by allowing the retrieval of
IZRC20Metadatacontract instances.
299-302: Approved: NewgetContractAtmethod forZRC20Events.The addition follows existing patterns and enhances the Hardhat environment's functionality by allowing the retrieval of
ZRC20Eventscontract instances.
304-307: Approved: NewgetContractAtmethod forUniversalContract.The addition follows existing patterns and enhances the Hardhat environment's functionality by allowing the retrieval of
UniversalContractcontract instances.typechain-types/factories/contracts/shared/MockSystemContract.sol/MockSystemContract__factory.ts (2)
345-345: Verify the updated bytecode for correctness and efficiency.The bytecode has been updated, which is a critical change as it directly affects the contract's behavior on the blockchain. It is essential to ensure that this new bytecode has been thoroughly tested and reviewed for correctness, security vulnerabilities, and gas efficiency.
Please confirm that the new bytecode has been audited and benchmarked for gas usage to prevent any potential issues in production.
345-345: Class Implementation Review:MockSystemContract__factory.The class is well-implemented with methods to deploy, attach, and connect to the
MockSystemContract. It correctly handles different constructor arguments and provides overrides for deployment transactions. This setup is crucial for flexibility and ease of use in different deployment scenarios.Ensure that all methods are covered by unit tests to verify their functionality across various use cases.
typechain-types/factories/@zetachain/protocol-contracts/contracts/zevm/SystemContract.sol/SystemContract__factory.ts (2)
432-432: Critical Review: Updated Bytecode inSystemContract__factory.The bytecode for
SystemContracthas been updated significantly. It is crucial to ensure that this bytecode is correct and optimized for gas usage, especially given the potential complexity of the contract's functionality.Please confirm that this bytecode has undergone thorough testing and security audits, particularly focusing on gas efficiency and security vulnerabilities.
432-432: Class Implementation Review:SystemContract__factory.The implementation of
SystemContract__factoryis robust, with comprehensive methods to manage deployment and interaction with theSystemContract. The new type declaration for constructor parameters is a welcome addition, enhancing the flexibility and usability of the factory.It is recommended to ensure comprehensive unit testing for all methods to validate their functionality in various scenarios.
typechain-types/factories/contracts/OnlySystem__factory.ts (1)
27-27: Verify the updated bytecode.The bytecode for the
OnlySystemcontract has been updated. It's crucial to ensure that this new bytecode corresponds accurately to the intended changes in the contract logic. Please verify the bytecode by comparing it with the output of the Solidity compiler to ensure it matches the new contract logic.typechain-types/factories/contracts/TestSystemContract__factory.ts (1)
278-278: Verify the updated bytecode.The bytecode for the
TestSystemContracthas been updated. It's crucial to ensure that this new bytecode corresponds accurately to the intended changes in the contract logic. Please verify the bytecode by comparing it with the output of the Solidity compiler to ensure it matches the new contract logic.typechain-types/@zetachain/protocol-contracts/contracts/zevm/interfaces/IZRC20.sol/IZRC20.ts (3)
28-28: Verify consistency of renamingPROTOCOL_FEE()toPROTOCOL_FLAT_FEE().Ensure that all references to the old
PROTOCOL_FEE()function are updated toPROTOCOL_FLAT_FEE()across the entire interface to maintain consistency and avoid potential integration errors.Also applies to: 43-43, 57-57, 106-106, 159-159, 213-213, 267-267, 324-324
32-32: Approve simplification ofburnfunction but verify impact.The simplification of the
burnfunction to only require the amount parameter is approved as it can enhance usability. However, ensure that this change aligns with the intended token burning logic, especially in scenarios where tokens might need to be burned from different accounts.Also applies to: 74-74, 190-190, 250-250, 310-310
28-28: Verify rationale behind removingdecreaseAllowanceandincreaseAllowance.Confirm that the removal of
decreaseAllowanceandincreaseAllowancefunctions aligns with the strategic goals for managing token allowances. Ensure that the contract still supports robust management of allowances through other means if these functions are removed.Also applies to: 43-43
typechain-types/factories/contracts/SwapHelperLib__factory.ts (1)
139-139: Updated bytecode needs verification.The bytecode for
SwapHelperLibhas been updated. It's crucial to ensure that this new bytecode corresponds accurately to the intended smart contract logic changes. This verification is essential to prevent any unintended behaviors in the deployed contract.
| export const solanaDeposit = async ( | ||
| args: any, | ||
| hre: HardhatRuntimeEnvironment | ||
| ) => { | ||
| const client = new ZetaChainClient({ network: "testnet" }); | ||
| let recipient; | ||
| try { | ||
| if ((bech32 as any).decode(args.recipient)) { | ||
| recipient = utils.solidityPack( | ||
| ["bytes"], | ||
| [utils.toUtf8Bytes(args.recipient)] | ||
| ); | ||
| } | ||
| } catch (e) { | ||
| recipient = args.recipient; | ||
| } | ||
| const { amount, api, idPath } = args; | ||
| const params = [JSON.parse(args.types), args.values]; | ||
| await client.solanaDeposit({ amount, api, idPath, params, recipient }); | ||
| }; |
There was a problem hiding this comment.
Enhance error handling and parameter validation in solanaDeposit.
The function solanaDeposit currently handles errors generically. Consider improving error handling by catching specific exceptions and providing more detailed error messages. Additionally, validate the input parameters to ensure they meet the expected format and type before proceeding with the deposit operation.
For instance, validate args.amount to ensure it's a positive number and args.recipient to confirm it's a valid address format. Here's a proposed enhancement:
+ if (typeof args.amount !== 'number' || args.amount <= 0) {
+ throw new Error('Invalid amount. Amount must be a positive number.');
+ }
+ if (!args.recipient) {
+ throw new Error('Recipient address is required.');
+ }This will help prevent runtime errors and improve the robustness of the deposit functionality.
Committable suggestion was skipped due to low confidence.
| export class UniversalContract__factory { | ||
| static readonly abi = _abi; | ||
| static createInterface(): UniversalContractInterface { | ||
| return new utils.Interface(_abi) as UniversalContractInterface; | ||
| } | ||
| static connect( | ||
| address: string, | ||
| signerOrProvider: Signer | Provider | ||
| ): UniversalContract { | ||
| return new Contract(address, _abi, signerOrProvider) as UniversalContract; | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactor to use simpler structures instead of a class with only static members.
The class UniversalContract__factory only contains static members, which could be simplified into standalone functions or a simple object. This would reduce complexity and align with best practices in TypeScript development.
Consider refactoring the class into a module with exported functions or an object. Here's a proposed refactor:
export const UniversalContractFactory = {
abi: _abi,
createInterface: (): UniversalContractInterface => new utils.Interface(_abi) as UniversalContractInterface,
connect: (address: string, signerOrProvider: Signer | Provider): UniversalContract => new Contract(address, _abi, signerOrProvider) as UniversalContract,
};Tools
Biome
[error] 105-116: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/tasks/src/evmDeposit.ts
Outdated
| export const evmDeposit = async (args: any, hre: HardhatRuntimeEnvironment) => { | ||
| const [signer] = await hre.ethers.getSigners(); | ||
| const { utils } = hre.ethers; | ||
|
|
||
| const gateway = new hre.ethers.Contract( | ||
| args.gatewayEvm, | ||
| GatewayABI.abi, | ||
| signer | ||
| ); | ||
|
|
||
| const revertOptions = { | ||
| abortAddress: "0x0000000000000000000000000000000000000000", | ||
| callOnRevert: args.callOnRevert, | ||
| onRevertGasLimit: args.onRevertGasLimit, | ||
| revertAddress: args.revertAddress, | ||
| // not used | ||
| revertMessage: utils.hexlify(utils.toUtf8Bytes(args.revertMessage)), | ||
| }; | ||
|
|
||
| const txOptions = { | ||
| gasLimit: args.gasLimit, | ||
| gasPrice: args.gasPrice, | ||
| }; | ||
|
|
||
| try { | ||
| let tx; | ||
| if (args.erc20) { | ||
| const erc20Contract = new hre.ethers.Contract( | ||
| args.erc20, | ||
| ERC20_ABI.abi, | ||
| signer | ||
| ); | ||
| const decimals = await erc20Contract.decimals(); | ||
| const value = utils.parseUnits(args.amount, decimals); | ||
| await erc20Contract.connect(signer).approve(args.gatewayEvm, value); | ||
| const method = | ||
| "deposit(address,uint256,address,(address,bool,address,bytes,uint256))"; | ||
| tx = await gateway[method]( | ||
| args.receiver, | ||
| value, | ||
| args.erc20, | ||
| revertOptions, | ||
| txOptions | ||
| ); | ||
| } else { | ||
| const value = utils.parseEther(args.amount); | ||
| const method = "deposit(address,(address,bool,address,bytes,uint256))"; | ||
| tx = await gateway[method](args.receiver, revertOptions, { | ||
| ...txOptions, | ||
| value, | ||
| }); | ||
| } | ||
| if (tx) { | ||
| const receipt = await tx.wait(); | ||
| console.log("Transaction hash:", receipt.transactionHash); | ||
| } | ||
| } catch (e) { | ||
| console.error("Transaction error:", e); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Enhance logging for better traceability.
The evmDeposit task function is well-implemented with comprehensive error handling and parameter management. However, consider enhancing the logging mechanism to provide more structured and searchable logs, especially useful in production environments.
Consider using a logging library that supports structured logs, such as Winston or Bunyan, to replace console.log and console.error. This would allow for better monitoring and troubleshooting.
Example using Winston:
import { createLogger, format, transports } from 'winston';
const logger = createLogger({
level: 'info',
format: format.combine(
format.timestamp(),
format.json()
),
transports: [
new transports.Console(),
new transports.File({ filename: 'evm-deposit.log' })
]
});
logger.info('Transaction hash:', { transactionHash: receipt.transactionHash });
logger.error('Transaction error:', { error: e });
packages/tasks/src/zetachainCall.ts
Outdated
| export const zetachainCall = async ( | ||
| args: any, | ||
| hre: HardhatRuntimeEnvironment | ||
| ) => { | ||
| const [signer] = await hre.ethers.getSigners(); | ||
| const { utils } = hre.ethers; | ||
|
|
||
| const gateway = new hre.ethers.Contract( | ||
| args.gatewayZetaChain, | ||
| GatewayABI.abi, | ||
| signer | ||
| ); | ||
|
|
||
| const revertOptions = { | ||
| abortAddress: "0x0000000000000000000000000000000000000000", | ||
| callOnRevert: args.callOnRevert, | ||
| onRevertGasLimit: args.onRevertGasLimit, | ||
| revertAddress: args.revertAddress, | ||
| // not used | ||
| revertMessage: utils.hexlify(utils.toUtf8Bytes(args.revertMessage)), | ||
| }; | ||
|
|
||
| const txOptions = { | ||
| gasLimit: args.gasLimit, | ||
| gasPrice: args.gasPrice, | ||
| }; | ||
|
|
||
| const functionSignature = utils.id(args.function).slice(0, 10); | ||
| const encodedParameters = utils.defaultAbiCoder.encode( | ||
| JSON.parse(args.types), | ||
| args.values | ||
| ); | ||
|
|
||
| const message = utils.hexlify( | ||
| utils.concat([functionSignature, encodedParameters]) | ||
| ); | ||
|
|
||
| try { | ||
| const zrc20 = new hre.ethers.Contract(args.zrc20, ZRC20ABI.abi, signer); | ||
| const [gasZRC20, gasFee] = await zrc20.withdrawGasFeeWithGasLimit( | ||
| args.gasLimit | ||
| ); | ||
| const gasZRC20Contract = new hre.ethers.Contract( | ||
| gasZRC20, | ||
| ZRC20ABI.abi, | ||
| signer | ||
| ); | ||
| const approve = await gasZRC20Contract.approve( | ||
| args.gatewayZetaChain, | ||
| gasFee, | ||
| txOptions | ||
| ); | ||
| await approve.wait(); | ||
| const tx = await gateway[ | ||
| "call(bytes,address,bytes,uint256,(address,bool,address,bytes,uint256))" | ||
| ]( | ||
| utils.hexlify(args.receiver), | ||
| gasZRC20, | ||
| message, | ||
| args.callGasLimit, | ||
| revertOptions, | ||
| txOptions | ||
| ); | ||
|
|
||
| const receipt = await tx.wait(); | ||
| console.log("Transaction hash:", receipt.transactionHash); | ||
| } catch (e) { | ||
| console.error("Transaction error:", e); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Review of the zetachainCall function
- Correctness: The function correctly initializes contracts and handles transactions. However, the error handling could be improved by providing more specific error messages based on the type of error encountered.
- Security: The function uses
awaitproperly to handle asynchronous calls, which is crucial in blockchain interactions to prevent race conditions. - Performance: The use of
utils.hexlifyandutils.concatfor message encoding is efficient for blockchain transactions. - Best Practices: The function could benefit from destructuring
argsto improve readability and maintainability.
Suggestions:
- Improve error handling by categorizing errors and providing more detailed messages.
- Consider destructuring
argsto enhance code clarity.
- console.error("Transaction error:", e);
+ console.error("Transaction error:", e.message); // Provide more specific error details- const { gatewayZetaChain, zrc20, receiver, function, types, values, gasLimit, gasPrice, callGasLimit, revertAddress, revertMessage, callOnRevert, onRevertGasLimit } = args;
+ const [signer] = await hre.ethers.getSigners();
+ const { utils } = hre.ethers;
+ const gateway = new hre.ethers.Contract(gatewayZetaChain, GatewayABI.abi, signer);Committable suggestion was skipped due to low confidence.
| export class ZRC20Events__factory { | ||
| static readonly abi = _abi; | ||
| static createInterface(): ZRC20EventsInterface { | ||
| return new utils.Interface(_abi) as ZRC20EventsInterface; | ||
| } | ||
| static connect( | ||
| address: string, | ||
| signerOrProvider: Signer | Provider | ||
| ): ZRC20Events { | ||
| return new Contract(address, _abi, signerOrProvider) as ZRC20Events; | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactor suggestion for ZRC20Events__factory
- Architecture Advice: Consider refactoring the class to use simple functions instead of static members to reduce complexity and improve maintainability.
- Best Practices: This change would align with modern JavaScript practices that favor functional programming for simpler constructs.
- export class ZRC20Events__factory {
- static readonly abi = _abi;
- static createInterface(): ZRC20EventsInterface {
- return new utils.Interface(_abi) as ZRC20EventsInterface;
- }
- static connect(
- address: string,
- signerOrProvider: Signer | Provider
- ): ZRC20Events {
- return new Contract(address, _abi, signerOrProvider) as ZRC20Events;
- }
- }
+ export const ZRC20EventsFactory = {
+ abi: _abi,
+ createInterface: (): ZRC20EventsInterface => new utils.Interface(_abi) as ZRC20EventsInterface,
+ connect: (address: string, signerOrProvider: Signer | Provider): ZRC20Events =>
+ new Contract(address, _abi, signerOrProvider) as ZRC20Events,
+ };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.
| export class ZRC20Events__factory { | |
| static readonly abi = _abi; | |
| static createInterface(): ZRC20EventsInterface { | |
| return new utils.Interface(_abi) as ZRC20EventsInterface; | |
| } | |
| static connect( | |
| address: string, | |
| signerOrProvider: Signer | Provider | |
| ): ZRC20Events { | |
| return new Contract(address, _abi, signerOrProvider) as ZRC20Events; | |
| } | |
| } | |
| export const ZRC20EventsFactory = { | |
| abi: _abi, | |
| createInterface: (): ZRC20EventsInterface => new utils.Interface(_abi) as ZRC20EventsInterface, | |
| connect: (address: string, signerOrProvider: Signer | Provider): ZRC20Events => | |
| new Contract(address, _abi, signerOrProvider) as ZRC20Events, | |
| }; |
Tools
Biome
[error] 166-177: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
| export class IZRC20Metadata__factory { | ||
| static readonly abi = _abi; | ||
| static createInterface(): IZRC20MetadataInterface { | ||
| return new utils.Interface(_abi) as IZRC20MetadataInterface; | ||
| } | ||
| static connect( | ||
| address: string, | ||
| signerOrProvider: Signer | Provider | ||
| ): IZRC20Metadata { | ||
| return new Contract(address, _abi, signerOrProvider) as IZRC20Metadata; | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider refactoring the class to use simple functions.
The static analysis tool flagged the use of a class with only static members. In TypeScript, it's often more modular and manageable to use simple functions instead of classes if the class does not maintain state. Consider refactoring this class to a set of exported functions if it aligns with the overall architecture and coding standards of the project.
Tools
Biome
[error] 285-296: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
| } from "../../../../../../common"; | ||
|
|
||
| export interface ZRC20EventsInterface extends utils.Interface { | ||
| functions: {}; |
There was a problem hiding this comment.
Define explicit types instead of using '{}'.
The use of '{}' as a type is flagged multiple times by the static analysis tool. It's recommended to define explicit types for better type safety and clarity. This change will enhance the maintainability and readability of the code.
Also applies to: 145-145, 147-147, 216-216, 218-218
Tools
Biome
[error] 16-16: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
packages/tasks/src/evmCall.ts
Outdated
| export const evmCall = async (args: any, hre: HardhatRuntimeEnvironment) => { | ||
| const [signer] = await hre.ethers.getSigners(); | ||
| const { utils } = hre.ethers; | ||
|
|
||
| const gateway = new hre.ethers.Contract( | ||
| args.gatewayEvm, | ||
| GatewayABI.abi, | ||
| signer | ||
| ); | ||
|
|
||
| const encodedParameters = utils.defaultAbiCoder.encode( | ||
| JSON.parse(args.types), | ||
| args.values | ||
| ); | ||
|
|
||
| try { | ||
| const tx = await gateway[ | ||
| "call(address,bytes,(address,bool,address,bytes,uint256))" | ||
| ]( | ||
| args.receiver, | ||
| encodedParameters, | ||
| { | ||
| abortAddress: "0x0000000000000000000000000000000000000000", | ||
| callOnRevert: args.callOnRevert, | ||
| onRevertGasLimit: args.onRevertGasLimit, | ||
| revertAddress: args.revertAddress, | ||
| // not used | ||
| revertMessage: utils.hexlify(utils.toUtf8Bytes(args.revertMessage)), | ||
| }, | ||
| { | ||
| gasLimit: args.gasLimit, | ||
| gasPrice: args.gasPrice, | ||
| } | ||
| ); | ||
| const receipt = await tx.wait(); | ||
| console.log("Transaction hash:", receipt.transactionHash); | ||
| } catch (e) { | ||
| console.error("Transaction error:", e); | ||
| } |
There was a problem hiding this comment.
Review the implementation of evmCall task.
The evmCall function is well-implemented with comprehensive parameter handling and error management. However, consider the following improvements:
- Parameter Defaults and Hardcoding: The use of hardcoded addresses and default values (lines 51-52, 58, 63, 69, 75) could be replaced with environment variables or configuration files to enhance flexibility and security.
- Error Logging: Enhance the error logging in line 43 to include more context about the error, possibly by logging additional error details.
These changes would make the code more robust and maintainable.
| value: BigNumber; | ||
| gasFee: BigNumber; | ||
| protocolFlatFee: BigNumber; | ||
| events: {}; |
There was a problem hiding this comment.
Question removal of all events from the contract interface.
The removal of events could impact system integrations and transparency. Discuss the rationale behind this decision and consider the potential need for events to facilitate external monitoring and interaction tracking.
Tools
Biome
[error] 129-129: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
Define a more explicit type for the events object.
Replace the '{}' type with a more explicit interface or type definition to enhance code clarity and maintainability. This change will also address the static analysis tool's hint and improve type safety.
Tools
Biome
[error] 129-129: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
| @@ -0,0 +1,474 @@ | |||
| /* Autogenerated file. Do not edit manually. */ | |||
There was a problem hiding this comment.
Comprehensive review of the new IZRC20Metadata interface.
The new IZRC20Metadata interface file is well-structured and aligns with the Ethereum smart contract development standards. However, there are a few areas for improvement:
- The use of
{}as a type in theeventsandfiltersproperties should be replaced with more specific type definitions. - Ensure that all method signatures and property types are correctly aligned with the ZRC20 standard and Ethereum best practices.
- events: {};
- filters: {};
+ events: Record<string, TypedEventFilter<any>>;
+ filters: Record<string, TypedEventFilter<any>>;This change enhances type safety and clarity by explicitly defining the expected types for events and filters.
Also applies to: 3-3, 5-24, 26-26, 44-44, 62-62, 114-114, 141-141, 144-144, 170-170, 231-231, 290-290, 351-351, 414-414
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range, codebase verification and nitpick comments (1)
typechain-types/factories/contracts/SwapHelperLib__factory.ts (1)
Line range hint
1-98: Ensure TypeScript best practices are followed in the factory implementation.The TypeScript implementation uses advanced features like conditional types and generics. Ensure that these are used correctly and efficiently to provide the best developer experience and type safety. Review the use of overrides and ensure that they are necessary and correctly applied.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- packages/client/src/client.ts (3 hunks)
- packages/client/src/evmCall.ts (1 hunks)
- packages/client/src/evmDeposit.ts (1 hunks)
- packages/client/src/evmDepositAndCall.ts (1 hunks)
- packages/client/src/index.ts (2 hunks)
- packages/client/src/zetachainCall.ts (1 hunks)
- packages/client/src/zetachainWithdraw.ts (1 hunks)
- packages/client/src/zetachainWithdrawAndCall.ts (1 hunks)
- packages/tasks/src/evmCall.ts (1 hunks)
- packages/tasks/src/evmDeposit.ts (1 hunks)
- packages/tasks/src/evmDepositAndCall.ts (1 hunks)
- packages/tasks/src/zetachainCall.ts (1 hunks)
- packages/tasks/src/zetachainWithdraw.ts (1 hunks)
- packages/tasks/src/zetachainWithdrawAndCall.ts (1 hunks)
- typechain-types/factories/contracts/SwapHelperLib__factory.ts (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- packages/client/src/client.ts
- packages/tasks/src/evmCall.ts
- packages/tasks/src/evmDeposit.ts
- packages/tasks/src/evmDepositAndCall.ts
- packages/tasks/src/zetachainCall.ts
- packages/tasks/src/zetachainWithdraw.ts
- packages/tasks/src/zetachainWithdrawAndCall.ts
Additional comments not posted (8)
packages/client/src/index.ts (7)
3-3: Export ofevmCallis correctly implemented.This export statement correctly exposes the
evmCallfunctionality to other parts of the application.
4-4: Export ofevmDepositis correctly implemented.This export statement correctly exposes the
evmDepositfunctionality to other parts of the application.
5-5: Export ofevmDepositAndCallis correctly implemented.This export statement correctly exposes the
evmDepositAndCallfunctionality to other parts of the application.
17-17: Export ofsolanaDepositis correctly implemented.This export statement correctly exposes the
solanaDepositfunctionality to other parts of the application.
20-20: Export ofzetachainCallis correctly implemented.This export statement correctly exposes the
zetachainCallfunctionality to other parts of the application.
21-21: Export ofzetachainWithdrawis correctly implemented.This export statement correctly exposes the
zetachainWithdrawfunctionality to other parts of the application.
22-22: Export ofzetachainWithdrawAndCallis correctly implemented.This export statement correctly exposes the
zetachainWithdrawAndCallfunctionality to other parts of the application.typechain-types/factories/contracts/SwapHelperLib__factory.ts (1)
139-139: Verify the updated bytecode for correctness and optimization.The bytecode for
SwapHelperLibhas been updated. It's crucial to ensure that this bytecode corresponds correctly to the intended smart contract logic and optimizations. Consider verifying this against the Solidity source to ensure that the changes are accurate and efficient.
| export const zetachainCall = async function ( | ||
| this: ZetaChainClient, | ||
| args: { | ||
| amount: string; | ||
| callOnRevert: boolean; | ||
| function: string; | ||
| gasLimit: number; | ||
| gasPrice: ethers.BigNumber; | ||
| gatewayZetaChain: string; | ||
| onRevertGasLimit: number; | ||
| receiver: string; | ||
| revertAddress: string; | ||
| revertMessage: string; | ||
| types: string; | ||
| values: any[]; | ||
| zrc20: string; | ||
| } | ||
| ) { | ||
| const signer = this.signer; | ||
| const { utils } = ethers; | ||
|
|
||
| const gateway = new ethers.Contract( | ||
| args.gatewayZetaChain, | ||
| GatewayABI.abi, | ||
| signer | ||
| ); | ||
|
|
||
| const revertOptions = { | ||
| abortAddress: "0x0000000000000000000000000000000000000000", | ||
| callOnRevert: args.callOnRevert, | ||
| onRevertGasLimit: args.onRevertGasLimit, | ||
| revertAddress: args.revertAddress, | ||
| // not used | ||
| revertMessage: utils.hexlify(utils.toUtf8Bytes(args.revertMessage)), | ||
| }; | ||
|
|
||
| const txOptions = { | ||
| gasLimit: args.gasLimit, | ||
| gasPrice: args.gasPrice, | ||
| }; | ||
|
|
||
| const functionSignature = utils.id(args.function).slice(0, 10); | ||
| const encodedParameters = utils.defaultAbiCoder.encode( | ||
| JSON.parse(args.types), | ||
| args.values | ||
| ); | ||
|
|
||
| const message = utils.hexlify( | ||
| utils.concat([functionSignature, encodedParameters]) | ||
| ); | ||
| const zrc20 = new ethers.Contract(args.zrc20, ZRC20ABI.abi, signer); | ||
| const [gasZRC20, gasFee] = await zrc20.withdrawGasFeeWithGasLimit( | ||
| args.gasLimit | ||
| ); | ||
| const gasZRC20Contract = new ethers.Contract(gasZRC20, ZRC20ABI.abi, signer); | ||
| const approve = await gasZRC20Contract.approve( | ||
| args.gatewayZetaChain, | ||
| gasFee, | ||
| txOptions | ||
| ); | ||
| await approve.wait(); | ||
| const tx = await gateway[ | ||
| "call(bytes,address,bytes,uint256,(address,bool,address,bytes,uint256))" | ||
| ]( | ||
| utils.hexlify(args.receiver), | ||
| gasZRC20, | ||
| message, | ||
| args.gasLimit, | ||
| revertOptions, | ||
| txOptions | ||
| ); | ||
| return tx; | ||
| }; |
There was a problem hiding this comment.
Review of zetachainCall Function: Security and Efficiency Concerns
- Security of Contract Interactions: The function interacts with contracts using addresses provided in the arguments. It's crucial to ensure these addresses are validated to prevent interactions with potentially malicious contracts.
- Efficiency of Gas Usage: The function calculates gas fees and handles transaction options. It's important to optimize gas usage, especially in loops or repeated contract calls.
- Error Handling: The function should include comprehensive error handling to manage exceptions during blockchain interactions, especially in contract calls and transaction submissions.
- Use of
ethersLibrary: The function makes extensive use of theetherslibrary. Ensure that the library's functions are used correctly and efficiently, particularly in encoding and transaction handling.
Consider adding validation for contract addresses and optimizing gas usage. Enhance error handling to manage exceptions more effectively.
| export const evmDepositAndCall = async function ( | ||
| this: ZetaChainClient, | ||
| args: { | ||
| amount: string; | ||
| callOnRevert: boolean; | ||
| erc20: string; | ||
| gasLimit: number; | ||
| gasPrice: ethers.BigNumber; | ||
| gatewayEvm: string; | ||
| onRevertGasLimit: number; | ||
| receiver: string; | ||
| revertAddress: string; | ||
| revertMessage: string; | ||
| types: string; | ||
| values: any[]; | ||
| } | ||
| ) { | ||
| const signer = this.signer; | ||
| const { utils } = ethers; | ||
| const gateway = new ethers.Contract(args.gatewayEvm, GatewayABI.abi, signer); | ||
|
|
||
| const revertOptions = { | ||
| abortAddress: "0x0000000000000000000000000000000000000000", | ||
| callOnRevert: args.callOnRevert, | ||
| onRevertGasLimit: args.onRevertGasLimit, | ||
| revertAddress: args.revertAddress, | ||
| // not used | ||
| revertMessage: utils.hexlify(utils.toUtf8Bytes(args.revertMessage)), | ||
| }; | ||
|
|
||
| const txOptions = { | ||
| gasLimit: args.gasLimit, | ||
| gasPrice: args.gasPrice, | ||
| }; | ||
|
|
||
| const encodedParameters = utils.defaultAbiCoder.encode( | ||
| JSON.parse(args.types), | ||
| args.values | ||
| ); | ||
| let tx; | ||
| if (args.erc20) { | ||
| const erc20Contract = new ethers.Contract( | ||
| args.erc20, | ||
| ERC20_ABI.abi, | ||
| signer | ||
| ); | ||
| const decimals = await erc20Contract.decimals(); | ||
| const value = utils.parseUnits(args.amount, decimals); | ||
| await erc20Contract.connect(signer).approve(args.gatewayEvm, value); | ||
| const method = | ||
| "depositAndCall(address,uint256,address,bytes,(address,bool,address,bytes,uint256))"; | ||
| tx = await gateway[method]( | ||
| args.receiver, | ||
| value, | ||
| args.erc20, | ||
| encodedParameters, | ||
| revertOptions, | ||
| txOptions | ||
| ); | ||
| } else { | ||
| const value = utils.parseEther(args.amount); | ||
| const method = | ||
| "depositAndCall(address,bytes,(address,bool,address,bytes,uint256))"; | ||
| tx = await gateway[method]( | ||
| args.receiver, | ||
| encodedParameters, | ||
| revertOptions, | ||
| { | ||
| ...txOptions, | ||
| value, | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| return tx; | ||
| }; |
There was a problem hiding this comment.
Review of evmDepositAndCall Function: Security and Efficiency Concerns
- Handling of ERC20 Tokens: The function interacts with ERC20 tokens for deposits. It's crucial to ensure that token addresses and amounts are validated to prevent issues like overflow or interactions with malicious tokens.
- Transaction Options Handling: The function configures transaction options such as gas limits and prices. Ensure these are optimized to prevent unnecessary high costs or failed transactions due to low gas limits.
- Error Handling and Reverts: The function should handle potential reverts or errors during the deposit and call operations, especially when interacting with external contracts.
- Use of
ethersLibrary: Ensure that theetherslibrary functions are used correctly, particularly in parsing token amounts and handling contract approvals.
Enhance validation of token addresses and amounts. Optimize transaction options for cost-efficiency. Improve error handling to manage exceptions and reverts more effectively.
| export const zetachainWithdraw = async function ( | ||
| this: ZetaChainClient, | ||
| args: { | ||
| amount: string; | ||
| callOnRevert: boolean; | ||
| gasLimit: number; | ||
| gasPrice: ethers.BigNumber; | ||
| gatewayZetaChain: string; | ||
| onRevertGasLimit: number; | ||
| receiver: string; | ||
| revertAddress: string; | ||
| revertMessage: string; | ||
| zrc20: string; | ||
| } | ||
| ) { | ||
| const signer = this.signer; | ||
| const { utils } = ethers; | ||
|
|
||
| const gateway = new ethers.Contract( | ||
| args.gatewayZetaChain, | ||
| GatewayABI.abi, | ||
| signer | ||
| ); | ||
|
|
||
| const revertOptions = { | ||
| abortAddress: "0x0000000000000000000000000000000000000000", | ||
| callOnRevert: args.callOnRevert, | ||
| onRevertGasLimit: args.onRevertGasLimit, | ||
| revertAddress: args.revertAddress, | ||
| revertMessage: utils.hexlify(utils.toUtf8Bytes(args.revertMessage)), | ||
| }; | ||
|
|
||
| const txOptions = { | ||
| gasLimit: args.gasLimit, | ||
| gasPrice: args.gasPrice, | ||
| }; | ||
|
|
||
| const zrc20 = new ethers.Contract(args.zrc20, ZRC20ABI.abi, signer); | ||
| const decimals = await zrc20.decimals(); | ||
| const value = utils.parseUnits(args.amount, decimals); | ||
| const [gasZRC20, gasFee] = await zrc20.withdrawGasFeeWithGasLimit( | ||
| args.gasLimit | ||
| ); | ||
| if (args.zrc20 === gasZRC20) { | ||
| const approveGasAndWithdraw = await zrc20.approve( | ||
| args.gatewayZetaChain, | ||
| value.add(gasFee), | ||
| txOptions | ||
| ); | ||
| await approveGasAndWithdraw.wait(); | ||
| } else { | ||
| const gasZRC20Contract = new ethers.Contract( | ||
| gasZRC20, | ||
| ZRC20ABI.abi, | ||
| signer | ||
| ); | ||
| const approveGas = await gasZRC20Contract.approve( | ||
| args.gatewayZetaChain, | ||
| gasFee, | ||
| txOptions | ||
| ); | ||
| await approveGas.wait(); | ||
| const approveWithdraw = await zrc20.approve( | ||
| args.gatewayZetaChain, | ||
| value, | ||
| txOptions | ||
| ); | ||
| await approveWithdraw.wait(); | ||
| } | ||
| const method = | ||
| "withdraw(bytes,uint256,address,(address,bool,address,bytes,uint256))"; | ||
| const tx = await gateway[method]( | ||
| utils.hexlify(args.receiver), | ||
| value, | ||
| args.zrc20, | ||
| revertOptions, | ||
| txOptions | ||
| ); | ||
| return tx; | ||
| }; |
There was a problem hiding this comment.
Review of zetachainWithdraw Function: Security and Efficiency Concerns
- Handling of ZRC20 Tokens: The function interacts with ZRC20 tokens for withdrawals. It's crucial to ensure that token addresses and amounts are validated to prevent issues like overflow or interactions with malicious tokens.
- Transaction Options Handling: The function configures transaction options such as gas limits and prices. Ensure these are optimized to prevent unnecessary high costs or failed transactions due to low gas limits.
- Error Handling and Reverts: The function should handle potential reverts or errors during the withdrawal operations, especially when interacting with external contracts.
- Use of
ethersLibrary: Ensure that theetherslibrary functions are used correctly, particularly in parsing token amounts and handling contract approvals.
Enhance validation of token addresses and amounts. Optimize transaction options for cost-efficiency. Improve error handling to manage exceptions and reverts more effectively.
| export const zetachainWithdrawAndCall = async function ( | ||
| this: ZetaChainClient, | ||
| args: { | ||
| amount: string; | ||
| callOnRevert: boolean; | ||
| function: string; | ||
| gasLimit: number; | ||
| gasPrice: ethers.BigNumber; | ||
| gatewayZetaChain: string; | ||
| onRevertGasLimit: number; | ||
| receiver: string; | ||
| revertAddress: string; | ||
| revertMessage: string; | ||
| types: string; | ||
| values: any[]; | ||
| zrc20: string; | ||
| } |
There was a problem hiding this comment.
Consider refactoring the function parameters into a structured object.
The function zetachainWithdrawAndCall takes a large number of parameters wrapped in an object. This is generally good for readability and maintainability, but the object itself is quite large. Consider breaking it down further into smaller objects or using TypeScript interfaces to improve type safety and clarity.
| const functionSignature = utils.id(args.function).slice(0, 10); | ||
| const encodedParameters = utils.defaultAbiCoder.encode( | ||
| JSON.parse(args.types), | ||
| args.values | ||
| ); | ||
|
|
||
| const message = utils.hexlify( | ||
| utils.concat([functionSignature, encodedParameters]) | ||
| ); |
There was a problem hiding this comment.
Optimize encoding of function calls.
The encoding of function calls using utils.id and slicing the result can be error-prone and hard to understand. Consider using ethers.js utilities more effectively to encode the function signature directly, or provide detailed comments explaining this operation.
| const gateway = new ethers.Contract( | ||
| args.gatewayZetaChain, | ||
| GatewayABI.abi, | ||
| signer | ||
| ); |
There was a problem hiding this comment.
Ensure proper error handling for contract instantiation.
Instantiating a new contract with ethers.Contract should include error handling to manage cases where the contract address or ABI might be incorrect, or the network connection fails. Consider wrapping this in a try-catch block.
+ try {
const gateway = new ethers.Contract(
args.gatewayZetaChain,
GatewayABI.abi,
signer
);
+ } catch (error) {
+ console.error('Failed to instantiate Gateway contract:', error);
+ throw error;
+ }Committable suggestion was skipped due to low confidence.
| const decimals = await zrc20.decimals(); | ||
| const value = utils.parseUnits(args.amount, decimals); | ||
| const [gasZRC20, gasFee] = await zrc20.withdrawGasFeeWithGasLimit( | ||
| args.gasLimit | ||
| ); | ||
| if (args.zrc20 === gasZRC20) { | ||
| const approveGasAndWithdraw = await zrc20.approve( | ||
| args.gatewayZetaChain, | ||
| value.add(gasFee), | ||
| txOptions | ||
| ); | ||
| await approveGasAndWithdraw.wait(); | ||
| } else { | ||
| const gasZRC20Contract = new ethers.Contract( | ||
| gasZRC20, | ||
| ZRC20ABI.abi, | ||
| signer | ||
| ); | ||
| const approveGas = await gasZRC20Contract.approve( | ||
| args.gatewayZetaChain, | ||
| gasFee, | ||
| txOptions | ||
| ); | ||
| await approveGas.wait(); | ||
| const approveWithdraw = await zrc20.approve( | ||
| args.gatewayZetaChain, | ||
| value, | ||
| txOptions | ||
| ); | ||
| await approveWithdraw.wait(); | ||
| } | ||
| const method = | ||
| "withdrawAndCall(bytes,uint256,address,bytes,uint256,(address,bool,address,bytes,uint256))"; | ||
| const tx = await gateway[method]( | ||
| utils.hexlify(args.receiver), | ||
| value, | ||
| args.zrc20, | ||
| message, | ||
| args.gasLimit, | ||
| revertOptions, | ||
| txOptions | ||
| ); | ||
| return tx; |
There was a problem hiding this comment.
Review and optimize token handling and transaction execution logic.
The logic for handling token approvals and executing the transaction is complex and involves multiple conditional branches. This could be simplified or at least better documented to improve maintainability. Additionally, ensure that all asynchronous operations are properly awaited and errors are handled appropriately.
+ try {
const decimals = await zrc20.decimals();
const value = utils.parseUnits(args.amount, decimals);
const [gasZRC20, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(args.gasLimit);
if (args.zrc20 === gasZRC20) {
const approveGasAndWithdraw = await zrc20.approve(
args.gatewayZetaChain,
value.add(gasFee),
txOptions
);
await approveGasAndWithdraw.wait();
} else {
// Handle other cases similarly...
}
const tx = await gateway['withdrawAndCall'](utils.hexlify(args.receiver), value, args.zrc20, message, args.gasLimit, revertOptions, txOptions);
return tx;
+ } catch (error) {
+ console.error('Transaction execution failed:', error);
+ throw error;
+ }Committable suggestion was skipped due to low confidence.
| export const evmCall = async function ( | ||
| this: ZetaChainClient, | ||
| args: { | ||
| callOnRevert: boolean; | ||
| gasLimit: number; | ||
| gasPrice: ethers.BigNumber; | ||
| gatewayEvm: string; | ||
| onRevertGasLimit: number; | ||
| receiver: string; | ||
| revertAddress: string; | ||
| revertMessage: string; | ||
| types: string; | ||
| values: any[]; | ||
| } | ||
| ) { | ||
| const signer = this.signer; | ||
| const { utils } = ethers; | ||
| const gateway = new ethers.Contract(args.gatewayEvm, GatewayABI.abi, signer); | ||
|
|
||
| const encodedParameters = utils.defaultAbiCoder.encode( | ||
| JSON.parse(args.types), | ||
| args.values | ||
| ); | ||
| const tx = await gateway[ | ||
| "call(address,bytes,(address,bool,address,bytes,uint256))" | ||
| ]( | ||
| args.receiver, | ||
| encodedParameters, | ||
| { | ||
| abortAddress: "0x0000000000000000000000000000000000000000", | ||
| callOnRevert: args.callOnRevert, | ||
| onRevertGasLimit: args.onRevertGasLimit, | ||
| revertAddress: args.revertAddress, | ||
| revertMessage: utils.hexlify(utils.toUtf8Bytes(args.revertMessage)), | ||
| }, | ||
| { | ||
| gasLimit: args.gasLimit, | ||
| gasPrice: args.gasPrice, | ||
| } | ||
| ); | ||
|
|
||
| return tx; | ||
| }; |
There was a problem hiding this comment.
Review of evmCall function: Potential security and maintainability concerns.
-
Security Concern: JSON Parsing in Transaction Call
Direct use ofJSON.parse(args.types)in the transaction setup could lead to security vulnerabilities if the input is not properly validated. Consider validating or sanitizing the input before parsing to ensure it does not introduce code injection risks. -
Hardcoded Address Usage
TheabortAddressis hardcoded, which might limit flexibility. If this address might need to change based on the network or other conditions, consider making it a configurable parameter. -
Dynamic Method Access
Accessing the contract method using a string key (gateway["call(address,bytes,(address,bool,address,bytes,uint256))"]) assumes the method name is stable. This could lead to maintenance issues if the ABI changes. Consider defining these method names as constants or ensuring they are updated when the ABI changes. -
Error Handling
The function does not explicitly handle errors that could occur during the transaction call. Consider adding try-catch blocks around the transaction call to handle possible rejections or failures gracefully.
| export const evmDeposit = async function ( | ||
| this: ZetaChainClient, | ||
| args: { | ||
| amount: string; | ||
| callOnRevert: boolean; | ||
| erc20: string; | ||
| gasLimit: number; | ||
| gasPrice: ethers.BigNumber; | ||
| gatewayEvm: string; | ||
| onRevertGasLimit: number; | ||
| receiver: string; | ||
| revertAddress: string; | ||
| revertMessage: string; | ||
| } | ||
| ) { | ||
| const signer = this.signer; | ||
| const { utils } = ethers; | ||
| const gateway = new ethers.Contract(args.gatewayEvm, GatewayABI.abi, signer); | ||
|
|
||
| const revertOptions = { | ||
| abortAddress: "0x0000000000000000000000000000000000000000", | ||
| callOnRevert: args.callOnRevert, | ||
| onRevertGasLimit: args.onRevertGasLimit, | ||
| revertAddress: args.revertAddress, | ||
| // not used | ||
| revertMessage: utils.hexlify(utils.toUtf8Bytes(args.revertMessage)), | ||
| }; | ||
|
|
||
| const txOptions = { | ||
| gasLimit: args.gasLimit, | ||
| gasPrice: args.gasPrice, | ||
| }; | ||
| let tx; | ||
| if (args.erc20) { | ||
| const erc20Contract = new ethers.Contract( | ||
| args.erc20, | ||
| ERC20_ABI.abi, | ||
| signer | ||
| ); | ||
| const decimals = await erc20Contract.decimals(); | ||
| const value = utils.parseUnits(args.amount, decimals); | ||
| await erc20Contract.connect(signer).approve(args.gatewayEvm, value); | ||
| const method = | ||
| "deposit(address,uint256,address,(address,bool,address,bytes,uint256))"; | ||
| tx = await gateway[method]( | ||
| args.receiver, | ||
| value, | ||
| args.erc20, | ||
| revertOptions, | ||
| txOptions | ||
| ); | ||
| } else { | ||
| const value = utils.parseEther(args.amount); | ||
| const method = "deposit(address,(address,bool,address,bytes,uint256))"; | ||
| tx = await gateway[method](args.receiver, revertOptions, { | ||
| ...txOptions, | ||
| value, | ||
| }); | ||
| } | ||
|
|
||
| return tx; | ||
| }; |
There was a problem hiding this comment.
Review of evmDeposit function: Good structure with some areas for improvement.
-
Dynamic Method Access
Similar toevmCall, the dynamic access of contract methods (gateway[method]) could lead to maintenance issues if the ABI changes. Consider using a more stable method of accessing these functions, possibly by defining method names as constants. -
Comment Clarity
The comment// not usednext torevertMessagein therevertOptionsobject is confusing. If this field is indeed not used, consider removing it to clean up the code. If it is used, update the comment to reflect its purpose. -
Error Handling
As withevmCall, adding explicit error handling around the transaction calls could improve the robustness of the function. Consider implementing try-catch blocks to handle potential rejections or exceptions gracefully.
|
@andresaiello @zeta-chain/fullstack please, review. |
Start localnet:
Deploy some contracts from
example-contracts/universal/hello:Depends on zeta-chain/localnet#32
Summary by CodeRabbit
New Features
evmDeposit,evmDepositAndCall,solanaDeposit,zetachainCall,zetachainWithdraw, andzetachainWithdrawAndCall.evmCall.Bug Fixes
Documentation
Chores