expectRevert fails when returning a struct

[k-g-j]

Component

Forge

Have you ensured that all of these are up to date?

• Foundry
• Foundryup

What version of Foundry are you on?

33dbaa5 2022-07-23T00:08:24.676044Z

What command(s) is the bug in?

forge test

Operating System

macOS (Apple Silicon)

Describe the bug

My team and I are having an issue where tests expect a revert with the onlyOwner modifier reason and the logs show the call reverting with the expected reason, but the test fails. If you remove the reason in the expectRevert the test fails, but with onlyOwner modifier reason. We debugged the test and found that the issue occurs when returning the metadata struct. If we just return the address of the deployed vault from the deployVaultV1 function, the test passes as it is written with the expected revert reason. When we also try to return the metadata in the function, the test fails and reverts without reason.

1

function being tested from the factory contract

  function deployVaultV1(
    VaultParams memory _vaultParams,
    uint256 _type,
    bool _enabled,
    address _stakingAddress,
    address _submitter,
    string memory _metadataCID,
    address[8] memory _swapTokenAddresses,
    address _swapAddress,
    uint256 _exchange
  ) external onlyOwner returns (VaultMetadata memory, address) {
    Vault vault = new Vault(
      _vaultParams.token,
      _vaultParams.yearnRegistry,
      _vaultParams.contractRegistry,
      _vaultParams.staking,
      _vaultParams.feeStructure
    );
    VaultMetadata memory metadata = VaultMetadata({
      vaultAddress: address(vault),
      vaultType: _type,
      enabled: _enabled,
      stakingAddress: _stakingAddress,
      submitter: _submitter,
      metadataCID: _metadataCID,
      swapTokenAddresses: _swapTokenAddresses,
      swapAddress: _swapAddress,
      exchange: _exchange
    });
    emit VaultV1Deployment(address(vault));
    return (metadata, address(vault));
  }

expectRevert onlyOwner test (passes if deploy function modified to only return vault address, fails, and reverts without reason when the function is as written and returning the vault address and metadata struct)

  function test__deployVaultV1NotOwnerReverts() public {
    vm.expectRevert("Only the contract owner may perform this action");
    (VaultMetadata memory metadata, address vault) = vaultsV1Factory.deployVaultV1(
      vaultParams,
      1,
      true,
      address(0x1111),
      vaultsV1ControllerOwner,
      "someCID",
      swapTokenAddresses,
      address(0x2222),
      1
    );
    assertEq(vault, address(0), "vault deployment failed");
    assertEq(metadata.vaultAddress, address(0), "metadata not constructed");
  }

Factory contract
Factory test contract

[onbjerg]

This is because vm.expectRevert changes the status of the call from 0 (revert) to 1 (success) and returns dummy data. You cannot read the data returned from the call, because it is gibberish, so the rest of your test will be incorrect. We don't return a mock of your internal data structures, just a tiny bit of data to make Solidity happy. Does it work if you don't assign the return value to anything?

Edit: Triaged the issue, but if your test works w/o assigning the return data then I will close since this is expected (as mentioned before, the return data is gibberish, so Solidity won't be able to interpret it in a meaningful manner)

[k-g-j]

Unfortunately, I rewrote the test not to assign data or read the return variable, and the test still fails and reverts without reason
updated test

  function test__deployVaultV1NotOwnerReverts() public {
    vm.expectRevert("Only the contract owner may perform this action");
    vaultsV1Factory.deployVaultV1(
      vaultParams,
      1,
      true,
      address(0x1111),
      vaultsV1ControllerOwner,
      "someCID",
      swapTokenAddresses,
      address(0x2222),
      1
    );
  }

[onbjerg]

Interesting. I guess it's because the return data is not long enough for what Solidity expects... Or maybe it requires a very specific format?

cc @brockelmore Any thoughts/context on the length you chose originally for expectRevert or how to resolve this?

[brockelmore]

very likely given the size of this return object, we aren't allocating enough memory in the return data. I believe we maxed it at 10 words (320 bytes). I was hoping i'd eventually come up with a better way to do this (and may still exist?), but i haven't. we can either boost data size or add an overload for this specific case

edit: technically... we could peek at the abi inside forge at runtime and create an accurately sized byte array

[onbjerg]

That would require us to continually identify contracts which is something we tried to avoid because of the performance penalty. Unsure what the best path forward is... We can either boost the returned data in general or we could add an override or something that sets the size of the returned data, although that might not be super nice UX (have to figure out the size yourself)

[k-g-j]

Is there a way we can write an override for our specific test? Would be happy to help you all with troubleshooting this.

[brockelmore]

@k-g-j, currently the best work around is to just use testFail semantics unfortunately. Or you can do a low level call and abi.decode the result.

@onbjerg I have asked the solc guys to add type(a).size (for unrelated reasons) which would report the size of an object but they haven't done so :/. that would be ideal. I will ask them if they are open to it again, otherwise I think we just add an overload with extra parameter for size and boost default size (issue with boosting size is potentially change in gas snapshots if a contract returndatacopys the whole object)?