Use native assertions

[klkvr]

Ref foundry-rs/foundry#6803 (comment)

Changes

1. Updates Vm interface to include native assertions
2. Removes DSTest dependency and moves all assert* functions to StdAssertions
3. Updates all implementations of assert* functions in StdAssertions with calls to vm.assert* cheatcodes
4. Updates tests for StdAssertions

Breaking changes

This PR is marked as a draft because in current version it introduces some breaking changes which I am not sure we want. I think it makes sense to discuss them here first and decide if we are ready to merge them.

1. DSTest includes some additional assert* functions which duplicate logic of their alternatives. For example assertEq0(bytes, bytes) equivalent to assertEq(bytes, bytes) or assertEq32(bytes32,bytes32) equivalent to assertEq(bytes32, bytes32). Do we want to keep those considering that we don't have native cheatcodes with such selectors or it makes sense to drop them now?
2. DSTest includes logic with failed() and fail() methods. Currently I haven't included this logic to StdAssertions because we are migrating to new assertions behavior when they revert immidiately. Removal of those should also not affect custom testing frameworks as they usually implement failed() method themselves on base Test contract. However, this will affect users or frameworks relying on explicit calls to fail() method on forge-std base Test contract. I am not sure if it is a common usecase and if it makes sense to keep it.
3. I have not migrated log_* events to StdAssertions except for log and log_named_bytes which are needed for assertEqCall assertions. Are we expecting those to be commonly used or all users should be using console.log calls at this point?
4. Some utility functions and modifiers from DSTest are not migrated too, not sure if they are used at all: hasHEVMContext, logs_gas, mayRevert, testopts, HEVM_ADDRESS

All changes are basically just something not being migrated from DSTest to StdAssertions and fixing it is as simple as copy-pasting it. However, I am not sure if we really want to keep compatibility with some of legacy methods above.

[mds1]

I haven't gotten to look at this closely yet, but regardless would like to get thoughts from @vdrg @gnkz @PaulRBerg since you maintain alternative assertion libs. In general what are your thoughts on these changes? Would it be a breaking change for you that's hard to support?

[PaulRBerg]

I'm OK with these changes.

It looks like it will continue to be possible to use Vm and StdCheats while ignoring the assertion features.

Also, if this gets implemented, I will consider stopping the development on PRBTest.

[mds1]

This PR looks great! No objections from me, answers to the your questions below:

1. DSTest includes some additional assert* functions which duplicate logic of their alternatives. For example assertEq0(bytes, bytes) equivalent to assertEq(bytes, bytes) or assertEq32(bytes32,bytes32) equivalent to assertEq(bytes32, bytes32). Do we want to keep those considering that we don't have native cheatcodes with such selectors or it makes sense to drop them now?

The reason for these duplicates is so users can avoid unnecessary casts when a type is ambiguous. For example "abc" is ambiguous and can be bytes, bytes32, or string. So you can either do assertEq(bytes32("abc"), x) or assertEq32("abc", x).

I searched on sourcegraph and found repos using these overloads. So we could add native cheats for these overloads to maintain backwards compatibility to mitigate this one (even though their names aren't the greatest)

2. DSTest includes logic with failed() and fail() methods. Currently I haven't included this logic to StdAssertions because we are migrating to new assertions behavior when they revert immidiately. Removal of those should also not affect custom testing frameworks as they usually implement failed() method themselves on base Test contract. However, this will affect users or frameworks relying on explicit calls to fail() method on forge-std base Test contract. I am not sure if it is a common usecase and if it makes sense to keep it.

Do we still respect the failed() method and fail tests if that returns true? i.e. now you can cause a test to fail by either calling vm.assert* OR by setting the failed flag? If so, I suspect this change should be ok, and it can be something we document so that assertion libs know to implement it

3. I have not migrated log_* events to StdAssertions except for log and log_named_bytes which are needed for assertEqCall assertions. Are we expecting those to be commonly used or all users should be using console.log calls at this point?

I would keep them for now for backwards compatibility reasons, and then in forge v1:

• Remove them
• Remove console.sol libs
• Replace with native vm.print cheatcodes

We should also upstream assertEqCall at some point

4.. Some utility functions and modifiers from DSTest are not migrated too, not sure if they are used at all: hasHEVMContext, logs_gas, mayRevert, testopts, HEVM_ADDRESS

AFAIK these should not be needed, and we have the HEVM_ADDRESS as VM_ADDRESS in Base.sol

[klkvr]

2. We haven't introduced any updates to foundry Rust code responsible for checking failed() return value, so any other projects relying on it should be fine.
3. Sounds good, assertEqCall will be upstreamed once we figure out a nice way to work with EVM directly from cheatcodes inspector, I believe @mattsse can give more context here

[gnkz]

Hey! This looks great! Our expect library doesn't use assertions from forge-std so I think we are safe

[mattsse]

for now we can be assertEqCall, so this isn't a blocker but ideally we figure out how to replace this as well

[mds1]

When I drop this version of forge-std in the optimism monorepo, two tests fail, even though they pass on the latest master. To reproduce:

1. git clone https://github.com/ethereum-optimism/optimism
2. cd optimism
3. pnpm install && pnpm build && cd packages/contracts-bedrock && pnpm build:go-ffi && forge install
4. Now you can run pnpm test from the packages/contracts-bedrock dir. Or just run the two tests that will later fail: testFuzz_slice_memorySafety_succeeds and testFuzz_toNibbles_memorySafety_succeeds. Everything should pass here
5. cd lib/forge-std and switch to this branch
6. cd ../.. and re-run those tests, and you'll see they fail

Both failing are failing on the vm.expectSafeMemory assertions

[klkvr]

@mds1 Interesting, it is probably happening because assertion cheatcodes previously operated only on stack (in cases of success) and now any assertion is performing a STATICCALL which needs the calldata to be stored in memory, thus expanding memory and going outside of the hardcoded range that is getting provided into expectSafeMemory

[brockelmore]

quick question: does the cheatcode set the storage of the vm contract? People may rely on this mechanism.

see: https://github.com/dapphub/ds-test/blob/e282159d5170298eb2455a6c05280ab5a73a4ef0/src/test.sol#L65-L76

[klkvr]

@brockelmore we don't set the bool vm storage on assertion failure, if I got your question right.

The new behavior for assertions is that they immediately revert on failure and I don't think that it makes sense to set that bool as any checks of it in Solidity code would be unreacheable

However, we are still checking it on test execution level and third-party testing frameworks can still set it and force tests to fail

[klkvr]

So, the minimal repro for this safeMemory bug looks like this

uint64 initPtr;
assembly {
    initPtr := mload(0x40)
}

vm.expectSafeMemory(initPtr, initPtr + 0x20);
assertEq(true, true);

This passes on current version of forge-std and fails on this PR version.

I am not sure what is a best thing to do here.

This can be mitigated by moving logic which needs to be checked for memory safety into some inner function and using vm.expectSafeMemoryCall on it instead. We can also add vm.stopExpectSafeMemory cheat which might be used to clear memory expectations and thus this issue will be fixable by only adding one line to failing tests.

[clabby]

This can be mitigated by moving logic which needs to be checked for memory safety into some inner function and using vm.expectSafeMemoryCall on it instead. We can also add vm.stopExpectSafeMemory cheat which might be used to clear memory expectations and thus this issue will be fixable by only adding one line to failing tests.

Was just in the process of suggesting this, front-ran :)

It is important that we preserve expectSafeMemory's operation on the current callframe IMO, but currently, it does create restrictions for the rest of the test (i.e., if we need to allocate memory for an external call, etc.). This is important because expectSafeMemory operates on a memory region, which we can only introspect if we are in the current context. Here's a good example of why being able to introspect the current context is important for using the cheatcode: https://github.com/ethereum-optimism/lib-keccak/blob/main/test/LibKeccak.t.sol#L103-L110

We know where the allocation begins definitively since we can introspect the current free memory pointer, whereas if it were in a separate context, we'd maybe assume that we started allocating at 0x80, but couldn't know for sure w/o manual inspection. expectSafeMemoryCall was included to allow for this, but generally, use of this cheatcode could be discouraged in favor of expectSafeMemory imo.

To allow for more flexibility in memory safety tests, a stopExpectSafeMemory-esq cheatcode would do the trick:

function test_whatever() public {
    // ... other logic ...

    vm.expectSafeMemory(0x420, 0x42069);
    // logic that's being checked.
    vm.stopExpectSafeMemory();

    // post-condition checks, etc. Before `stopExpectSafeMemory`, any memory allocations here are liable to throw.
}

The implementation of this cheatcode should also be very straightforwards, just clear the allowed_mem_writes map for the current call depth in the cheatcodes inspector on invocation.

[mds1]

@klkvr Two open questions before we can merge this:

1. It's a big change, so could you drop this forge-std branch into some of the integration test repos and make sure nothing breaks?
2. It's also a breaking change since users will need to add vm.stopExpectSafeMemory. So the options are:
   1. Releasing this as v1.8.0. We've done forge-std minor bump breaking changes in the past but try to avoid it, since we've (rightfully) gotten pushback
   2. Release this as v2.0.0
   3. Wait until foundry v1 and sync forge-std v2.0.0 release with foundry v1.0.0, and maintain this PR on a forge-std v2.0.0 feature branch for now

I lean towards (iii) because I'd like to avoid (i), and my concern with (ii) is we might have other breaking changes soon as part of foundry-rs/foundry#3782

[klkvr]

I am not sure if we should really treat expectSafeMemory as a blocker here as we will still probably have users with expectSafeMemory blocks not terminated via stopExpectSafeMemory because their tests are not using assertions and won't break after 2.0.0. Thus, we'd have to treat any change of the way forge-std manages memory as breaking change which wouldn't really let us do anything :/

I don't really want to go with (ii) as there is much more to do in terms of upstreaming stuff to native cheats, so IMO we should do either (i) or (iii)

It's a big change, so could you drop this forge-std branch into some of the integration test repos and make sure nothing breaks?

I've just tried doing it with several repos I've had on my machine already, it seems to break https://github.com/a16z/erc4626-tests as they rely on explicit calls to fail() method, thus openzeppein-contracts compilation fails also because it has erc4626-tests as a dependency.

I think if we decide to go with option (i) it makes sense to keep fail and failed

[mds1]

So a search of expectSafeMemory users show's there aren't many:

• Optimism, which we knew
• Blast, which is there because it's a fork of Optimism
• https://github.com/devtooligan/memory-strux but it has no assertions afterwards, cc @devtooligan to be safe
• https://github.com/aloelabs/aloe-ii but it has no assertions afterwards, cc @haydenshively to be safe
• https://github.com/matter-labs/foundry-zksync/blob/main/testdata/cheats/MemSafety.t.sol but this is just from foundry upstream

So I do think you're right that there should be no other unexpected breaking changes and a v1.8.0 is ok.

If we can go with (i) that is the easiest from a maintenance perspective, so we probably should add fail and failed back for now then?

[klkvr]

If we can go with (i) that is the easiest from a maintenance perspective, so we probably should add fail and failed back for now then?

Yep, will return them and do some more testing to ensure that we don't break anything else

[klkvr]

@mds1 so I've tested it on all repos from foundry integration tests that are using forge-std and on several external repos and haven't noticed any regression.

The only thing that a little bit concerns me is the result of experiment I did:

1. Firstly I installed forge-std via forge install from master of my forge-std fork (it didn't have those updates merged yet)
2. Then I merged updates into master and ran forge update

After that I got a warning from git: warning: unable to rmdir 'lib/ds-test': Directory not empty. And forge-std/lib/ds-test is still there with all the files. However, the forge-std code contains all updates.

I am not sure exactly what causes this, probably some strange behavior related to submodules getting removed, I will look into this

[mds1]

I've seen that before too, I think it's unrelated to this change and just wonky submodule / forge update behavior. These days I usually just do the equivalent of rm -rf lib/forge-std && forge install foundry-rs/[email protected] to update deps 😅

Since this worked on all repos, I think we should be good to merge and release as v1.8.0? Would like to get #505 in too for that if we can, pending comments from brock

[klkvr]

It's kind of a known issue https://git-scm.com/book/en/v2/Git-Tools-Submodules#Issues-with-Submodules, but yeah, I am still not sure what which gitmodule-related flag of git does exactly and considering that we already use several of them when dealing with submodules, we probably shoudln't try fixing it rn

[klkvr]

Since this worked on all repos, I think we should be good to merge and release as v1.8.0?

yep, I think it's ready to be merged

[mds1]

Thank you!