Shepherd MVP Integration #1413

HenryNguyen5 · 2018-03-29T02:33:08Z

Depends on #1456 since shepherd package uses new types introduced in TS 2.8.1 for its published types

Closes #1419
Closes #1309
Closes #1293

This current iteration of shepherd MVP is at feature parity with the current mycrypto beta.
Notable codebase changes:

All nodes are now just the shepherd provider
The selector for getting a node, just returns the shepherd provider (done to minimize line changes)
getTokensBalance / getTokensBalances are proxied above the shepherd level
getTokensBalances uses a batch splitting method that ensures batches are no bigger than size 10
web3 actually gets its own set of networks, e.g. ropsten / eth / kovan all have their web3 counterpart that is switched to when metamask wallet is loaded. This is done as to disable sending transactions / signing messages on the balanced providers, and to force them to go through the web3 provider.
All node implementations are removed except for web3/rpc. We currently still have those two as web3 initialisation process is needed on app side, and it inherits rpc implementation.

The remaining TODO's do not effect the functionality of the product compared to pre-shepherd.

TODO:

manual mode on metamask/web3 nodes
Disable method checks for manual provider mode

How to monitor the balancer:
Clone the shepherd repo and install dependencies
Run mycrypto app and open page
Run npm run remotedev inside repo
Open remote mode on redux-dev-tools
Point to localhost port 8000

wbobeirne

It's working great for me, saw some instances of a node failing but the call still went through by re-requesting from another node. Just dropped a few comments in code, but I'm happy to approve if they don't seem worth addressing.

Just a couple of other things I noticed though, were some console logs. Looks like shepherd is doing a lot of logging that it should probably not do unless configured to do so in someway. Likewise, I got some errors about a socket not being able to connect. Is that also related to shepherd?

wbobeirne · 2018-04-04T16:52:49Z

common/components/Header/components/CustomNodeModal.tsx

@@ -343,6 +341,7 @@ class CustomNodeModal extends React.Component<Props, State> {

  private saveAndAdd = () => {
    const node = this.makeCustomNodeConfigFromState();
+    shepherd.useProvider('myccustom', node.id, makeProviderConfig({ network: node.network }), node);


Should this be happening at the saga layer? This seems like something that could get out of sync with redux, and might not belong in the component.

Shepherd handles these race conditions internally, useProvider is considered a synchronous method from the caller's view

Sorry if I wasn't clear, I wasn't worried about asynchronosity, I just thought this was a little too much logic for a component to implement. If there's ever another different component or process that can add custom nodes (e.g. pulling in a node from the URL) they would have to implement this as well. I figured this might be better off in a saga.

wbobeirne · 2018-04-04T16:53:40Z

common/libs/nodes/index.ts

+import { shepherd, redux } from 'myc-shepherd';
+import { INode } from '.';
+import { tokenBalanceHandler } from './tokenBalanceProxy';
+import { IProviderConfig } from 'myc-shepherd/dist/lib/ducks/providerConfigs';


Would be great to get types from an myc-shepherd/index.d.ts file instead so that we don't need to specify paths.

Currently waiting on this to be resolved so that works: MyCryptoHQ/shepherd#44

wbobeirne · 2018-04-04T16:58:21Z

common/libs/nodes/tokenBalanceProxy.ts

+export const tokenBalanceHandler: ProxyHandler<any> = {
+  get(target, propKey) {
+    const tokenBalanceShim = (address: string, token: Token) => {
+      const sendCallRequest: (...rpcArgs: any[]) => Promise<string> = Reflect.get(


What's the value in doing Reflect.get over just target.sendCallRequest?

Nothing really, just is more specific semantics wise

wbobeirne · 2018-04-04T16:58:49Z

common/libs/nodes/tokenBalanceProxy.ts

+import { TokenValue } from 'libs/units';
+
+export const tokenBalanceHandler: ProxyHandler<any> = {
+  get(target, propKey) {


Should these be typed?

Right, ill add the type to proxyhandler

wbobeirne · 2018-04-04T16:59:44Z

common/libs/nodes/tokenBalanceProxy.ts

+        .then(result => ({ balance: TokenValue(result), error: null }))
+        .catch(err => ({
+          balance: TokenValue('0'),
+          error: 'Caught error:' + err


Err could be an object so it may not work with addition like this. Prefer Caught error: ${err} so that it properly stringifies.

This was copied over from our old code, ill change it though

wbobeirne · 2018-04-04T17:08:02Z

common/sagas/config/web3.ts

@@ -34,6 +108,13 @@ export function* unsetWeb3NodeOnWalletEvent(action: SetWalletAction): SagaIterat
    return;
  }

+  const network: NetworkConfig = yield select(getNetworkConfig);
+
+  if (getShepherdManualMode()) {


Is there any harm in calling shepherd.auto even if it's not in manual? Seems like it would reduce some amount of boilerplate in checking.

switching networks is disabled in shepherd if you're in manual mode, since manual mode implies that everything is overridden

wbobeirne · 2018-04-04T17:10:14Z

common/selectors/config/nodes.ts

-  const isWeb3Node = (nodeId: string, _: StaticNodeConfig | Web3NodeConfig): _ is Web3NodeConfig =>
-    nodeId === 'web3';
+export const getWeb3Node = (state: AppState): StaticNodeConfig | null => {
+  const isWeb3Node = (nodeId: string, _: StaticNodeConfig) => nodeId === 'web3';


No need to specify second param if it's not used anymore.

wbobeirne · 2018-04-04T17:12:04Z

shared/types/node.d.ts

  UBQ = 'ubq',
+  EXP_AUTO = 'exp_auto',


Do we need all of these extra autos, if they only have the one node?

This is so that when custom nodes get added with the same network, the user will also have the option to balance between them

wbobeirne · 2018-04-04T17:13:16Z

webpack_config/makeConfig.js

      new HardSourceWebpackPlugin({
        environmentHash: {
          root: process.cwd(),
          directories: ['webpack_config'],
          files: ['package.json']
        }
-      }),
+      }),*/


What was this removed for?

My bad ill revert this, it was breaking my builds

wbobeirne · 2018-04-05T13:45:32Z

common/store/configAndTokens.ts

+    } else {
+      shepherd.manual(savedNodeId, false);
+    }
+  }


We might do well to implement something in here that sets users to auto who have previously loaded the beta. I was defaulted to the MyCrypto node, because I had a stored config already.

I think we should first implement a versioning spec for local storage so we can properly invalidate data for situations like this. Since this case pops up often enough to warrant it imo

…to shepherd-mvp

dternyak · 2018-04-06T02:30:28Z

spec/reducers/config/nodes/staticNodes.spec.ts

 configuredStore.getState();

 const expectedInitialState = {
  eth_mycrypto: {
    network: 'ETH',
    isCustom: false,
-    lib: new RPCNode('https://api.mycryptoapi.com/eth'),
+    lib: {},


Can we refactor to just remove lib?

coveralls · 2018-04-06T02:39:03Z

Coverage increased (+0.3%) to 33.854% when pulling 4aad39e on shepherd-mvp into fde8867 on develop.

wbobeirne

Confirming it all works great, including with a custom node.

initial mvp

08feec3

HenryNguyen5 added the wip label Mar 29, 2018

HenryNguyen5 added 4 commits March 30, 2018 00:36

First functioning pass

6fdedcf

Add token balance shim

a1d8507

Add working web3 implementation

3b28061

Fix tests

176ba33

dternyak changed the title ~~Shepherd MVP integration~~ Shepherd MVP Integration Apr 3, 2018

HenryNguyen5 added 4 commits April 3, 2018 20:03

Fix tsc errors

6d5189a

Implement token batch splitting

0c939dd

Undo logger change

b5f62c6

Fix linting errors

e5a5a36

HenryNguyen5 added status: ready for review Open PR's that are ready for technical review. Replaced with "changes needed" or "ready to merge". and removed wip labels Apr 4, 2018

HenryNguyen5 requested review from skubakdj, wbobeirne, dternyak and james-prado April 4, 2018 01:50

wbobeirne reviewed Apr 5, 2018

View reviewed changes

HenryNguyen5 added 10 commits April 5, 2018 19:10

Revert makeconfig change

bee4766

Add typing to token proxy + use string interpolation

8c698e8

Remove useless parameter

b584e56

Remove logging

96f16e6

Use type coercion to fix proxied methods

5256f48

Update shepherd

107e29c

Update to typescript 2.8.1

1705475

Merge develop

bec75a4

Merge branch 'update-typescript' of github.com:MyCryptoHQ/MyCrypto in…

06c3387

…to shepherd-mvp

Fix merged typings

2bbd65a

dternyak reviewed Apr 6, 2018

View reviewed changes

Merge branch 'develop' into shepherd-mvp

e607dac

HenryNguyen5 added 2 commits April 6, 2018 14:17

Merge develop

26daa58

Address PR comments

21eda0c

wbobeirne previously approved these changes Apr 6, 2018

View reviewed changes

dternyak dismissed wbobeirne’s stale review via 84270df April 6, 2018 20:11

replace myc-shepherd with mycrypto-shepherd

6fdc177

dternyak force-pushed the shepherd-mvp branch from 95c67b6 to 6fdc177 Compare April 6, 2018 20:30

Merge branch 'develop' into shepherd-mvp

4aad39e

dternyak approved these changes Apr 6, 2018

View reviewed changes

dternyak merged commit 04eaa08 into develop Apr 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shepherd MVP Integration #1413

Shepherd MVP Integration #1413

HenryNguyen5 commented Mar 29, 2018 •

edited

Loading

wbobeirne left a comment •

edited

Loading

wbobeirne Apr 4, 2018

HenryNguyen5 Apr 5, 2018

wbobeirne Apr 6, 2018

wbobeirne Apr 4, 2018

HenryNguyen5 Apr 5, 2018

wbobeirne Apr 4, 2018

HenryNguyen5 Apr 5, 2018

wbobeirne Apr 4, 2018

HenryNguyen5 Apr 5, 2018

wbobeirne Apr 4, 2018

HenryNguyen5 Apr 5, 2018

wbobeirne Apr 4, 2018

HenryNguyen5 Apr 5, 2018

wbobeirne Apr 4, 2018

HenryNguyen5 Apr 5, 2018

wbobeirne Apr 4, 2018

HenryNguyen5 Apr 5, 2018

wbobeirne Apr 4, 2018

HenryNguyen5 Apr 5, 2018

wbobeirne Apr 5, 2018

HenryNguyen5 Apr 5, 2018

dternyak Apr 6, 2018

coveralls commented Apr 6, 2018 •

edited

Loading

wbobeirne left a comment

Shepherd MVP Integration #1413

Shepherd MVP Integration #1413

Conversation

HenryNguyen5 commented Mar 29, 2018 • edited Loading

wbobeirne left a comment • edited Loading

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

coveralls commented Apr 6, 2018 • edited Loading

wbobeirne left a comment

Choose a reason for hiding this comment

HenryNguyen5 commented Mar 29, 2018 •

edited

Loading

wbobeirne left a comment •

edited

Loading

coveralls commented Apr 6, 2018 •

edited

Loading