-
Notifications
You must be signed in to change notification settings - Fork 47
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
Bitswap Complete (1.0.0 + 1.1.0) #45
Conversation
daviddias
commented
Oct 20, 2016
•
edited
Loading
edited
- Needs to support both bitswap 1.1.0 and 1.0.0
- Needs js-multistream to support custom validation functions (mainly semver)
- New Bitswap message.proto (supports both old and new)
- https://github.com/ipfs/go-ipfs/blob/master/exchange/bitswap/message/pb/message.proto
|
||
module.exports = Bitswap | ||
|
||
function noop () {} |
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.
didn't we agree that we would stop doing style refactorings that are based on personal preferences?
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 might be wrong, but I don't see this as a style preference. Seeing cb = cb || noop
is more simpler than cb = cb || (() => {})
everytime I want a noop.
I can revert back though.
As I said at the top of the PR: "Nothing to see here", which was meant to save time from CR, this is far from complete (note that I didn't add the "needs review" label).
addBlock (block) { | ||
this.blocks.set(mh.toB58String(block.key), block) | ||
addBlock (block, hashAlg) { | ||
const key = block.key(hashAlg) |
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 going to be async with the new crypto. so might be better to base it on top of that
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.
It would be incredibly hard to work on top of a PR of PRs and do PR of PRs. One giant at a time, that is why we have git :)
const mainBlob = new Store(id) | ||
const blocksBlob = new Store(`${id}/blocks`) | ||
const mainBlob = new Store(id + Math.random()) | ||
const blocksBlob = new Store(`${id}/blocks` + Math.random()) |
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.
why?
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 was due to ipfs/js-ipfs#528 (comment)
@@ -46,7 +46,7 @@ module.exports = (repo) => { | |||
}) | |||
|
|||
describe('receive message', () => { | |||
it('simple block message', (done) => { | |||
it.only('simple block message', (done) => { |
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.
TODO: remove before merge
...someone needs to pay better attention to headlines *cough Just ignore me till you are ready :) |