feat: automatically handle solc configuration for Etherscan Platform #544
Conversation
Sync master <> dev
prepare 0.3.5 release
merge dev into master
sync master<>dev
0.3.6 release
shortdoom
requested review from
montyly,
0xalpharush and
bohendo
as code owners
|
I think it's a great idea to automate this What is the reason to guess the remapping instead of saving the ones downloaded from etherscan here in crytic-compile/crytic_compile/platform/etherscan.py Lines 185 to 190 in 1e5deb2 We can probably automatically load the Wdyt? |
|
Those as far as I could understood, CompilationUnit actually just uses absolute paths from here to compile on the first run: hence why I also generated just absolute paths for the
|
|
Hi @shortdoom! thanks for working on this! I verified locally and the remapping variable does have proper remappings, is it possible you tested with a contract on etherscan that does not have the metadata? I printed the remappings like the following: diff --git a/crytic_compile/platform/etherscan.py b/crytic_compile/platform/etherscan.py
index a8f54b4..cbec350 100644
--- a/crytic_compile/platform/etherscan.py
+++ b/crytic_compile/platform/etherscan.py
@@ -187,6 +187,10 @@ def _handle_multiple_files(
remappings = dict_source_code.get("settings", {}).get("remappings", None)
+ print("Remappings:", remappings)
+ print("Directory:", directory)
+ print("Sanitized:", _sanitize_remappings(remappings, directory))
+
return list(filtered_paths), directory, _sanitize_remappings(remappings, directory)
And fetching an address, I do get remappings: % ./venv/bin/crytic-compile 0x2a311e451491091d2a1d3c43f4f5744bdb4e773a
Remappings: ['@0x/contracts-utils=/home/cluracan/code/0x-protocol/node_modules/@0x/contracts-utils', '@0x/contracts-erc20=/home/cluracan/code/0x-protocol/contracts/zero-ex/node_modules/@0x/contracts-erc20']
Directory: crytic-export/etherscan-contracts/0x2a311e451491091d2a1d3c43f4f5744bdb4e773a-NativeOrdersFeature
Sanitized: ['@0x/contracts-utils=home/cluracan/code/0x-protocol/node_modules/@0x/contracts-utils/', '@0x/contracts-erc20=home/cluracan/code/0x-protocol/contracts/zero-ex/node_modules/@0x/contracts-erc20/']
INFO:CryticCompile:'solc --standard-json --allow-paths /Users/emilio/git/crytic-compile/crytic-export/etherscan-contracts/0x2a311e451491091d2a1d3c43f4f5744bdb4e773a-NativeOrdersFeature' running |
|
Huh, yes! It seems that targets I was running it against were without the metadata. That definitely changes the purpose of this PR, happy to hear how you would want to proceed. Ps. Nice target address for testing path resolution edge cases :) |
|
yeah that's a cool address to test things with, there's other ones on the by the way, regarding the address you were using to test on Slack ( emilio@mbpro 0x29d2bcf0d70f95ce16697e645e2b76d218d66109-OxODexPool % crytic-compile contracts/OxODexPool.sol
INFO:CryticCompile:'solc --version' running
INFO:CryticCompile:'solc contracts/OxODexPool.sol --combined-json abi,ast,bin,bin-runtime,srcmap,srcmap-runtime,userdoc,devdoc,hashes --allow-paths .,/Users/emilio/git/crytic-compile/crytic-export/etherscan-contracts/0x29d2bcf0d70f95ce16697e645e2b76d218d66109-OxODexPool/contracts' running
INFO:CryticCompile:Compilation warnings/errors on contracts/OxODexPool.sol:
Warning: SPDX license identifier not provided in source file. Before publishing, consider adding a comment containing "SPDX-License-Identifier: <SPDX-License>" to each source file. Use "SPDX-License-Identifier: UNLICENSED" for non-open-source code. Please see https://spdx.org for more information.
--> contracts/interfaces/IWETH9.sol
Warning: Unnamed return variable can remain unassigned. Add an explicit return with value to all non-reverting code paths or name the variable.
--> contracts/lib/LSAG.sol:21:18:
|
21 | returns (uint256[2] memory)
| ^^^^^^^^^^^^^^^^^
Warning: Unused local variable.
--> contracts/OxODexPool.sol:337:24:
|
337 | (bool success, bytes memory data) = _recipient.call{value: _amount - relayerFee}("");
| ^^^^^^^^^^^^^^^^^
On an unrelated note, it seems globbing of files is not working as expected @0xalpharush , I had to patch it as follows to build all the files at the same time. I saw there's a TODO on that area so I'm bringing it to your attention here. diff --git a/crytic_compile/crytic_compile.py b/crytic_compile/crytic_compile.py
index 3996150..8f2b896 100644
--- a/crytic_compile/crytic_compile.py
+++ b/crytic_compile/crytic_compile.py
@@ -717,8 +717,8 @@ def compile_all(target: str, **kwargs: str) -> List[CryticCompile]:
else:
compilations.append(CryticCompile(target, **kwargs))
elif os.path.isdir(target):
- solidity_filenames = glob.glob(os.path.join(target, "*.sol"))
- vyper_filenames = glob.glob(os.path.join(target, "*.vy"))
+ solidity_filenames = glob.glob(os.path.join(target, "**", "*.sol"), recursive=True)
+ vyper_filenames = glob.glob(os.path.join(target, "**", "*.vy"), recursive=True)
# Determine if we're using --standard-solc option to
# aggregate many files into a single compilation.
if use_solc_standard_json:With that change you can build all the files from emilio@mbpro 0x29d2bcf0d70f95ce16697e645e2b76d218d66109-OxODexPool % ../../../venv/bin/crytic-compile . --solc-standard-json
INFO:CryticCompile:'solc --version' running
INFO:CryticCompile:'solc --standard-json --allow-paths /Users/emilio/git/crytic-compile/crytic-export/etherscan-contracts/0x29d2bcf0d70f95ce16697e645e2b76d218d66109-OxODexPool' running
WARNING:CryticCompile:Warning: Warning: SPDX license identifier not provided in source file. Before publishing, consider adding a comment containing "SPDX-License-Identifier: <SPDX-License>" to each source file. Use "SPDX-License-Identifier: UNLICENSED" for non-open-source code. Please see https://spdx.org for more information.
--> ./contracts/interfaces/IWETH9.sol
Warning: Warning: Unnamed return variable can remain unassigned. Add an explicit return with value to all non-reverting code paths or name the variable.
--> ./contracts/lib/LSAG.sol:21:18:
|
21 | returns (uint256[2] memory)
| ^^^^^^^^^^^^^^^^^
Warning: Warning: Unused local variable.
--> ./contracts/OxODexPool.sol:337:24:
|
337 | (bool success, bytes memory data) = _recipient.call{value: _amount - relayerFee}("");
| ^^^^^^^^^^^^^^^^^
Going back to the PR @shortdoom, I think it would be handy to preserve more of the metadata we fetch. From experience, some etherscan contracts don't build without using the same optimization/compiler flags (e.g. via-ir, or the evm target version). I think writing a suitable |
|
I dug a little deeper as some of targets were still failing for me, including Can you try first downloading and then I found few more targets like this, each having
However, for the above - in contrary to all of the other examples - it helps to manually provide remappings as argument And, two more, also with node_modules mapping
Coming back to how to built it into the better automation I think we could just use crytic-compile/crytic_compile/platform/etherscan.py Lines 185 to 190 in 1e5deb2 remappings.txt file then crytic-compile could just check for existence of this file and and compile accordingly, as @0xalpharush suggested.
|
|
@shortdoom I see It is also a good example why other metadata such as the solc version is important; even though the contracts claim Manually providing the remappings from the metadata does work [note I'm using the globbing patch from my earlier message still, to make things easier to build] So yeah, I think just storing the (sanitized) remappings we get from etherscan should be sufficient for remapping purposes 👍 |
WalkthroughThe recent update to the Changes
Related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
crytic_compile/platform/etherscan.py
Outdated
| metadata_config = { | ||
| "solc_remaps": remappings if remappings else {}, | ||
| "solc_solcs_select": compiler_version, | ||
| "solc_args": ("--via-ir" if via_ir_enabled else "") | ||
| + ("--optimize --optimize-runs " + str(optimize_runs) if optimize_runs else "") | ||
| + ("--evm-version " + evm_version if evm_version else ""), |
There was a problem hiding this comment.
The construction of the metadata_config dictionary combines several conditional string concatenations for solc_args. This approach, while functional, can lead to maintenance challenges and readability issues. Consider refactoring this into a more modular approach, where each condition is checked and the corresponding string is appended to a list. This list can then be joined with spaces to form the final solc_args string. This would improve readability and make future modifications easier.
- "solc_args": ("--via-ir" if via_ir_enabled else "")
- + ("--optimize --optimize-runs " + str(optimize_runs) if optimize_runs else "")
- + ("--evm-version " + evm_version if evm_version else ""),
+ "solc_args": " ".join(filter(None, [
+ "--via-ir" if via_ir_enabled else "",
+ "--optimize --optimize-runs " + str(optimize_runs) if optimize_runs else "",
+ "--evm-version " + evm_version if evm_version else ""
+ ])),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.
| metadata_config = { | |
| "solc_remaps": remappings if remappings else {}, | |
| "solc_solcs_select": compiler_version, | |
| "solc_args": ("--via-ir" if via_ir_enabled else "") | |
| + ("--optimize --optimize-runs " + str(optimize_runs) if optimize_runs else "") | |
| + ("--evm-version " + evm_version if evm_version else ""), | |
| metadata_config = { | |
| "solc_remaps": remappings if remappings else {}, | |
| "solc_solcs_select": compiler_version, | |
| "solc_args": " ".join(filter(None, [ | |
| "--via-ir" if via_ir_enabled else "", | |
| "--optimize --optimize-runs " + str(optimize_runs) if optimize_runs else "", | |
| "--evm-version " + evm_version if evm_version else "" | |
| ])), |
crytic_compile/platform/etherscan.py
Outdated
| with open( | ||
| os.path.join(working_dir if working_dir else export_dir, "crytic_compile.config.json"), | ||
| "w", | ||
| ) as f: | ||
| json.dump(metadata_config, f) |
There was a problem hiding this comment.
Writing the metadata_config dictionary directly to a file without any error handling could lead to uncaught exceptions if there are issues with file access or disk space. It's recommended to wrap this operation in a try-except block to catch and handle potential IOError or PermissionError exceptions gracefully. This would improve the robustness of the code by ensuring that such errors are handled appropriately, possibly logging an error message or retrying the operation.
+ try:
with open(
os.path.join(working_dir if working_dir else export_dir, "crytic_compile.config.json"),
"w",
) as f:
json.dump(metadata_config, f)
+ except (IOError, PermissionError) as e:
+ LOGGER.error("Failed to write metadata_config to file: %s", e)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.
| with open( | |
| os.path.join(working_dir if working_dir else export_dir, "crytic_compile.config.json"), | |
| "w", | |
| ) as f: | |
| json.dump(metadata_config, f) | |
| try: | |
| with open( | |
| os.path.join(working_dir if working_dir else export_dir, "crytic_compile.config.json"), | |
| "w", | |
| ) as f: | |
| json.dump(metadata_config, f) | |
| except (IOError, PermissionError) as e: | |
| LOGGER.error("Failed to write metadata_config to file: %s", e) |
|
I decided to output metadata directly to the |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- crytic_compile/platform/etherscan.py (1 hunks)
- scripts/ci_test_etherscan.sh (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- crytic_compile/platform/etherscan.py
scripts/ci_test_etherscan.sh
Outdated
| # From crytic/crytic-compile#544 | ||
| echo "::group::Etherscan #8" | ||
| crytic-compile 0x9AB6b21cDF116f611110b048987E58894786C244 --etherscan-apikey "$GITHUB_ETHERSCAN" | ||
|
|
||
| if [ $? -ne 0 ] | ||
| then | ||
| echo "Etherscan #8 test failed" | ||
| exit 255 | ||
| fi | ||
|
|
||
| dir_name=$(ls crytic-export/etherscan-contracts/ | grep 0x9AB6b21cDF116f611110b048987E58894786C244) | ||
| cd crytic-export/etherscan-contracts/$dir_name | ||
|
|
||
| if [ ! -f crytic_compile.config.json ]; then | ||
| echo "crytic_compile.config.json does not exist" | ||
| exit 255 | ||
| fi | ||
|
|
||
| # TODO: Globbing at crytic_compile.py:720 to run with '.' | ||
| crytic-compile 'contracts/InterestRates/InterestRatePositionManager.f.sol' --config-file crytic_compile.config.json | ||
|
|
||
| if [ $? -ne 0 ] | ||
| then | ||
| echo "crytic-compile command failed" | ||
| exit 255 | ||
| fi | ||
|
|
||
| cd ../../../ | ||
|
|
||
| echo "::endgroup::" |
There was a problem hiding this comment.
The addition of Etherscan test #8 introduces a new test case for compiling a specific contract and checking for the existence of crytic_compile.config.json before running the compilation process. This is a positive step towards automating the generation of solc-remaps files for projects downloaded from Etherscan. However, there are a few areas that could be improved for clarity, maintainability, and robustness:
-
Use of
$?for Exit Status: Directly after running a command, the script checks$?for the exit status. This is a common practice but can lead to errors if additional commands are inadvertently inserted between the command and its status check. Consider capturing the exit status immediately in a variable to avoid such issues. -
Hardcoded Contract Address: The contract address
0x9AB6b21cDF116f611110b048987E58894786C244is used twice, suggesting it has a specific significance for this test. It would be beneficial to define this as a variable at the start of the test section for clarity and ease of maintenance. -
Checking for Configuration File Existence: The script checks for the existence of
crytic_compile.config.jsonbut does not verify its content or structure. While the presence of the file is a good first step, ensuring it contains the expected configuration (even in a basic form) could prevent future errors. -
TODO Comment on Globbing: The TODO comment regarding globbing at
crytic_compile.py:720to run with'.'suggests an improvement or a fix that is pending. It's important to track these TODOs outside of the codebase, such as in an issue tracker, to ensure they are not forgotten. -
Directory Navigation: The script navigates into and out of directories using
cd. While this works, it can be error-prone if the script fails or exits unexpectedly before reaching thecd ../../../. Consider usingpushdandpopdfor more robust directory navigation, or running the commands in a subshell to avoid changing the working directory of the script. -
Error Handling and Messages: The error messages are clear but could include more context to aid in debugging. For example, specifying which part of the test failed or including the contract address in the error message could provide immediate clues without needing to dig through the script.
Improvements in these areas could enhance the script's readability, maintainability, and error resilience.
+ CONTRACT_ADDRESS="0x9AB6b21cDF116f611110b048987E58894786C244"
+ echo "Testing Etherscan compilation for contract $CONTRACT_ADDRESS"
- 105~
- 106~ if [ $? -ne 0 ]
+ COMPILE_STATUS=$?
+ if [ $COMPILE_STATUS -ne 0 ]
- 117~ # TODO: Globbing at crytic_compile.py:720 to run with '.'
+ # Consider tracking this TODO in an issue tracker
- 119~ if [ $? -ne 0 ]
+ COMPILE_STATUS=$?
+ if [ $COMPILE_STATUS -ne 0 ]
- 124~ cd ../../../
+ popdCommittable 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.
| # From crytic/crytic-compile#544 | |
| echo "::group::Etherscan #8" | |
| crytic-compile 0x9AB6b21cDF116f611110b048987E58894786C244 --etherscan-apikey "$GITHUB_ETHERSCAN" | |
| if [ $? -ne 0 ] | |
| then | |
| echo "Etherscan #8 test failed" | |
| exit 255 | |
| fi | |
| dir_name=$(ls crytic-export/etherscan-contracts/ | grep 0x9AB6b21cDF116f611110b048987E58894786C244) | |
| cd crytic-export/etherscan-contracts/$dir_name | |
| if [ ! -f crytic_compile.config.json ]; then | |
| echo "crytic_compile.config.json does not exist" | |
| exit 255 | |
| fi | |
| # TODO: Globbing at crytic_compile.py:720 to run with '.' | |
| crytic-compile 'contracts/InterestRates/InterestRatePositionManager.f.sol' --config-file crytic_compile.config.json | |
| if [ $? -ne 0 ] | |
| then | |
| echo "crytic-compile command failed" | |
| exit 255 | |
| fi | |
| cd ../../../ | |
| echo "::endgroup::" | |
| # From crytic/crytic-compile#544 | |
| echo "::group::Etherscan #8" | |
| CONTRACT_ADDRESS="0x9AB6b21cDF116f611110b048987E58894786C244" | |
| echo "Testing Etherscan compilation for contract $CONTRACT_ADDRESS" | |
| crytic-compile $CONTRACT_ADDRESS --etherscan-apikey "$GITHUB_ETHERSCAN" | |
| COMPILE_STATUS=$? | |
| if [ $COMPILE_STATUS -ne 0 ] | |
| then | |
| echo "Etherscan #8 test failed" | |
| exit 255 | |
| fi | |
| dir_name=$(ls crytic-export/etherscan-contracts/ | grep $CONTRACT_ADDRESS) | |
| cd crytic-export/etherscan-contracts/$dir_name | |
| if [ ! -f crytic_compile.config.json ]; then | |
| echo "crytic_compile.config.json does not exist" | |
| exit 255 | |
| fi | |
| # Consider tracking this TODO in an issue tracker | |
| crytic-compile 'contracts/InterestRates/InterestRatePositionManager.f.sol' --config-file crytic_compile.config.json | |
| COMPILE_STATUS=$? | |
| if [ $COMPILE_STATUS -ne 0 ] | |
| then | |
| echo "crytic-compile command failed" | |
| exit 255 | |
| fi | |
| popd | |
| echo "::endgroup::" |
Improve handling of "version" string from Etherscan
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crytic_compile/platform/etherscan.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- crytic_compile/platform/etherscan.py
0xalpharush
changed the title
0xalpharush
changed the title
Related: #543
PoC as it only deals with the most common scenario when relative imports were originally structured around @package-name. For foundry projects it's most common to specify remappings string without the "@" and there are also more messier examples in the wild. I think it should be possible to cover most of them though.
What does it do?
After downloading and compiling code with the use of Etherscan platform it will outputsolc_remaps.txtfile with "guessed" remappings strings, built out of absolute paths used internally by crytic-compile at first download. Then, it will save the file to the working directory of project (downloaded files)How to use?
No additional command is needed. Run crytic-compile as usual with address on the network as the target. After it finishescdinto the root of downloaded project (crytic-export/etherscan-contracts/0x1-Test) and run:crytic-compile contracts/Target.sol --solc-remaps $(cat solc_remaps.txt)Edit/Update: Changed approach from the original
Summary by CodeRabbit
metadata_configfor Solidity compilation settings, improving project configuration and setup.ci_test_etherscan.shto validate contract compilation and configuration file existence.