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

chore: use ed25519 keys in tests #669

Merged
merged 5 commits into from
Oct 7, 2020
Merged

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented Jun 12, 2020

@jacobheun jacobheun added the status/blocked Unable to be worked further until needs are met label Jul 2, 2020
@jacobheun
Copy link
Contributor

This should now be unblocked in master.

@vasco-santos
Copy link
Member Author

I will work on rebase this and get it finished

@vasco-santos vasco-santos force-pushed the chore/use-ed25519-keys-in-tests branch from 5abf287 to 5580435 Compare August 6, 2020 09:29
@vasco-santos
Copy link
Member Author

There were some tests failing now, due to the persistent peer store, more precisely as a consequence of the following lines:

With ed25519 keys, if we create a peer from the CID, we can get its pubKey through the peerId.pubKey getter. As a consequence, we will have always pubKey. The cleanest solution here is to get from the keyBook again in: https://github.com/libp2p/js-libp2p/blob/master/src/peer-store/persistent/index.js#L213

@vasco-santos
Copy link
Member Author

On another note, this PR changes all the tests that use our fixtures (the majority of the tests atm) to use ed25519 keys. For the remaining ones, the easiest solution is to change the peer-id create to use ed25519 as a breaking change release. We don't need to do it now, as well, but I think it is easier (and perhaps more correct for future changes) to do that than manually change all the tests do use it instead of RSA

@vasco-santos vasco-santos force-pushed the chore/use-ed25519-keys-in-tests branch from d9ff73b to 9b4b655 Compare August 6, 2020 09:43
@vasco-santos vasco-santos marked this pull request as ready for review August 6, 2020 09:46
@jacobheun
Copy link
Contributor

With ed25519 keys, if we create a peer from the CID, we can get its pubKey through the peerId.pubKey getter. As a consequence, we will have always pubKey. The cleanest solution here is to get from the keyBook again in

For inlined public keys there's no reason to store their public key, we should avoid doing this it's just a waste of space/datastore transactions. We may want to add an easy way of checking this in peer-id. Currently only rsa keys aren't inline.

@vasco-santos vasco-santos added status/ready Ready to be worked and removed status/blocked Unable to be worked further until needs are met labels Sep 23, 2020
@vasco-santos
Copy link
Member Author

Inline public keys check was added and tests updated accordingly

@vasco-santos vasco-santos requested review from jacobheun and removed request for jacobheun September 29, 2020 08:45
@@ -364,7 +364,7 @@ describe('Identify', () => {
expect(libp2p.identifyService.identify.callCount).to.equal(1)

// The connection should have no open streams
expect(connection.streams).to.have.length(0)
await pWaitFor(() => connection.streams.length === 0)
Copy link
Member Author

@vasco-santos vasco-santos Sep 29, 2020

Choose a reason for hiding this comment

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

This was added to prevent a flakey test in Firefox where sometimes the stream was still closing at this moment

@jacobheun jacobheun merged commit 0d48fc4 into master Oct 7, 2020
@jacobheun jacobheun deleted the chore/use-ed25519-keys-in-tests branch October 7, 2020 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/ready Ready to be worked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants