Enable access to root hashes#151
Conversation
ghost
mentioned this pull request
|
Since this is very different than the other verify method I'd much prefer to add thing under a new method called |
|
Looks great otherwise |
|
@mafintosh it's done |
test/basic.js
Outdated
|
|
||
| tape('verify', function (t) { | ||
| t.plan(9) | ||
| t.plan(4) |
There was a problem hiding this comment.
hey @denim2x this change looks like a leftover error from previous work ^_^ I think this should say 9 here?
I noticed this because it's me who wrote the old code that got changed.
|
|
||
| evilfeed.commend(0, function (err, roots) { | ||
| t.ok(err === null, 'no error') | ||
| t.ok(roots instanceof Array) |
There was a problem hiding this comment.
This is good test coverage. A nitpick would be that the test doesn't check that evilfeed and feed have different root hashes (the point of getting root hashes is to ensure matching content is the same and differing content is different without having to compare all the content).
It would be good to check evilfeed after the roots of feed are returned, and assert that the two have the same number of hashes but that those hashes are different.
This is so that if somebody accidentally changes the code such that the function no longer has the proper behavior, the test suite will catch them.
README.md
Outdated
| Callback is called with `(err, success)` where success is true only if the signature is | ||
| correct. | ||
|
|
||
| #### `feed.commend(index, callback)` |
There was a problem hiding this comment.
I'm not sure what 'commend' means personally =S I might call the function 'rootHashes' if nobody else cares !
There was a problem hiding this comment.
Yes I was just about to comment that, rootHashes is better. I had to lookup commend in the dictionary.
|
@denim2x I noticed a bug on line 57 |
|
@denim2x LGTM on @xloem's comments, sorry for being busy. |
|
thanks for the feedback |
index.js
Outdated
| } | ||
|
|
||
| Feed.prototype.rootHashes = function (index, cb) { | ||
| this._withRoots(index, function (err, roots) { |
There was a problem hiding this comment.
quick question, why is this not just passing index, cb directly?
index.js
Outdated
|
|
||
| Feed.prototype.rootHashes = function (index, cb) { | ||
| this._withRoots(index, function (err, roots) { | ||
| if (err) { cb(err) } else { cb(null, roots) } |
There was a problem hiding this comment.
if (err) return cb(err)
cb(null, roots)instead, assuming this closure is necessary (see above q)
test/basic.js
Outdated
| t.equal(result[0].length, result[1].length) | ||
| t.notEqual(Buffer.compare(result[0][0].hash, result[1][0].hash), 0) | ||
| } | ||
| }) |
There was a problem hiding this comment.
this is a bit confusing to read. why not just name the function (ev) here and call it after doing result.push below? also removes the need to an additional dep.
var results = []
feed.rootHashes(0, onroot)
evilfeed.rootHashes(0, onroot)
function onroots (err, roots) {
t.error(err, 'no error')
t.ok(roots instanceof Array)
result.push(roots)
if (result.length < 2) return
t.notEqual(result[0], result[1])
t.equal(result[0].length, result[1].length)
t.notEqual(Buffer.compare(result[0][0].hash, result[1][0].hash), 0)
}
|
fixed it |
|
Nice! @denim2x is this an issue? https://github.com/mafintosh/hypercore/pull/151/files#r185249278 other than that LGTM from me 🎉 |
|
@mafintosh doubtful - since the number assertions in |
|
@denim2x hmm, you sure? I count 9 assertions in verify still |
|
i’d imagine the test could be terminating early because 4 assertions are planned for in verify, even though 9 are present. it would still appear to pass but should be fixed. |
|
@mafintosh that's right (I misunderstood @xloem's comment) |
index.js
Outdated
| var self = this | ||
|
|
||
| this._getRootsToVerify(index * 2 + 2, {}, [], function (err, roots) { | ||
| this._withRoots(function (err, roots) { |
There was a problem hiding this comment.
Isn't this missing index? a bit confused actually
|
@denim2x I'm sorry but that failure indicates your changes have broken the verify function. It's important to keep the tests all active; they indicate when something is broken ! |
Root hashes are accessible via Feed::rootHashes Signed-off-by: denim2x <denim2x@cyberdude.com>
|
the call to |
|
merrrrged |
|
6.14.0 🎉 |
2 participants
Root hashes can be accessed via Feed::rootHashes; fixes #142