Enable all testing modes by default, update property mode testing, improve UX, allow for contracts to have starting balances, and fix coverage panic #216
Conversation
- created testing provider utils file to evaluate whether an abi.method is an optimization / property test - updated fuzzer tests - made default property test prefix "invariant_"
anishnaik
changed the title
anishnaik
changed the title
|
Need to fix bug with target balances such that it accepts scientific notation instead of hex values. |
# Conflicts: # fuzzing/config/config.go # fuzzing/fuzzer.go # fuzzing/fuzzer_test.go # go.mod
WalkthroughThe overarching update focuses on refining and enhancing a fuzzing framework. Key modifications include renaming flags and functions for clarity, improving JSON marshaling for configuration, and adjusting logic across various components to better handle contract deployment, testing modes, and value generation. These changes streamline the framework's usability and functionality, making it more intuitive and effective for users aiming to identify vulnerabilities in smart contracts. Changes
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}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 22
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
go.sumis excluded by:!**/*.sum
Files selected for processing (39)
- cmd/fuzz_flags.go (6 hunks)
- cmd/init_flags.go (1 hunks)
- fuzzing/config/config.go (7 hunks)
- fuzzing/config/config_defaults.go (3 hunks)
- fuzzing/config/gen_fuzzing_config.go (1 hunks)
- fuzzing/coverage/coverage_maps.go (2 hunks)
- fuzzing/coverage/report_generation.go (1 hunks)
- fuzzing/executiontracer/execution_trace.go (1 hunks)
- fuzzing/fuzzer.go (7 hunks)
- fuzzing/fuzzer_test.go (26 hunks)
- fuzzing/test_case_assertion_provider.go (5 hunks)
- fuzzing/test_case_optimization_provider.go (4 hunks)
- fuzzing/test_case_property_provider.go (4 hunks)
- fuzzing/testdata/contracts/assertions/assert_and_property_test.sol (1 hunks)
- fuzzing/testdata/contracts/chain/tx_out_of_gas.sol (1 hunks)
- fuzzing/testdata/contracts/corpus_mutation/specific_call_sequence.sol (1 hunks)
- fuzzing/testdata/contracts/deployments/deploy_payable_constructors.sol (1 hunks)
- fuzzing/testdata/contracts/deployments/deployment_order.sol (2 hunks)
- fuzzing/testdata/contracts/deployments/deployment_with_args.sol (2 hunks)
- fuzzing/testdata/contracts/deployments/inner_deployment.sol (1 hunks)
- fuzzing/testdata/contracts/deployments/inner_deployment_on_construction.sol (1 hunks)
- fuzzing/testdata/contracts/deployments/inner_inner_deployment.sol (1 hunks)
- fuzzing/testdata/contracts/deployments/internal_library.sol (1 hunks)
- fuzzing/testdata/contracts/deployments/testing_scope.sol (2 hunks)
- fuzzing/testdata/contracts/value_generation/generate_all_types.sol (1 hunks)
- fuzzing/testdata/contracts/value_generation/match_addr_contract.sol (1 hunks)
- fuzzing/testdata/contracts/value_generation/match_addr_exact.sol (1 hunks)
- fuzzing/testdata/contracts/value_generation/match_addr_sender.sol (1 hunks)
- fuzzing/testdata/contracts/value_generation/match_ints_xy.sol (1 hunks)
- fuzzing/testdata/contracts/value_generation/match_payable_xy.sol (1 hunks)
- fuzzing/testdata/contracts/value_generation/match_string_exact.sol (1 hunks)
- fuzzing/testdata/contracts/value_generation/match_structs_xy.sol (1 hunks)
- fuzzing/testdata/contracts/value_generation/match_uints_xy.sol (1 hunks)
- fuzzing/testdata/contracts/vm_tests/block_hash_store_check.sol (1 hunks)
- fuzzing/testdata/contracts/vm_tests/block_number_increasing.sol (1 hunks)
- fuzzing/testdata/contracts/vm_tests/block_timestamp_increasing.sol (1 hunks)
- fuzzing/valuegeneration/abi_values.go (1 hunks)
- logging/logger.go (3 hunks)
- utils/testing_provider_utils.go (1 hunks)
Files skipped from review due to trivial changes (2)
- fuzzing/testdata/contracts/value_generation/match_addr_contract.sol
- fuzzing/testdata/contracts/vm_tests/block_timestamp_increasing.sol
Additional comments: 31
fuzzing/testdata/contracts/assertions/assert_and_property_test.sol (1)
- 8-8: Renaming
fuzz_failing_propertytoproperty_failing_propertyaligns with the updated property testing requirements and the new default test prefix. This change is consistent and clear.fuzzing/testdata/contracts/value_generation/match_addr_sender.sol (1)
- 11-11: Renaming
fuzz_never_specific_valuestoproperty_never_specific_valuesaligns with the updated property testing requirements and the new default test prefix. This change is consistent and clear.fuzzing/testdata/contracts/value_generation/match_string_exact.sol (1)
- 9-9: Renaming
fuzz_never_specific_valuestoproperty_never_specific_valuesaligns with the updated property testing requirements and the new default test prefix. This change is consistent and clear.fuzzing/testdata/contracts/deployments/inner_deployment.sol (1)
- 4-4: Renaming
fuzz_inner_deploymenttoproperty_inner_deploymentaligns with the updated property testing requirements and the new default test prefix. This change is consistent and clear.cmd/init_flags.go (1)
- 13-24: Renaming the flag from "target" to "compilation-target" improves clarity and aligns with the PR objectives. The implementation correctly updates the project configuration based on this flag.
fuzzing/test_case_assertion_provider.go (4)
- 8-8: Import of
utilsadded.- 48-57: Modified logic in
isTestableMethodto exclude optimization and property tests based on configurations.- 87-87: Updated usage of
AssertionModesConfigtoPanicCodeConfiginencounteredAssertionFailure.- 101-102: Changed verification of contracts to use
TargetContractsinstead ofDeploymentOrder.fuzzing/coverage/coverage_maps.go (1)
- 375-378: Adjusted the
setCoveredAtfunction to handle cases where the program counter exceeds the code size by returningfalsewithout an error message.fuzzing/config/config.go (6)
- 8-13: Added imports for
hexutil,big, andos.- 59-64: Modified
FuzzingConfigstruct fields: RenamedDeploymentOrdertoTargetContractsand addedTargetContractsBalances.- 99-104: Added
fuzzingConfigMarshalingstruct for custom JSON marshaling.- 132-132: Renamed
PropertyTestConfigtoPropertyTestingConfig.- 146-147: Renamed
AssertionModesConfigtoPanicCodeConfig.- 275-279: Added validation checks and logging in
ProjectConfig'sValidatemethod.fuzzing/test_case_property_provider.go (2)
- 49-53: Refactored the logic to check for testing prefixes before attaching a test case provider.
- 143-143: Updated the check for property test methods using a utility function from
utils.fuzzing/executiontracer/execution_trace.go (1)
- 198-210: The added condition to handle
outputArgumentsDisplayTextbeing nil andcallFrame.ReturnDatalength greater than 0 is correctly implemented. It ensures that return data is displayed appropriately when available, enhancing the method's functionality.fuzzing/test_case_optimization_provider.go (4)
- 8-10: Replacing the
abiimport withutilsand introducing a new import forslicesaligns with the changes in logic and functionality. Ensure that theutilspackage provides the necessary functionality previously covered byabi.- 49-52: The condition to return
nilif no testing prefixes are provided simplifies the initialization process by avoiding unnecessary attachment of a test case provider. This is a logical optimization.- 134-138: The use of
slices.Containsto check if the current contract is specified in the target contracts improves readability and maintainability of the code by utilizing standard library functions.- 141-141: The integration of
utils.IsOptimizationTestfor verifying optimization test methods aligns with the changes in import and ensures that the method identification logic is encapsulated within a utility function, promoting modularity.logging/logger.go (1)
- 59-63: Initializing structured and unstructured loggers with
zerolog.New(nil)changes the default behavior of where log output is directed. Confirm that this change is intentional and that log output is correctly managed elsewhere in the code.Verification successful
The grep results indicate that the loggers (
structuredLoggerandunstructuredLogger) are initially created withzerolog.New(nil)but are later configured withzerolog.MultiLevelWriterto manage log output destinations dynamically. This setup allows for flexible configuration of log output destinations, suggesting that the initial concern about log output management is addressed by the subsequent logger configuration. Therefore, the change to initialize loggers withnilappears to be intentional and part of a design to configure log output destinations dynamically.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that log output is correctly managed after initializing loggers with nil. grep -r "structuredLogger" . grep -r "unstructuredLogger" .Length of output: 4134
fuzzing/fuzzer.go (3)
- 6-9: Added imports for
coverage,logging,colors, andzerologare necessary for the new functionality and logging enhancements.- 111-129: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [95-126]
The global logger initialization and configuration have been updated. Ensure that the logger's level is correctly set based on the project configuration and that error handling is properly implemented when creating the log file.
Verification successful
The verification process has confirmed that the logger's level is correctly set based on the project configuration as shown in the provided code snippet. Additionally, the error handling when creating the log file is properly implemented, with the code checking for errors and logging them if any occur during the file creation process. This aligns with the review comment's concerns, verifying that both aspects are correctly addressed in the code.
* 334-353: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [337-425]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the logger's level is set correctly in all scenarios. ast-grep --lang go --pattern $'logging.GlobalLogger.SetLevel($_)' # Verify proper error handling when creating the log file. ast-grep --lang go --pattern $'if $_, $_ := utils.CreateFile($_, $_); $_ != nil {'Length of output: 572
Adjustments in handling target contracts and their balances during deployment need to ensure that the configuration is validated correctly, and that the deployment process accounts for specified balances. Verify that the deployment logic correctly handles the case where
TargetContractsBalancesis specified for each target contract.Verification successful
The provided code snippet explicitly handles
TargetContractsBalancesby checking its length and setting thecontractBalancefor each target contract accordingly. This indicates that the deployment logic correctly accounts for specified balances for target contracts as described in the review comment. Therefore, the review comment is consistent with the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the deployment logic correctly handles specified balances for target contracts. ast-grep --lang go --pattern $'if len($_.config.Fuzzing.TargetContractsBalances) > $_ {'Length of output: 434
fuzzing/valuegeneration/abi_values.go (1)
- 450-451: The conversion of dynamic-sized bytes to a hex string using
hex.EncodeToStringis correct and aligns with the PR objectives of standardizing their representation.fuzzing/fuzzer_test.go (3)
- 239-239: The configuration update here aligns with the PR objectives by enabling
AssertionTesting.Enabledand specifyingEnableFFIfor cheat code testing. This change is approved as it demonstrates an understanding of the testing framework's capabilities and the PR's goals.- 733-733: This test function appears to be a duplicate of the one starting at line 712, both testing block number increasing. Verify if this duplication is intentional or if it's a mistake. If it's a mistake, consider removing or modifying this test to cover a different aspect.
- 752-752: The configuration updates here are appropriate for the test scenario. However, ensure that the explicit disabling of testing modes aligns with the PR objectives of enabling all testing modes by default.
|
Left some comments here! All looks good besides a few smaller nitpicks. Once you resolve them all, we can merge! |
Will not be fixing this |
…ytic/medusa into dev/merge-assertion-and-property-mode
…prove UX, allow for contracts to have starting balances, and fix coverage panic (crytic#216) * - merged all three testing modes - created testing provider utils file to evaluate whether an abi.method is an optimization / property test - updated fuzzer tests - made default property test prefix "invariant_" * enable all testing modes, update fuzzer tests * add verification for config * improve config-related errors * update deploymentOrder to targetContracts * update edge case where coverage is 0/0 lines * add support for payable constructors * linting * fix bug * fix panic and improve return data printing in execution trace * encode bytes and byteX as hex strings * fix console * updates from PR review * change config language
…prove UX, allow for contracts to have starting balances, and fix coverage panic (#216) * - merged all three testing modes - created testing provider utils file to evaluate whether an abi.method is an optimization / property test - updated fuzzer tests - made default property test prefix "invariant_" * enable all testing modes, update fuzzer tests * add verification for config * improve config-related errors * update deploymentOrder to targetContracts * update edge case where coverage is 0/0 lines * add support for payable constructors * linting * fix bug * fix panic and improve return data printing in execution trace * encode bytes and byteX as hex strings * fix console * updates from PR review * change config language
Here are the following changes:
--assertion-modeand--optimization-modeflags were removed to simplify the CLI. First time users should not have to worry about disabling/enabling different testing modes. This PR closes Merge property and assertion mode #191.fuzz_toproperty_. This PR closes Rename fuzz_ default #182.deploymentOrderhas been renamed totargetContracts. This is purely a UX change to make it easier for people to understand what it means. The--targetflag has been changed to--compilation-targetto make the differentiation between--target-contractsand--compilation-targeta bit easier.targetContractsBalancesconfig option. Each index in thetargetContractsarray is associated with the same index in thetargetContractsBalancesarray. Thus, if you have[A, B, C]as yourtargetContractsandtargetContractsBalancesis[0, 1e18, 2e18]thenAwill have 0 wei,Bwill have 1 ether, andCwill have 2 ether. This PR closes Debugging "contract deployment tx returned a failed status: execution reverted module=fuzzer" #212. This PR closes Feature Request: Starting balance for fuzzing contract #213.Closes #145
Summary by CodeRabbit