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

Publishing option for signing a message with a custom private key #486

Merged
merged 7 commits into from May 26, 2022

Conversation

iulianpascalau
Copy link
Contributor

@iulianpascalau iulianpascalau commented May 25, 2022

  • added publishing option for signing a message with a custom private key

Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

This looks fine, but would it make more sense to introduce a topic option WithPublisher and using that?

topic.go Outdated
// (which can be cached) will improve the performance as it does not require the generation of the public key
// and the resulting peer ID.
// This method can be used as to be able to publish messages on the network without having a "real", connectable host.
func (t *Topic) PublishWithSk(ctx context.Context, data []byte, signKey crypto.PrivKey, pid peer.ID, opts ...PubOpt) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's not abbreviate the Sk and say PublishWithKey or PublishWithSecretKey.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will refactor

@iulianpascalau
Copy link
Contributor Author

This looks fine, but would it make more sense to introduce a topic option WithPublisher and using that?

I will explore that. I was on the verge of pushing a new commit for code duplication removal.

@iulianpascalau iulianpascalau changed the title PublishWithSk functionality on the Topic implementation Publishing options for signing a message with a custom private key May 25, 2022
@iulianpascalau iulianpascalau changed the title Publishing options for signing a message with a custom private key Publishing option for signing a message with a custom private key May 25, 2022
Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

there are a couple of bugs, but otherwise looks good.

topic.go Outdated Show resolved Hide resolved
topic.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

just some final nits and it is ready to merge.

topic_test.go Outdated
@@ -920,3 +921,101 @@ func TestWithTopicMsgIdFunction(t *testing.T) {
t.Fatal("msg ids are equal")
}
}

func TestTopic_PublishWithKeyInvalidParameters(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we remove the underscore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the Elrond pattern used in tests :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

topic_test.go Outdated
})
}

func TestTopicRelay_PublishWithKey(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

here too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

thank you!

@vyzo vyzo merged commit 0ea9140 into libp2p:master May 26, 2022
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.

None yet

2 participants