Skip to content
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

fix!: attach ipld view instead of single block #303

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions packages/core/src/delegation.js
Original file line number Diff line number Diff line change
Expand Up @@ -228,18 +228,20 @@ export class Delegation {
return data
}
/**
* Attach a block to the delegation DAG so it would be included in the
* block iterator.
* Attach blocks from IPLD view to the delegation DAG so it would be included
* in the block iterator.
* ⚠️ You can only attach blocks that are referenced from the `capabilities`
* or `facts`.
*
* @param {API.Block} block
* @param {API.IPLDView} view
*/
attach(block) {
if (!this.attachedLinks.has(`${block.cid.link()}`)) {
throw new Error(`given block with ${block.cid} is not an attached link`)
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`)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think should only require that root is listed in the attachments, all the other blocks will be linked somewhere from that root.

}
this.blocks.set(`${block.cid}`, block)
}
this.blocks.set(`${block.cid}`, block)
}
export() {
return exportDAG(this.root, this.blocks, this.attachedLinks)
Expand Down Expand Up @@ -407,9 +409,11 @@ export const delegate = async (
const delegation = new Delegation({ cid, bytes }, blocks)
Object.defineProperties(delegation, { proofs: { value: proofs } })

for (const block of attachedBlocks.values()) {
delegation.attach(block)
}
const attached = Array.from(attachedBlocks.values())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}

attached.length && delegation.attach({
iterateIPLDBlocks: () => attachedBlocks.values(),
root: attached[0]
})

return delegation
}
Expand Down
15 changes: 8 additions & 7 deletions packages/core/src/invocation.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,16 +87,17 @@ class IssuedInvocation {
}

/**
* Attach a block to the invocation DAG so it would be included in the
* block iterator.
* Attach block from IPLD View to the invocation DAG so it would be included
* in the block iterator.
* ⚠️ You should only attach blocks that are referenced from the `capabilities`
* or `facts`, if that is not the case you probably should reconsider.
* ⚠️ Once a delegation is de-serialized the attached blocks will not be re-attached.
* or `facts`.
*
* @param {API.Block} block
* @param {API.IPLDView} view
*/
attach(block) {
this.attachedBlocks.set(`${block.cid}`, block)
attach(view) {
for (const block of view.iterateIPLDBlocks()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.attachedBlocks.set(`${block.cid}`, block)
}
}

delegate() {
Expand Down
15 changes: 12 additions & 3 deletions packages/core/test/delegation.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,10 @@ test('delegation.attach block in capabiliy', async () => {
],
})

ucan.attach(block)
ucan.attach({
iterateIPLDBlocks: () => [block].values(),
root: block
})

const delegationBlocks = []
for (const b of ucan.iterateIPLDBlocks()) {
Expand Down Expand Up @@ -334,7 +337,10 @@ test('delegation.attach block in facts', async () => {
]
})

ucan.attach(block)
ucan.attach({
iterateIPLDBlocks: () => [block].values(),
root: block
})

const delegationBlocks = []
for (const b of ucan.iterateIPLDBlocks()) {
Expand All @@ -357,5 +363,8 @@ test('delegation.attach fails to attach block with not attached link', async ()
})

const block = await getBlock({ test: 'inlineBlock' })
assert.throws(() => ucan.attach(block))
assert.throws(() => ucan.attach({
iterateIPLDBlocks: () => [block].values(),
root: block
}))
})
5 changes: 4 additions & 1 deletion packages/core/test/invocation.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ test('encode invocation with attached block in capability nb', async () => {
},
proofs: [],
})
add.attach(block)
add.attach({
iterateIPLDBlocks: () => [block].values(),
root: block
})

/** @type {import('@ucanto/interface').BlockStore<unknown>} */
const blockStore = new Map()
Expand Down
4 changes: 2 additions & 2 deletions packages/interface/src/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ export interface Delegation<C extends Capabilities = Capabilities>
toJSON(): DelegationJSON<this>
delegate(): Await<Delegation<C>>

attach(block: Block): void
attach(block: IPLDView): void
}

/**
Expand Down Expand Up @@ -549,7 +549,7 @@ export interface IssuedInvocation<C extends Capability = Capability>
readonly proofs: Proof[]

delegate(): Await<Delegation<[C]>>
attach(block: Block): void
attach(block: IPLDView): void
}

export type ServiceInvocation<
Expand Down