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: update peer-id #173

Merged
merged 3 commits into from
Dec 2, 2021
Merged

Conversation

achingbrain
Copy link
Collaborator

@achingbrain achingbrain commented Dec 2, 2021

Pulls in the new peer-id

Needs a new release of @chainsafe/libp2p-noise though it's just a dev dep so this could go out without it.

BREAKING CHANGE: requires node 15+

@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2021

Codecov Report

Merging #173 (42d8546) into master (751ea73) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #173   +/-   ##
=======================================
  Coverage   76.58%   76.58%           
=======================================
  Files           2        2           
  Lines        1905     1905           
  Branches      144      144           
=======================================
  Hits         1459     1459           
  Misses        446      446           
Impacted Files Coverage Δ
test/utils/index.js 87.50% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d7b2aa...42d8546. Read the comment docs.

@dapplion
Copy link
Contributor

dapplion commented Dec 2, 2021

@achingbrain what breaks with Node < v16?

@achingbrain
Copy link
Collaborator Author

Node only used to be able to give us keypairs in pem format, which we'd then use pem-jwk to transform to jwk - starting with node 15 it can give us the keys in jwk directly so we don't need the extra dep any more.

https://github.com/libp2p/js-libp2p-crypto/blob/master/src/keys/rsa.js#L14-L15

@achingbrain
Copy link
Collaborator Author

Same with signing/verifying using RSA, we used to have to transform the key format before hand, now we can just pass it to node crypto directly.

@dapplion
Copy link
Contributor

dapplion commented Dec 2, 2021

Node only used to be able to give us keypairs in pem format, which we'd then use pem-jwk to transform to jwk - starting with node 15 it can give us the keys in jwk directly so we don't need the extra dep any more.

https://github.com/libp2p/js-libp2p-crypto/blob/master/src/keys/rsa.js#L14-L15

Is it feasible to have a fallback for Node v14? I haven't though in depth our node version support strategy but if the cost is low it's a good to have.

@achingbrain
Copy link
Collaborator Author

It means adding the pem-jwk dep back in and not using the features from node 15. Node 16 is Active LTS now though, so that's the minimum version we support, officially.

@dapplion
Copy link
Contributor

dapplion commented Dec 2, 2021

It means adding the pem-jwk dep back in and not using the features from node 15. Node 16 is Active LTS now though, so that's the minimum version we support, officially.

Agree with your policy, let's stick to v16 then

dapplion
dapplion previously approved these changes Dec 2, 2021
achingbrain added a commit to achingbrain/gossipsub-js that referenced this pull request Dec 2, 2021
There's no need to use node buffers, everything is `Uint8Array`
compatible now.

Needs the `libp2p-interfaces-compliance-tests` upgrade from
ChainSafe#173 before
CI will pass.
Pulls in the new peer-id

Needs a new release of `libp2p-floodsub` and `@chainsafe/libp2p-noise`
though they are just dev deps so this could go out without them.

BREAKING CHANGE: requires node 15+
@achingbrain
Copy link
Collaborator Author

Phew, CI passing. I think long term all the delay(...)s need to be removed from the tests, some of the values are too aggressive for resource constrained environments like GH actions.

await connectGossipsubs(nodes)
// await subscription propagation
await delay(50)
await waitForAllNodesToBePeered(nodes)
Copy link
Collaborator

Choose a reason for hiding this comment

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

awesome

Copy link
Collaborator

@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

@vasco-santos vasco-santos merged commit e61668e into ChainSafe:master Dec 2, 2021
achingbrain added a commit to achingbrain/gossipsub-js that referenced this pull request Dec 2, 2021
There's no need to use node buffers, everything is `Uint8Array`
compatible now.

Needs the `libp2p-interfaces-compliance-tests` upgrade from
ChainSafe#173 before
CI will pass.
vasco-santos pushed a commit that referenced this pull request Dec 3, 2021
There's no need to use node buffers, everything is `Uint8Array`
compatible now.

Needs the `libp2p-interfaces-compliance-tests` upgrade from
#173 before
CI will pass
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.

4 participants