Skip to content
This repository was archived by the owner on Jun 26, 2023. It is now read-only.

Conversation

@wemeetagain
Copy link
Member

@wemeetagain wemeetagain commented Oct 11, 2020

See libp2p/specs#294 and libp2p/specs#299

Add a "global" signature policy to pubsub. This is a first-step towards having a "topic-specific" signature policy as described in the specs.
This "signature policy" replaces the ad-hoc options signMessages and strictSigning that were previously in use.

Only 'StrictSign' and 'StrictNoSign' signature policies are implemented, the 'LaxSign' and 'LaxNoSign' signature policies are not recommended and will be deprecated, so are not included.

BREAKING CHANGE:
signMessages and strictSigning pubsub configuration options replaced
with a globalSignaturePolicy option

BREAKING CHANGE:
`signMessages` and `strictSigning` pubsub configuration options replaced
with a `globalSignaturePolicy` option
@vasco-santos
Copy link
Member

@wemeetagain is this ready for review? If so, can you get the linting fixed?

@wemeetagain
Copy link
Member Author

@vasco-santos should be ready for a review

One thing that might be nice to add here, that isn't yet included:

  • Using strict no-sign right now requires explicitly passing in a different msgId function. It may be nice to provide a default in that case, something with an implementation like hash(msg.data)

Do you have a preference for which library/hash could/should be used in this case?

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Overall this looks good. Just needs some minor changes and docs/examples update.

Can you update https://github.com/libp2p/js-libp2p-interfaces/tree/master/src/pubsub#example to use the policy? Might be worth documenting the constructor params in the API docs as well.
As the end goal is to support topic based policies, I think we should have tests for it, as well as an example in the README for the applications

@vasco-santos
Copy link
Member

Using strict no-sign right now requires explicitly passing in a different msgId function. It may be nice to provide a default in that case, something with an implementation like hash(msg.data)

This is not included in go, right? We should leverage multiformats/js-multihashing-async

@wemeetagain
Copy link
Member Author

This is not included in go, right? We should leverage multiformats/js-multihashing-async

Yea i don't think the go impl includes default msg id functions.

Is there a reason why we need the async version? Somehow it seems that a message id would be something that is always synchronously retrieved.

@vasco-santos
Copy link
Member

vasco-santos commented Oct 27, 2020

Yea i don't think the go impl includes default msg id functions.

Ok, either way I think this is a nice addition!

Is there a reason why we need the async version? Somehow it seems that a message id would be something that is always synchronously retrieved.

Good point, we should use the base module then: https://github.com/multiformats/js-multihash

Furthermore, we should create an issue to track updating this to the topic level per the spec

@vasco-santos
Copy link
Member

@wemeetagain Can you get the lint job fixed?

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM

@wemeetagain
Copy link
Member Author

bump

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.

LGTM 🚢

@vasco-santos vasco-santos merged commit 946b046 into libp2p:master Nov 3, 2020
@wemeetagain wemeetagain deleted the feat/strict-no-sign branch November 3, 2020 17:30
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