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: integrate libp2p-keychain into js-libp2p (#633) #634

Merged
merged 2 commits into from
May 13, 2020

Conversation

vasco-santos
Copy link
Member

Integrates the libp2p-keychain codebase into libp2p.keychain.

This is the second part of decoupling #631 and replaces it.

@vasco-santos vasco-santos force-pushed the feat/libp2p-keychain branch 2 times, most recently from 8128e6f to f8a80e8 Compare May 13, 2020 07:10
'use strict'

module.exports = require('./keychain')
Copy link
Member Author

Choose a reason for hiding this comment

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

content of src/keychain/keychain was moved here to be consistent with the rest of the libp2p codebase

Integrates the libp2p-keychain codebase into this repo
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.

Just a minor thing around the default configuration, but otherwise this looks good.

doc/CONFIGURATION.md Outdated Show resolved Hide resolved
src/config.js Outdated
@@ -17,6 +18,9 @@ const DefaultConfig = {
maxDialsPerPeer: Constants.MAX_PER_PEER_DIALS,
dialTimeout: Constants.DIAL_TIMEOUT
},
keychain: {
datastore: new MemoryDatastore()
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to cause us to carry around the MemoryDatastore even if we don't use the keychain. I think we should just default this to null and check the datastore before creating the keychain. There's no real value in a memory datastore for the keychain.

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right! just removed it and added a validation. In the docs it is already defined as required

Co-authored-by: Jacob Heun <jacobheun@gmail.com>
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

@jacobheun jacobheun merged commit 171af8a into 0.28.x May 13, 2020
@jacobheun jacobheun deleted the feat/libp2p-keychain branch May 13, 2020 12:53
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.

2 participants