Refactor and deprecate ChainSetup and setup helpers #62

hobofan · 2019-02-06T08:47:47Z

FIXES #58 and starts deprecation of ChainSetup as per #57

schemar

Looks very promising ✨ Still some way to go, though.

Almost all JSDocs are missing 😞

src/ContractInteract/Anchor.js

src/ContractInteract/EIP20CoGateway.js

src/ContractInteract/validation.js

src/helpers/setup/AnchorHelper.js

src/utils/Utils.js

test_integration/01_chain_setup/01_chain_setup.js

test_integration/02_sequential_setup/01_libs_helper.js

deepesh-kn · 2019-02-07T16:05:19Z

I did a quick scan, I like the approach 👍. I will go through this again once it is completed.

src/ContractInteract/Anchor.js

deepesh-kn

Overall looks good to me: 👍
I just have few comments in general

The txOptions are not used in the RawTx methods.
The errors do not include the value (@schemar do we need this in PR? or can a ticket will do for later change?).
Maybe we should move the static methods to be end of the class.

src/ContractInteract/validation.js

src/ContractInteract/Anchor.js

src/helpers/setup/CoGatewayHelper.js

src/helpers/setup/GatewayHelper.js

schemar

😳 big one. Good work 💪
I still have some questions.

Are the setup helpers changes backwards compatible?
Why do the integration tests still use the helpers, when we can use the contract interacts now to setup the contracts? Wasn't that the plan?

src/ChainSetup.js

src/ContractInteract/Anchor.js

src/ContractInteract/EIP20CoGateway.js

src/ContractInteract/validation.js

src/helpers/setup/CoGatewayHelper.js

src/utils/Utils.js

test_integration/01_sequential_setup/01_libs_helper.js

test_integration/01_sequential_setup/02_organization_helper.js

schemar · 2019-02-11T12:36:32Z

Overall looks good to me: 👍
I just have few comments in general

* The `txOptions` are not used in the RawTx methods.

* The errors do not include the value (@schemar do we need this in PR? or can a ticket will do for later change?).

I don't know. Depends on the effort.

* Maybe we should move the static methods to be end of the class.

hobofan · 2019-02-11T14:15:55Z

Why do the integration tests still use the helpers, when we can use the contract interacts now to setup the contracts? Wasn't that the plan?

-> #99

deepesh-kn

This looks good. 🚴🚴🚴🚴🚴

I have added few small comments. Either we can fix if its small or create can create a followup ticket. 👍

1.Validations of txOptions and txOptions.from is missing for all the deploy methods.

2.Unit tests for the following is missing, maybe we can create a followup ticket.

Anchor:
- setCoAnchorAddress
- setCoAnchorAddressRawTx

EIP20Gateway:
- getStakeVault
- activateGateway
- activateGatewayRawTx

OSTPrime:
- initialize
- initializeRawTx
- setCoGateway
- setCoGatewayRawTx

3.All the integration tests of new code is in old helper files, that confused me alot.

src/ContractInteract/EIP20Gateway.js

src/ContractInteract/GatewayLib.js

src/ContractInteract/MerklePatriciaProof.js

src/ContractInteract/OSTPrime.js

deepesh-kn · 2019-02-12T11:39:43Z

test_integration/01_sequential_setup/02_organization_helper.js

-      shared.origin.addresses.Organization = subject.address;
-    });
+
+    return Organization.setup(shared.origin.web3, _orgConfig).then(


What is difference in this test and after()?

origin and auxiliary. In after, we deploy an Organization on auxiliary to be used by later tests.

Adding a comment will be helpful.

I added a comment to the after.

hobofan · 2019-02-12T12:06:17Z

2.Unit tests for the following is missing, maybe we can create a followup ticket.

#108

3.All the integration tests of new code is in old helper files, that confused me alot.

There is a related ticket: #99

deepesh-kn

LGTM 🎢 👍

schemar

Only two small open points ✨

schemar

hobofan mentioned this pull request Feb 6, 2019

Fix EIP20Gateway naming & Use Mosaic for Contracts #55

Merged

hobofan force-pushed the refactor_chain_setup branch from f21c05d to 528a4e1 Compare February 6, 2019 09:49

0xsarvesh assigned hobofan Feb 6, 2019

hobofan force-pushed the refactor_chain_setup branch from a82d1f0 to f7f0c11 Compare February 6, 2019 15:16

hobofan requested review from deepesh-kn and schemar February 6, 2019 15:28

hobofan changed the title ~~[WIP] Refactor and deprecate ChainSetup and setup helpers~~ Refactor and deprecate ChainSetup and setup helpers Feb 7, 2019

schemar suggested changes Feb 7, 2019

View reviewed changes

hobofan force-pushed the refactor_chain_setup branch 2 times, most recently from b9cb47d to 2e480b3 Compare February 11, 2019 09:51

schemar reviewed Feb 11, 2019

View reviewed changes

src/ContractInteract/Anchor.js Outdated Show resolved Hide resolved

src/ContractInteract/Anchor.js Show resolved Hide resolved

hobofan force-pushed the refactor_chain_setup branch 2 times, most recently from ab1c998 to 459078a Compare February 11, 2019 10:55

deepesh-kn reviewed Feb 11, 2019

View reviewed changes

schemar suggested changes Feb 11, 2019

View reviewed changes

hobofan mentioned this pull request Feb 11, 2019

Use more descriptive name than subject in integration tests #100

Closed

deepesh-kn mentioned this pull request Feb 11, 2019

Facilitator should read gateway and cogateway address from address variable. #102

Merged

hobofan added 10 commits February 12, 2019 11:48

OrganizationHelper -> Organization refactor & deprecation

2b4dc50

AnchorHelper -> Anchor refactor & deprecation

3a08c3f

CoGatewayHelper -> EIP20CoGateway refactor & deprecation

db03757

GatewayHelper -> EIP20Gateway refactor & deprecation

8c6a58a

LibsHelper -> individual deploys refactor & deprecation

daaf9ad

Remove ChainSetup setup functions

9d0713c

OSTPrimeHelper -> OSTPrime refactor & deprecation

e01e1e9

Add docs for OSTPrime

4d7ee2b

Remove 01_chain_setup integration test directory

9aa7030

membersManager -> organization

1eef818

hobofan added 16 commits February 12, 2019 11:50

Add docs for Anchor

2c36334

Remove license headers

f619304

Fix EIP20CoGateway not using instance address correctly

ed85140

Add docs for library ContractInteracts

c45a4b4

Add remaining documentation

1492d85

Fix OSTPrime constructor tests

64d7622

Address PR feedback

0be2c56

Fix EIP20Gateway not using instance address correctly

a216362

Fix style offenses

18dfd1f

Move estimateGas into sendTransaction

589de08

Revert Anchor param naming change

f6d7c87

anchor -> stateRootProvider in gateway params

26222a0

More consistent early returns in constructors

9697302

Make setup validation more consistent

25870e0

Check deploy params

7ff17ed

Address PR feedback

4e3b744

hobofan force-pushed the refactor_chain_setup branch from ab001c1 to 4e3b744 Compare February 12, 2019 10:51

deepesh-kn reviewed Feb 12, 2019

View reviewed changes

hobofan mentioned this pull request Feb 12, 2019

Add missing unit tests for ContractInteracts #108

Closed

Address PR feedback

404819a

deepesh-kn approved these changes Feb 12, 2019

View reviewed changes

schemar approved these changes Feb 12, 2019

View reviewed changes

Address PR feedback

0b22226

schemar approved these changes Feb 12, 2019

View reviewed changes

hobofan merged commit 2e0c1f1 into develop Feb 12, 2019

benjaminbollen deleted the refactor_chain_setup branch March 28, 2019 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and deprecate ChainSetup and setup helpers #62

Refactor and deprecate ChainSetup and setup helpers #62

hobofan commented Feb 6, 2019 •

edited

Loading

schemar left a comment

deepesh-kn commented Feb 7, 2019

deepesh-kn left a comment

schemar left a comment

schemar commented Feb 11, 2019

hobofan commented Feb 11, 2019 •

edited

Loading

deepesh-kn left a comment

deepesh-kn Feb 12, 2019

hobofan Feb 12, 2019

deepesh-kn Feb 12, 2019

hobofan Feb 12, 2019

hobofan commented Feb 12, 2019

deepesh-kn left a comment

schemar left a comment

schemar left a comment

Refactor and deprecate ChainSetup and setup helpers #62

Refactor and deprecate ChainSetup and setup helpers #62

Conversation

hobofan commented Feb 6, 2019 • edited Loading

schemar left a comment

Choose a reason for hiding this comment

deepesh-kn commented Feb 7, 2019

deepesh-kn left a comment

Choose a reason for hiding this comment

schemar left a comment

Choose a reason for hiding this comment

schemar commented Feb 11, 2019

hobofan commented Feb 11, 2019 • edited Loading

deepesh-kn left a comment

Choose a reason for hiding this comment

deepesh-kn Feb 12, 2019

Choose a reason for hiding this comment

hobofan Feb 12, 2019

Choose a reason for hiding this comment

deepesh-kn Feb 12, 2019

Choose a reason for hiding this comment

hobofan Feb 12, 2019

Choose a reason for hiding this comment

hobofan commented Feb 12, 2019

deepesh-kn left a comment

Choose a reason for hiding this comment

schemar left a comment

Choose a reason for hiding this comment

schemar left a comment

Choose a reason for hiding this comment

hobofan commented Feb 6, 2019 •

edited

Loading

hobofan commented Feb 11, 2019 •

edited

Loading