-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(forge): add accessList and cold/warm cheatcodes #10112
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm,
one concern re the accesslist cheatcode but we can also patch this separately if this is indeed not working as expected.
FYI @tynes
crates/cheatcodes/src/evm.rs
Outdated
impl Cheatcode for noAccessListCall { | ||
fn apply(&self, state: &mut Cheatcodes) -> Result { | ||
let Self {} = self; | ||
state.access_list = Some(alloy_rpc_types::AccessList::default()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this just set to None
can we add a note here that this should set Some(empty) to force override this when applied to the evm env
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, add comment and set to empty only if prev set in 78da249
// Apply EIP-2930 access lists. | ||
if let Some(access_list) = &self.access_list { | ||
ecx.env.tx.access_list = access_list.to_vec(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not exactly sure if this has the desired effect, because idk when the evm does respect the configured accesslist, which could be already be before execution starts?@rakita
so maybe in order to get the desired effect here we'd need to replicate how accesslists are actually handled in the evm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be great to have some insights @rakita, I assumed they're applied because the test below (included in PR) shows different gas usage (access list consist of single address, no storage slot, so the difference of 2400 sums up. If adding another address there's another 2400 of gas, if adding also a storage slot, an additional 1900 is added)
uint256 initial = gasleft();
write.setNumber(1);
assertEq(initial - gasleft(), 26762);
vm.accessList(accessList);
uint256 initial1 = gasleft();
write.setNumber(2);
assertEq(initial1 - gasleft(), 29162);
// reset access list, should take same gas as before
vm.noAccessList();
uint256 initial4 = gasleft();
write.setNumber(4);
assertEq(initial4 - gasleft(), 26762);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like it, I think we can proceed with this as is and change in case this isn't actually the case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, they are loaded in execution, this is okay.
what about vm.cool? |
vm.cool is for entire account while the new one takes the slot to be set, I don't think they're redundant, wdyt? |
right, but i would like for the names to align or at least provide some more context like coolSlot |
makes sense, will follow up with a PR to rename coolSlot/warmSlot |
follow-up pr #10128 pls check |
- adds new cheatcodes introduced in foundry-rs/foundry#10112 foundry-rs/foundry#10128 - add `cool` by removing Experimental status - include `expectCreate` / `expectCreate2` and `foundryVersionAtLeast/foundryVersionCmp`
Motivation
Solution
PR Checklist