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

feat: added allowPublishToZeroPeers as optional param to publish function #395

Merged

Conversation

maschad
Copy link
Contributor

@maschad maschad commented Jan 8, 2023

Closes #367

The default for allowPublishToZeroPeers will be false but a user can optionally call with allowPublishToZeroPeers set to true to suppress InsufficientPeers errors

@maschad maschad requested a review from a team as a code owner January 8, 2023 21:34
src/index.ts Outdated
async publish(
topic: TopicStr,
data: Uint8Array,
publishOptions: Partial<GossipsubOpts> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Using GossipsubOpts here is extremely broad, just allow allowPublishToZeroPeers and nothing else. You don't need to default to false, that what will happen anyway

Copy link
Contributor Author

@maschad maschad Jan 9, 2023

Choose a reason for hiding this comment

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

Understood. I had made it more generic in case we may want to pass other params such as emitSelf in the future. I set the default to false just for readability to be more explicit.

@maschad maschad changed the title Added Publish config as optional params (#367) feat: added Publish config as optional params Jan 9, 2023
@maschad maschad changed the title feat: added Publish config as optional params feat: added allowPublishToZeroPeers as optional param Jan 9, 2023
src/index.ts Outdated
@@ -1995,7 +1995,7 @@ export class GossipSub extends EventEmitter<GossipsubEvents> implements PubSub<G
*
* For messages not from us, this class uses `forwardMessage`.
*/
async publish(topic: TopicStr, data: Uint8Array): Promise<PublishResult> {
async publish(topic: TopicStr, data: Uint8Array, allowPublishToZeroPeers = false): Promise<PublishResult> {
Copy link
Contributor

@dapplion dapplion Jan 9, 2023

Choose a reason for hiding this comment

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

Suggested change
async publish(topic: TopicStr, data: Uint8Array, allowPublishToZeroPeers = false): Promise<PublishResult> {
type PublishOpts {
allowPublishToZeroPeers?: boolean
}
async publish(topic: TopicStr, data: Uint8Array, opts?: PublishOpts): Promise<PublishResult> {
// Current publish opt takes precedence global opts, while preserving false value
const allowPublishToZeroPeers = opts?.allowPublishToZeroPeers ?? this.opts.allowPublishToZeroPeers;
...
if (
tosend.size === 0 &&
!allowPublishToZeroPeers
!willSendToSelf
) {
throw Error('PublishError.InsufficientPeers')
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the PublishOpts type to the types file.

@maschad maschad force-pushed the feat/add-optional-config-publish-function branch from 3c5d6ff to c4b1bea Compare January 9, 2023 03:19
@maschad maschad changed the title feat: added allowPublishToZeroPeers as optional param feat: added allowPublishToZeroPeers as optional param to publish function Jan 9, 2023
dapplion
dapplion previously approved these changes Jan 9, 2023
@maschad
Copy link
Contributor Author

maschad commented Jan 9, 2023

Added a test

@maschad maschad requested a review from dapplion January 9, 2023 13:47
@codecov-commenter
Copy link

codecov-commenter commented Jan 9, 2023

Codecov Report

Base: 83.41% // Head: 83.42% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (c4b1bea) compared to base (41740b5).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head c4b1bea differs from pull request most recent head d067b9a. Consider uploading reports for the commit d067b9a to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #395   +/-   ##
=======================================
  Coverage   83.41%   83.42%           
=======================================
  Files          48       48           
  Lines       11766    11771    +5     
  Branches     1266     1267    +1     
=======================================
+ Hits         9815     9820    +5     
  Misses       1951     1951           
Impacted Files Coverage Δ
src/index.ts 70.48% <100.00%> (+0.04%) ⬆️
src/types.ts 94.54% <100.00%> (+0.13%) ⬆️
test/gossip.spec.ts 96.31% <0.00%> (-0.06%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@maschad maschad force-pushed the feat/add-optional-config-publish-function branch from ce30c04 to d067b9a Compare January 12, 2023 17:55
Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

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

LGTM thanks @maschad

@wemeetagain wemeetagain merged commit e7c88ac into ChainSafe:master Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add per publish call allowPublishToZeroPeers option
4 participants