Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make pubsub option emitSelf configurable #401

Closed
a1300 opened this issue Aug 10, 2019 · 3 comments · May be fixed by adamlaska/ipfs-js-ipfs#3
Closed

Make pubsub option emitSelf configurable #401

a1300 opened this issue Aug 10, 2019 · 3 comments · May be fixed by adamlaska/ipfs-js-ipfs#3
Assignees
Labels
exp/novice Someone with a little familiarity can pick up kind/bug A bug in existing code (including security flaws) P2 Medium: Good to have, but can wait until someone steps up status/ready Ready to be worked

Comments

@a1300
Copy link
Contributor

a1300 commented Aug 10, 2019

First and foremost: Thanks for this great package and the hard work! 👍


  • Version: 0.26.0
  • Platform: Linux Kubuntu-18 4.18.0-25-generic
  • Subsystem:

Type: Feature

Severity: Low

Description:

Currently is the option emitSelf for pubsub packages (libp2p/js-libp2p-floodsub and ChainSafe/gossipsub-js) hardcoded in the code.

const pubsub = new Pubsub(node, { emitSelf: true })

Both pubsub packages support the emitSelf option. It would be nice to add the option to specify if self-published messages should be received.

Example:

export class Bundle extends libp2p {
  constructor(_options) {
    const defaults = {
      modules: {
        transport: [TCP],
        streamMuxer: [Mplex],
        connEncryption: [SECIO],
        peerDiscovery: [Bootstrap],
        dht: DHT,
        pubsub: GossipSub,
      },
      config: {
        pubsub: {
          enabled: true,
+         emitSelf: false,
        },
      },
    };
    super(defaultsDeep(_options, defaults));
  }

Steps to reproduce the error:

@a1300
Copy link
Contributor Author

a1300 commented Aug 10, 2019

If this feature request is granted I would like to have this issue assigned to me 🙂

@jacobheun
Copy link
Contributor

Yes, we should pass the configuration into the Pubsub initialization. We should account for other Pubsub options as well when doing this, such as the strict signing and verification params https://github.com/libp2p/js-libp2p-pubsub/blob/v0.2.0/src/index.js#L30-L31. I'd expect the whole pubsub config object to get passed.

@jacobheun jacobheun added kind/bug A bug in existing code (including security flaws) exp/novice Someone with a little familiarity can pick up P2 Medium: Good to have, but can wait until someone steps up status/ready Ready to be worked labels Aug 12, 2019
jacobheun pushed a commit that referenced this issue Aug 19, 2019
* fix: add pubsub default config (#401)

License: MIT
Signed-off-by: Matthias Knopp <matthias-knopp@gmx.net>

* docs: add default pubsub config to README (#401)

License: MIT
Signed-off-by: Matthias Knopp <matthias-knopp@gmx.net>

* fix: pass config to provided PubSub (#401)

License: MIT
Signed-off-by: Matthias Knopp <matthias-knopp@gmx.net>

* docs: adapt pubsub/example for new config (#401)

License: MIT
Signed-off-by: Matthias Knopp <matthias-knopp@gmx.net>

* Update examples/pubsub/README.md

Co-Authored-By: Jacob Heun <jacobheun@gmail.com>

* test: add pubsub config tests (#401)

License: MIT
Signed-off-by: Matthias Knopp <matthias-knopp@gmx.net>
@jacobheun
Copy link
Contributor

Fixed in #404

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/novice Someone with a little familiarity can pick up kind/bug A bug in existing code (including security flaws) P2 Medium: Good to have, but can wait until someone steps up status/ready Ready to be worked
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants