Waffle icon indicating copy to clipboard operation
Waffle copied to clipboard

Ethers caching block number causes problems with fixtures

Open vanruch opened this issue 4 years ago • 7 comments

When there are some tests with fixtures, the block numbers are returned incorrectly by provider.getBlockNumber(). It's caused by Ethers caching last block number.

Steps to reproduce

const fixture = async (wallets, _provider) => {
  [owner] = wallets
  provider = _provider
}

beforeEach(async () => {
  await loadFixture(fixture)
})

it('0', async () => {
  console.log(await provider.getBlockNumber())
  await owner.sendTransaction({ value: '0x1', to: AddressZero })
  await provider.getBlockNumber()
})

it('1', async () => {
  console.log(await provider.getBlockNumber())
})

It prints 0 and 1, while should print 0 for both tests

vanruch avatar Oct 14 '20 12:10 vanruch

I need some help for to fix that properly. Since it's marked 'Good first issue' it shouldn't spawn such concerns I have in #466 . Most likely I'm missing some pieces in the provider facilities. Could you guide me through @vanruch ?

wachulski avatar Mar 15 '21 12:03 wachulski

Thanks for looking into it @wachulski To be fair, I can't think of any other solution other than your's. Ethers for example sets _maxInternalBlockNumber to -1024 on every network change https://github.com/ethers-io/ethers.js/blob/5e0e3de52ecead2304ae8db68b7e67ceed9197a2/packages/providers/src.ts/base-provider.ts#L779. Maybe we should even copy all this code for resetting block number guards and caches?

vanruch avatar Mar 16 '21 11:03 vanruch

That's very fragile IMHO. That's internals:

// Reset all internal block number guards and caches

If we do so, there is significant risk it will interfere with some other state we are not aware yet and hit fixture creation/load with dirty provider state delivered. Now or in the future (as base provider authors may change their implementation in minor version bumps, no?)

Ethers for example sets _maxInternalBlockNumber to -1024 on every network change https://github.com/ethers-io/ethers.js/blob/5e0e3de52ecead2304ae8db68b7e67ceed9197a2/packages/providers/src.ts/base-provider.ts#L779

Yeah, but it's internals they do have control over (changes, releasing without major version bump, etc.) I was thinking about a sequence of provider exposed API calls that reset the internal state/caches (e.g. simulate network reset), but I still wasn't sure if it's against the primary design of the fixtures facility - to be efficient/performant. The obvious 'reset' option is to create a new provider, but for me it was obvious against the goals of this library.

wachulski avatar Mar 16 '21 12:03 wachulski

If we do so, there is significant risk it will interfere with some other state we are not aware yet and hit fixture creation/load with dirty provider state delivered.

Yep, true

reset the internal state/caches (e.g. simulate network reset

Not a bad idea, but that would still mean that we depend on ether provider's side effect of clearing the cache when network is changed. I still lean towards manual change of _maxInternalBlockNumber on each fixture load and submitting a PR to ethers adding public clearCache() method.

vanruch avatar Mar 16 '21 14:03 vanruch

still mean that we depend on ether provider's side effect of clearing the cache when network is changed

Ah, that's correct! I just realized network change does NOT guarantee any cleanup, so it's side effect effectively.

I still lean towards manual change of _maxInternalBlockNumber on each fixture load

Sounds pragmatic, sure. I will turn #466 to a regular PR then, but previously I submit a PR to ethers project, accordingly to your suggestion. Thanks!

wachulski avatar Mar 16 '21 17:03 wachulski

It seems clearing the internal cache is not feasible as it destroys ongoing operations. See: https://github.com/ethers-io/ethers.js/pull/1382 They recommend spawning a new instance of the Provider which I was afraid of (looks like it'd undermine the primary goal to have fast test setup - is this correct?) What do you think?

wachulski avatar Apr 26 '21 05:04 wachulski

Until the changes are made in each project, is there any workaround for this directly in our tests or do we just have to put up with failing tests for now?

GavinPacini avatar Jul 15 '21 02:07 GavinPacini