feat: new IPLD Format API #51
Conversation
BREAKING CHANGE: The API is now async/await based There are numerous changes, the most significant one is that the API is no longer callback based, but it using async/await. For the full new API please see the [IPLD Formats spec]. [IPLD Formats spec]: https://github.com/ipld/interface-ipld-format
ghost
assigned 
Codecov Report
@@ Coverage Diff @@
## master #51 +/- ##
==========================================
+ Coverage 96.84% 99.37% +2.52%
==========================================
Files 22 20 -2
Lines 792 635 -157
==========================================
- Hits 767 631 -136
+ Misses 25 4 -21
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
This is all a bit too complex for me to give an explicit "Approve" to, I just don't understand the breadth of it so I'll leave it up to you. But here are my two general concerns:
- I think the heavy use of
Object.definePropertyis risky, and I've pointed to some problems I see already with the eth-account-snapshot stuff. If all of theseEth*objects are predictable in shape, maybe we should just be exposing new plain objects with copies of the data (or getters if the data is expensive to generate). Taking the raw objects and adjusting the properties is pretty messy. The original path-collection process seems a little safer, although probably unoptimal. - I'd remove all uses of
new Promise(), it's a good code smell indicator.promisify()is going to fix all of the instances in here in various ways; in fact, you're really just reinventing a simplisticpromisify()in most cases.
| value: account.isEmpty() | ||
| }) | ||
| const values = { | ||
| isEmpty: function () { return this.isEmpty() }, |
There was a problem hiding this comment.
These don't look right, this is going to be the object they are attached to so this.isEmpty() should result in a recursive call. It's set as a 'value' but it's just a function, so I think you're replacing the existing EthAccount#isEmpty() with a new recursive one. What's the intent here?
| 'codeHash', | ||
| 'nonce', | ||
| 'balance', | ||
| 'isEmpty', |
There was a problem hiding this comment.
Would it be a big deal to have resolve() calls for each of these to properly sanity check? I suspect we'll find these two is*() calls resolve to functions.
| cb(null, cid) | ||
| function populateTrie (trie) { | ||
| return new Promise((resolve, reject) => { | ||
| async.series([ |
There was a problem hiding this comment.
you could just let put = promisify(trie.put.bind(trie)) and then await put(...) of these calls, then async can be removed as a dependency
| return array.indexOf(item) !== -1 | ||
| function dumpTrieNonInlineNodes (trie, fullNodes) { | ||
| return new Promise((resolve) => { | ||
| trie._findDbNodes((nodeRef, node, key, next) => { |
There was a problem hiding this comment.
this should probably be promisified too, especially if the callback can return an error, in which case it's going to get turned into a return value via resolve
There was a problem hiding this comment.
I have no clue how to promisify this one. Can you help?
There was a problem hiding this comment.
Does _findDbNodes() resolve to something useful here or is the only purpose here to add the nodes to fullNodes?
There was a problem hiding this comment.
Regardless, I think it would be:
return promisify((...args) => trie._findDbNodes(...args))((n, node, k, next) => {
fullNodes.push(node)
next()
})Or, more readably:
let _find = promisify((...args) => trie._findDbNodes(...args))
let _iter = (n, node, k, next) => {
fullNodes.push(node)
next()
}
return _find(_iter)There was a problem hiding this comment.
Actually, to be more consistent with Rod’s earlier recommendation, you’d use promisify(trie._findDbNodes.bind(trie)) instead of promisify((...args) => trie._findDbNodes(...args)).
There was a problem hiding this comment.
Try this and see if it still works.
function dumpTrieNonInlineNodes (trie, fullNodes) {
return new Promise((resolve, reject) => {
trie._findDbNodes((nodeRef, node, key, next) => {
fullNodes[nibbleToPath(key)] = node
next()
}, err => { if (err) return reject(err); resolve() })
})
}If it fails with that, then the reason it was succeeding is because you were swallowing the error.
If this still works then let’s just move on because we’ve handled the primary concern and it’s not like we’re worried about the perf benefits of promisify since this is just a test.
There was a problem hiding this comment.
Works, not with promisify though ;) Thanks!
There was a problem hiding this comment.
Very strange, they should do the same thing, but I’m not that surprised that an API that is using a one-off continuation passing function inside a function that’s a parameter to a standard callback is behaving oddly ;)
There was a problem hiding this comment.
ok, sounds messy, forget my pickiness then and just make it work
There was a problem hiding this comment.
No worries. In the other case promisify-ing it really made sense. I found it interesting to see that there are cases where strange callback-based APIs can't easily be promisified.
| // context | ||
| const createCustomEthTrieNode = function (codec, leafResolver) { | ||
| return function (serialized) { | ||
| const rawNode = rlp.decode(serialized) |
There was a problem hiding this comment.
this block needs to be indented by another space
|
Closing this one in favour of #52. |
BREAKING CHANGE: The API is now async/await based
There are numerous changes, the most significant one is that the API
is no longer callback based, but it using async/await.
For the full new API please see the IPLD Formats spec.