fix: prioritize error states in waitForTransaction evaluation

Author: penovicp

__tests__/fixtures.ts

@@ -54,11 +54,8 @@ export const getTestProvider = (): ProviderInterface => {
   if (process.env.IS_LOCALHOST_DEVNET === 'true') {
     // accelerate the tests when running locally
     const originalWaitForTransaction = provider.waitForTransaction.bind(provider);
-    provider.waitForTransaction = (
-      txHash: string,
-      { retryInterval }: waitForTransactionOptions = {}
-    ) => {
-      return originalWaitForTransaction(txHash, { retryInterval: retryInterval || 1000 });
+    provider.waitForTransaction = (txHash: string, options: waitForTransactionOptions = {}) => {
+      return originalWaitForTransaction(txHash, { retryInterval: 1000, ...options });
     };
   }
 

__tests__/rpcProvider.test.ts

@@ -1,8 +1,17 @@
 import { getStarkKey, utils } from '@scure/starknet';
 
-import { Account, Contract, GetBlockResponse, RpcProvider, stark } from '../src';
+import {
+  Account,
+  CallData,
+  Contract,
+  GetBlockResponse,
+  RPC,
+  RpcProvider,
+  TransactionExecutionStatus,
+  stark,
+  waitForTransactionOptions,
+} from '../src';
 import { StarknetChainId } from '../src/constants';
-import { CallData } from '../src/utils/calldata';
 import { felt, uint256 } from '../src/utils/calldata/cairo';
 import { toHexString } from '../src/utils/num';
 import {
@@ -102,6 +111,66 @@ describeIfRpc('RPCProvider', () => {
     });
   });
 
+  describe('waitForTransaction', () => {
+    const receipt = {};
+    const transactionStatusSpy = jest.spyOn(rpcProvider as any, 'getTransactionStatus');
+    const transactionReceiptSpy = jest.spyOn(rpcProvider as any, 'getTransactionReceipt');
+
+    const generateOptions = (o: waitForTransactionOptions) => ({ retryInterval: 10, ...o });
+    const generateTransactionStatus = (
+      finality_status: RPC.SPEC.TXN_STATUS,
+      execution_status?: RPC.SPEC.TXN_EXECUTION_STATUS
+    ): RPC.TransactionStatus => ({
+      finality_status,
+      execution_status,
+    });
+    const response = {
+      successful: generateTransactionStatus('ACCEPTED_ON_L1', 'SUCCEEDED'),
+      reverted: generateTransactionStatus('ACCEPTED_ON_L2', 'REVERTED'),
+      rejected: generateTransactionStatus('REJECTED'),
+    };
+
+    beforeAll(() => {
+      transactionStatusSpy.mockResolvedValue(null);
+      transactionReceiptSpy.mockResolvedValue(receipt);
+    });
+
+    afterAll(() => {
+      transactionStatusSpy.mockRestore();
+      transactionReceiptSpy.mockRestore();
+    });
+
+    test('successful - default', async () => {
+      transactionStatusSpy.mockResolvedValueOnce(response.successful);
+      await expect(rpcProvider.waitForTransaction(0)).resolves.toBe(receipt);
+    });
+
+    test('reverted - default', async () => {
+      transactionStatusSpy.mockResolvedValueOnce(response.reverted);
+      await expect(rpcProvider.waitForTransaction(0)).resolves.toBe(receipt);
+    });
+
+    test('rejected - default', async () => {
+      transactionStatusSpy.mockResolvedValueOnce(response.rejected);
+      await expect(rpcProvider.waitForTransaction(0)).rejects.toThrow(
+        `${undefined}: ${RPC.ETransactionStatus.REJECTED}`
+      );
+    });
+
+    test('reverted - as error state', async () => {
+      transactionStatusSpy.mockResolvedValueOnce(response.reverted);
+      const options = generateOptions({ errorStates: [TransactionExecutionStatus.REVERTED] });
+      await expect(rpcProvider.waitForTransaction(0, options)).rejects.toThrow(
+        `${RPC.ETransactionExecutionStatus.REVERTED}: ${RPC.ETransactionStatus.ACCEPTED_ON_L2}`
+      );
+    });
+
+    test('no error state; timed-out', async () => {
+      const options = generateOptions({ errorStates: [] });
+      await expect(rpcProvider.waitForTransaction(0, options)).rejects.toThrow(/timed-out/);
+    });
+  });
+
   describe('RPC methods', () => {
     let latestBlock: GetBlockResponse;
 

src/provider/rpc.ts

@@ -321,7 +321,9 @@ export class RpcProvider implements ProviderInterface {
     const retryInterval = options?.retryInterval ?? 5000;
     const errorStates: any = options?.errorStates ?? [
       RPC.ETransactionStatus.REJECTED,
-      RPC.ETransactionExecutionStatus.REVERTED,
+      // TODO: commented out to preserve the long-standing behavior of "reverted" not being treated as an error by default
+      // should decide which behavior to keep in the future
+      // RPC.ETransactionExecutionStatus.REVERTED,
     ];
     const successStates: any = options?.successStates ?? [
       RPC.ETransactionExecutionStatus.SUCCEEDED,
@@ -347,14 +349,17 @@ export class RpcProvider implements ProviderInterface {
           throw error;
         }
 
-        if (successStates.includes(executionStatus) || successStates.includes(finalityStatus)) {
-          onchain = true;
-        } else if (errorStates.includes(executionStatus) || errorStates.includes(finalityStatus)) {
+        if (errorStates.includes(executionStatus) || errorStates.includes(finalityStatus)) {
           const message = `${executionStatus}: ${finalityStatus}`;
           const error = new Error(message) as Error & { response: RPC.TransactionStatus };
           error.response = txStatus;
           isErrorState = true;
           throw error;
+        } else if (
+          successStates.includes(executionStatus) ||
+          successStates.includes(finalityStatus)
+        ) {
+          onchain = true;
         }
       } catch (error) {
         if (error instanceof Error && isErrorState) {