🧹 Make retry a stable feature (LayerZero-Labs#622)

janjakubnanista · web-flow · commit 87e1704dc476 · 2024-05-31T13:02:05.000-07:00
diff --git a/.changeset/pretty-suits-enjoy.md b/.changeset/pretty-suits-enjoy.md
@@ -0,0 +1,6 @@
+---
+"@layerzerolabs/devtools": patch
+"@layerzerolabs/toolbox-hardhat": patch
+---
+
+Remove LZ_ENABLE_EXPERIMENTAL_RETRY feature flag
diff --git a/EXPERIMENTAL.md b/EXPERIMENTAL.md
@@ -24,8 +24,6 @@ By default, the RPC calls that check the current state of your contracts are exe
 
 Parallel execution can improve the speed of this process significantly. However, since it requires a large number of RPC calls to be executed simultaneously, it can result in `HTTP 429 Too Many Requests` RPC errors when used with public RPCs.
 
-In general, we recommend combining this feature with <a href="#automatic-retries">automatic retries</a>.
-
 ### To enable
 
 `LZ_ENABLE_EXPERIMENTAL_PARALLEL_EXECUTION=1`
@@ -34,18 +32,6 @@ In general, we recommend combining this feature with <a href="#automatic-retries
 
 `LZ_ENABLE_EXPERIMENTAL_PARALLEL_EXECUTION=`
 
-## Automatic retries <a id="automatic-retries"></a>
-
-By default, the RPC calls that check the current state of your contracts are executed without retrying any failed requests. This feature flag enables an exponential backoff retry functionality on the RPC reads.
-
-### To enable
-
-`LZ_ENABLE_EXPERIMENTAL_RETRY=1`
-
-### To disable
-
-`LZ_ENABLE_EXPERIMENTAL_RETRY=`
-
 ## Batched transaction sending <a id="batched-send"></a>
 
 Some signers might support batched transaction sending (e.g. Gnosis Safe signer). If turned on, this feature flag will make use of the batched sending. If this feature flag is on and batched sending is not available, regular sending will be used instead.
diff --git a/packages/devtools/src/common/retry.ts b/packages/devtools/src/common/retry.ts
@@ -55,8 +55,7 @@ export const createDefaultRetryHandler = (loggerName: string = 'AsyncRetriable')
 }
 
 export const AsyncRetriable = ({
-    // We'll feature flag this functionality for the time being
-    enabled = !!process.env.LZ_ENABLE_EXPERIMENTAL_RETRY,
+    enabled = true,
     maxDelay,
     numAttempts = 3,
     onRetry = createDefaultRetryHandler(),
diff --git a/packages/devtools/test/common/retry.test.ts b/packages/devtools/test/common/retry.test.ts
@@ -2,185 +2,108 @@ import { AsyncRetriable } from '@/common/retry'
 
 describe('common/retry', () => {
     describe('AsyncRetriable', () => {
-        describe('when LZ_ENABLE_EXPERIMENTAL_RETRY is off', () => {
-            beforeAll(() => {
-                // We'll enable the AsyncRetriable for these tests
-                process.env.LZ_ENABLE_EXPERIMENTAL_RETRY = ''
-            })
-
-            it('shoult not retry', async () => {
-                const error = new Error('Told ya')
-                const handleRetry = jest.fn()
-                const mock = jest.fn().mockRejectedValue(error)
-
-                class WithAsyncRetriable {
-                    @AsyncRetriable({ onRetry: handleRetry })
-                    async iAlwaysFail(value: string) {
-                        return mock(value)
-                    }
+        it('should retry a method call 3 times by default', async () => {
+            const error = new Error('Told ya')
+            const mock = jest.fn().mockRejectedValue(error)
+
+            class WithAsyncRetriable {
+                @AsyncRetriable()
+                async iAlwaysFail(value: string) {
+                    return mock(value)
                 }
+            }
 
-                await expect(new WithAsyncRetriable().iAlwaysFail('y')).rejects.toBe(error)
+            await expect(new WithAsyncRetriable().iAlwaysFail('y')).rejects.toBe(error)
 
-                expect(mock).toHaveBeenCalledTimes(1)
-                expect(handleRetry).not.toHaveBeenCalled()
-            })
-
-            it('should retry if enabled is set to true', async () => {
-                const error = new Error('Told ya')
-                const mock = jest.fn().mockRejectedValue(error)
-
-                class WithAsyncRetriable {
-                    @AsyncRetriable({ enabled: true })
-                    async iAlwaysFail(value: string) {
-                        return mock(value)
-                    }
-                }
-
-                await expect(new WithAsyncRetriable().iAlwaysFail('y')).rejects.toBe(error)
-
-                expect(mock).toHaveBeenCalledTimes(3)
-                expect(mock).toHaveBeenNthCalledWith(1, 'y')
-                expect(mock).toHaveBeenNthCalledWith(2, 'y')
-                expect(mock).toHaveBeenNthCalledWith(3, 'y')
-            })
-
-            it('should have access to non-prototype properties', async () => {
-                const mock = jest.fn().mockResolvedValue(true)
+            expect(mock).toHaveBeenCalledTimes(3)
+            expect(mock).toHaveBeenNthCalledWith(1, 'y')
+            expect(mock).toHaveBeenNthCalledWith(2, 'y')
+            expect(mock).toHaveBeenNthCalledWith(3, 'y')
+        })
 
-                class WithAsyncRetriable {
-                    constructor(protected readonly property = 7) {}
+        it('should retry a method call N times if numAttempts is specified', async () => {
+            const error = new Error('Told ya')
+            const mock = jest.fn().mockRejectedValue(error)
 
-                    @AsyncRetriable({ enabled: true })
-                    async iHaveAccessToProperties() {
-                        return mock(this.property)
-                    }
+            class WithAsyncRetriable {
+                @AsyncRetriable({ numAttempts: 2 })
+                async iAlwaysFail(value: string) {
+                    return mock(value)
                 }
+            }
 
-                await expect(new WithAsyncRetriable().iHaveAccessToProperties()).resolves.toBe(true)
+            await expect(new WithAsyncRetriable().iAlwaysFail('y')).rejects.toBe(error)
 
-                expect(mock).toHaveBeenCalledTimes(1)
-                expect(mock).toHaveBeenNthCalledWith(1, 7)
-            })
+            expect(mock).toHaveBeenCalledTimes(2)
         })
 
-        describe('when LZ_ENABLE_EXPERIMENTAL_RETRY is on', () => {
-            beforeAll(() => {
-                // We'll enable the AsyncRetriable for these tests
-                process.env.LZ_ENABLE_EXPERIMENTAL_RETRY = '1'
-            })
-
-            afterAll(() => {
-                process.env.LZ_ENABLE_EXPERIMENTAL_RETRY = ''
-            })
-
-            it('should retry a method call 3 times by default', async () => {
-                const error = new Error('Told ya')
-                const mock = jest.fn().mockRejectedValue(error)
-
-                class WithAsyncRetriable {
-                    @AsyncRetriable()
-                    async iAlwaysFail(value: string) {
-                        return mock(value)
-                    }
+        it('should stop retrying if the onRetry handler returns false', async () => {
+            const error = new Error('Told ya')
+            const mock = jest.fn().mockRejectedValue(error)
+            const handleRetry = jest
+                .fn()
+                // We check that if we return undefined/void we'll keep trying
+                .mockReturnValueOnce(undefined)
+                // We check that if we return true we keep trying
+                .mockReturnValueOnce(true)
+                // After the third attempt we return false
+                .mockReturnValueOnce(false)
+
+            class WithAsyncRetriable {
+                @AsyncRetriable({ numAttempts: 10_000, onRetry: handleRetry })
+                async iAlwaysFail(value: string) {
+                    return mock(value)
                 }
+            }
 
-                await expect(new WithAsyncRetriable().iAlwaysFail('y')).rejects.toBe(error)
-
-                expect(mock).toHaveBeenCalledTimes(3)
-                expect(mock).toHaveBeenNthCalledWith(1, 'y')
-                expect(mock).toHaveBeenNthCalledWith(2, 'y')
-                expect(mock).toHaveBeenNthCalledWith(3, 'y')
-            })
+            await expect(new WithAsyncRetriable().iAlwaysFail('y')).rejects.toBe(error)
 
-            it('should retry a method call N times if numAttempts is specified', async () => {
-                const error = new Error('Told ya')
-                const mock = jest.fn().mockRejectedValue(error)
+            expect(mock).toHaveBeenCalledTimes(3)
+            expect(handleRetry).toHaveBeenCalledTimes(3)
+        })
 
-                class WithAsyncRetriable {
-                    @AsyncRetriable({ numAttempts: 2 })
-                    async iAlwaysFail(value: string) {
-                        return mock(value)
-                    }
-                }
+        it('should call the onRetry callback if provided', async () => {
+            const error = new Error('Told ya')
+            const handleRetry = jest.fn()
+            const mock = jest.fn().mockRejectedValue(error)
 
-                await expect(new WithAsyncRetriable().iAlwaysFail('y')).rejects.toBe(error)
-
-                expect(mock).toHaveBeenCalledTimes(2)
-            })
-
-            it('should stop retrying if the onRetry handler returns false', async () => {
-                const error = new Error('Told ya')
-                const mock = jest.fn().mockRejectedValue(error)
-                const handleRetry = jest
-                    .fn()
-                    // We check that if we return undefined/void we'll keep trying
-                    .mockReturnValueOnce(undefined)
-                    // We check that if we return true we keep trying
-                    .mockReturnValueOnce(true)
-                    // After the third attempt we return false
-                    .mockReturnValueOnce(false)
-
-                class WithAsyncRetriable {
-                    @AsyncRetriable({ numAttempts: 10_000, onRetry: handleRetry })
-                    async iAlwaysFail(value: string) {
-                        return mock(value)
-                    }
+            class WithAsyncRetriable {
+                @AsyncRetriable({ onRetry: handleRetry })
+                async iAlwaysFail(value: string) {
+                    return mock(value)
                 }
+            }
 
-                await expect(new WithAsyncRetriable().iAlwaysFail('y')).rejects.toBe(error)
+            const withAsyncRetriable = new WithAsyncRetriable()
 
-                expect(mock).toHaveBeenCalledTimes(3)
-                expect(handleRetry).toHaveBeenCalledTimes(3)
-            })
+            await expect(withAsyncRetriable.iAlwaysFail('y')).rejects.toBe(error)
 
-            it('should call the onRetry callback if provided', async () => {
-                const error = new Error('Told ya')
-                const handleRetry = jest.fn()
-                const mock = jest.fn().mockRejectedValue(error)
+            expect(handleRetry).toHaveBeenCalledTimes(3)
+            expect(handleRetry).toHaveBeenNthCalledWith(1, 1, 3, error, withAsyncRetriable, 'iAlwaysFail', ['y'])
+            expect(handleRetry).toHaveBeenNthCalledWith(2, 2, 3, error, withAsyncRetriable, 'iAlwaysFail', ['y'])
+            expect(handleRetry).toHaveBeenNthCalledWith(3, 3, 3, error, withAsyncRetriable, 'iAlwaysFail', ['y'])
+        })
 
-                class WithAsyncRetriable {
-                    @AsyncRetriable({ onRetry: handleRetry })
-                    async iAlwaysFail(value: string) {
-                        return mock(value)
-                    }
-                }
+        it('should resolve if the method resolves within the specified number of attempts', async () => {
+            const error = new Error('Told ya')
+            const value = {}
+            const handleRetry = jest.fn()
+            const mock = jest.fn().mockRejectedValueOnce(error).mockRejectedValueOnce(error).mockResolvedValue(value)
 
-                const withAsyncRetriable = new WithAsyncRetriable()
-
-                await expect(withAsyncRetriable.iAlwaysFail('y')).rejects.toBe(error)
-
-                expect(handleRetry).toHaveBeenCalledTimes(3)
-                expect(handleRetry).toHaveBeenNthCalledWith(1, 1, 3, error, withAsyncRetriable, 'iAlwaysFail', ['y'])
-                expect(handleRetry).toHaveBeenNthCalledWith(2, 2, 3, error, withAsyncRetriable, 'iAlwaysFail', ['y'])
-                expect(handleRetry).toHaveBeenNthCalledWith(3, 3, 3, error, withAsyncRetriable, 'iAlwaysFail', ['y'])
-            })
-
-            it('should resolve if the method resolves within the specified number of attempts', async () => {
-                const error = new Error('Told ya')
-                const value = {}
-                const handleRetry = jest.fn()
-                const mock = jest
-                    .fn()
-                    .mockRejectedValueOnce(error)
-                    .mockRejectedValueOnce(error)
-                    .mockResolvedValue(value)
-
-                class WithAsyncRetriable {
-                    @AsyncRetriable({ onRetry: handleRetry })
-                    async iAlwaysFail(value: string) {
-                        return mock(value)
-                    }
+            class WithAsyncRetriable {
+                @AsyncRetriable({ onRetry: handleRetry })
+                async iAlwaysFail(value: string) {
+                    return mock(value)
                 }
+            }
 
-                const withAsyncRetriable = new WithAsyncRetriable()
+            const withAsyncRetriable = new WithAsyncRetriable()
 
-                await expect(withAsyncRetriable.iAlwaysFail('y')).resolves.toBe(value)
+            await expect(withAsyncRetriable.iAlwaysFail('y')).resolves.toBe(value)
 
-                expect(handleRetry).toHaveBeenCalledTimes(2)
-                expect(handleRetry).toHaveBeenNthCalledWith(1, 1, 3, error, withAsyncRetriable, 'iAlwaysFail', ['y'])
-                expect(handleRetry).toHaveBeenNthCalledWith(2, 2, 3, error, withAsyncRetriable, 'iAlwaysFail', ['y'])
-            })
+            expect(handleRetry).toHaveBeenCalledTimes(2)
+            expect(handleRetry).toHaveBeenNthCalledWith(1, 1, 3, error, withAsyncRetriable, 'iAlwaysFail', ['y'])
+            expect(handleRetry).toHaveBeenNthCalledWith(2, 2, 3, error, withAsyncRetriable, 'iAlwaysFail', ['y'])
         })
     })
 })

Original file line number	Diff line number	Diff line change
`@@ -55,8 +55,7 @@ export const createDefaultRetryHandler = (loggerName: string = 'AsyncRetriable')`
`55`	`55`	`}`
`56`	`56`
`57`	`57`	`export const AsyncRetriable = ({`
`58`		`- // We'll feature flag this functionality for the time being`
`59`		`- enabled = !!process.env.LZ_ENABLE_EXPERIMENTAL_RETRY,`
	`58`	`+ enabled = true,`
`60`	`59`	`maxDelay,`
`61`	`60`	`numAttempts = 3,`
`62`	`61`	`onRetry = createDefaultRetryHandler(),`