Require block number when working with cache (#83)

packages/discovery/CHANGELOG.md

@@ -1,5 +1,11 @@
 # @l2beat/discovery
 
+## 0.19.0
+
+### Minor Changes
+
+- Block number is now required when working with cache despite invoked method
+
 ## 0.18.4
 
 ### Patch Changes

packages/discovery/package.json

@@ -1,7 +1,7 @@
 {
   "name": "@l2beat/discovery",
   "description": "L2Beat discovery - engine & tooling utilized for keeping an eye on L2s",
-  "version": "0.18.4",
+  "version": "0.19.0",
   "main": "dist/index.js",
   "types": "dist/index.d.ts",
   "bin": {

packages/discovery/src/discovery/provider/ProviderWithCache.test.ts

@@ -3,6 +3,7 @@ import { providers } from 'ethers'
 
 import { ChainId } from '../../utils/ChainId'
 import { EtherscanLikeClient } from '../../utils/EtherscanLikeClient'
+import { Hash256 } from '../../utils/Hash256'
 import { DiscoveryLogger } from '../DiscoveryLogger'
 import { DiscoveryCache, ProviderWithCache } from './ProviderWithCache'
 
@@ -35,26 +36,6 @@ function setupProviderWithMockCache(values: {
 
 describe(ProviderWithCache.name, () => {
   describe(ProviderWithCache.prototype.cacheOrFetch.name, () => {
-    it('works when reorgSafeDepth and blockNumber is undefined, value cached', async () => {
-      const { providerWithCache, mockCache } = setupProviderWithMockCache({
-        curBlockNumber: 1000,
-        reorgSafeDepth: undefined,
-      })
-
-      const blockNumber = undefined
-      const result = await providerWithCache.cacheOrFetch(
-        'mockCachedKey',
-        blockNumber,
-        async () => 'mockNotCachedValue',
-        (value) => value,
-        (value) => value,
-      )
-
-      expect(result).toEqual('mockCachedValue')
-      expect(mockCache.get).toHaveBeenCalledWith('mockCachedKey')
-      expect(mockCache.set).toHaveBeenCalledTimes(0)
-    })
-
     it('works when reorgSafeDepth is undefined, value cached', async () => {
       const { providerWithCache, mockCache } = setupProviderWithMockCache({
         curBlockNumber: 1000,
@@ -75,30 +56,6 @@ describe(ProviderWithCache.name, () => {
       expect(mockCache.set).toHaveBeenCalledTimes(0)
     })
 
-    it('works when reorgSafeDepth and blockNumber is undefined, value not cached', async () => {
-      const { providerWithCache, mockCache } = setupProviderWithMockCache({
-        curBlockNumber: 1000,
-        reorgSafeDepth: undefined,
-      })
-
-      const blockNumber = undefined
-      const result = await providerWithCache.cacheOrFetch(
-        'mockNotCachedKey',
-        blockNumber,
-        async () => 'mockNotCachedValue',
-        (value) => value,
-        (value) => value,
-      )
-
-      expect(result).toEqual('mockNotCachedValue')
-      expect(mockCache.set).toHaveBeenCalledWith(
-        'mockNotCachedKey',
-        'mockNotCachedValue',
-        1,
-        blockNumber,
-      )
-    })
-
     it('works when reorgSafeDepth is undefined, value not cached', async () => {
       const { providerWithCache, mockCache } = setupProviderWithMockCache({
         curBlockNumber: 1000,
@@ -265,4 +222,34 @@ describe(ProviderWithCache.name, () => {
       expect(mockProvider.getBlockNumber).toHaveBeenCalledTimes(3)
     })
   })
+
+  describe(ProviderWithCache.prototype.getTransaction.name, () => {
+    it('throws and does not cache non-mined transactions', async () => {
+      const txHash = Hash256.random()
+
+      const nonMinedTransactions = {
+        transactionHash: txHash,
+        blockNumber: undefined,
+        blockHash: undefined,
+        timestamp: undefined,
+      }
+
+      const { providerWithCache, mockCache, mockProvider } =
+        setupProviderWithMockCache({
+          curBlockNumber: 1000, // Doesn't matter
+          reorgSafeDepth: 1000, // Doesn't matter
+        })
+
+      // Force cache-miss to trigger getTransaction
+      mockCache.get = mockFn().resolvesToOnce(undefined)
+      mockProvider.getTransaction = mockFn().resolvesTo(nonMinedTransactions)
+
+      await expect(() =>
+        providerWithCache.getTransaction(txHash),
+      ).toBeRejectedWith('Transaction not mined')
+
+      expect(mockCache.get).toHaveBeenCalledTimes(1)
+      expect(mockCache.set).not.toHaveBeenCalled()
+    })
+  })
 })

packages/discovery/src/discovery/provider/ProviderWithCache.ts

@@ -1,3 +1,4 @@
+import { assert } from '@l2beat/backend-tools'
 import { createHash } from 'crypto'
 import { providers } from 'ethers'
 
@@ -19,7 +20,7 @@ export interface DiscoveryCache {
     key: string,
     value: string,
     chainId: number,
-    blockNumber: number | undefined,
+    blockNumber: number,
   ): Promise<void>
   get(key: string): Promise<string | undefined>
 }
@@ -47,7 +48,7 @@ export class ProviderWithCache extends DiscoveryProvider {
 
   public async cacheOrFetch<R>(
     key: string,
-    blockNumber: number | undefined,
+    blockNumber: number,
     fetch: () => Promise<R>,
     toCache: (value: R) => string,
     fromCache: (value: string) => R,
@@ -60,6 +61,7 @@ export class ProviderWithCache extends DiscoveryProvider {
     const result = await fetch()
 
     const isReorgSafe = await this.isBlockNumberReorgSafe(blockNumber)
+
     if (isReorgSafe) {
       await this.cache.set(
         key,
@@ -224,13 +226,25 @@ export class ProviderWithCache extends DiscoveryProvider {
   ): Promise<providers.TransactionResponse> {
     const key = this.buildKey('getTransaction', [hash])
 
-    return this.cacheOrFetch(
+    const cachedResult = await this.cache.get(key)
+
+    if (cachedResult !== undefined) {
+      return fromJSON(cachedResult)
+    }
+
+    const result = await super.getTransaction(hash)
+
+    // We don't want to cache nor return non-mined transactions
+    assert(result.blockNumber, 'Transaction not mined')
+
+    await this.cache.set(
       key,
-      undefined,
-      () => super.getTransaction(hash),
-      toJSON,
-      fromJSON,
+      toJSON(result),
+      this.chainId.valueOf(),
+      result.blockNumber,
     )
+
+    return result
   }
 
   override async getBlock(blockNumber: number): Promise<providers.Block> {

packages/discovery/src/discovery/provider/SQLiteCache.test.ts

@@ -64,27 +64,6 @@ describe('SQLiteCache', () => {
       expect(result.blockNumber).toEqual(newBlockNumber)
       expect(result.chainId).toEqual(newChainId)
     }))
-
-  it('transforms undefined into null', () =>
-    withTemporaryFile(async (sqlCache, rqe) => {
-      const key = 'key'
-      const value = 'value'
-      const chainId = 1
-      const blockNumber = undefined
-
-      await sqlCache.set(key, value, chainId, blockNumber)
-
-      const resultRaw = await rqe.query<CacheEntry[]>(
-        'SELECT * FROM cache WHERE key=$1',
-        [key],
-      )
-
-      const [result] = resultRaw
-
-      assert(result)
-
-      expect(result.blockNumber).toEqual(null!)
-    }))
 })
 
 interface CacheEntry {