Smock call record is wiped out after every transaction

[azf20]

Describe the bug When implementing onERC721Received functionality on an ERC721 Gateway, the smocked pattern used in the other tests fails with:

TypeError: Cannot read property 'map' of undefined
      at Object.get calls [as calls] (node_modules/@eth-optimism/smock/src/smockit/smockit.ts:38:12)

The functionality is working in integration tests (i.e. the token is being passed across the bridge when this function is called), verified on this branch: https://github.com/austintgriffith/scaffold-eth/tree/oo-wip

To Reproduce https://github.com/azf20/contracts/tree/erc721-bridge-smocking-onERC721Received

yarn install
yarn test ./test/contracts/OVM/bridge/assets/OVM_ERC721Gateway.spec.ts

Fails with TypeError: Cannot read property 'map' of undefined

Expected behavior The smocked call information is returned to verify that the implementation is correct

System Specs:

• OS: Mac M1 Big Sur 11.2.2
• Package Version (or commit hash): "@eth-optimism/smock": "0.2.1-alpha.0",

Additional context This could be user error (on my part), but the function is working as intended and the pattern works in the other tests.

[smartcontracts]

Looking into this!

[smartcontracts]

I see that you're using 0.2.1-alpha.0. We're making pretty big changes in 1.0.0-alpha.X. I'm going to try running your stuff against the latest 1.0.0-alpha.X to see if this is still an issue.

[smartcontracts]

Hmmm I ran this locally but didn't get any errors. Could you double check the reproduction steps? Maybe I'm just looking at the wrong file

[azf20]

Hey sorry I had not pushed the failing line 🤦 it should work (or rather not work) now

[smartcontracts]

Hah, no problem. Trying again now.

[smartcontracts]

Ahhh okay. This is an issue I've been aware of but haven't written up a ticket for yet. You can actually get your test to pass if you swap the ordering of statements in your test as follows:

// original
      const newTokenOwner = await ERC721.ownerOf(depositTokenId)
      expect(newTokenOwner).to.equal(OVM_ERC721Gateway.address)

      const depositCallToMessenger =
        Mock__OVM_L1CrossDomainMessenger.smocked.sendMessage.calls[0]

      // Message should be sent to the L2ERC20Gateway on L2
      expect(depositCallToMessenger._target).to.equal(
        Mock__OVM_DepositedERC721.address
      )

// "fixed"
      const depositCallToMessenger =
        Mock__OVM_L1CrossDomainMessenger.smocked.sendMessage.calls[0]

      // Message should be sent to the L2ERC20Gateway on L2
      expect(depositCallToMessenger._target).to.equal(
        Mock__OVM_DepositedERC721.address
      )

      const newTokenOwner = await ERC721.ownerOf(depositTokenId)
      expect(newTokenOwner).to.equal(OVM_ERC721Gateway.address)

The underlying reason for this is because I implemented the .calls logic in a terribly hacky way. Calls get reset on every transaction -- the ERC721.ownerOf call ends up resetting the calls and you get an error. We're planning to fix this long-term in 1.0.0 by changing how the call assertions work and making it look more like python's mocks. So it'll look more like:

expect(mock.smocked.function.calledWithArgs(...)).to.be.true

[azf20]

Dreamy that worked! And the long-term fix sounds great too. Thanks so much. Sorry I thought I had tried changing the order around a bit but clearly not enough.

[smartcontracts]

No problem! I'm going to leave this issue open as a reference + add to the 1.0 roadmap. Thank you again for the report!!

[maurelian]

I came here to report a similar issue, ie. if no calls have been made to a function, then the calls array does not exist.

Example, I wanted to test that under a certain condition, no call is made to a contract. Using smock I would do something like:

expect(Mock__OVM_L2ToL1MessagePasser.smocked.sendMessage.calls.length).to.deep.equal(0)

That results in the same error in the above description (TypeError: Cannot read property 'map' of undefined). If it helps for context, here is what I wrote instead.

LMK if you'd like a separate issue to support an empty calls array, or if this comment suffices.

[smartcontracts]

LMK if you'd like a separate issue to support an empty calls array, or if this comment suffices.

@maurelian could you make another issue for this? Supporting empty calls is much easier than the full update to the call assertion format so I'd like to push out a temp fix for that ASAP. Having a separate issue will make that specific bug easier to track.