Skip to content
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(cheatcodes): random* cheatcodes to aid in symbolic testing #8882

Merged
merged 19 commits into from
Sep 26, 2024

Conversation

grandizzy
Copy link
Collaborator

@grandizzy grandizzy commented Sep 17, 2024

Motivation

Re #4072 : add random* cheatcodes

Solution

  • added randomUint(bits), randomInt() and randomInt(bits), randomBool() and randomBytes(len) cheatcodes
  • use proptest and DynSolValue::type_strategy to draw arbitrary values for uint, address, bool, int and bytes
  • randomBytes(len) ensures returned byte array is of given length
  • ported Kontrol tests

@grandizzy grandizzy changed the title feat(cheatcodes): additional random cheatcodes to aid in symbolic testing feat(cheatcodes): arbitrary* cheatcodes to aid in symbolic testing Sep 17, 2024
@grandizzy grandizzy marked this pull request as ready for review September 18, 2024 13:49
crates/cheatcodes/spec/src/vm.rs Outdated Show resolved Hide resolved
function randomUint(uint256 min, uint256 max) external returns (uint256);

/// Returns a random `address`.
#[cheatcode(group = Utilities)]
/// `randomAddress` is being deprecated in favor of `arbitraryAddress`. It will be removed in future versions
#[cheatcode(group = Utilities, status = Deprecated(Some("replaced by `arbitraryAddress`")))]
function randomAddress() external returns (address);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to deprecate these? I like random more

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there were some discussions re how to name these, per @ehildenb
random would be a bit weird to use for symbolic execution, fresh or abitrary would be better
and we already added setArbitraryStorage instead setRandomStorage (see #8807 (comment))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not specific to symbolic execution

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, @ehildenb is ok to stick with random per Dani point?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed from arbitrary to random in 6820df6, we can discuss further namings but should be OK to stick with random for now

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delayed reply. I still would prefer fresh or arbitrary. The point of this change is to allow Foundry property tests to either be interpreted existentially (with random sampling), or universally (with symbolic execution). So I guess to me, the point is that fresh and arbitrary are neither specific to fuzzing nor symbolic execution (and work for both), and that's a bonus because it means the same test can be interpreted in both ways.

But we'll be fine with random, it just means the tests will read a bit funny for symbolic execution, where we'll have to tell users "random is interpreted differently here, as a fresh logical variable".

crates/cheatcodes/src/evm.rs Outdated Show resolved Hide resolved
crates/cheatcodes/src/utils.rs Outdated Show resolved Hide resolved
crates/cheatcodes/src/utils.rs Outdated Show resolved Hide resolved
crates/cheatcodes/src/utils.rs Outdated Show resolved Hide resolved
grandizzy and others added 5 commits September 21, 2024 12:35
- add fn rng back
- make sure cheats for uint/int/bytes doesn't panic + added tests
@grandizzy grandizzy changed the title feat(cheatcodes): arbitrary* cheatcodes to aid in symbolic testing feat(cheatcodes): random* cheatcodes to aid in symbolic testing Sep 23, 2024
@grandizzy grandizzy enabled auto-merge (squash) September 23, 2024 06:10
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these can just use rand, why do we have to use proptest?

crates/cheatcodes/src/inspector.rs Outdated Show resolved Hide resolved
crates/cheatcodes/src/evm.rs Outdated Show resolved Hide resolved
crates/cheatcodes/src/utils.rs Outdated Show resolved Hide resolved
@grandizzy
Copy link
Collaborator Author

All of these can just use rand, why do we have to use proptest?

the initial intent was to move bound logic in strategy and reuse uint / int strategies (hence having same logic for edge values and potentially fuzz from state as well) but probably better to reduce scope of this PR (and use plain rng) and follow up with another one, would this work?

@DaniPopes
Copy link
Member

If we have custom strategies sure, but as of right now these just wrap rand rng with a lot of extra steps for nothing

@grandizzy
Copy link
Collaborator Author

If we have custom strategies sure, but as of right now these just wrap rand rng with a lot of extra steps for nothing

kk, will bring them in when needed

@grandizzy
Copy link
Collaborator Author

If we have custom strategies sure, but as of right now these just wrap rand rng with a lot of extra steps for nothing

@DaniPopes Can you suggest a nice way to generate random Uint/Int with given number of bits without using strategies baked in logic?

@DaniPopes
Copy link
Member

DaniPopes commented Sep 26, 2024

We can reuse those, that's fine; I mean stuff like bytes and bool are simply functions on rng so we should just use those

@DaniPopes DaniPopes merged commit d15d71a into foundry-rs:master Sep 26, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants