Addressing PR comments

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>

Author: matthew1001

README.md

@@ -122,28 +122,6 @@ The following APIs are not part of the fftokens standard, but are exposed under
 
 - `GET /receipt/:id` - Get receipt for a previous request
 
-## Retry behaviour
-
-Most short-term outages should be handled by the blockchain connector. For example if the blockchain node returns `HTTP 429` due to rate limiting
-it is the blockchain connector's responsibility to use appropriate back-off retries to attempt to make the required blockchain call successfully.
-
-There are cases where the token connector may need to perform it's own back-off retry for a blockchain action. For example if the blockchain connector
-microservice has crashed and is in the process of restarting just as the token connector is trying to query an NFT token URI to enrich a token event, if
-the token connector doesn't perform a retry then the event will be returned without the token URI populated.
-
-The token connector has configurable retry behaviour for all blockchain related calls. By default the connector will perform up to 15 retries with a back-off
-interval between each one. The default first retry interval is 100ms and doubles up to a maximum of 10s per retry interval. Retries are only performed where
-the error returned from the REST call matches a configurable regular expression retry condition. The default retry condition is `._ECONN._` which ensures
-retries take place for common TCP errors such as `ECONNRESET` and `ECONNREFUSED`.
-
-Setting the retry condition to "" disables retries. The configurable retry settings are:
-
-- `RETRY_BACKOFF_FACTOR` (default `2`)
-- `RETRY_BACKOFF_LIMIT_MS` (default `10000`)
-- `RETRY_BACKOFF_INITIAL_MS` (default `100`)
-- `RETRY_CONDITION` (default `.*ECONN.*`)
-- `RETRY_MAX_ATTEMPTS` (default `15`)
-
 ## Running the service
 
 The easiest way to run this service is as part of a stack created via
@@ -202,3 +180,27 @@ $ npm run lint
 # formatting
 $ npm run format
 ```
+
+## Blockchain retry behaviour
+
+Most short-term outages should be handled by the blockchain connector. For example if the blockchain node returns `HTTP 429` due to rate limiting
+it is the blockchain connector's responsibility to use appropriate back-off retries to attempt to make the required blockchain call successfully.
+
+There are cases where the token connector may need to perform its own back-off retry for a blockchain action. For example if the blockchain connector
+microservice has crashed and is in the process of restarting just as the token connector is trying to query an NFT token URI to enrich a token event, if
+the token connector doesn't perform a retry then the event will be returned without the token URI populated.
+
+The token connector has configurable retry behaviour for all blockchain related calls. By default the connector will perform up to 15 retries with a back-off
+interval between each one. The default first retry interval is 100ms and doubles up to a maximum of 10s per retry interval. Retries are only performed where
+the error returned from the REST call matches a configurable regular expression retry condition. The default retry condition is `.*ECONN.*` which ensures
+retries take place for common TCP errors such as `ECONNRESET` and `ECONNREFUSED`.
+
+The configurable retry settings are:
+
+- `RETRY_BACKOFF_FACTOR` (default `2`)
+- `RETRY_BACKOFF_LIMIT_MS` (default `10000`)
+- `RETRY_BACKOFF_INITIAL_MS` (default `100`)
+- `RETRY_CONDITION` (default `.*ECONN.*`)
+- `RETRY_MAX_ATTEMPTS` (default `15`)
+
+Setting `RETRY_CONDITION` to `""` disables retries. Setting `RETRY_MAX_ATTEMPTS` to `-1` causes it to retry indefinitely.

src/main.ts

@@ -25,7 +25,7 @@ import { EventStreamReply } from './event-stream/event-stream.interfaces';
 import { EventStreamService } from './event-stream/event-stream.service';
 import { requestIDMiddleware } from './request-context/request-id.middleware';
 import { RequestLoggingInterceptor } from './request-logging.interceptor';
-import { BlockchainConnectorService } from './tokens/blockchain.service';
+import { BlockchainConnectorService, RetryConfiguration } from './tokens/blockchain.service';
 import {
   TokenApprovalEvent,
   TokenBurnEvent,
@@ -84,12 +84,14 @@ async function bootstrap() {
   const legacyERC20 = config.get<string>('USE_LEGACY_ERC20_SAMPLE', '').toLowerCase() === 'true';
   const legacyERC721 = config.get<string>('USE_LEGACY_ERC721_SAMPLE', '').toLowerCase() === 'true';
 
-  // Configuration for retries
-  const retryBackOffFactor = config.get<number>('RETRY_BACKOFF_FACTOR', 2);
-  const retryBackOffLimit = config.get<number>('RETRY_BACKOFF_LIMIT_MS', 10000);
-  const retryBackOffInitial = config.get<number>('RETRY_BACKOFF_INITIAL_MS', 100);
-  const retryCondition = config.get<string>('RETRY_CONDITION', '.*ECONN.*');
-  const retriesMax = config.get<number>('RETRY_MAX_ATTEMPTS', 15);
+  // Configuration for blockchain call retries
+  const blockchainRetryCfg: RetryConfiguration = {
+    retryBackOffFactor: config.get<number>('RETRY_BACKOFF_FACTOR', 2),
+    retryBackOffLimit: config.get<number>('RETRY_BACKOFF_LIMIT_MS', 10000),
+    retryBackOffInitial: config.get<number>('RETRY_BACKOFF_INITIAL_MS', 100),
+    retryCondition: config.get<string>('RETRY_CONDITION', '.*ECONN.*'),
+    retriesMax: config.get<number>('RETRY_MAX_ATTEMPTS', 15),
+  };
 
   const passthroughHeaders: string[] = [];
   for (const h of passthroughHeaderString.split(',')) {
@@ -100,18 +102,7 @@ async function bootstrap() {
   app.get(TokensService).configure(ethConnectUrl, topic, factoryAddress);
   app
     .get(BlockchainConnectorService)
-    .configure(
-      ethConnectUrl,
-      fftmUrl,
-      username,
-      password,
-      passthroughHeaders,
-      retryBackOffFactor,
-      retryBackOffLimit,
-      retryBackOffInitial,
-      retryCondition,
-      retriesMax,
-    );
+    .configure(ethConnectUrl, fftmUrl, username, password, passthroughHeaders, blockchainRetryCfg);
   app.get(AbiMapperService).configure(legacyERC20, legacyERC721);
 
   if (autoInit.toLowerCase() !== 'false') {

src/tokens/blockchain.service.ts

@@ -30,6 +30,14 @@ import { Context } from '../request-context/request-context.decorator';
 import { FFRequestIDHeader } from '../request-context/constants';
 import { EthConnectAsyncResponse, EthConnectReturn, IAbiMethod } from './tokens.interfaces';
 
+export interface RetryConfiguration {
+  retryBackOffFactor: number;
+  retryBackOffLimit: number;
+  retryBackOffInitial: number;
+  retryCondition: string;
+  retriesMax: number;
+}
+
 const sendTransactionHeader = 'SendTransaction';
 const queryHeader = 'Query';
 
@@ -43,11 +51,7 @@ export class BlockchainConnectorService {
   password: string;
   passthroughHeaders: string[];
 
-  retryBackOffFactor: number;
-  retryBackOffLimit: number;
-  retryBackOffInitial: number;
-  retryCondition: string;
-  retriesMax: number;
+  retryConfiguration: RetryConfiguration;
 
   constructor(public http: HttpService) {}
 
@@ -57,22 +61,14 @@ export class BlockchainConnectorService {
     username: string,
     password: string,
     passthroughHeaders: string[],
-    retryBackOffFactor: number,
-    retryBackOffLimit: number,
-    retryBackOffInitial: number,
-    retryCondition: string,
-    retriesMax: number,
+    retryConfiguration: RetryConfiguration,
   ) {
     this.baseUrl = baseUrl;
     this.fftmUrl = fftmUrl;
     this.username = username;
     this.password = password;
     this.passthroughHeaders = passthroughHeaders;
-    this.retryBackOffFactor = retryBackOffFactor;
-    this.retryBackOffLimit = retryBackOffLimit;
-    this.retryBackOffInitial = retryBackOffInitial;
-    this.retryCondition = retryCondition;
-    this.retriesMax = retriesMax;
+    this.retryConfiguration = retryConfiguration;
   }
 
   private requestOptions(ctx: Context): AxiosRequestConfig {
@@ -106,58 +102,65 @@ export class BlockchainConnectorService {
 
   // Check if retry condition matches the err that's been hit
   private matchesRetryCondition(err: any): boolean {
-    return this.retryCondition != '' && err?.toString().match(this.retryCondition) !== null;
+    return (
+      this.retryConfiguration.retryCondition != '' &&
+      err?.toString().match(this.retryConfiguration.retryCondition) !== null
+    );
   }
 
   // Delay by the appropriate amount of time given the iteration the caller is in
   private async backoffDelay(iteration: number) {
     const delay = Math.min(
-      this.retryBackOffInitial * Math.pow(this.retryBackOffFactor, iteration),
-      this.retryBackOffLimit,
+      this.retryConfiguration.retryBackOffInitial *
+        Math.pow(this.retryConfiguration.retryBackOffFactor, iteration),
+      this.retryConfiguration.retryBackOffLimit,
     );
     await new Promise(resolve => setTimeout(resolve, delay));
   }
 
-  // Generic helper function that makes an given blockchain function retryable
-  // by using synchronous, back-off delays for cases where the function returns
+  // Generic helper function that makes a given blockchain function retryable
+  // by using synchronous back-off delays for cases where the function returns
   // an error which matches the configured retry condition
   private async retryableCall<T = any>(
     blockchainFunction: () => Promise<AxiosResponse<T>>,
   ): Promise<AxiosResponse<T>> {
-    let response: any;
-    for (let retries = 0; retries <= this.retriesMax; retries++) {
+    let retries = 0;
+    for (
+      ;
+      this.retryConfiguration.retriesMax == -1 || retries <= this.retryConfiguration.retriesMax;
+      this.retryConfiguration.retriesMax == -1 || retries++ // Don't inc 'retries' if 'retriesMax' if set to -1 (infinite retries)
+    ) {
       try {
-        response = await blockchainFunction();
-        break;
+        return await blockchainFunction();
       } catch (e) {
         if (this.matchesRetryCondition(e)) {
           this.logger.debug(`Retry condition matched for error ${e}`);
           // Wait for a backed-off delay before trying again
           await this.backoffDelay(retries);
         } else {
           // Whatever the error was it's not one we will retry for
-          break;
+          throw e;
         }
       }
     }
 
-    return response;
+    throw new InternalServerErrorException(
+      `Call to blockchain connector failed after ${retries} attempts`,
+    );
   }
 
   async query(ctx: Context, to: string, method?: IAbiMethod, params?: any[]) {
     const url = this.baseUrl;
-    const response = await this.retryableCall<EthConnectReturn>(
-      async (): Promise<AxiosResponse<EthConnectReturn>> => {
-        return await this.wrapError(
-          lastValueFrom(
-            this.http.post(
-              url,
-              { headers: { type: queryHeader }, to, method, params },
-              this.requestOptions(ctx),
-            ),
+    const response = await this.wrapError(
+      this.retryableCall<EthConnectReturn>(async (): Promise<AxiosResponse<EthConnectReturn>> => {
+        return await lastValueFrom(
+          this.http.post(
+            url,
+            { headers: { type: queryHeader }, to, method, params },
+            this.requestOptions(ctx),
           ),
         );
-      },
+      }),
     );
     return response.data;
   }
@@ -172,35 +175,33 @@ export class BlockchainConnectorService {
   ) {
     const url = this.fftmUrl !== undefined && this.fftmUrl !== '' ? this.fftmUrl : this.baseUrl;
 
-    const response = await this.retryableCall<EthConnectAsyncResponse>(
-      async (): Promise<AxiosResponse<EthConnectAsyncResponse>> => {
-        return await this.wrapError(
-          lastValueFrom(
+    const response = await this.wrapError(
+      this.retryableCall<EthConnectAsyncResponse>(
+        async (): Promise<AxiosResponse<EthConnectAsyncResponse>> => {
+          return await lastValueFrom(
             this.http.post(
               url,
               { headers: { id, type: sendTransactionHeader }, from, to, method, params },
               this.requestOptions(ctx),
             ),
-          ),
-        );
-      },
+          );
+        },
+      ),
     );
     return response.data;
   }
 
   async getReceipt(ctx: Context, id: string): Promise<EventStreamReply> {
     const url = this.baseUrl;
-    const response = await this.retryableCall<EventStreamReply>(
-      async (): Promise<AxiosResponse<EventStreamReply>> => {
-        return await this.wrapError(
-          lastValueFrom(
-            this.http.get(new URL(`/reply/${id}`, url).href, {
-              validateStatus: status => status < 300 || status === 404,
-              ...this.requestOptions(ctx),
-            }),
-          ),
+    const response = await this.wrapError(
+      this.retryableCall<EventStreamReply>(async (): Promise<AxiosResponse<EventStreamReply>> => {
+        return await lastValueFrom(
+          this.http.get(new URL(`/reply/${id}`, url).href, {
+            validateStatus: status => status < 300 || status === 404,
+            ...this.requestOptions(ctx),
+          }),
         );
-      },
+      }),
     );
     if (response.status === 404) {
       throw new NotFoundException();

src/tokens/tokens.service.spec.ts

@@ -33,7 +33,7 @@ import {
 import { EventStreamService } from '../event-stream/event-stream.service';
 import { EventStreamProxyGateway } from '../eventstream-proxy/eventstream-proxy.gateway';
 import { AbiMapperService } from './abimapper.service';
-import { BlockchainConnectorService } from './blockchain.service';
+import { BlockchainConnectorService, RetryConfiguration } from './blockchain.service';
 import {
   AsyncResponse,
   EthConnectAsyncResponse,
@@ -232,10 +232,18 @@ describe('TokensService', () => {
       .useValue(eventstream)
       .compile();
 
+    let blockchainRetryCfg: RetryConfiguration = {
+      retryBackOffFactor: 2,
+      retryBackOffLimit: 10000,
+      retryBackOffInitial: 100,
+      retryCondition: '.*ECONN.*',
+      retriesMax: 15,
+    };
+
     service = module.get(TokensService);
     service.configure(BASE_URL, TOPIC, '');
     blockchain = module.get(BlockchainConnectorService);
-    blockchain.configure(BASE_URL, '', '', '', [], 2, 1000, 250, '.*ECONN.*', 15);
+    blockchain.configure(BASE_URL, '', '', '', [], blockchainRetryCfg);
   });
 
   it('should be defined', () => {