refactor(snapshot): code split #198
Conversation
core/src/snapshot/mod.rs
Outdated
| }) | ||
| } | ||
|
|
||
| fn set_logger_env(verbosity: &clap_verbosity_flag::Verbosity) { |
There was a problem hiding this comment.
let's move extra funcs to separate files, mod.rs should be for the main function only imo
There was a problem hiding this comment.
this could also be a heimdall-common function since I use it in each module.
There was a problem hiding this comment.
Completely agree. The first step was to split the function into smaller ones, now I want to distribute this functions to other files or group them into structs
common/src/ether/bytecode.rs
Outdated
| let (logger, _) = Logger::new(""); | ||
|
|
||
| if ADDRESS_REGEX.is_match(target)? { | ||
| // We are snapshotting a contract address, so we need to fetch the bytecode from the RPC |
There was a problem hiding this comment.
Update comments to be module agnostic
common/src/ether/bytecode.rs
Outdated
| logger.debug_max("using provided bytecode for snapshotting."); | ||
| Ok(target.replacen("0x", "", 1)) | ||
| } else { | ||
| logger.debug_max("using provided file for snapshotting."); |
There was a problem hiding this comment.
yes, it's available. I had to merge main in this PR due to some conflicts and already updated some places to use the new macro, but ended up missing this one
| @@ -144,7 +144,7 @@ pub async fn get_code( | |||
| None, | |||
| ); | |||
|
|
|||
| Ok(bytecode_as_bytes.to_string()) | |||
| Ok(bytecode_as_bytes.to_string().replacen("0x", "", 1)) | |||
There was a problem hiding this comment.
i added this, which normalizes the get_code response and never includes the 0x prefix.
previously, we stored the value in cache without the prefix, and returned it with the prefix. this just keeps everything consistent.
| use crate::debug_max; | ||
|
|
||
| // Find all function selectors and all the data associated to this function, represented by | ||
| // [`ResolvedFunction`] | ||
| pub async fn get_resolved_selectors( |
There was a problem hiding this comment.
ooh nice! we should use this function in other modules (such as decompile)
common/src/ether/bytecode.rs
Outdated
| }; | ||
| use std::fs; | ||
|
|
||
| pub async fn get_contract_bytecode( |
There was a problem hiding this comment.
we should use this function in other modules as well, such as decompile
There was a problem hiding this comment.
should we do this in this PR or in a separate one? I can def do on this one, it's just a matter of how you prefer reviewing the code.
the same question stands for get_resolved_selectors and get_shortned_target
common/src/ether/selectors.rs
Outdated
| resolved_selectors = | ||
| resolve_selectors::<ResolvedFunction>(selectors.keys().cloned().collect()).await; | ||
|
|
||
| // if resolved selectors are empty, we can't perform symbolic execution |
There was a problem hiding this comment.
update comment, we dont do symbolic execution here
There was a problem hiding this comment.
also, if resolved_selectors is empty, nothing fails. heimdall can guess things!
There was a problem hiding this comment.
so this log can be removed, right?
| /// let verbosity = clap_verbosity_flag::Verbosity::new(-1, 0); | ||
| /// set_logger_env(&verbosity); | ||
| /// ``` | ||
| pub fn set_logger_env(verbosity: &clap_verbosity_flag::Verbosity) { |
common/src/utils/io/logging.rs
Outdated
| /// let verbosity = clap_verbosity_flag::Verbosity::new(-1, 0); | ||
| /// get_logger_and_trace(&verbosity); | ||
| /// ``` | ||
| pub fn get_logger_and_trace(verbosity: &clap_verbosity_flag::Verbosity) -> (Logger, TraceFactory) { |
There was a problem hiding this comment.
for now you can actually just use Logger::new, it'll return (Logger, TraceFactory)
I do have #126 open which will update this functionality eventually (all logger methods being moved to macros, TraceFactory separated from Logger.
common/src/utils/strings.rs
Outdated
| /// let long_target = "0".repeat(80); | ||
| /// let shortened_target = get_shortned_target(&long_target); | ||
| /// ``` | ||
| pub fn get_shortned_target(target: &String) -> String { |
There was a problem hiding this comment.
nice! let's make sure to use this function throughout the codebase
core/src/snapshot/resolve.rs
Outdated
| Ok(()) | ||
| } | ||
|
|
||
| async fn resolve_custom_events_signatures( |
There was a problem hiding this comment.
rename to resolve_event_signatures
Motivation
The core logic for the snapshot module is currently implemented into a single function. At times, due to its length, this function can be hard to fully grasp. Also the internal logic is not testable at the moment, which can make debugging hard in the future.
Solution
This PR aims to improve code quality, testability and maintainability of
snapshotmodule by splitting its core logic into smaller chunks