Add option to clear etherscan cache #1807
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lock data and etherscan data, respectively. before: ~/.foundry/cache/<chain>/block, ~/.foundry/cache/<chain>/etherscan after: ~/.foundry/cache/rpc/<chain>, ~/.foundry/cache/etherscan/<chain>
|
Hi folks! I'm a rust noob and still learning idiomatic rust so please feel free to nitpick to your heart's content. Also, I wanted to add some unit tests but wasn't sure exactly where to put them and couldn't find pre-existing tests for |
jaeaster
commented
Jun 1, 2022
config/src/lib.rs
Outdated
| @@ -1049,9 +1049,13 @@ impl Config { | |||
| } | |||
| if let Ok(entries) = cache_dir.as_path().read_dir() { | |||
| for entry in entries.flatten().filter(|x| x.path().is_dir()) { | |||
| let chain = Chain::from_str(&entry.file_name().to_string_lossy()).unwrap(); | |||
There was a problem hiding this comment.
As of now, every entry inside the cache/rpc directory is a valid chain_id. Maybe this changes in the future...
Is it a bad idea to use unwrap() here instead of handling the exceptional case?
There was a problem hiding this comment.
this shouldn't happen but just to be safe we could filter here by Some and continue the loop otherwise?
jaeaster
commented
Jun 1, 2022
config/src/lib.rs
Outdated
| cache.chains.push(ChainCache { | ||
| name: entry.file_name().to_string_lossy().into_owned(), | ||
| blocks: Self::get_cached_blocks(&entry.path())?, | ||
| block_explorer: Self::get_cached_block_explorer_data( | ||
| &Config::foundry_etherscan_chain_cache_dir(chain).unwrap(), |
There was a problem hiding this comment.
Similar to above ... this will panic if $HOME doesn't exist. It's a rare scenario, but my assumption is that unwrap() is not ok here.
There was a problem hiding this comment.
it should available for most OS but I agree, let's return an error in that case?
There was a problem hiding this comment.
Added explicit handling with clear error message if this directory doesn't exist
jaeaster
commented
Jun 1, 2022
| for block in chain_path | ||
| .read_dir()? | ||
| .flatten() | ||
| .filter(|x| x.file_type().unwrap().is_dir() && !x.file_name().eq("etherscan")) |
There was a problem hiding this comment.
removed the "etherscan" filter as I changed the cache directory structure to hoist the etherscan cache outside of this directory.
jaeaster
commented
Jun 1, 2022
| NumberPrefix::Standalone(size) => { | ||
| writeln!(f, "-️ {} ({:.1} B)", chain.name, size)?; | ||
| } | ||
| NumberPrefix::Prefixed(prefix, size) => { | ||
| writeln!(f, "-️ {} ({:.1} {}B)", chain.name, size, prefix)?; | ||
| } | ||
| } | ||
| match NumberPrefix::decimal(chain.block_explorer as f32) { | ||
| NumberPrefix::Standalone(size) => { | ||
| writeln!(f, "\t-️ Block Explorer ({:.1} B)\n", size)?; |
There was a problem hiding this comment.
Note the additional newline - it was cleaner to my eye to add an additional line to separate the etherscan data and the block data
jaeaster
commented
Jun 1, 2022
| #[derive(Debug)] | ||
| pub struct ChainCache { | ||
| /// The name of the chain | ||
| pub name: String, | ||
|
|
||
| /// A tuple containing block number and the block directory size in bytes | ||
| pub blocks: Vec<(String, u64)>, | ||
|
|
||
| /// The size of the block explorer directory in bytes | ||
| pub block_explorer: u64, |
There was a problem hiding this comment.
I suppose this could be block_explorer_size. Could also be named etherscan_size to keep it consistent with the flag --etherscan. It's always difficult to determine whether to keep names specific to ethereum or generalize for all EVM chains.
|
Found some integration tests here so will add a few of my own! https://github.com/foundry-rs/foundry/blob/master/cli/tests/it/cmd.rs#L41 |
mattsse
requested changes
Jun 2, 2022
config/src/lib.rs
Outdated
| @@ -1049,9 +1049,13 @@ impl Config { | |||
| } | |||
| if let Ok(entries) = cache_dir.as_path().read_dir() { | |||
| for entry in entries.flatten().filter(|x| x.path().is_dir()) { | |||
| let chain = Chain::from_str(&entry.file_name().to_string_lossy()).unwrap(); | |||
There was a problem hiding this comment.
this shouldn't happen but just to be safe we could filter here by Some and continue the loop otherwise?
config/src/lib.rs
Outdated
| cache.chains.push(ChainCache { | ||
| name: entry.file_name().to_string_lossy().into_owned(), | ||
| blocks: Self::get_cached_blocks(&entry.path())?, | ||
| block_explorer: Self::get_cached_block_explorer_data( | ||
| &Config::foundry_etherscan_chain_cache_dir(chain).unwrap(), |
There was a problem hiding this comment.
it should available for most OS but I agree, let's return an error in that case?
config/src/lib.rs
Outdated
Comment on lines
1072
to
1071
| let block_explorer_data_size = match Config::foundry_etherscan_chain_cache_dir(chain) { | ||
| Some(cache_dir) => Self::get_cached_block_explorer_data(&cache_dir).unwrap(), | ||
| None => 0, | ||
| }; |
There was a problem hiding this comment.
let's make it consistently no unwrap
There was a problem hiding this comment.
Removed all unwraps in favor of skipping or explicit errors. Also simplified the code by reusing the method to list a single cache entry when listing all cache entries.
jaeaster
force-pushed
the
etherscanCache
branch
2 times, most recently
from
f89fb53
to
89faba0
Compare
…eleting the etherscan cache data. e.g. forge cache clean --etherscan # deletes ~/.foundry/cache/etherscan forge cache clean mainnet --etherscan # deletes ~/.foundry/cache/etherscan/mainnet * test: Add tests for cache clean --etherscan
Oh and last thing, if you specify the On this note ^, I could use |
mattsse
approved these changes
Jun 2, 2022
|
Is this working properly? Doesn't seem to detect any chain now. Should we remove the |
|
A few more updates: @gakonst I believe it is working correctly. If it isn't, you should be getting an error message, rather than a silent non-print. What does your cache layout look like? The tricky thing about the cache integration tests is they share the filesystem state. So it's inconvenient for devs as you have to blow up your own cache and it's inconvenient for CI as you have to run them single-threaded. At the moment, they're really only useful to automate manual testing rather than ensuring correctness. On second thought, I can convert my integration test to a few unit tests for Lastly, I caught a bug calculating the cache size. The size was a shallow calculation rather than summing the files within subdirectories. Added a test for this as well. |
test: add integration test for 'forge cache ls' fix: calculate etherscan cache size based on all files in each subdirectory test: Unit test cache.to_string() used in 'forge cache ls'
…'forge cache clean'
|
I added a unit test to cover the discrepancy in cache output that you experienced @gakonst For future reference, is it better to wait until the PR is in a final state to squash commits? I know I keep making fixes and then overwriting previous commits and that may be difficult to see what has changed since a prior review. |
|
No need to squash before merging, we always do squash merge via Github so it's not required for you to do it! |
|
This is how my cache looks like: |
|
I think you may not have run the code from this branch. The reason I think so is because the layout of the cache has changed. The code now expects: Without the |
gakonst
approved these changes
Jun 4, 2022
|
closes #1477 |
Do you mean adding an option like I can open a separate issue to track this if you think it is a worthwhile use case. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Currently forge cache clean only clears RPC cache, but it would be beneficial to also be able to clear the Etherscan verification status cache.
Solves #1477
Solution
This PR:
Adds the
--etherscanflag toforge cache cleanDisplays the etherscan cache storage with
forge cache ls: