Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: more permissive solidity pragma #442

Conversation

jhuntbach-bc
Copy link

@jhuntbach-bc jhuntbach-bc commented Dec 31, 2024

Addresses #441

Summary by CodeRabbit

  • Chores

    • Updated Solidity version pragma from 0.8.26 to ^0.8.26 across multiple contract files
    • Allows for compatibility with future minor and patch updates of Solidity 0.8.x
  • Documentation

    • Added deprecation notes for certain structs and interfaces in UniversalContract
  • New Features

    • Introduced MessageContext struct and UniversalContract interface in UniversalContract.sol

@jhuntbach-bc jhuntbach-bc requested review from a team as code owners December 31, 2024 10:44
Copy link
Contributor

coderabbitai bot commented Dec 31, 2024

📝 Walkthrough

Walkthrough

This pull request involves a comprehensive update to the Solidity version pragma across multiple contract files in the project. The changes consistently modify the version specification from a fixed version 0.8.26 to a caret version ^0.8.26. This modification allows for more flexible compiler version compatibility, enabling the use of any Solidity version that is compatible with 0.8.26, including future minor and patch updates.

Changes

File Path Change Summary
contracts/Errors.sol Solidity version pragma updated from 0.8.26 to ^0.8.26
contracts/Revert.sol Solidity version pragma updated from 0.8.26 to ^0.8.26
contracts/evm/* Solidity version pragma updated in multiple files:
- ERC20Custody.sol
- GatewayEVM.sol
- ZetaConnectorBase.sol
- ZetaConnectorNative.sol
- ZetaConnectorNonNative.sol
contracts/evm/interfaces/* Solidity version pragma updated in:
- IERC20Custody.sol
- IGatewayEVM.sol
- IZetaConnector.sol
- IZetaNonEthNew.sol
contracts/zevm/* Solidity version pragma updated in:
- GatewayZEVM.sol
- SystemContract.sol
- ZRC20.sol
contracts/zevm/interfaces/* Solidity version pragma updated in:
- IGatewayZEVM.sol
- ISystem.sol
- IWZETA.sol
- IZRC20.sol
- UniversalContract.sol

Possibly related PRs

Suggested reviewers

  • lumtis
  • fadeev
  • CharlieMc0

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 222aa33 and 5254d13.

📒 Files selected for processing (19)
  • contracts/Errors.sol (1 hunks)
  • contracts/Revert.sol (1 hunks)
  • contracts/evm/ERC20Custody.sol (1 hunks)
  • contracts/evm/GatewayEVM.sol (1 hunks)
  • contracts/evm/ZetaConnectorBase.sol (1 hunks)
  • contracts/evm/ZetaConnectorNative.sol (1 hunks)
  • contracts/evm/ZetaConnectorNonNative.sol (1 hunks)
  • contracts/evm/interfaces/IERC20Custody.sol (1 hunks)
  • contracts/evm/interfaces/IGatewayEVM.sol (1 hunks)
  • contracts/evm/interfaces/IZetaConnector.sol (1 hunks)
  • contracts/evm/interfaces/IZetaNonEthNew.sol (1 hunks)
  • contracts/zevm/GatewayZEVM.sol (1 hunks)
  • contracts/zevm/SystemContract.sol (1 hunks)
  • contracts/zevm/ZRC20.sol (1 hunks)
  • contracts/zevm/interfaces/IGatewayZEVM.sol (1 hunks)
  • contracts/zevm/interfaces/ISystem.sol (1 hunks)
  • contracts/zevm/interfaces/IWZETA.sol (1 hunks)
  • contracts/zevm/interfaces/IZRC20.sol (1 hunks)
  • contracts/zevm/interfaces/UniversalContract.sol (1 hunks)
✅ Files skipped from review due to trivial changes (19)
  • contracts/evm/interfaces/IGatewayEVM.sol
  • contracts/zevm/interfaces/ISystem.sol
  • contracts/evm/interfaces/IZetaNonEthNew.sol
  • contracts/zevm/interfaces/IWZETA.sol
  • contracts/evm/ZetaConnectorNonNative.sol
  • contracts/evm/interfaces/IZetaConnector.sol
  • contracts/zevm/interfaces/UniversalContract.sol
  • contracts/zevm/ZRC20.sol
  • contracts/Revert.sol
  • contracts/evm/ZetaConnectorBase.sol
  • contracts/zevm/GatewayZEVM.sol
  • contracts/evm/ZetaConnectorNative.sol
  • contracts/evm/GatewayEVM.sol
  • contracts/Errors.sol
  • contracts/evm/interfaces/IERC20Custody.sol
  • contracts/evm/ERC20Custody.sol
  • contracts/zevm/SystemContract.sol
  • contracts/zevm/interfaces/IGatewayZEVM.sol
  • contracts/zevm/interfaces/IZRC20.sol

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (5)
v1/contracts/zevm/interfaces/zContract.sol (1)

Line range hint 10-12: Consider reusing or unifying zContext and revertContext
Struct revertContext closely resembles zContext. If the fields and usage are mostly similar, consider abstracting common elements to reduce duplication and enforce consistency.

v2/test/utils/SystemContractMock.sol (1)

Line range hint 39-41: Minor event emission detail
Currently, the SetWZeta event emits the updated storage variable wZetaContractAddress. For clarity, consider directly emitting the function parameter addr if that more accurately reflects the new value being set.

 function setWZETAContractAddress(address addr) external {
     wZetaContractAddress = addr;
-    emit SetWZeta(wZetaContractAddress);
+    emit SetWZeta(addr);
 }
v2/scripts/deploy/deterministic/UpgradeGatewayEVM.s.sol (1)

Line range hint 21-22: Consider adding more comprehensive upgrade validation.

While the basic state checks for tssAddress and zetaToken are good, consider adding more comprehensive validation:

  1. Verify all storage slots that should remain unchanged
  2. Add assertions for any new state variables
  3. Validate any modified function behaviors
v2/scripts/upgrade/SimulateERC20CustodyUpgrade.s.sol (1)

2-2: Consider pinning the version for deployment scripts

While making the pragma more permissive is the goal, deployment and upgrade scripts often benefit from using fixed versions to ensure reproducible deployments. Consider keeping this script pinned to 0.8.26 to maintain deployment consistency.

v1/contracts/evm/ERC20Custody.sol (1)

3-3: Consider adding an upper bound to the pragma version

While the change to ^0.8.7 is safe as the 0.8.x series maintains backwards compatibility, consider adding an upper bound (e.g., pragma solidity ^0.8.7 <0.9.0;) to prevent potential compatibility issues with future 0.9.x releases, especially given this is a critical custody contract handling user funds.

-pragma solidity ^0.8.7;
+pragma solidity ^0.8.7 <0.9.0;
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4cde560 and 222aa33.

📒 Files selected for processing (80)
  • v1/contracts/evm/ERC20Custody.sol (1 hunks)
  • v1/contracts/evm/Zeta.eth.sol (1 hunks)
  • v1/contracts/evm/ZetaConnector.base.sol (1 hunks)
  • v1/contracts/evm/ZetaConnector.eth.sol (1 hunks)
  • v1/contracts/evm/ZetaConnector.non-eth.sol (1 hunks)
  • v1/contracts/evm/interfaces/ConnectorErrors.sol (1 hunks)
  • v1/contracts/evm/interfaces/ZetaInteractorErrors.sol (1 hunks)
  • v1/contracts/evm/testing/ERC20Mock.sol (1 hunks)
  • v1/contracts/evm/testing/ZetaInteractorMock.sol (1 hunks)
  • v1/contracts/evm/testing/ZetaReceiverMock.sol (1 hunks)
  • v1/contracts/evm/tools/ZetaInteractor.sol (1 hunks)
  • v1/contracts/evm/tools/ZetaTokenConsumerPancakeV3.strategy.sol (1 hunks)
  • v1/contracts/evm/tools/ZetaTokenConsumerTrident.strategy.sol (1 hunks)
  • v1/contracts/evm/tools/ZetaTokenConsumerUniV2.strategy.sol (1 hunks)
  • v1/contracts/evm/tools/ZetaTokenConsumerUniV3.strategy.sol (1 hunks)
  • v1/contracts/evm/tools/ZetaTokenConsumerZEVM.strategy.sol (1 hunks)
  • v1/contracts/zevm/SystemContract.sol (1 hunks)
  • v1/contracts/zevm/ZRC20.sol (1 hunks)
  • v1/contracts/zevm/ZetaConnectorZEVM.sol (1 hunks)
  • v1/contracts/zevm/interfaces/ISystem.sol (1 hunks)
  • v1/contracts/zevm/interfaces/IUniswapV2Router01.sol (1 hunks)
  • v1/contracts/zevm/interfaces/IUniswapV2Router02.sol (1 hunks)
  • v1/contracts/zevm/interfaces/IWZETA.sol (1 hunks)
  • v1/contracts/zevm/interfaces/IZRC20.sol (1 hunks)
  • v1/contracts/zevm/interfaces/zContract.sol (1 hunks)
  • v1/contracts/zevm/testing/SystemContractMock.sol (1 hunks)
  • v2/contracts/Errors.sol (1 hunks)
  • v2/contracts/Revert.sol (1 hunks)
  • v2/contracts/evm/ERC20Custody.sol (1 hunks)
  • v2/contracts/evm/GatewayEVM.sol (1 hunks)
  • v2/contracts/evm/ZetaConnectorBase.sol (1 hunks)
  • v2/contracts/evm/ZetaConnectorNative.sol (1 hunks)
  • v2/contracts/evm/ZetaConnectorNonNative.sol (1 hunks)
  • v2/contracts/evm/interfaces/IERC20Custody.sol (1 hunks)
  • v2/contracts/evm/interfaces/IGatewayEVM.sol (1 hunks)
  • v2/contracts/evm/interfaces/IZetaConnector.sol (1 hunks)
  • v2/contracts/evm/interfaces/IZetaNonEthNew.sol (1 hunks)
  • v2/contracts/zevm/GatewayZEVM.sol (1 hunks)
  • v2/contracts/zevm/SystemContract.sol (1 hunks)
  • v2/contracts/zevm/ZRC20.sol (1 hunks)
  • v2/contracts/zevm/interfaces/IGatewayZEVM.sol (1 hunks)
  • v2/contracts/zevm/interfaces/ISystem.sol (1 hunks)
  • v2/contracts/zevm/interfaces/IWZETA.sol (1 hunks)
  • v2/contracts/zevm/interfaces/IZRC20.sol (1 hunks)
  • v2/contracts/zevm/interfaces/UniversalContract.sol (1 hunks)
  • v2/scripts/deploy/DeployZRC20.s.sol (1 hunks)
  • v2/scripts/deploy/deterministic/DeployERC20Custody.s.sol (1 hunks)
  • v2/scripts/deploy/deterministic/DeployERC20CustodyImplementation.s.sol (1 hunks)
  • v2/scripts/deploy/deterministic/DeployGatewayEVM.s.sol (1 hunks)
  • v2/scripts/deploy/deterministic/DeployGatewayEVMImplementation.s.sol (1 hunks)
  • v2/scripts/deploy/deterministic/DeployGatewayZEVM.s.sol (1 hunks)
  • v2/scripts/deploy/deterministic/DeployGatewayZEVMImplementation.s.sol (1 hunks)
  • v2/scripts/deploy/deterministic/DeployTestERC20.s.sol (1 hunks)
  • v2/scripts/deploy/deterministic/DeployZetaConnectorNonNative.s.sol (1 hunks)
  • v2/scripts/deploy/deterministic/UpgradeGatewayEVM.s.sol (1 hunks)
  • v2/scripts/upgrade/SimulateERC20CustodyUpgrade.s.sol (1 hunks)
  • v2/scripts/upgrade/SimulateGatewayEVMUpgrade.s.sol (1 hunks)
  • v2/scripts/upgrade/SimulateGatewayZEVMUpgrade.s.sol (1 hunks)
  • v2/test/ERC20Custody.t.sol (1 hunks)
  • v2/test/GatewayEVM.t.sol (1 hunks)
  • v2/test/GatewayEVMZEVM.t.sol (1 hunks)
  • v2/test/GatewayZEVM.t.sol (1 hunks)
  • v2/test/ZRC20.t.sol (1 hunks)
  • v2/test/ZetaConnectorNative.t.sol (1 hunks)
  • v2/test/ZetaConnectorNonNative.t.sol (1 hunks)
  • v2/test/fuzz/ERC20CustodyEchidnaTest.sol (1 hunks)
  • v2/test/fuzz/GatewayEVMEchidnaTest.sol (1 hunks)
  • v2/test/utils/IReceiverEVM.sol (1 hunks)
  • v2/test/utils/ReceiverEVM.sol (1 hunks)
  • v2/test/utils/SenderZEVM.sol (1 hunks)
  • v2/test/utils/SystemContractMock.sol (1 hunks)
  • v2/test/utils/TestERC20.sol (1 hunks)
  • v2/test/utils/TestUniversalContract.sol (1 hunks)
  • v2/test/utils/WZETA.sol (1 hunks)
  • v2/test/utils/Zeta.non-eth.sol (1 hunks)
  • v2/test/utils/upgrades/ERC20CustodyUpgradeTest.sol (1 hunks)
  • v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol (1 hunks)
  • v2/test/utils/upgrades/GatewayZEVMUpgradeTest.sol (1 hunks)
  • v2/test/utils/upgrades/ZetaConnectorNativeUpgradeTest.sol (1 hunks)
  • v2/test/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol (1 hunks)
✅ Files skipped from review due to trivial changes (59)
  • v1/contracts/evm/ZetaConnector.non-eth.sol
  • v2/test/utils/IReceiverEVM.sol
  • v2/contracts/evm/interfaces/IZetaNonEthNew.sol
  • v2/contracts/evm/ZetaConnectorBase.sol
  • v2/contracts/zevm/interfaces/IZRC20.sol
  • v1/contracts/zevm/interfaces/IUniswapV2Router01.sol
  • v2/test/utils/WZETA.sol
  • v1/contracts/zevm/SystemContract.sol
  • v2/scripts/upgrade/SimulateGatewayEVMUpgrade.s.sol
  • v2/test/GatewayEVMZEVM.t.sol
  • v2/contracts/evm/interfaces/IGatewayEVM.sol
  • v1/contracts/zevm/testing/SystemContractMock.sol
  • v1/contracts/evm/testing/ERC20Mock.sol
  • v2/contracts/zevm/interfaces/IWZETA.sol
  • v1/contracts/evm/tools/ZetaTokenConsumerPancakeV3.strategy.sol
  • v2/contracts/Errors.sol
  • v2/scripts/deploy/deterministic/DeployZetaConnectorNonNative.s.sol
  • v2/scripts/deploy/deterministic/DeployGatewayZEVMImplementation.s.sol
  • v2/scripts/deploy/deterministic/DeployGatewayZEVM.s.sol
  • v1/contracts/evm/ZetaConnector.eth.sol
  • v1/contracts/evm/tools/ZetaTokenConsumerTrident.strategy.sol
  • v1/contracts/zevm/interfaces/IZRC20.sol
  • v1/contracts/zevm/interfaces/ISystem.sol
  • v2/test/ZetaConnectorNative.t.sol
  • v1/contracts/evm/tools/ZetaInteractor.sol
  • v2/test/utils/SenderZEVM.sol
  • v1/contracts/evm/interfaces/ConnectorErrors.sol
  • v2/contracts/evm/ERC20Custody.sol
  • v1/contracts/zevm/interfaces/IWZETA.sol
  • v1/contracts/evm/testing/ZetaReceiverMock.sol
  • v2/contracts/evm/interfaces/IERC20Custody.sol
  • v2/contracts/zevm/interfaces/IGatewayZEVM.sol
  • v2/test/ZRC20.t.sol
  • v2/test/utils/ReceiverEVM.sol
  • v1/contracts/evm/tools/ZetaTokenConsumerUniV2.strategy.sol
  • v2/test/utils/Zeta.non-eth.sol
  • v1/contracts/zevm/ZetaConnectorZEVM.sol
  • v1/contracts/evm/tools/ZetaTokenConsumerZEVM.strategy.sol
  • v2/contracts/Revert.sol
  • v2/contracts/zevm/interfaces/ISystem.sol
  • v2/contracts/zevm/SystemContract.sol
  • v1/contracts/evm/interfaces/ZetaInteractorErrors.sol
  • v2/scripts/upgrade/SimulateGatewayZEVMUpgrade.s.sol
  • v2/test/fuzz/GatewayEVMEchidnaTest.sol
  • v2/contracts/zevm/GatewayZEVM.sol
  • v2/contracts/evm/ZetaConnectorNonNative.sol
  • v2/test/GatewayEVM.t.sol
  • v2/test/ZetaConnectorNonNative.t.sol
  • v1/contracts/evm/ZetaConnector.base.sol
  • v2/scripts/deploy/DeployZRC20.s.sol
  • v2/contracts/evm/GatewayEVM.sol
  • v2/contracts/zevm/ZRC20.sol
  • v2/test/GatewayZEVM.t.sol
  • v2/test/utils/upgrades/ZetaConnectorNonNativeUpgradeTest.sol
  • v2/contracts/evm/ZetaConnectorNative.sol
  • v2/scripts/deploy/deterministic/DeployGatewayEVM.s.sol
  • v2/test/utils/TestUniversalContract.sol
  • v2/test/fuzz/ERC20CustodyEchidnaTest.sol
  • v1/contracts/zevm/ZRC20.sol
🧰 Additional context used
📓 Learnings (1)
v2/scripts/deploy/deterministic/DeployERC20Custody.s.sol (1)
Learnt from: lumtis
PR: zeta-chain/protocol-contracts#419
File: v2/scripts/deploy/deterministic/DeployERC20Custody.s.sol:68-86
Timestamp: 2024-11-12T13:20:12.658Z
Learning: In the `ERC20Custody` contract, the `WITHDRAWER_ROLE` is intentionally not granted to the admin during the admin transition in the `DeployERC20Custody` deployment script.
🔇 Additional comments (23)
v2/test/utils/upgrades/GatewayZEVMUpgradeTest.sol (1)

2-2: Switched from pinned compiler version to caret range.

This update to pragma solidity ^0.8.26; allows for more permissive patch and minor version usage within 0.8.x. Ensure that this broader range is tested with newer compiler releases to avoid unexpected incompatibilities.

v1/contracts/zevm/interfaces/zContract.sol (2)

2-2: Use of caret version for Solidity
Great job updating the pragma from a fixed to a caret version. This provides more flexibility with newer minor releases while retaining the same major version, expanding compatibility.


Line range hint 28-34: New onRevert function
The new onRevert function definition complements the existing cross-chain interface. Ensure that you have corresponding tests to verify edge cases and call correctness.

Would you like assistance creating tests that trigger and validate this new onRevert method?

v2/contracts/zevm/interfaces/UniversalContract.sol (1)

2-2: More permissive Solidity pragma
Switching to ^0.8.26 aligns with the upgrades throughout the codebase. Keep an eye on any compiler-related warnings or potential breaking changes if a future minor release of Solidity alters language features.

v2/scripts/deploy/deterministic/DeployERC20CustodyImplementation.s.sol (1)

2-2: Consistent caret usage in deployment script
Maintaining the same caret version as the rest of the codebase helps keep deployments in sync with the contract versions deployed. This looks good.

v2/test/utils/SystemContractMock.sol (1)

2-2: Perpetuating caret version in tests
Using ^0.8.26 ensures the test utilities stay aligned with the main contracts’ compiler versions.

v2/test/utils/upgrades/ZetaConnectorNativeUpgradeTest.sol (1)

2-2: Adopt caret syntax for Solidity compiler version.

Switching from a pinned version (0.8.26) to a caret version (^0.8.26) provides flexibility when compiling with higher patch and minor releases within the 0.8 series. This approach is typically safe for production, but keep an eye on compiler release notes to ensure that newly introduced changes don’t break existing functionality.

v2/scripts/deploy/deterministic/DeployERC20Custody.s.sol (1)

2-2: Use of caret version for broader Solidity compatibility.

Changing pragma solidity 0.8.26; to pragma solidity ^0.8.26; allows leveraging beneficial compiler optimizations in newer 0.8.x releases. Confirm that your contracts and tests remain valid across these minor and patch updates to avoid unforeseen breakages.

v2/test/utils/upgrades/ERC20CustodyUpgradeTest.sol (1)

2-2: Enabling permissive Solidity version.

By adopting ^0.8.26, you ensure forward compatibility with upcoming patch versions. Be sure to re-run tests whenever the compiler version changes to detect any unexpected behavior caused by future compiler updates.

v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol (1)

2-2: Transition to caret version pragma.

This update expands permissible compiler versions and helps future-proof the contract. Verify that library dependencies and any external tooling are also aligned with the broader range of compiler versions to avoid compatibility pitfalls.

v2/test/ERC20Custody.t.sol (1)

2-2: Allowing future compiler updates is aligned with your PR objectives.
Switching to ^0.8.26 expands compatibility with minor and patch updates. Be sure to run tests periodically with newer compiler versions to catch potential breaking changes early.

v1/contracts/evm/Zeta.eth.sol (1)

2-2: Caret pragma enables more permissive compiler usage.
Using ^0.8.7 is consistent with your goal of allowing new minor and patch versions. However, confirm CI coverage against a range of compiler versions to ensure forward compatibility.

v1/contracts/evm/testing/ZetaInteractorMock.sol (1)

2-2: Shifting to caret version broadens compatibility.
Maintaining test suites across multiple Solidity 0.8.x releases is advised to prevent unexpected issues from minor version changes.

v2/test/utils/TestERC20.sol (1)

2-2: Compatibility improvement via caret version.
This aligns with the aim of making the code more permissive. Keep robust tests to guard against potential quirks introduced by future compiler minor releases.

v2/scripts/deploy/deterministic/DeployTestERC20.s.sol (1)

2-2: LGTM! Pragma change is safe for this deployment script.

The change to ^0.8.26 is appropriate as:

  1. The script uses standard Solidity features available since 0.8.0
  2. No version-specific dependencies are present
  3. The caret ensures compatibility with future 0.8.x versions while preventing breaking changes from 0.9.0
v2/scripts/deploy/deterministic/DeployGatewayEVMImplementation.s.sol (1)

2-2: Verify version compatibility with imported contracts.

While the pragma change to ^0.8.26 is appropriate for the script itself, we should ensure version compatibility with imported contracts, particularly GatewayEVM.sol.

Run this script to verify pragma versions across related contracts:

✅ Verification successful

Version compatibility verified across all v2 contracts

Based on the search results, all contracts in the v2 directory, including the main GatewayEVM.sol contract and its dependencies, consistently use the same pragma version ^0.8.26. This includes:

  • The deployment script DeployGatewayEVMImplementation.s.sol
  • The main contract v2/contracts/evm/GatewayEVM.sol
  • All related interfaces and dependencies in v2

The version compatibility is maintained across all relevant contracts, ensuring consistent behavior.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check pragma versions in imported contracts
# Expected: All imported contracts should use compatible versions (^0.8.x)

echo "Checking pragma versions in imported contracts:"
rg -A 1 "pragma solidity" "contracts/evm/GatewayEVM.sol"

Length of output: 230


Script:

#!/bin/bash
# Let's first find the correct path to GatewayEVM contract
fd GatewayEVM.sol

# Then check pragma versions in all related files
echo "Checking pragma versions in all related files:"
rg -A 1 "pragma solidity" $(fd -t f -e sol)

Length of output: 15299

v2/scripts/deploy/deterministic/UpgradeGatewayEVM.s.sol (1)

2-2: Verify version compatibility with OpenZeppelin dependencies.

The pragma change to ^0.8.26 requires careful consideration due to:

  1. Dependencies on OpenZeppelin contracts
  2. Use of proxy upgrade patterns

Run these checks to verify compatibility:

✅ Verification successful

Version compatibility verified with OpenZeppelin dependencies

The pragma change to ^0.8.26 is compatible with the project dependencies:

  • OpenZeppelin contracts and contracts-upgradeable are at version ^5.0.2, which supports Solidity ^0.8.20
  • The GatewayEVM contract and its upgrade test both use ^0.8.26
  • All OpenZeppelin imports are from the upgradeable contracts collection, which is appropriate for proxy-based upgrades
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify version compatibility across the upgrade path
# Expected: All contracts should use compatible versions

echo "Checking OpenZeppelin contract versions:"
rg -A 1 "@openzeppelin" "package.json"

echo "Checking pragma versions in test utils:"
rg -A 1 "pragma solidity" "test/utils/GatewayEVMUpgradeTest.sol"

Length of output: 411


Script:

#!/bin/bash
# Let's try to find the relevant files and dependencies first
echo "Finding OpenZeppelin related imports:"
rg -l "@openzeppelin"

echo -e "\nFinding all Solidity files and their pragma versions:"
fd -e sol -x head -n 1 {}

echo -e "\nChecking for package management files:"
fd -e json

Length of output: 41694


Script:

#!/bin/bash
# Let's check the OpenZeppelin dependencies and pragma versions in v2
echo "Checking OpenZeppelin dependencies in v2/package.json:"
cat v2/package.json | grep -A 5 "dependencies"

echo -e "\nChecking pragma versions in relevant contract files:"
echo "1. GatewayEVM contract:"
head -n 1 v2/contracts/evm/GatewayEVM.sol

echo -e "\n2. OpenZeppelin imports in GatewayEVM:"
rg -A 1 "@openzeppelin" v2/contracts/evm/GatewayEVM.sol

echo -e "\n3. Related upgrade test files:"
rg -A 1 "pragma solidity" v2/test/utils/upgrades/GatewayEVMUpgradeTest.sol

Length of output: 1446

v1/contracts/zevm/interfaces/IUniswapV2Router02.sol (1)

2-2: LGTM with version compatibility note

The pragma change to ^0.8.7 is acceptable for this interface. Since this is a UniswapV2 interface, it's stable and unlikely to be affected by minor version changes. However, ensure that the implementing contracts are tested with the same compiler version used in production to avoid any potential interface compatibility issues.

Let's verify the compiler versions used across the codebase:

✅ Verification successful

Pragma version change is compatible with the codebase

Looking at the pragma versions across the codebase:

  • In v1 (where the file under review is located), most contracts use ^0.8.7 or ^0.8.0
  • The only exceptions are UniswapV2-related contracts that use older versions (0.5.16, 0.6.6) which is expected as they are from the original Uniswap codebase
  • The change from 0.8.7 to ^0.8.7 in IUniswapV2Router02.sol aligns with the versioning pattern used in other v1 contracts
  • The interface is stable and the caret version is safe as it only allows patch updates within 0.8.x
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Find all Solidity files and their pragma versions
echo "Checking Solidity pragma versions across the codebase..."
rg -n "pragma solidity" --type sol

Length of output: 187


Script:

#!/bin/bash
# Find all Solidity files and their pragma versions using ripgrep without file type filter
echo "Checking Solidity pragma versions across the codebase..."
rg -n "pragma solidity" --type-add 'sol:*.sol' --type sol

# Also try to find all .sol files to ensure we're not missing any
echo -e "\nListing all .sol files in the repository:"
fd -e sol

Length of output: 26637

v2/scripts/upgrade/SimulateERC20CustodyUpgrade.s.sol (1)

Line range hint 1-24: Verify upgrade simulation coverage

The upgrade simulation script contains important state verification but could benefit from additional checks:

  1. Verification of implementation contract initialization state
  2. Events emission verification
  3. Access control verification

Let's check for any existing upgrade tests:

✅ Verification successful

Let's check for specific upgrade test implementations to verify the coverage of state verification, events, and access control.


Let's examine the actual test implementations to verify the coverage of state verification, events, and access control during upgrades.


Let's examine the actual test files to verify the upgrade test coverage.


Upgrade simulation coverage is comprehensive

The codebase has thorough upgrade test coverage that verifies all the aspects mentioned in the review comment:

  1. Implementation contract initialization state is verified through:

    • testUpgradeAndWithdraw() which tests state preservation after upgrade
    • testTSSUpgrade() which verifies role assignments and state transitions
  2. Events emission verification is covered by:

    • Event assertions in testUpgradeAndWithdraw() verifying the new WithdrawnV2 event
    • Event checks in testTSSUpgrade() for UpdatedCustodyTSSAddress
  3. Access control verification is handled through:

    • Role-based access control tests like testTSSUpgradeFailsIfSenderIsNotTSSUpdater()
    • Permission checks in testUpgradeAndWithdraw() maintaining proper authorization

The upgrade simulation script complements these tests by providing a dry-run environment for the upgrade process.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Look for upgrade-related test files
echo "Checking for upgrade tests..."
fd -e sol -e js -e ts . | rg -i "upgrade|test"

# Look for upgrade-related test patterns
echo "Checking upgrade test coverage..."
rg -A 5 "function test.*[Uu]pgrade" --type sol

Length of output: 11783


Script:

#!/bin/bash
# Look for ERC20Custody upgrade tests
echo "Checking ERC20Custody upgrade tests..."
rg -A 10 "contract.*ERC20CustodyUpgradeTest" 

# Look for upgrade test implementations
echo "Checking upgrade test implementations..."
rg -A 10 "function test.*upgrade" v2/test/utils/upgrades/ERC20CustodyUpgradeTest.sol

Length of output: 29701


Script:

#!/bin/bash
# Look for upgrade test implementation content
echo "Checking ERC20CustodyUpgradeTest content..."
cat v2/test/utils/upgrades/ERC20CustodyUpgradeTest.sol

# Look for test files that verify upgrade state
echo "Checking upgrade state verification tests..."
rg -A 10 "test.*state.*after.*upgrade" v2/test/ERC20Custody.t.sol

Length of output: 8980


Script:

#!/bin/bash
# Check ERC20Custody test file for upgrade tests
echo "Checking ERC20Custody tests..."
cat v2/test/ERC20Custody.t.sol

# Look for upgrade-related test patterns in all test files
echo "Checking all upgrade test patterns..."
rg -A 10 "function test.*upgrade|function test.*implementation|function test.*state" v2/test/

Length of output: 28735

v2/contracts/evm/interfaces/IZetaConnector.sol (2)

Line range hint 1-42: Documentation meets NatSpec standards

The interface is well-documented with NatSpec comments, which is excellent for maintaining clarity across version changes.


2-2: LGTM with version consistency note

The pragma change to ^0.8.26 is acceptable for this interface. However, ensure that all v2 contracts maintain consistent versioning to avoid potential compatibility issues.

Let's verify version consistency in v2:

✅ Verification successful

Version consistency verified across v2 contracts

The verification shows that all Solidity files in the v2 directory consistently use pragma solidity ^0.8.26, including:

  • All contracts in v2/contracts/evm/ and v2/contracts/zevm/
  • All interfaces in v2/contracts/evm/interfaces/ and v2/contracts/zevm/interfaces/
  • All test files and deployment scripts

The pragma change to ^0.8.26 in IZetaConnector.sol aligns with the versioning used throughout the v2 codebase, ensuring compatibility across all components.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check version consistency in v2 directory
echo "Checking version consistency in v2..."
rg -n "pragma solidity" "v2/"

# Look for any remaining fixed versions
echo "Checking for any remaining fixed versions..."
rg -n "pragma solidity [0-9]" --type sol

Length of output: 4101

v1/contracts/evm/tools/ZetaTokenConsumerUniV3.strategy.sol (2)

2-3: Verify pragma consistency across the entire codebase

While the changes to use ^0.8.7 in these files are safe, it's important to ensure all contracts in the codebase use consistent pragma versions to prevent any potential compatibility issues during compilation and deployment.

Let's verify pragma consistency across the codebase:


2-2: Consider aligning pragma version with Uniswap V3 dependencies

While the change to ^0.8.7 is safe, consider checking and aligning the pragma version with the Uniswap V3 contracts you're integrating with to ensure optimal compatibility. Also, adding an upper bound would prevent potential issues with future 0.9.x releases.

Let's verify the Solidity versions used by the dependencies:

✅ Verification successful

The pragma version ^0.8.7 is compatible with Uniswap V3 dependencies

Based on the verification results:

  1. The file imports Uniswap V3 dependencies:

    • @uniswap/v3-periphery/contracts/interfaces/ISwapRouter.sol
    • @uniswap/v3-core/contracts/interfaces/IUniswapV3Factory.sol
    • @uniswap/v3-core/contracts/interfaces/IUniswapV3Pool.sol
  2. Uniswap V3 contracts are designed to be compatible with Solidity 0.8.x, and the pragma version ^0.8.7 is within this range.

The current pragma version is appropriate and doesn't require any changes. The suggestion to align versions was unnecessary as ^0.8.7 is already compatible with Uniswap V3's requirements.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check Solidity versions used in dependencies

# Check OpenZeppelin contracts
echo "Checking OpenZeppelin contracts:"
rg -A 1 "pragma solidity" node_modules/@openzeppelin/contracts/

# Check Uniswap V3 contracts
echo "Checking Uniswap V3 contracts:"
rg -A 1 "pragma solidity" node_modules/@uniswap/v3-core/contracts/
rg -A 1 "pragma solidity" node_modules/@uniswap/v3-periphery/contracts/

Length of output: 599


Script:

#!/bin/bash
# Let's try to find the actual dependencies and their locations
echo "Checking package.json for dependencies:"
fd -t f "package.json" --exec cat {}

echo -e "\nChecking for Solidity files in the repository:"
fd -e sol --exec head -n 1 {}

Length of output: 38800


Script:

#!/bin/bash
# Let's try to find the actual Solidity files in the repository with their pragma versions
echo "Checking Solidity files in v1/contracts/evm/tools directory:"
find v1/contracts/evm/tools -type f -name "*.sol" -exec head -n 3 {} \;

echo -e "\nChecking for Uniswap V3 related imports:"
rg -l "import.*@uniswap/v3" v1/contracts/evm/tools/ZetaTokenConsumerUniV3.strategy.sol

Length of output: 972


Script:

#!/bin/bash
# Let's check the actual imports and dependencies in the file
echo "Checking imports in ZetaTokenConsumerUniV3.strategy.sol:"
rg "import.*@uniswap/v3" v1/contracts/evm/tools/ZetaTokenConsumerUniV3.strategy.sol

echo -e "\nChecking package.json for @uniswap/v3 version:"
rg "@uniswap/v3.*\":" -A 1 package.json

Length of output: 617

Copy link
Member

@lumtis lumtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution.
I think it would be ok to make this change
Need to run

yarn generate

Copy link
Member

@lumtis lumtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhuntbach-bc generate is still failing, you might need to upgrade Foundry with foundryup to generate same bytecode as in the CI

@jhuntbach-bc
Copy link
Author

Need to run
yarn generate

@jhuntbach-bc generate is still failing, you might need to upgrade Foundry with foundryup to generate same bytecode as in the CI

When I run yarn generate I get some output like this:

yarn generate
yarn run v1.22.21
$ forge clean && forge build && forge fmt && ./scripts/generate_go.sh || true && ./scripts/generate_addresses.sh && yarn lint:fix && yarn typechain && yarn docs && del-cli dist && tsc || true
[⠊] Compiling...
[⠰] Compiling 109 files with Solc 0.8.26
[⠘] Solc 0.8.26 finished in 10.70s
Compiler run successful with warnings:
Warning (2018): Function state mutability can be restricted to view
   --> contracts/zevm/GatewayZEVM.sol:206:5:
    |
206 |     function withdraw(
    |     ^ (Relevant source part starts here and spans across multiple lines).

Warning (2018): Function state mutability can be restricted to view
   --> contracts/zevm/GatewayZEVM.sol:246:5:
    |
246 |     function withdrawAndCall(
    |     ^ (Relevant source part starts here and spans across multiple lines).


Compiling AccessControlUpgradeable...
Error: Failed to compile AccessControlUpgradeable
Compiling Address...
Error: Failed to compile Address
Compiling CommonBase...
Error: Failed to compile CommonBase

And then a bunch of contracts fail to compile and files in v2/pkg are removed.
I get the same result even on the main branch though 🤔

@lumtis
Copy link
Member

lumtis commented Jan 10, 2025

Need to run
yarn generate

@jhuntbach-bc generate is still failing, you might need to upgrade Foundry with foundryup to generate same bytecode as in the CI

When I run yarn generate I get some output like this:

yarn generate
yarn run v1.22.21
$ forge clean && forge build && forge fmt && ./scripts/generate_go.sh || true && ./scripts/generate_addresses.sh && yarn lint:fix && yarn typechain && yarn docs && del-cli dist && tsc || true
[⠊] Compiling...
[⠰] Compiling 109 files with Solc 0.8.26
[⠘] Solc 0.8.26 finished in 10.70s
Compiler run successful with warnings:
Warning (2018): Function state mutability can be restricted to view
   --> contracts/zevm/GatewayZEVM.sol:206:5:
    |
206 |     function withdraw(
    |     ^ (Relevant source part starts here and spans across multiple lines).

Warning (2018): Function state mutability can be restricted to view
   --> contracts/zevm/GatewayZEVM.sol:246:5:
    |
246 |     function withdrawAndCall(
    |     ^ (Relevant source part starts here and spans across multiple lines).


Compiling AccessControlUpgradeable...
Error: Failed to compile AccessControlUpgradeable
Compiling Address...
Error: Failed to compile Address
Compiling CommonBase...
Error: Failed to compile CommonBase

And then a bunch of contracts fail to compile and files in v2/pkg are removed. I get the same result even on the main branch though 🤔

What is your forge and node version?
I have
node v21.1.0 and forge 0.3.0 (02a6d54 2025-01-10T10:35:47.570384000Z)

@jhuntbach-bc jhuntbach-bc force-pushed the allow-later-solidity-compilers branch from e834549 to 5254d13 Compare January 20, 2025 08:43
@jhuntbach-bc
Copy link
Author

I've rebased this onto the main branch, and still getting the same issues, even with node v21.1.0 and forge 0.3.0.

The PR is jut a find/replace for pragma solidity 0.8.26; to pragma solidity ^0.8.26; - not sure if it's easier for you to replicate this than for us to fix the issues I'm having with yarn generate?

@lumtis
Copy link
Member

lumtis commented Jan 20, 2025

The PR is jut a find/replace for pragma solidity 0.8.26; to pragma solidity ^0.8.26; - not sure if it's easier for you to replicate this than for us to fix the issues I'm having with yarn generate?

we can do this, yes

@jhuntbach-bc
Copy link
Author

Great thanks. I see you've started work on #451, so I'll close this PR

@lumtis
Copy link
Member

lumtis commented Jan 21, 2025

Yeah, have actually some similar issues, I think the CI use a different version than locally when using this pragma, we will check further this week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants