-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Awesome Async Crypto + Less magic to 'run in the browser' #485
Conversation
awesome! :D remember: if > half a day :) |
b8c27be
to
fd22c82
Compare
@diasdavid ready for review |
@dignifiedquire will defer my review until there are good benchmarks. Thank you for pushing this! :) |
Doing more work on this, so not ready anymore ;) |
fd22c82
to
2d0c802
Compare
@diasdavid tests are passing locally for node, so you can start reviewing :) |
8a2688e
to
d6a7d98
Compare
Added new todo |
Should |
@nikuda yes it should :D |
Proto files buffer to string progress:
|
@nikuda awesome, thank you |
From IRC
Glad that I mentioned that several times :) Have you added CI tests to guarantee that interop remains? What were the tests you performed to verify? |
This interop are the tests you wanted to create scripts for. I just start a go and js node manually and connect them manually. There are a lot of tests to verify that we interop with go on libp2p-crypto already if you look at the tests there. But for the full integration test we need a full go-ipfs node from |
@dignifiedquire reviewed a bunch of this PR. ipfs-block needs some changes that will cascade to to the remaining PRs that I haven't finalized reviewing (i.e removed the 'needs review label') |
Important to handle before moving forward with this PR - libp2p/js-libp2p-crypto#18 |
Now that the changes have happened we need to update https://github.com/ipfs/community/blob/master/js-project-guidelines.md accordingly |
Unblocked with libp2p/js-libp2p-crypto#29 |
All dependencies were merged and released, doing the last updates now 🎉 |
@diasdavid can I get some last review on this? |
@dignifiedquire all CI is failing, is that what you are expecting? |
"libp2p-ipfs": "libp2p-ipfs-browser", | ||
"./src/core/default-repo.js": "./src/core/default-repo-browser.js", | ||
"./src/core/components/init-assets.js": false, | ||
"fs-pull-blob-store": false, |
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.
Needs to be reviewed
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.
need it as it is used in the tests, and does no harm otherwise
? argv.key | ||
: new Buffer(bs58.decode(argv.key)) | ||
: mh.fromB58String(argv.key) |
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.
Wait, does this still apply? we have standardised the block API
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.
yes, tests will fail otherwise
const mh = new Buffer(bs58.decode(argv.key)) | ||
|
||
ipfs.block.del(mh, (err) => { | ||
ipfs.block.del(mh.fromB58String(argv.key), (err) => { |
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.
block.del should support multihash as strings
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.
that's exactly what I'm doing here, decoding the multihash from a string
(cb) => utils.getIPFS(cb), | ||
(ipfs, cb) => ipfs.object.get(argv.key, {enc: 'base58'}, cb), | ||
(node, cb) => node.toJSON(cb) | ||
], (err, res) => { |
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.
s/res/nodeJSON
(cb) => utils.getIPFS(cb), | ||
(ipfs, cb) => ipfs.object.new(cb), | ||
(node, cb) => node.toJSON(cb) | ||
], (err, node) => { |
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.
s/node/nodeJSON
waterfall([ | ||
(cb) => ipfs.object.get(key, cb), | ||
(node, cb) => node.toJSON(cb) | ||
], (err, res) => { |
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.
s/res/nodeJSON
waterfall([ | ||
(cb) => ipfs.object.patch.setData(key, data, cb), | ||
(node, cb) => node.toJSON(cb) | ||
], (err, res) => { |
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.
s/res/nodeJSON/g
}, | ||
(link, cb) => ipfs.object.patch.addLink(root, link, cb), | ||
(node, cb) => node.toJSON(cb) | ||
], (err, res) => { |
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.
s/res/nodeJSON/g
waterfall([ | ||
(cb) => ipfs.object.patch.rmLink(root, link, cb), | ||
(node, cb) => node.toJSON(cb) | ||
], (err, res) => { |
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.
s/res/nodeJSON
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 overly verbose, and I really dislike all caps naming. Not sure about a better name
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.
well, toJSON is also JSON all caps
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.
because apis in JavaScript are note very consistent when it comes to naming :/
require('./test-files') | ||
require('./test-generic') | ||
require('./test-init') | ||
require('./test-object') |
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.
👍
yes, waiting for the bitswap PR I pinged you about |
@dignifiedquire there you go ipfs/js-ipfs-bitswap#67 |
CI is finally green, browsers are not perfect atm, most are failing due to one or the other issue Firefox fails with this
though this code runs fine in Chrome. @diasdavid if you are happy with the code please merge and ship at your earliest convenience, and I will investigate and fix various browser related issues next week. |
🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 |
* chore(package): update aegir to version 19.0.3 Closes ipfs#480 * fix: appease linter License: MIT Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
These are the changes that needed to bubble up from deep down in libp2p-crypto and
multihashing
.This reduces the size of the browser build down to
2.4M
and1.2M
unminified and minified respectively.Associated PRs that need to be merged and shipped before this can get merged are
TODOs
multihashing-async
instead ofmultihashing