-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Conversation
f2bc0e0
to
4fa279c
Compare
ad978f0
to
a45a814
Compare
@@ -70,6 +70,8 @@ class IPFS { | |||
}) | |||
const dns = createDNSAPI() | |||
const isOnline = createIsOnlineAPI({ network }) | |||
// @ts-ignore This type check fails as options. | |||
// libp2p can be a function, while IPNS router config expects libp2p config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not an easy fix. I recommend we do it individually
const exportKey = (name, password, options) => | ||
keychain.exportKey(name, password, options) | ||
const exportKey = (name, password) => | ||
keychain.exportKey(name, password) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not support Aborts/options in keychain.
@@ -41,6 +41,7 @@ module.exports = ({ network, config }) => { | |||
*/ | |||
async function subscribe (topic, handler, options) { | |||
const { libp2p } = await network.use(options) | |||
// @ts-ignore Libp2p Pubsub is deprecating the handler, using the EventEmitter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point we might get https://github.com/libp2p/js-libp2p/blob/0.30.x/src/pubsub-adapter.js this out. I can add in a follow up PR the handlers behaviour for IPFS
@@ -111,7 +113,7 @@ module.exports = ({ network, config }) => { | |||
*/ | |||
async function ls (options) { | |||
const { libp2p } = await network.use(options) | |||
return libp2p.pubsub.getTopics(options) | |||
return libp2p.pubsub.getTopics() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pubsub has no options for getTopics + getSubscribers
@@ -43,6 +43,8 @@ class Storage { | |||
|
|||
const { peerId, keychain, isNew } = await loadRepo(repo, options) | |||
|
|||
// TODO: throw error? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keychain is not available in init IIRC. That is why it might exist or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there was / is this code:
js-ipfs/packages/ipfs-core/src/components/storage.js
Lines 137 to 156 in 2bafc88
const libp2p = createLibP2P({ | |
options: undefined, | |
multiaddrs: undefined, | |
peerId, | |
repo, | |
config, | |
keychainConfig: { | |
pass: options.pass | |
} | |
}) | |
if (libp2p.keychain && libp2p.keychain.opts) { | |
await libp2p.loadKeychain() | |
await repo.config.set('Keychain', { | |
dek: libp2p.keychain.opts.dek | |
}) | |
} | |
return { peerId, keychain: libp2p.keychain } |
And this code path
js-ipfs/packages/ipfs-core/src/components/storage.js
Lines 214 to 230 in 2bafc88
const libp2p = createLibP2P({ | |
options: undefined, | |
multiaddrs: undefined, | |
peerId, | |
repo, | |
config: changed, | |
keychainConfig: { | |
pass, | |
...changed.Keychain | |
} | |
}) | |
if (libp2p.keychain) { | |
await libp2p.loadKeychain() | |
} | |
return { peerId, keychain: libp2p.keychain } |
And I'm still not sure why libp2p is created in two different ways to be honest. It would be good to get these cleaned up (in some followup). I'd also happily pair on this as lack of uncertainty here really bothered me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you on this, it also confused me. We should improve this on a followup
40a6ac5
to
95499e7
Compare
Co-authored-by: Hugo Dias <[email protected]>
2fc5b0c
to
c487184
Compare
c487184
to
2bafc88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull @vasco-santos, looks good to me.
@@ -43,6 +43,8 @@ class Storage { | |||
|
|||
const { peerId, keychain, isNew } = await loadRepo(repo, options) | |||
|
|||
// TODO: throw error? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there was / is this code:
js-ipfs/packages/ipfs-core/src/components/storage.js
Lines 137 to 156 in 2bafc88
const libp2p = createLibP2P({ | |
options: undefined, | |
multiaddrs: undefined, | |
peerId, | |
repo, | |
config, | |
keychainConfig: { | |
pass: options.pass | |
} | |
}) | |
if (libp2p.keychain && libp2p.keychain.opts) { | |
await libp2p.loadKeychain() | |
await repo.config.set('Keychain', { | |
dek: libp2p.keychain.opts.dek | |
}) | |
} | |
return { peerId, keychain: libp2p.keychain } |
And this code path
js-ipfs/packages/ipfs-core/src/components/storage.js
Lines 214 to 230 in 2bafc88
const libp2p = createLibP2P({ | |
options: undefined, | |
multiaddrs: undefined, | |
peerId, | |
repo, | |
config: changed, | |
keychainConfig: { | |
pass, | |
...changed.Keychain | |
} | |
}) | |
if (libp2p.keychain) { | |
await libp2p.loadKeychain() | |
} | |
return { peerId, keychain: libp2p.keychain } |
And I'm still not sure why libp2p is created in two different ways to be honest. It would be good to get these cleaned up (in some followup). I'd also happily pair on this as lack of uncertainty here really bothered me.
@@ -43,6 +43,8 @@ class Storage { | |||
|
|||
const { peerId, keychain, isNew } = await loadRepo(repo, options) | |||
|
|||
// TODO: throw error? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like throw is not a great option here, if we keychain does not exists we should probably create it ? Otherwise I don't see how user is supposed to use this without getting an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, Keychain will always exist at this point as this is only used after start. The problem is that the types are not sure about it.
@@ -16,7 +16,7 @@ module.exports = ({ network }) => { | |||
*/ | |||
async function disconnect (addr, options) { | |||
const { libp2p } = await network.use(options) | |||
return libp2p.hangUp(addr, options) | |||
return libp2p.hangUp(addr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to do the same thing you did with dial
return libp2p.hangUp(addr) | |
await libp2p.hangUp(addr) |
const filters = require('libp2p-websockets/src/filters') | ||
const transportKey = WS.prototype[Symbol.toStringTag] | ||
|
||
module.exports = () => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be just an object since it doesn't take any input, it would reduce LoC in the others files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason I created a function here is to enable the options to be customized from the caller without modifying the actual content thanks to the new reference. We had issues in the past in libp2p tests, where its base configuration used in tests was modified in some tests, which caused other tests to have different configurations than expected required.
We currently only use it directly and I can turn that into an object. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good maybe freeze it to be safe if it makes sense.
Integrates
[email protected]
release.This PR changes needed types accordingly.
Needs:
A note should be included in the next release notes regarding the
websockets
update to anticipate possible problems for local environment tests of IPFS users