Minor API-related readability cleanups (#1333)

* Minor API-related readability cleanups

* Oops, remove trial protected, revert to private

* Unneeded as Observable<...> casts

* Split state retrieval from signAndSend

* Typo in comment

* Cleanup descriptions

Author: jacogr

packages/api/src/SignerPayload.ts

@@ -37,18 +37,11 @@ const _Payload: Constructor<SignerPayloadType> = Struct.with({
 });
 
 export default class Payload extends _Payload {
-  /**
-   * @description Returns this as a SignerPayloadType. This works since the Struct.with injects all the getters automatically (just ensure the 2 definitiona are matching)
-   */
-  public get self (): SignerPayloadType {
-    return this as any as SignerPayloadType;
-  }
-
   /**
    * @description Creates an representation of the structure as an ISignerPayload JSON
    */
   public toPayload (): SignerPayload {
-    const { address, blockHash, blockNumber, era, genesisHash, method, nonce, runtimeVersion: { specVersion }, tip, version } = this.self;
+    const { address, blockHash, blockNumber, era, genesisHash, method, nonce, runtimeVersion: { specVersion }, tip, version } = this;
 
     return {
       address: address.toString(),

packages/api/src/SubmittableExtrinsic.ts

@@ -2,7 +2,7 @@
 // This software may be modified and distributed under the terms
 // of the Apache-2.0 license. See the LICENSE file for details.
 
-import { AccountId, Address, Call, ExtrinsicEra, ExtrinsicStatus, EventRecord, Hash, Header, Index, SignedBlock } from '@polkadot/types/interfaces';
+import { AccountId, Address, Call, ExtrinsicEra, ExtrinsicStatus, EventRecord, Hash, Header, Index } from '@polkadot/types/interfaces';
 import { AnyNumber, AnyU8a, Callback, Codec, IExtrinsic, IExtrinsicEra, IKeyringPair, SignatureOptions } from '@polkadot/types/types';
 import { ApiInterfaceRx, ApiTypes } from './types';
 
@@ -110,8 +110,7 @@ export default function createSubmittableExtrinsic<ApiType> (
   type: ApiTypes,
   api: ApiInterfaceRx,
   decorateMethod: ApiBase<ApiType>['decorateMethod'],
-  extrinsic: Call | Uint8Array | string,
-  trackingCb?: Callback<ISubmittableResult>
+  extrinsic: Call | Uint8Array | string
 ): SubmittableExtrinsic<ApiType> {
   const _extrinsic = createType('Extrinsic', extrinsic, { version: api.extrinsicType }) as unknown as SubmittableExtrinsic<ApiType>;
   const _noStatusCb = type === 'rxjs';
@@ -124,35 +123,27 @@ export default function createSubmittableExtrinsic<ApiType> (
 
   function statusObservable (status: ExtrinsicStatus): Observable<ISubmittableResult> {
     if (!status.isFinalized) {
-      const result = new SubmittableResult({ status });
-
-      trackingCb && trackingCb(result);
-
-      return of(result);
+      return of(new SubmittableResult({ status }));
     }
 
     const blockHash = status.asFinalized;
 
     return combineLatest([
-      api.rpc.chain.getBlock(blockHash) as Observable<SignedBlock>,
+      api.rpc.chain.getBlock(blockHash),
       api.query.system.events.at(blockHash) as Observable<Vec<EventRecord>>
     ]).pipe(
-      map(([signedBlock, allEvents]): SubmittableResult => {
-        const result = new SubmittableResult({
+      map(([signedBlock, allEvents]): SubmittableResult =>
+        new SubmittableResult({
           events: filterEvents(_extrinsic.hash, signedBlock, allEvents),
           status
-        });
-
-        trackingCb && trackingCb(result);
-
-        return result;
-      })
+        })
+      )
     );
   }
 
   function sendObservable (updateId: number = -1): Observable<Hash> {
-    return (api.rpc.author
-      .submitExtrinsic(_extrinsic) as Observable<Hash>)
+    return api.rpc.author
+      .submitExtrinsic(_extrinsic)
       .pipe(
         tap((hash): void => {
           updateSigner(updateId, hash);
@@ -161,8 +152,8 @@ export default function createSubmittableExtrinsic<ApiType> (
   }
 
   function subscribeObservable (updateId: number = -1): Observable<ISubmittableResult> {
-    return (api.rpc.author
-      .submitAndWatchExtrinsic(_extrinsic) as Observable<ExtrinsicStatus>)
+    return api.rpc.author
+      .submitAndWatchExtrinsic(_extrinsic)
       .pipe(
         switchMap((status): Observable<ISubmittableResult> =>
           statusObservable(status)
@@ -206,6 +197,20 @@ export default function createSubmittableExtrinsic<ApiType> (
     });
   }
 
+  function getPrelimState (address: string, options: Partial<SignerOptions>): Observable<[Index, Header | null]> {
+    return combineLatest([
+      // if we have a nonce already, don't retrieve the latest, use what is there
+      isUndefined(options.nonce)
+        ? api.query.system.accountNonce<Index>(address)
+        : of(createType('Index', options.nonce)),
+      // if we have an era provided already or eraLength is <= 0 (immortal)
+      // don't get the latest block, just pass null, handle in mergeMap
+      (isUndefined(options.era) || (isNumber(options.era) && options.era > 0))
+        ? api.rpc.chain.getHeader()
+        : of(null)
+    ]);
+  }
+
   const signOrigin = _extrinsic.sign;
 
   Object.defineProperties(
@@ -246,71 +251,60 @@ export default function createSubmittableExtrinsic<ApiType> (
           let updateId: number | undefined;
 
           return decorateMethod(
-            (): Observable<Codec> => ((
-              combineLatest([
-                // if we have a nonce already, don't retrieve the latest, use what is there
-                isUndefined(options.nonce)
-                  ? api.query.system.accountNonce<Index>(address)
-                  : of(createType('Index', options.nonce)),
-                // if we have an era provided already or eraLength is <= 0 (immortal)
-                // don't get the latest block, just pass null, handle in mergeMap
-                (isUndefined(options.era) || (isNumber(options.era) && options.era > 0))
-                  ? api.rpc.chain.getHeader() as Observable<Header>
-                  : of(null)
-              ])
-            ).pipe(
-              first(),
-              mergeMap(async ([nonce, header]): Promise<void> => {
-                const eraOptions = expandEraOptions(options, { header, nonce });
-
-                // FIXME This is becoming real messy with all the options - way past
-                // "a method should fit on a single screen" stage. (Probably want to
-                // clean this when we remove `api.signer.sign` in the next beta cycle)
-                if (isKeyringPair(account)) {
-                  this.sign(account, eraOptions);
-                } else if (api.signer) {
-                  const payload = new SignerPayload({
-                    ...eraOptions,
-                    address,
-                    method: _extrinsic.method,
-                    blockNumber: header ? header.number : 0
-                  });
-
-                  if (api.signer.signPayload) {
-                    const { id, signature } = await api.signer.signPayload(payload.toPayload());
-
-                    // Here we explicitly call `toPayload()` again instead of working with an object
-                    // (reference) as passed to the signer. This means that we are sure that the
-                    // payload data is not modified from our inputs, but the signer
-                    _extrinsic.addSignature(address, signature, payload.toPayload());
-                    updateId = id;
-                  } else if (api.signer.signRaw) {
-                    const { id, signature } = await api.signer.signRaw(payload.toRaw());
-
-                    // as above, always trust our payload as the signle sourec of truth
-                    _extrinsic.addSignature(address, signature, payload.toPayload());
-                    updateId = id;
-                  } else if (api.signer.sign) {
-                    console.warn('The Signer.sign interface is deprecated and will be removed in a future version, Swap to using the Signer.signPayload interface instead.');
-
-                    updateId = await api.signer.sign(_extrinsic, address, {
+            (): Observable<Codec> => (
+              getPrelimState(address, options).pipe(
+                first(),
+                mergeMap(async ([nonce, header]): Promise<void> => {
+                  const eraOptions = expandEraOptions(options, { header, nonce });
+
+                  // FIXME This is becoming real messy with all the options - way past
+                  // "a method should fit on a single screen" stage. (Probably want to
+                  // clean this when we remove `api.signer.sign` in the next beta cycle)
+                  if (isKeyringPair(account)) {
+                    this.sign(account, eraOptions);
+                  } else if (api.signer) {
+                    const payload = new SignerPayload({
                       ...eraOptions,
-                      blockNumber: header ? header.number.toBn() : new BN(0),
-                      genesisHash: api.genesisHash
+                      address,
+                      method: _extrinsic.method,
+                      blockNumber: header ? header.number : 0
                     });
+
+                    if (api.signer.signPayload) {
+                      const { id, signature } = await api.signer.signPayload(payload.toPayload());
+
+                      // Here we explicitly call `toPayload()` again instead of working with an object
+                      // (reference) as passed to the signer. This means that we are sure that the
+                      // payload data is not modified from our inputs, but the signer
+                      _extrinsic.addSignature(address, signature, payload.toPayload());
+                      updateId = id;
+                    } else if (api.signer.signRaw) {
+                      const { id, signature } = await api.signer.signRaw(payload.toRaw());
+
+                      // as above, always trust our payload as the signle sourec of truth
+                      _extrinsic.addSignature(address, signature, payload.toPayload());
+                      updateId = id;
+                    } else if (api.signer.sign) {
+                      console.warn('The Signer.sign interface is deprecated and will be removed in a future version, Swap to using the Signer.signPayload interface instead.');
+
+                      updateId = await api.signer.sign(_extrinsic, address, {
+                        ...eraOptions,
+                        blockNumber: header ? header.number.toBn() : new BN(0),
+                        genesisHash: api.genesisHash
+                      });
+                    } else {
+                      throw new Error('Invalid signer interface');
+                    }
                   } else {
-                    throw new Error('Invalid signer interface');
+                    throw new Error('no signer exists');
                   }
-                } else {
-                  throw new Error('no signer exists');
-                }
-              }),
-              switchMap((): Observable<ISubmittableResult> | Observable<Hash> => {
-                return isSubscription
-                  ? subscribeObservable(updateId)
-                  : sendObservable(updateId);
-              })
-            ) as Observable<Codec>)
+                }),
+                switchMap((): Observable<ISubmittableResult> | Observable<Hash> => {
+                  return isSubscription
+                    ? subscribeObservable(updateId)
+                    : sendObservable(updateId);
+                })
+              ) as Observable<Codec>) // FIXME This is wrong, SubmittableResult is _not_ a codec
           )(statusCb);
         }
       }

packages/api/src/base/Decorate.ts

@@ -218,7 +218,7 @@ export default abstract class Decorate<ApiType> extends Events {
     decorated.key = (arg1?: Arg, arg2?: Arg): string =>
       u8aToHex(compactStripLength(creator(creator.meta.type.isDoubleMap ? [arg1, arg2] : arg1))[1]);
 
-    // When using double map storage function, user need to path double map key as an array
+    // When using double map storage function, user need to pass double map key as an array
     decorated.multi = decorateMethod((args: (Arg | Arg[])[]): Observable<Codec[]> =>
       this._rpcCore.state.subscribeStorage(
         args.map((arg: Arg[] | Arg): [StorageEntry, Arg | Arg[]] => [creator, arg])));

packages/api/src/promise/Api.ts

@@ -12,13 +12,57 @@ import { isFunction, assert } from '@polkadot/util';
 import ApiBase from '../base';
 import Combinator, { CombinatorCallback, CombinatorFunction } from './Combinator';
 
+interface Tracker {
+  reject: (value: Error) => Observable<never>;
+  resolve: (value: () => void) => void;
+}
+
+// extract the arguments and callback params from a value array possibly containing a callback
+function extractArgs (args: any[], needsCallback: boolean): [any[], Callback<Codec> | undefined] {
+  let callback: Callback<Codec> | undefined;
+  const actualArgs = args.slice();
+
+  // If the last arg is a function, we pop it, put it into callback.
+  // actualArgs will then hold the actual arguments to be passed to `method`
+  if (args.length && isFunction(args[args.length - 1])) {
+    callback = actualArgs.pop();
+  }
+
+  // When we need a subscription, ensure that a valid callback is actually passed
+  assert(!needsCallback || isFunction(callback), 'Expected a callback to be passed with subscriptions');
+
+  return [actualArgs, callback];
+}
+
+// a Promise completion tracker, wrapping an isComplete variable that ensures the promise only resolves once
+function promiseTracker (resolve: (value: () => void) => void, reject: (value: Error) => void): Tracker {
+  let isCompleted = false;
+  const complete = (fn: Function, value: any): void => {
+    if (!isCompleted) {
+      isCompleted = true;
+
+      fn(value);
+    }
+  };
+
+  return {
+    reject: (error: Error): Observable<never> => {
+      complete(reject, error);
+
+      return EMPTY;
+    },
+    resolve: (value: any): void => {
+      complete(resolve, value);
+    }
+  };
+}
+
 /**
  * # @polkadot/api/promise
  *
  * ## Overview
  *
  * @name ApiPromise
- *
  * @description
  * ApiPromise is a standard JavaScript wrapper around the RPC and interfaces on the Polkadot network. As a full Promise-based, all interface calls return Promises, including the static `.create(...)`. Subscription calls utilise `(value) => {}` callbacks to pass through the latest values.
  *
@@ -102,10 +146,8 @@ export default class ApiPromise extends ApiBase<'promise'> {
 
   /**
    * @description Creates an ApiPromise instance using the supplied provider. Returns an Promise containing the actual Api instance.
-   *
    * @param options options that is passed to the class contructor. Can be either [[ApiOptions]] or a
    * provider (see the constructor arguments)
-   *
    * @example
    * <BR>
    *
@@ -125,10 +167,8 @@ export default class ApiPromise extends ApiBase<'promise'> {
 
   /**
    * @description Creates an instance of the ApiPromise class
-   *
    * @param options Options to create an instance. This can be either [[ApiOptions]] or
    * an [[WsProvider]].
-   *
    * @example
    * <BR>
    *
@@ -197,49 +237,31 @@ export default class ApiPromise extends ApiBase<'promise'> {
     };
   }
 
+  /**
+   * @description Decorate method for ApiPromise, where the results are converted to the Promise equivalent
+   */
   protected decorateMethod<Method extends AnyFunction> (method: Method, options?: DecorateMethodOptions): StorageEntryPromiseOverloads {
     const needsCallback = options && options.methodName && options.methodName.includes('subscribe');
 
     return function (...args: any[]): Promise<ObsInnerType<ReturnType<Method>>> | UnsubscribePromise {
-      let callback: Callback<Codec> | undefined;
-      const actualArgs = args.slice();
-
-      // If the last arg is a function, we pop it, put it into callback.
-      // actualArgs will then hold the actual arguments to be passed to `method`
-      if (args.length && isFunction(args[args.length - 1])) {
-        callback = actualArgs.pop();
-      }
-
-      // When we need a subscription, ensure that a valid callback is actually passed
-      assert(!needsCallback || isFunction(callback), 'Expected a callback to be passed with subscriptions');
+      const [actualArgs, callback] = extractArgs(args, !!needsCallback);
 
       if (!callback) {
         return method(...actualArgs).pipe(first()).toPromise() as Promise<ObsInnerType<ReturnType<Method>>>;
       }
 
       return new Promise((resolve, reject): void => {
-        let isCompleted = false;
+        const tracker = promiseTracker(resolve, reject);
         const subscription = method(...actualArgs)
           .pipe(
             // if we find an error (invalid params, etc), reject the promise
-            catchError((error): Observable<never> => {
-              if (!isCompleted) {
-                isCompleted = true;
-
-                reject(error);
-              }
-
-              // we don't want to continue, so empty observable it is
-              return EMPTY;
-            }),
+            catchError((error): Observable<never> =>
+              tracker.reject(error)
+            ),
             // upon the first result, resolve the with the unsub function
-            tap((): void => {
-              if (!isCompleted) {
-                isCompleted = true;
-
-                resolve((): void => subscription.unsubscribe());
-              }
-            })
+            tap((): void =>
+              tracker.resolve((): void => subscription.unsubscribe())
+            )
           )
           .subscribe(callback);
       }) as UnsubscribePromise;