DO NOT MERGE: Save cra receiver proxy - #211
Conversation
dmytro-horbatenko
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds the SaveCreReceiverProxy deployment infrastructure, including submodules, Hardhat plugin configurations, and a setup script. Feedback highlights an invalid Solidity version (0.8.33), an incorrect file path for the dependency compiler, and the need to use environment variables for the sources path to maintain build compatibility. Further improvements are suggested regarding hardcoded addresses, non-standard package versioning, and the brittle nature of patching submodules with sed.
| // directory and let `hardhat-dependency-compiler` pull | ||
| // `save/.../SaveCreReceiverProxy.sol` (forge remapping) in as the only graph. | ||
| // Main branch must NOT merge this; restore `sources: 'contracts'`. | ||
| sources: 'contracts-save-cre', |
There was a problem hiding this comment.
The sources path has been changed to contracts-save-cre, which disables the main project contracts. To make this configuration safe for merging and avoid breaking the build for other developers, consider using an environment variable to toggle the source directory.
| sources: 'contracts-save-cre', | |
| sources: process.env.DEPLOY_SAVE_CRE ? 'contracts-save-cre' : 'contracts', |
| }, | ||
| dependencyCompiler: { | ||
| paths: [ | ||
| 'save/contracts/src/save-cre-receiver-proxy/SaveCreReceiverProxy.sol', |
There was a problem hiding this comment.
The path for SaveCreReceiverProxy.sol appears to be missing the lib/ prefix. Since the submodule is located in lib/save, the path should reflect this to ensure the hardhat-dependency-compiler plugin can locate the file.
| 'save/contracts/src/save-cre-receiver-proxy/SaveCreReceiverProxy.sol', | |
| 'lib/save/contracts/src/save-cre-receiver-proxy/SaveCreReceiverProxy.sol', |
| // upstream `llama-risk/save`'s `foundry.toml` (`optimizer`, | ||
| // `optimizer_runs`, `via_ir`) before mainnet deploy. Tracked in | ||
| // docs/superpowers/specs/2026-04-23-save-cre-receiver-proxy-mainnet-deploy-design.md §10. | ||
| version: '0.8.33', |
There was a problem hiding this comment.
Solidity version 0.8.33 is not a released version of the compiler. This will cause Hardhat to fail when attempting to download the compiler. Please use a valid version such as 0.8.23 or 0.8.28.
| const REGISTRY = '0x2D6e9F608807436DE5D9603B00Abe3FEd1Bc809d'; | ||
| const EXPECTED_FORWARDER = '0x0b93082D9b3C7C97fAcd250082899BAcf3af3885'; | ||
| const EXPECTED_AUTHOR = '0xFD1ebE54d435bDd9a81A4BF204756a20AeB7Ee71'; |
There was a problem hiding this comment.
Hardcoding addresses for REGISTRY, EXPECTED_FORWARDER, and EXPECTED_AUTHOR limits the script's usability across different environments. It is recommended to move these values to a configuration file or environment variables.
| sed -i 's|@openzeppelin/contracts/access/|@openzeppelin/contracts@5.0.2/access/|' "$proxy" | ||
| sed -i 's|@openzeppelin/contracts/utils/|@openzeppelin/contracts@5.0.2/utils/|' "$abs" |
There was a problem hiding this comment.
Modifying submodule files with sed is a brittle workaround for import conflicts. These changes are not persistent across submodule updates or resets. It is better to resolve these conflicts using Hardhat's remapping capabilities or by updating the upstream repository.
| "@layerzerolabs/oft-evm": "3.2.1", | ||
| "@layerzerolabs/toolbox-hardhat": "0.6.12", | ||
| "@nomicfoundation/hardhat-chai-matchers": "1.0.4", | ||
| "@nomicfoundation/hardhat-foundry": "hh2", |
There was a problem hiding this comment.
The version alias "hh2" for @nomicfoundation/hardhat-foundry is non-standard and potentially confusing. It is recommended to use a standard semver range.
| "@nomicfoundation/hardhat-foundry": "hh2", | |
| "@nomicfoundation/hardhat-foundry": "^1.2.1", |
1 participant
No description provided.