Feature/readsecrets + brief readme #6

victorfinbourne · 2020-04-03T09:38:53Z

Pull Request Checklist

Read the contributing guidelines
Tests pass
Raised the PR against the develop branch

Description of the PR

Describe the code changes for the reviewers, explain the solution you have provided and how it fixes the issue

MikeMcGarry · 2020-04-03T11:59:30Z

sdk/test/tutorials/clientBuilder.ts

+/*
+  Read from Environment Variables
+*/
+
+// export var client = new Client(
+//   [Source.Environment, 'FBN_TOKEN_URL'],
+//   [Source.Environment, 'FBN_USERNAME'],
+//   [Source.Environment, 'FBN_PASSWORD'],
+//   [Source.Environment, 'FBN_CLIENT_ID'],
+//   [Source.Environment, 'FBN_CLIENT_SECRET'],
+//   [Source.Environment, 'FBN_LUSID_API_URL'],
+// )
+
+/*
+  Read from secrets.json
+*/
+
 export var client = new Client(
-  [Source.Environment, 'FBN_TOKEN_URL'],
-  [Source.Environment, 'FBN_USERNAME'],
-  [Source.Environment, 'FBN_PASSWORD'],
-  [Source.Environment, 'FBN_CLIENT_ID'],
-  [Source.Environment, 'FBN_CLIENT_SECRET'],
-  [Source.Environment, 'FBN_LUSID_API_URL'],
-)
+  [Source.Secrets, 'tokenUrl'],
+  [Source.Secrets, 'username'],
+  [Source.Secrets, 'password'],
+  [Source.Secrets, 'clientId'],
+  [Source.Secrets, 'clientSecret'],
+  [Source.Secrets, 'apiUrl']
+)


As this client runs the tests we need some way to ensure that we can run them locally using a secrets file and in our continuous integration pipeline which will use environment variables.

The logic in the Python SDK is to use a secrets file if provided, falling back to environment variables if it can't find a secret file. Do you think we could implement similar logic here?

https://github.com/finbourne/lusid-sdk-python-preview/tree/master/sdk/lusid/utilities

I would suggest that this lives in the sdk/client/client.ts but then you can delete completely the commented out code which references environment variables.

yep. will do.

sdk/client/client.ts

MikeMcGarry

Hey Victor,

Great work, looking much more loved than before!

Overall we should have a chat about the semantics of Node.JS Typescript vs Python to think about the best way to achieve parity while maintaining the language specific flows.

I've made some comments in-line as well.

MikeMcGarry · 2020-04-07T07:16:42Z

sdk/client/client.ts

  // The path to the secrets file which may be used to store credentials
-  secretsFilePath: string
+  secretsFilePath: string = process.cwd() + '/secrets.json'


Now that we are looking at this in detail, do we think the "secrets.json" path should live in the client class or should the file path be passed into the constructor? This allows any file path to be specified when creating a client.

MikeMcGarry · 2020-04-07T07:17:25Z

sdk/client/client.ts

  // The refresh limit in seconds before token expiry to trigger a refresh
-  refreshLimit: number = refreshLimit
+  refreshLimit: number = 3580


Should we also allow providing a custom refreshLimit with this set as a sensible default?

MikeMcGarry · 2020-04-07T07:18:39Z

sdk/client/client.ts

+    try
+    {
+      this.secretsFileContent = require( this.secretsFilePath );
+    }
+    catch( e )
+    {
+      // not necessarily a problem. even if the format of the source file is bad, this will reset the internal validation to an empty configuration
+
+      this.secretsFileContent = { api: {} };
+    }


What are your thoughts on try/catch handling vs if/then handling? We could check that the file exists and if it does not raise a warning rather than silently defaulting to a default empty set of values.

I'm not sure which is better suited to Typescript and more specifically Node.js

MikeMcGarry · 2020-04-07T07:19:06Z

sdk/client/client.ts

+      {
+        tokenUrlDetails,
+        usernameDetails,
+        passwordDetails,
+        clientIdDetails,
+        clientSecretDetails,
+        apiUrlDetails
+      }: {
+        tokenUrlDetails?: [Source, string],
+        usernameDetails?: [Source, string],
+        passwordDetails?: [Source, string],
+        clientIdDetails?: [Source, string],
+        clientSecretDetails?: [Source, string],
+        apiUrlDetails?: [Source, string],
+      } = {
+        tokenUrlDetails: [Source.Secrets, 'tokenUrl'],
+        usernameDetails: [Source.Secrets, 'username'],
+        passwordDetails: [Source.Secrets, 'password'],
+        clientIdDetails: [Source.Secrets, 'clientId'],
+        clientSecretDetails: [Source.Secrets, 'clientSecret'],
+        apiUrlDetails: [Source.Secrets, 'apiUrl']
+      }


This is interesting! What are the benefits to this over having the types in the constructor as there were previously?

MikeMcGarry · 2020-04-07T07:20:36Z

sdk/client/client.ts

+    this.tokenUrlDetails = !!tokenUrlDetails ? tokenUrlDetails : (
+      // [Source.Environment, 'FBN_TOKEN_URL']
+      [Source.Secrets, 'tokenUrl']
+    );


If the defaults are already specified in the constructor above in what situation would !!tokenUrlDetails resolve to False? In that situation what is the value of providing the same details "[Source.Secrets, 'tokenUrl']" as the default?

MikeMcGarry · 2020-04-07T07:35:18Z

sdk/test/tutorials/createHoldings.test.ts

+
+      Object.keys( res.values ).forEach( ( insertedInstrument ) => {
+
+        mlog.log( `Upserted instrument ${insertedInstrument} -> ${res.values[ insertedInstrument ].lusidInstrumentId}`)


Should we also assert that the response contains the instruments that we expect?

Or is that outside the scope of this test as it is just something we need to run the holding tests? If so, should the functions for creating these live in a utilities folder instead of inside this test file?

MikeMcGarry · 2020-04-07T07:35:29Z

sdk/test/tutorials/createHoldings.test.ts

+
+      this.portfolioObject = res;
+
+      mlog.log( `Created portfolio ${res.displayName}` );


Should we assert that the displayName and other values are as we expect?

Or is that outside the scope of this test as it is just something we need to run the holding tests? If so, should the functions for creating these live in a utilities folder instead of inside this test file?

MikeMcGarry · 2020-04-07T07:35:53Z

sdk/test/tutorials/createHoldings.test.ts

+      portfolioObject: this.portfolioObject,
+      instrumentsObject: this.instrumentsObject,


Nice how you grab these from the testcase using this

MikeMcGarry · 2020-04-07T07:38:23Z

sdk/test/tutorials/createPortfolio.test.ts

+const createTransactionPortfolio = (
+  {
+    scope
+  }: {
+    scope: string,
+  }
+) :Promise<{
+  portfolio:Portfolio;
+  createRequest:CreateTransactionPortfolioRequest}
+> => {
+
+  return new Promise((resolve, reject) => {
+
+    const
+      createRequest = Object.assign(
+        new CreateTransactionPortfolioRequest(),
+        {
+          displayName: "UK Equities",
+          code: "UKEQTY",
+          baseCurrency: "GBP",
+          created: moment([2018, 1, 1, 0, 0, 0]).utc()
+        }
+      );
+
+    client.api.transactionPortfolios.createPortfolio(
+      scope,
+      createRequest
+    )
+    .then((res: {response: IncomingMessage; body: Portfolio}) => {
+      resolve( { portfolio: res.body, createRequest } )
+    })
+    .catch((err: {response: IncomingMessage; body: LusidProblemDetails}) => reject(err))
+
+  } );
+}


Seeing some duplication here between this function and the one in the getholdings tests

MikeMcGarry · 2020-04-07T07:38:40Z

sdk/test/tutorials/createPortfolio.test.ts

+const upsertInstruments = (
+  identifiers: Object = {}
+) :Promise<UpsertInstrumentsResponse> => {
+
+  return new Promise( (resolve, reject) => {
+
+    client.api.instruments.upsertInstruments(
+
+      Object.keys( identifiers ).reduce( ( accumulator, currentValue ) => {
+
+        accumulator[ currentValue ] = Object.assign(
+          new InstrumentDefinition(),
+          {
+            name: identifiers[ currentValue ].Name,
+            identifiers: {
+              Figi: Object.assign(
+                new InstrumentIdValue(),
+                {
+                  value: currentValue
+                }
+              )
+            }
+          }
+        );
+
+        return accumulator;
+
+      }, {} )
+    )
+    .then((res: {response: IncomingMessage; body: UpsertInstrumentsResponse}) => {
+      resolve( res.body )
+    })
+    .catch((err: {response: IncomingMessage; body: LusidProblemDetails}) => reject(err));
+
+  } );
+
+}


Duplication of the function between this file and getholdings?

MikeMcGarry · 2020-04-08T08:47:27Z

sdk/client/client.ts

-  public clientId: string
-  public clientSecret: string
+// polyfill for Object.fromEntries until Typescript has ES2019 included
+const _ObjectFromEntries = ( iter ): Object => {


I understand that this is used to convert an array of key, value pairs into an object.

Some questions that someone reading this function might have and could be worth answering with a JSDoc compliant docstring:

What is the input exactly for this function?

What happens with nested objects?

What are you checking with the equality check if (Object(pair) !== pair)?

MikeMcGarry · 2020-04-08T08:47:36Z

sdk/client/client.ts

+    // Consistency with Map: contract is that entry has "0" and "1" keys, not
+    // that it is an array or iterable.
+
+    const { '0': key, '1': val } = pair;


What is the significance of this line?

MikeMcGarry · 2020-04-08T08:47:48Z

sdk/client/client.ts

+      configurable: true,
+      enumerable: true,
+      writable: true,


What would happen if you didn't set these to true?

MikeMcGarry · 2020-04-08T08:49:52Z

sdk/client/client.ts

+
+  secretsFileContent: Object
+
+  private configurationMapping: Array<Array<number|Array<string>>> = [


When would the nested array be an array of numbers? Is that in the case of the Enum? If so why not Array<Array<Source|Array<string>>>

Also is this saying that the nested array will either be a flat Array of numbers or another nested array which contains strings? Or is it saying that it will be a flat array of either numbers or strings.

MikeMcGarry · 2020-04-08T08:55:26Z

sdk/client/client.ts

+
+    try
+    {
+      return this.secretsFileContent = require( !!secretsFilePath ? secretsFilePath : this.configuration.secretsFilePath ).api;


Is falling back to this.configuration.secretsFilePath redundant as you do this on lines 221 to 223? Or do you expect this to be used in another context inside this class where you aren't confident that secretsFilePath will have already been populated with a sensible default?

MikeMcGarry · 2020-04-08T08:57:46Z

sdk/client/client.ts

+        if(
+          typeof this.api[apiName][prop] !== 'function'
+          || ['setDefaultAuthentication', 'setApiKey'].includes( prop )
+        ) {


Nice, much neater :)

sdk/client/client.ts

MikeMcGarry · 2020-04-08T09:04:16Z

sdk/client/client.ts

+    this.configuration.tokenUrlDetails = !!tokenUrlDetails
+      ? this.fetchConfigurationItem( Source.Raw, tokenUrlDetails, true )
+      : this.fetchConfigurationItem( Source.Secrets, 'tokenUrl', false );


In this instance, if I provide in tokenUrlDetails when creating my client 'tokenUrl' then won't this actually use the value of 'tokenUrl' as my tokenUrl?

Victor Popescu added 4 commits April 3, 2020 10:28

quick fix

e2bd89c

added comments

c71e92f

added absolute path

033358e

added some usage docs

c9f9732

MikeMcGarry requested changes Apr 3, 2020

View reviewed changes

Victor Popescu and others added 20 commits April 3, 2020 14:08

cascading secrets

e6c7ac4

optional client constructor parameters

8e895b8

better error reporting

3220f70

removed superflous commentary

923bc00

added portfolio delete test + holdings test inception

c37c1d8

ES6 upgrades + nicer logging

158f1c8

holdings interim

ba45d72

holdings interim

dc50d15

holdings assertions alpha

0e890ba

removal of unused reference

970cc0f

lusidTools inception + defineBuyRequest

6144dd0

getHoldings beta

7ff33c9

adjust holdings beta

6656321

delete back in test

bc466bd

portfolios beta

84c4f30

better title

11b624a

made error reporting broader

ade1a80

broader error reporting

d25862c

removed unused libraries

7ccd1f3

defaults update

90ab2fc

MikeMcGarry requested changes Apr 7, 2020

View reviewed changes

Victor Popescu added 4 commits April 7, 2020 16:20

fallback beta

1986cd1

configuration

581abff

removed comments

de7e7b7

+comments

5a144d8

Victor Popescu added 6 commits April 7, 2020 16:31

explicit fallbacks

63a7fb0

oauth2

2e7a030

lusidUtilities

d31c5ab

elegant update

b310b89

fix docs

38b4024

one transaction test

c44409f

MikeMcGarry requested changes Apr 8, 2020

View reviewed changes

Victor Popescu added 7 commits April 8, 2020 13:01

improvements

abefcf1

createTransactions beta

d872778

bitemporal alpha

bbb0e73

bitemporal with JS patch

a226e38

cut labels alpha

3ef9303

cut labels alpha

c71e870

cut labels alpha

7da1325

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/readsecrets + brief readme #6

Feature/readsecrets + brief readme #6

victorfinbourne commented Apr 3, 2020

MikeMcGarry Apr 3, 2020

victorfinbourne Apr 3, 2020

MikeMcGarry left a comment

MikeMcGarry Apr 7, 2020

MikeMcGarry Apr 7, 2020

MikeMcGarry Apr 7, 2020

MikeMcGarry Apr 7, 2020

MikeMcGarry Apr 7, 2020

MikeMcGarry Apr 7, 2020

MikeMcGarry Apr 7, 2020

MikeMcGarry Apr 7, 2020

MikeMcGarry Apr 7, 2020

MikeMcGarry Apr 7, 2020

MikeMcGarry Apr 7, 2020

MikeMcGarry Apr 7, 2020

MikeMcGarry Apr 8, 2020

MikeMcGarry Apr 8, 2020

MikeMcGarry Apr 8, 2020

MikeMcGarry Apr 8, 2020

MikeMcGarry Apr 8, 2020

MikeMcGarry Apr 8, 2020

MikeMcGarry Apr 8, 2020


		Object.keys( res.values ).forEach( ( insertedInstrument ) => {

		mlog.log( `Upserted instrument ${insertedInstrument} -> ${res.values[ insertedInstrument ].lusidInstrumentId}`)


		this.portfolioObject = res;

		mlog.log( `Created portfolio ${res.displayName}` );

		portfolioObject: this.portfolioObject,
		instrumentsObject: this.instrumentsObject,


		secretsFileContent: Object

		private configurationMapping: Array<Array<number\|Array<string>>> = [

Feature/readsecrets + brief readme #6

Are you sure you want to change the base?

Feature/readsecrets + brief readme #6

Conversation

victorfinbourne commented Apr 3, 2020

Pull Request Checklist

Description of the PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcGarry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment