Init start support gatewayconfig #231

gulshanvasnani · 2019-10-17T08:57:06Z

Do not merge before #225
PR contains following changes :

Introduce gatewayconfig option in facilitator-init
Introduce gatewayconfig option in facilitator-start
Readme for facilitator-init and faciltiator-start command

fixes #212

…gatewayconfig

…t_gatewayconfig

…ases for getConfig method

… not allowed

0xsarvesh

Good work 👍

Some feedback and comments.

0xsarvesh · 2019-10-22T07:01:04Z

src/Config/Config.ts

    const facilitator: FacilitatorConfig = FacilitatorConfig.fromFile(facilitatorConfigPath);
+    const gatewayAddresses = mosaicConfigPath


Both params mosaicConfig and gatewayConfig are optional. It should check one of both exists.

Compiler is also complaining on same point on statement gatewayConfigPath! because of !

As discussed, added configType parameters in fromFile.

0xsarvesh · 2019-10-22T07:07:35Z

src/Config/ConfigFactory.ts

      );
+    } else {
+      // only facilitator config is given.


It should check if mosaic config exists. Because from chain with return blank object if it doesn't exist.

I have added suggested approach.
But i think, fromChain should throw error when path is not present.
It will be aligned to what we do in fromFile method.
What do you think ?

Created ticket to align fromChain and fromFile methods OpenST/mosaic-chains#196

0xsarvesh · 2019-10-22T07:10:27Z

src/bin/facilitator-init.ts

+ * @param mosaicConfigPath Path to mosaic config.
+ * @returns originchain id and gatewayaddresses object.
+ */
+function getFromMosaicConfig(


Please move these methods to some other class.

You could move whole logic to file lib/facilitator-init and commander method will have only one statement of calling class in lib.

I would suggest to create new ticket for moving other logic to file lib/facilitator-init

Created ticket #240 to address it.

0xsarvesh · 2019-10-22T07:11:30Z

src/bin/facilitator-init.ts

 commander
  .option('-m, --mosaic-config <mosaic-config>', 'path to mosaic configuration')
+  .option('-q, --gateway-config <gateway-config>', 'path to gateway configuration')


why did you decide on q as option?
I think we should revisit all the options.

I initially thought of -g, but it's already being used for --origin-graph-rpc option in facilitator-init command.
To make short-name for --gateway-config, I used -q in both init and start command.

@abhayks1 @puneet-khushwani-eth what do you think?

As discussed with @puneet-khushwani-eth and @abhayks1 , decided to go for -g for --gateway-config. So, -n is now used for --origin-graph-rpc.

0xsarvesh · 2019-10-22T07:14:46Z

src/bin/facilitator.ts

@@ -20,6 +20,6 @@
 import facilitator from 'commander';

 facilitator
-  .command('init <mosaic-config> <aux-chain-id> <origin-password> <auxiliary-password> <origin-rpc> <auxiliary-rpc> <origin-graph-ws> <origin-graph-rpc> <auxiliary-graph-ws> <auxiliary-graph-rpc> <db-path>', 'initializes the facilitator config')
+  .command('init <token-config> <mosaic-config> <aux-chain-id> <origin-password> <auxiliary-password> <origin-rpc> <auxiliary-rpc> <origin-graph-ws> <origin-graph-rpc> <auxiliary-graph-ws> <auxiliary-graph-rpc> <db-path>', 'initializes the facilitator config')


what is token config?

Changed it to gateway-config

Her does mosaic-config option needed if gateway-config is present?

0xsarvesh · 2019-10-22T07:19:31Z

test/ConfigFactory/getConfig.test.ts

  const originChain = '2';
  const auxChain = 3;
  const facilitatorConfigPath = './facilitator-config.json';
  const mosaicConfigPath = './test/mosaic-config.json';
+  const gatewayConfigPath = './gateway-config.json';
+  const mosaicJson = `{"originChain":{"chain":"${originChain}"},"auxiliaryChains":{"${auxChain}":{"chainId": ${auxChain}}}}`;


This is unreadable, extract this json out in a method.

Instead of json, method can return JS object directly because you are parsing json.

0xsarvesh · 2019-10-22T07:20:17Z

test/ConfigFactory/getConfig.test.ts

@@ -40,6 +56,16 @@ describe('FacilitatorOptionParser.getConfig()', () => {
    return spy;
  }

+  function spyGatewayConfigFromFile(gatewayConfig: any): any {


Please remove any type

There are still many any types 🤔

for method creating stub should return SinonStubStatic.

abhayks1

Few inline comments.

README.md

abhayks1 · 2019-10-22T11:40:12Z

src/bin/facilitator.ts

@@ -20,6 +20,6 @@
 import facilitator from 'commander';

 facilitator
-  .command('init <mosaic-config> <aux-chain-id> <origin-password> <auxiliary-password> <origin-rpc> <auxiliary-rpc> <origin-graph-ws> <origin-graph-rpc> <auxiliary-graph-ws> <auxiliary-graph-rpc> <db-path>', 'initializes the facilitator config')
+  .command('init <token-config> <mosaic-config> <aux-chain-id> <origin-password> <auxiliary-password> <origin-rpc> <auxiliary-rpc> <origin-graph-ws> <origin-graph-rpc> <auxiliary-graph-ws> <auxiliary-graph-rpc> <db-path>', 'initializes the facilitator config')


Her does mosaic-config option needed if gateway-config is present?

gulshanvasnani · 2019-10-22T15:06:19Z

@sarvesh-ost @abhayks1
PR is available for review

…t_gatewayconfig

0xsarvesh

LGTM 👍

gulshanvasnani added 11 commits October 16, 2019 13:25

Changes done to use token config in facilitator init

755112e

Merge branch 'common_interface_mosaic_token' into init_start_support_…

8279ecb

…gatewayconfig

Added gatewayconfig option in facilitator init

0e0ee78

Merge branch 'common_interface_mosaic_token' into init_start_support_…

f9de283

…gatewayconfig

Merge remote-tracking branch 'upstream/master' into init_start_suppor…

0264766

…t_gatewayconfig

Added gateway config option in facilitator-start command; Unit test c…

f497842

…ases for getConfig method

Reverted changes

c69e555

Removed console

af25fd9

Added validation so that together gateway config and mosaic-config is…

d57c7b8

… not allowed

Documentation changes

939b7c4

Added validation and updated readme

e29cef4

0xsarvesh self-assigned this Oct 22, 2019

Fixed failing test case

dcd07f9

0xsarvesh suggested changes Oct 22, 2019

View reviewed changes

0xsarvesh removed their assignment Oct 22, 2019

abhayks1 suggested changes Oct 22, 2019

View reviewed changes

gulshanvasnani added 2 commits October 22, 2019 18:53

Added FacilitatorInit class, ConfiyType enum, unit test cases

fbeacbf

Updated readme; Fixed test cases

e9b7336

gulshanvasnani added 2 commits October 23, 2019 16:07

Updated gateway-config short option; Added types of stub methods

f681282

Merge remote-tracking branch 'upstream/master' into init_start_suppor…

a89317c

…t_gatewayconfig

0xsarvesh approved these changes Oct 23, 2019

View reviewed changes

abhayks1 approved these changes Oct 23, 2019

View reviewed changes

0xsarvesh merged commit 94a95c6 into OpenST:master Oct 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Init start support gatewayconfig #231

Init start support gatewayconfig #231

gulshanvasnani commented Oct 17, 2019 •

edited

Loading

0xsarvesh left a comment

0xsarvesh Oct 22, 2019

gulshanvasnani Oct 22, 2019

0xsarvesh Oct 22, 2019

gulshanvasnani Oct 22, 2019

gulshanvasnani Oct 23, 2019

0xsarvesh Oct 22, 2019

0xsarvesh Oct 23, 2019

gulshanvasnani Oct 23, 2019

gulshanvasnani Oct 23, 2019

0xsarvesh Oct 22, 2019

gulshanvasnani Oct 22, 2019

0xsarvesh Oct 23, 2019

gulshanvasnani Oct 23, 2019

0xsarvesh Oct 22, 2019

gulshanvasnani Oct 22, 2019

abhayks1 Oct 22, 2019

0xsarvesh Oct 22, 2019

gulshanvasnani Oct 22, 2019

0xsarvesh Oct 23, 2019

gulshanvasnani Oct 23, 2019

0xsarvesh Oct 22, 2019

0xsarvesh Oct 23, 2019

0xsarvesh Oct 23, 2019

gulshanvasnani Oct 23, 2019

abhayks1 left a comment

abhayks1 Oct 22, 2019

gulshanvasnani commented Oct 22, 2019 •

edited

Loading

0xsarvesh left a comment

		const facilitator: FacilitatorConfig = FacilitatorConfig.fromFile(facilitatorConfigPath);
		const gatewayAddresses = mosaicConfigPath

Init start support gatewayconfig #231

Init start support gatewayconfig #231

Conversation

gulshanvasnani commented Oct 17, 2019 • edited Loading

0xsarvesh 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

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

abhayks1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gulshanvasnani commented Oct 22, 2019 • edited Loading

0xsarvesh left a comment

Choose a reason for hiding this comment

gulshanvasnani commented Oct 17, 2019 •

edited

Loading

gulshanvasnani commented Oct 22, 2019 •

edited

Loading