refactor: zkstack based plugins #1774
Conversation
| content.tokenAddress.toLowerCase() !== ETH_ADDRESS.toLowerCase() | ||
| ) { | ||
| // Convert amount to proper token decimals | ||
| const tokenInfo = | ||
| ERC20_OVERRIDE_INFO[content.tokenAddress.toLowerCase()]; | ||
| const decimals = tokenInfo?.decimals ?? 18; // Default to 18 decimals if not specified |
There was a problem hiding this comment.
@aalimsahin The handling of token amounts should include error handling for invalid inputs. Ensure that 'content.amount' is a valid number before proceeding with calculations to avoid runtime errors.
| .refine((address) => isAddress(address, { strict: false }), { | ||
| message: "Abstract address must be a valid address", |
There was a problem hiding this comment.
@aalimsahin The use of 'isAddress' from 'viem' should be reviewed for security. Ensure that the address validation logic is robust against potential attacks, such as address spoofing. Consider adding additional checks or using a well-established library for address validation.
| .refine((key) => /^[a-fA-F0-9]{64}$/.test(key), { | ||
| message: | ||
| "Abstract private key must be a 64-character hexadecimal string (32 bytes) without the '0x' prefix", |
There was a problem hiding this comment.
@aalimsahin The regex used for validating the private key may not cover all edge cases. Ensure that it strictly enforces the expected format and consider using a more comprehensive validation method. For example, you could use a library designed for cryptographic key validation.
| const PRIVATE_KEY = runtime.getSetting("CRONOSZKEVM_PRIVATE_KEY"); | ||
| const account = privateKeyToAccount(`0x${PRIVATE_KEY}`); |
There was a problem hiding this comment.
@aalimsahin The PRIVATE_KEY should be handled securely. Avoid exposing sensitive information in logs or error messages. Consider using environment variables or secure vaults for sensitive data.
| // Check if the token is native | ||
| if ( | ||
| content.tokenAddress.toLowerCase() !== | ||
| ZKCRO_ADDRESS.toLowerCase() |
There was a problem hiding this comment.
@aalimsahin Ensure that the ZKCRO_ADDRESS is not hardcoded. This can lead to issues if the address changes. Consider fetching it from a configuration file or environment variable.
| // Convert amount to proper token decimals | ||
| const tokenInfo = | ||
| ERC20_OVERRIDE_INFO[content.tokenAddress.toLowerCase()]; | ||
| const decimals = tokenInfo?.decimals ?? 18; // Default to 18 decimals if not specified |
There was a problem hiding this comment.
@aalimsahin The default decimal value of 18 may not be appropriate for all tokens. Ensure that the token's actual decimal value is fetched or validated to prevent potential transfer errors.
| .refine((address) => isAddress(address, { strict: false }), { | ||
| message: "Cronos zkEVM address must be a valid address", | ||
| }), |
There was a problem hiding this comment.
@aalimsahin The use of 'isAddress' from 'viem' should be validated for security. Ensure that the library is well-maintained and does not have known vulnerabilities. Consider adding unit tests to verify the behavior of this validation.
| .refine((key) => /^[a-fA-F0-9]{64}$/.test(key), { | ||
| message: | ||
| "Cronos zkEVM private key must be a 64-character hexadecimal string (32 bytes) without the '0x' prefix", |
There was a problem hiding this comment.
@aalimsahin The regex used for validating the private key should be reviewed for potential edge cases. Ensure that it strictly enforces the expected format and consider using a more robust validation library if necessary.
| const tokenInfo = | ||
| ERC20_OVERRIDE_INFO[content.tokenAddress.toLowerCase()]; | ||
| const decimals = tokenInfo?.decimals ?? 18; // Default to 18 decimals if not specified | ||
| const tokenAmount = parseUnits( |
There was a problem hiding this comment.
@aalimsahin Ensure that the token amount conversion handles potential overflows or underflows. Use libraries like 'bignumber.js' for safe arithmetic operations.
| export const zksyncEnvSchema = z.object({ | ||
| ZKSYNC_ADDRESS: z.string().min(1, "ZKsync address is required"), | ||
| ZKSYNC_PRIVATE_KEY: z.string().min(1, "ZKsync private key is required"), | ||
| ZKSYNC_ADDRESS: z | ||
| .string() | ||
| .min(1, "ZKsync Era address is required") | ||
| .refine((address) => isAddress(address, { strict: false }), { | ||
| message: "ZKsync Era address must be a valid address", | ||
| }), | ||
| ZKSYNC_PRIVATE_KEY: z | ||
| .string() | ||
| .min(1, "ZKsync Era private key is required") | ||
| .refine((key) => /^[a-fA-F0-9]{64}$/.test(key), { | ||
| message: | ||
| "ZKsync Era private key must be a 64-character hexadecimal string (32 bytes) without the '0x' prefix", | ||
| }), | ||
| }); |
There was a problem hiding this comment.
@aalimsahin The validation for the private key should also include a check for its length to ensure it is exactly 64 characters. This is crucial for security to prevent invalid keys from being accepted.
Relates to:
N/A (Improving zkstack plugin architecture and consistency)
Risks
Low. The changes involve refactoring and updating multiple plugins (plugin-abstract, plugin-zksync-era, plugin-cronoszkevm) to use viem for consistency, improving environment validation and fixing outdated codes.
Background
What does this PR do?
Environment Validation for plugin-abstract, plugin-cronoszkevm, plugin-zksync-era.
Integrated viem for functionality in plugin-cronoszkevm, plugin-zksync-era for maintaince.
Fixed issues in plugin-zksync-era and plugin-cronoszkevm where previous changes caused them to break.
What kind of change is this?
Fixed cronoszkevm and zksync era plugins, moved them to viem and some refactors
Bug fixes and Improvements
Documentation changes needed?
As I check, no
Testing
Where should a reviewer start?
Almost all plugins are the same, so you can choose one. After that, others will be the same.
Detailed testing steps
Initialize plugins in character.
You can choose to test one of them.
Add envs for the plugin. Try to wrong one to test first.
Start one client to check it. You can say to agent like send x amount eth to y address in z chain
Discord username
aalimsahin