feat: Add ability to defer compilation #547
Conversation
WalkthroughThe recent update introduces a significant enhancement to the 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 (
|
|
|
iliastsa
requested review from
montyly,
0xalpharush and
bohendo
as code owners
![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: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- crytic_compile/crytic_compile.py (3 hunks)
- tests/test_metadata.py (2 hunks)
Additional comments: 4
tests/test_metadata.py (1)
- 46-55: The
test_metadata_deferredfunction correctly tests the deferred compilation feature by settingdefer_compilation=Trueand then explicitly callingcrytic_compile_instance.compile(). This aligns with the PR objectives and demonstrates the intended use of the new feature. However, ensure that thecompilemethod ofCryticCompileis designed to handle being called in this manner, especially considering the initial state with no compilation units.crytic_compile/crytic_compile.py (3)
- 120-120: The addition of the
defer_compilationparameter in the__init__method introduces flexibility in managing the compilation process, allowing it to be deferred. This is a positive change that aligns with the PR objectives.- 209-210: The conditional check for
defer_compilationto decide whether to compile immediately or defer is correctly implemented. However, ensure that all callers ofCryticCompileare aware of this new behavior and correctly pass thedefer_compilationflag based on their requirements.- 640-641: The new
compilemethod enables deferred compilation by calling_compilewith previously stored compilation arguments. This method is well-implemented and adheres to the DRY principle by reusing the_compilemethod. Ensure that this method is called appropriately wherever deferred compilation is needed.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- crytic_compile/crytic_compile.py (2 hunks)
- tests/test_metadata.py (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- crytic_compile/crytic_compile.py
- tests/test_metadata.py
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crytic_compile/crytic_compile.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- crytic_compile/crytic_compile.py
| self._compile(**kwargs) | ||
| self.compilation_kwargs = kwargs | ||
|
|
||
| if not kwargs.get("crytic_defer_compilation") == "true": |
There was a problem hiding this comment.
Could this be a boolean instead of a magic string?
| if not kwargs.get("crytic_defer_compilation") == "true": | |
| if not kwargs.get("crytic_defer_compilation", False): |
| def test_metadata_deferred() -> None: | ||
| crytic_compile_instance = CryticCompile( | ||
| "0x6B175474E89094C44Da98b954EedeAC495271d0F", # solc 0.5.12 | ||
| crytic_defer_compilation="true", |
There was a problem hiding this comment.
| crytic_defer_compilation="true", | |
| crytic_defer_compilation=True, |
There was a problem hiding this comment.
That how I had it initially, but I was getting mypy errors due to the type of kwargs (only takes str right now). If we are ok with changing that to something like str | bool then I'm all for changing it.
The idea is to be able to use various inferred facts from Crytic (config file paths, inferred build system etc). In our use-case we'd like to modify configuration files right before compilation. One way to emulate this without adding additional flags is to subclass the CryticCompile class and override the class MyCryticCompile(CryticCompile):
def _compile(self, **kwargs):
# My logic here before compilation
...
super()._compile(kwargs)
# My logic here after compilation
...but I think offering this capability with out having to create a new class is a bit cleaner. |
|
We just need to get the linters passing @iliastsa |
This PR adds the ability to defer compilation at a later stage, instead of doing it at the time of the
CryticCompileobject. This is useful when the client code wants to make use of some properties that are initialized before compilation is triggered.Summary by CodeRabbit