Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat: new libp2p config #1401

Merged
merged 1 commit into from
Jul 2, 2018
Merged

feat: new libp2p config #1401

merged 1 commit into from
Jul 2, 2018

Conversation

daviddias
Copy link
Member

Preparing js-ipfs for js-ipfs libp2p/js-libp2p#166 and at the same time testing the decisions made.

@ghost ghost assigned daviddias Jun 19, 2018
@ghost ghost added the status/in-progress In progress label Jun 19, 2018
webRTCStar: get(config, 'Discovery.webRTCStar.Enabled'),
bootstrap: get(config, 'Bootstrap')
},
EXPERIMENTAL: {
Copy link
Member Author

@daviddias daviddias Jun 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • EXPERIMENTAL should be part of the config (easier to config).

@daviddias
Copy link
Member Author

libp2p 0.21.0 has been released. This PR can be updated. (I'll possibly be able to do this tomorrow)

@alanshaw
Copy link
Member

I'll take a look

@ghost ghost assigned alanshaw Jun 28, 2018
@alanshaw
Copy link
Member

@diasdavid can you take a look at what I've done here - tests are all passing

},
EXPERIMENTAL: {
dht: get(self._options, 'EXPERIMENTAL.dht', false),
pubsub: get(self._options, 'EXPERIMENTAL.pubsub', false)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! For the next turn, we should optimize js-ipfs options just like js-libp2p so that when we do new IPFS(options), it is clear that:

const options = {
  ... // any specifics on spawning the daemon this way on the first level
  config: {
    // everything else comes from the config
  }
}

Copy link
Member Author

@daviddias daviddias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work @alanshaw thank you!

Now it just needs to have the examples and README checked for these breaking changes.

active: get(self._options, 'EXPERIMENTAL.relay.hop.active',
get(config, 'EXPERIMENTAL.relay.hop.active', false))
}
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diasdavid do you want to move relay out from behind EXPERIMENTAL in js-ipfs too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense :)

@alanshaw
Copy link
Member

@diasdavid in 71883e3 I've documented the change to the config libp2p.modules.discovery to libp2p.modules.peerDiscovery and altered the code so that people creating their own IPFS node can now pass config to their peer discovery classes:

new IPFS({
  libp2p: {
    modules: {
      peerDiscovery: [MyDiscovery]
    },
    config: {
      peerDiscovery: {
        [MyDiscovery.tag]: {
          enabled: true,
          myCustomConfig: 'property'
        }
      }
    }
  }
})

So actually the whole of the libp2p config value is passed to the libp2p.Node (after defaults applied) so the user can configure anything to do with libp2p now.

I couldn't find any examples where the libp2p config option was being used so there's nothing to do there.

The only breaking change I can see here is the change to the config libp2p.modules.discovery to libp2p.modules.peerDiscovery. I have added a "BREAKING CHANGE" section to the last commit and I'll update the release notes shortly.

@jacobheun
Copy link
Contributor

jacobheun commented Jun 28, 2018

@alanshaw just a heads up, in libp2p we ended up switching over to @nodeutils/defaults-deep instead of lodash when doing testing, as lodash didn't play nicely with arrays or instances (it clones).

When arrays are combined, like module.transports, the defaults will stomp on the original based on array index. Options like:

modules: { 
  transport: [ TCP, WS ]
}

and defaults:

modules: { 
  transport: [ wrtc ]
}

would end up being:

modules: { 
  transport: [ wrtc, WS ]
}

libp2p/js-libp2p#206. It leverages lodash merge under the hood with some custom merge configuration.

@alanshaw
Copy link
Member

@jacobheun I've made that switch. It means that if you pass discovery or transport modules in the config to your IPFS constructor then the default transports and discoveries will be replaced.

I don't think that's what we want - I think we do want to add to the list (and the user can now disable any they want to in the config - hooray!). @diasdavid do you agree?

@jacobheun
Copy link
Contributor

There's a trade off to be made either way, but I think having to be explicit when you are specifying arrays is the better option. Here's the two approaches I am thinking of:

  1. Make options be an add to defaults for arrays
  • If I want to add an additional transport, this becomes very easy to do. Just supply the additional ones you want in your custom options and boom, you have them all.
  • If I want to only use my transports, I have to create the libp2p node, remove the transports and then add mine. This is not so fun.
  1. Make options be explicit for arrays
  • If I want to add an additional transport, I need to specify everything, which means looking up the defaults and adding those in. This is a bit annoying, but easier than "cleaning up" stuff I don't want. I could also add my transports after creation and before startup as an alternative approach.
  • If I want to only use my transports, there's no difference, I just specify the things I want and I'm done.

We could also potentially expose the defaults, to give users more flexibility there if they wanted to customize operations. This has a nice middle ground of being explicit, but allowing for easier customization.

@alanshaw alanshaw mentioned this pull request Jun 29, 2018
23 tasks
@daviddias
Copy link
Member Author

Right now, the only way to disable some of the options (transports, muxers, encryption) is to explicitly overload the default array for them.

We will need something such as libp2p/js-libp2p#201 to be able to disable them through an option.

@alanshaw
Copy link
Member

Good to merge @diasdavid? Only CI failure is commit lint...

@jacobheun
Copy link
Contributor

With the bump to Aegir 14 it looks like the package version of ipfs-http-response and interface-datastore need to be changed to ~ to fix code linting. David's original "wip" commit message is breaking the commit lint, we could amend or rebase that to fix.

Other than that, this LGTM!

@daviddias
Copy link
Member Author

@alanshaw
Copy link
Member

alanshaw commented Jul 2, 2018

@diasdavid @jacobheun I could use some help with that failing test...

@jacobheun
Copy link
Contributor

@alanshaw I can take a look at this today. This test looks flaky, I'll see if I can figure out what's causing that flakiness and get it fixed to prevent future failures.

Copy link
Member Author

@daviddias daviddias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@daviddias daviddias force-pushed the feat/new-libp2p branch 2 times, most recently from 73f25ae to 3b0ac0e Compare July 2, 2018 11:38
License: MIT
Signed-off-by: David Dias <mail@daviddias.me>

License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
@jacobheun jacobheun self-requested a review July 2, 2018 13:23
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I'll look at the flaky tests referenced in #1401 (comment) and add a separate PR for any issues with that.

@alanshaw alanshaw merged commit 9c60909 into master Jul 2, 2018
@ghost ghost removed the status/in-progress In progress label Jul 2, 2018
@alanshaw alanshaw deleted the feat/new-libp2p branch July 2, 2018 13:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants