fix!: attach ipld view instead of single block #303
Conversation
BREAKING CHANGE: delegation.attach() and invocation.attach() now receive IPLDView instead of Block
Gozala
left a comment
Gozala
left a comment
There was a problem hiding this comment.
Thanks for tackling this. Unfortunately I'm no longer sure this is a good idea and maybe leaving it at the block level now is a better option. This is why
- Right now capabilities & facts are not fully traversed implying that after transporting the delegation (transitive) nodes, linked from the attachment will be lost, unless they were explicitly linked from the
capability.nborfacts.- Although
attachright now throws if transitive blocks aren't linked, but then it's not clear to me if there is any benefit to just allowing only a block.
- Although
What I was picturing when suggesting this was that capabilities and facts would only link to the attachment roots. That however would require deep traversal of the capabilities and facts as opposed to shallow one implemented now.
In other words I think we should either do full traversal, otherwise this going to be more error prone than helpful. However I could be missing something so maybe we can talk sync about this.
| attach(view) { | ||
| for (const block of view.iterateIPLDBlocks()) { | ||
| if (!this.attachedLinks.has(`${block.cid.link()}`)) { | ||
| throw new Error(`given block with ${block.cid} is not an attached link`) |
There was a problem hiding this comment.
I think should only require that root is listed in the attachments, all the other blocks will be linked somewhere from that root.
| for (const block of attachedBlocks.values()) { | ||
| delegation.attach(block) | ||
| } | ||
| const attached = Array.from(attachedBlocks.values()) |
There was a problem hiding this comment.
I find this very confusing, can we have attachments iterable as an argument instead ? That way we'll simply:
for (const view of attachments) {
delegation.attach(view)
}| attach(block) { | ||
| this.attachedBlocks.set(`${block.cid}`, block) | ||
| attach(view) { | ||
| for (const block of view.iterateIPLDBlocks()) { |
There was a problem hiding this comment.
I think it would be a lot cleaner if we stick blocks into this.blocks and instead have a set of links to the attachment roots.
|
@Gozala we are now doing full traversal of capabilities. Also checking facts as an entry in array, or value of an object. I guess, we only then need to do a full traversal of facts? |
Attach view instead of single block per #298 (comment)
BREAKING CHANGE: delegation.attach() and invocation.attach() now receive IPLDView instead of Block