Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

RFC: perf: use peer-id-lite #1007

Closed
wants to merge 1 commit into from
Closed

RFC: perf: use peer-id-lite #1007

wants to merge 1 commit into from

Conversation

alanshaw
Copy link
Contributor

@alanshaw alanshaw commented May 17, 2019

peer-id-lite removes the libp2p-crypto dependency to reduce the total bundle size significantly.

peer-id-lite will export window.PeerId if available (in browser) or in Node it'll try require('peer-id'). It allows people to bring their own full fat peer-id and fallback to the lite version if not.

PASS  ./dist/index.min.js: 133.94KB < maxSize 232KB (gzip)(97.17KB smaller than master, good job!) 

What does this mean?

  1. Exported PeerId cannot be used to create a PeerId instance with a public and/or private key.
  2. Exported PeerId.create will always fail with error
  3. Exported PeerId.createFromPubKey will always fail with error
  4. Exported PeerId.createFromPrivKey will always fail with error
  5. Exported PeerId.createFromJSON will fail if passed object has privKey or pubKey properties

Arguably, we should stop exporting PeerId because the instance we're exporting isn't real.

Right now, in terms of actually using PeerId lite instances returned by ipfs-http-client, the only issue you'll come across is:

  1. You cannot set privKey or pubKey on PeerId lite instances

Possibly un-obvious behaviour of PeerId lite:

  1. marshalPubKey and marshalPrivKey will always return undefined because an instance can not have a public or private key - this is no different to the behaviour of the real implementation when an instance has no public/private key
  2. Object returned from toJSON will not have privKey or pubKey because an instance can not have a public or private key - this is no different to the behaviour of the real implementation when an instance has no public/private key

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@alanshaw alanshaw changed the title perf: use peer-id-lite RFC: perf: use peer-id-lite May 17, 2019
@jacobheun
Copy link
Contributor

Did you check if there was a significant impact on cli command run time with js-ipfs? I'd assume the reduced bundle size would reduce the require time node needs before executing daemon commands.

@alanshaw
Copy link
Contributor Author

No I didn't but yeah that's likely to be another benefit!

@dirkmc
Copy link
Contributor

dirkmc commented May 20, 2019

As Eddie Izzard would say that's awesome 👍 Seems like a big win in terms of bundle size and also performance, as Jacob mentioned. Given those caveats about peer-id, I wonder if it should be an option rather than the default?

@vasco-santos
Copy link
Contributor

Awesome @alanshaw ! This can be really beneficial for several browser use cases.

My main concern here is a good way of providing documentation around this to the users that may want the regular version of peer-id. And also, explain users the advantages and disadvantages of each version.

I think this should be an optimization that advanced users should be able to do. As a result, I think an approach with an option rather than default could work better.

@hugomrdias
Copy link
Contributor

IMO we should look at a simpler solution, this looks like the type of thing that would generate lots of issues.
This "simpler solution" could be harder and take more time to implement but in the long run should be better.

So looking a the code:

  • peer-info doesn't really need peer-id just mh.fromB58String(str) and mh.toB58String(this.id)
  • all the other files changed in this PR same as above
  • about mh.toB58String(this.id) the this.id part is also just a multihash buffer from the public key which is strange cause peer-id also accepts a public key as the third argument
  • we can get way with just multihash in most places
  • regarding the public key and peer-id i have seem lots places (ie. ipns) where its a weird dance between buffers and instances back and forth.

We should try to look at this in a wider way to identify where we can just use multihash and where we really need crypto and maybe create new abstractions that make more sense.

@alanshaw
Copy link
Contributor Author

Closing this in favour of removing internal classes entirely in the API client.

@alanshaw alanshaw closed this Jul 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants