fix: pubsub configuration

[a1300]

This PR fixes issue #401 and adds the following new pubsub configuration options:

• emitSelf (boolean)
• signMessages (boolean)
• strictSigning (boolean)

@jacobheun I wasn't sure if I should add tests for this bug fix because it seemed to me that this repository isn't testing if configuration is correctly passed to the Modules. Should I add some tests?

Thanks!

[jacobheun]

Yes, some tests in ./test/pubsub.node.js to verify that toggling emitSelf works as expected would be great.

Thanks for updating the example as well!

[a1300]

@jacobheun Ok, I will soon add some tests!

[a1300]

@jacobheun quick question: Do you want that I test the toggling of emitSelf with both libp2p-gossipsub and libp2p-floodsub?

[jacobheun]

@a1300 1 implementation is fine, ultimately we're just verifying that it's getting the configuration. Those modules should handle more in depth testing for themselves.

[jacobheun]

Even just testing that a mock Pubsub implementation is getting the right options would be sufficient.

[a1300]

@jacobheun I am currently writing the tests, and I wondered if the following is not a bug on its own:

The following code is the config that defines the default properties of the PubSub config:

  // Pubsub config
  pubsub: s('object?', {
    // Pubsub defaults
    enabled: true,
    emitSelf: true,
    signMessages: true,
    strictSigning: true
  })

If I now create a Bundle with only a subset of the options above then I would expect that the default values would be set, but they aren't.

const bundle = new libp2p({
  modules: {
    transport: [ TCP ],
    streamMuxer: [ Mplex ],
    connEncryption: [ SECIO ],
    peerDiscovery: [ MulticastDNS ],
    pubsub: Gossipsub
  },
  config: {
    peerDiscovery: {
      mdns: {
        interval: 2000,
        enabled: true
      }
   },
   pubsub: {
      enabled: true,
      emitSelf: true
    }
  }
}

The PubSub implementation is getting called only with:

{
  enabled: true,
  emitSelf: true
}

But I would expect that it would be called with:

{
  enabled: true,
  emitSelf: true,
  signMessages: true,
  strictSigning: true
}

Is this a bug or expected behavior? Thanks!

[jacobheun]

It's expected, but not ideal. The config needs some improvements. Right now it's validating an optional object and not their sub properties. If any object is supplied it uses that and doesn't do any copying of the default properties. Ideally we'd get the defaults populated, while avoiding making the config too strict. The reason for this is that if we add a new config option in the future to pubsub or another module, we need to avoid libp2p config validation from throwing an error, so that it doesn't have to be updated to use the latest pubsub.

I'll create a ticket to track improving the config, don't worry about it for this, you may just need to be verbose with configuration for some of the tests. (Note: Both existing pubsub modules do properly add defaults for the missing values, so we are catching it there as we should be).

[a1300]

@jacobheun please review the tests 🙂

[jacobheun]

@a1300 looks good! I ran the example as well and everything looks good there too. Thanks for the PR!