Refactor, Mock API, more tests #1

fazo96 · 2017-12-06T10:33:20Z

This PR is big, but the code was not very modular and the Mock API required moving most of the main logic, so I did some general refactoring.

Highlights of this PR:

Mock API! just pass mock: true when creating the ChluIPFS instance. The mock API returns stubbed values and even fake-waits a couple of seconds on calls
Mock API and index.js are now unit tested
the combined functionality test between Customer and Service Node uses real IPFS nodes and runs with the other tests now
all tests run in Travis

… runs under the test environment

kulpreet

Awesome work @fazo96. Really loving the progress you are making.

I have left a few notes and suggestions. I would say, those comments that you think are future work, we should create issues for those. Can you do that?

And those you think you want to address as part of this PR itself, you can address them. The decision is up to you.

Send me a ping on slack later and I can merge once you confirm.

kulpreet · 2017-12-06T17:40:31Z

README.md

+This module has not been release to NPM yet. You can however install it and import/require it, but it's still WIP. Check out [index.js](https://github.com/ChluNetwork/chlu-ipfs-support/blob/master/src/index.js) to see the available API calls, and the tests for examples.
+
+You can install this module globally and use `chlu-service-node` to run a Chlu Service Node.


@fazo96 why do we need this to be installed globally? Can't one install it in a local folder and then run it with the required permissions?

It can also be installed locally: in any folder, run npm install chlu-ipfs-support. The binary should be available under ./node_modules/chlu-ipfs-support/bin/ like any other npm module.

This binary just starts ChluIPFS and its service node routines for now, there are still a lot of improvements to make and tests to run

kulpreet · 2017-12-06T17:43:06Z

src/ChluIPFS.js

+        Addresses: {
+            Swarm: [
+                // Enable WebSocketStar transport
+                '/dns4/ws-star.discovery.libp2p.io/tcp/443/wss/p2p-websocket-star'


@fazo96 did you check this endpoint is the same one used by orbitdb in their web examples?

Yes, it is: https://github.com/orbitdb/orbit-db/blob/master/examples/browser/example.js#L57

For now this is the only IPFS transport that is stable in the browser.

kulpreet · 2017-12-06T17:54:42Z

src/ChluIPFS.js

+            store: String(Math.random() + Date.now())
+        };
+        // TODO: review persisting data in orbitDb. we don't need persistence in the browser but it would help.
+        this.orbitDbDirectory = options.orbitDbDirectory || '../orbit-db';


What will happen in the case of browser? I guess we should make sure we are setup properly for the browser use case.

I am also thinking if we should have a separate repo for chlu-service-node, so there we can have options and setup optimisations for a node process, and in this repo we only have browser setup options. Does that make sense?

In the browser at the moment we don't persist OrbitDB. This is acceptable because the owner of the DB can replicate it from other nodes in the network, but we will change this anyway since there's no reason not to keep a copy in the browser.

When I wrote this code OrbitDB only had filesystem persistence (which obviously does not work in the browser) however they just changed it to an implementation using LevelDB: orbitdb/orbitdb#272

I opened an issue to track how we persist data: #4

I am also thinking if we should have a separate repo for chlu-service-node, so there we can have options and setup optimisations for a node process, and in this repo we only have browser setup options. Does that make sense?

In the future we should probably take out the chlu service node as it becomes a more fleshed out application but for now it's so simple that I think it's ok for it to be in here.

About splitting the browser and node.js parts, I believe it's not a good idea because most of the code is the same and many of the differences are handled automatically by OrbitDB and IPFS themselves. It would introduce complexity for no benefits.

However we need to make sure everything works fine in the browser. I opened an issue about this: #5

kulpreet · 2017-12-06T17:58:31Z

src/ChluIPFS.js

+            this.orbitDb = new OrbitDB(this.ipfs, this.orbitDbDirectory);
+        }
+        if (this.type === constants.types.customer && !this.db) {
+            this.db = await this.orbitDb.feed('chlu-experimental-customer-review-updates');


I wonder if we should have names like these in a constants file somewhere. What do you think?

Good idea, I should put it with the other constants

kulpreet · 2017-12-06T18:02:57Z

src/ChluIPFS.js

+        if (this.ipfs.pin) {
+            await this.ipfs.pin.add(multihash, { recursive: true });
+        } else {
+            // TODO: Chlu service node need to be able to pin, so we should support using go-ipfs


I do think the chlu service node should really be based on go-ipfs. Couple of reasons, 1) probably more mature implementation with more features and 2) probably less resource intensive. For the same reason I am in favour of a chlu-service-node-js and later a chlu-service-node-go repo

I agree, when possible we should use go-ipfs. Lucky for us, we can talk to go-ipfs using js-ipfs-api, which has the same exact calls to js-ipfs with the difference that js-ipfs-api forwards calls to go-ipfs via HTTP and js-ipfs executes them in javascript.

This means we can support running ChluIPFS on top of go-ipfs (or any other ipfs implementation really) with configuration options.

We need to verify that go-ipfs can connect to js-ipfs nodes running in the browser. This might be an issue, in the browser we can only use the websocket-star transport and it's unclear wether it can connect to the go-ipfs websocket transport at the moment (https://github.com/libp2p/go-ws-transport) even though it should eventually.

I created an issue for this: #3

kulpreet · 2017-12-06T18:11:20Z

src/ChluIPFS.js

+        const dagNode = await this.ipfs.object.put(reviewRecord);
+        const multihash = this.utils.multihashToString(dagNode.multihash);
+        // Broadcast request for pin, then wait for response
+        // TODO: handle a timeout and also rebroadcast periodically, otherwise new peers won't see the message


It would be good to have a timeout as you said, but maybe use an exponential backoff while retrying. I am surprised orbitdb doesn't do that already

I never thought about retrying but it makes a lot of sense

I opened an issue about this: #6

kulpreet · 2017-12-06T18:13:54Z

src/ChluIPFS.mock.js

+    }
+
+    async start(){
+        await time.milliseconds(1000);


The time out thing is interesting. I can see it will be nice in the development mode. But it might unnecessarily slow us down for testing in a CI env. Can this be made optional? or maybe we have production, development, testing, three different modes? What do you think?

I put these in to simulate that these actions take some time with a real implementation. They are already disabled in the tests because the time.milliseconds function is mocked to return immediately so there are no artificial slowdowns when running the automated tests

At the moment I don't think we need three separate modes

kulpreet · 2017-12-06T18:15:54Z

src/index.js

    }

-    async importData() {
-        throw new Error('not implemented');
+    async importData(exportedData) {


what data do exportData and importData do? Maybe we can change these function names to more meaningful or descriptive names?

Yes that's a good idea, but first I need to figure out a good format for exporting what we need.

Both OrbitDB and IPFS have almost no support for exporting and importing keys so this could require some hacks, although I'm confident it's possible

kulpreet · 2017-12-06T18:22:36Z

tests/CustomerAndServiceNode.test.js

@@ -0,0 +1,49 @@
+#!/usr/bin/env node


This is an awesome integration test. WIll travis ci be able to run this? I guess we'll find out soon. I'll set up travis as soon as we have merged this PR

Yes, I enabled travis for my fork and this test passed 👍 I just copied the way OrbitDB did tests.

fazo96 · 2017-12-12T08:46:52Z

I moved a couple of hardcoded strings to the constants.js file.

I think this PR can be merged now

fazo96 added 6 commits December 5, 2017 17:03

update tests and documentation

f56d81c

update travis configuration

0da87f6

huge refactor, added mock api, wrote more tests

2de03f8

removed no-console rule from eslint for now. Fixed README link

822ea53

milestone: the integration test between service node and customer now…

07f1c1b

… runs under the test environment

Update README, add test coverage

3d76458

fazo96 requested a review from kulpreet December 6, 2017 10:33

fazo96 mentioned this pull request Dec 6, 2017

Customer review updates first implementation #2

Merged

3 tasks

kulpreet reviewed Dec 6, 2017

View reviewed changes

move OrbitDB name and pubsub room name to constants.js

ceed726

fazo96 mentioned this pull request Dec 12, 2017

Added in-browser tests #7

Merged

kulpreet merged commit 76958be into ChluNetwork:master Dec 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor, Mock API, more tests #1

Refactor, Mock API, more tests #1

fazo96 commented Dec 6, 2017

kulpreet left a comment

kulpreet Dec 6, 2017

fazo96 Dec 12, 2017

kulpreet Dec 6, 2017

fazo96 Dec 12, 2017

kulpreet Dec 6, 2017

fazo96 Dec 12, 2017

fazo96 Dec 12, 2017 •

edited

Loading

kulpreet Dec 6, 2017

fazo96 Dec 12, 2017

kulpreet Dec 6, 2017

fazo96 Dec 12, 2017

kulpreet Dec 6, 2017

fazo96 Dec 12, 2017

fazo96 Dec 12, 2017

kulpreet Dec 6, 2017

fazo96 Dec 12, 2017 •

edited

Loading

kulpreet Dec 6, 2017

fazo96 Dec 12, 2017

kulpreet Dec 6, 2017

fazo96 Dec 12, 2017 •

edited

Loading

fazo96 commented Dec 12, 2017

		This module has not been release to NPM yet. You can however install it and import/require it, but it's still WIP. Check out [index.js](https://github.com/ChluNetwork/chlu-ipfs-support/blob/master/src/index.js) to see the available API calls, and the tests for examples.

		You can install this module globally and use `chlu-service-node` to run a Chlu Service Node.

Refactor, Mock API, more tests #1

Refactor, Mock API, more tests #1

Conversation

fazo96 commented Dec 6, 2017

kulpreet 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

fazo96 Dec 12, 2017 • 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

fazo96 Dec 12, 2017 • 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

fazo96 Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

fazo96 commented Dec 12, 2017

fazo96 Dec 12, 2017 •

edited

Loading

fazo96 Dec 12, 2017 •

edited

Loading

fazo96 Dec 12, 2017 •

edited

Loading