-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: support inline blocks in invocation #288
Conversation
d24b126
to
669b758
Compare
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 missing something but I find adding blocks to the ucanto message
awkward as clients and servers operate at the layer above it. I am also not sure that adding inlineLinks
to the message interface is a good idea, instead I would encourage surfacing links in invoked capabilities.
I would like to propose alternative design that:
- Adds
attach(block: Block)
method to theDelegation
/Invocation
so that one could attach additional blocks to be included with the invocation. - Update
iterateIPLDBlocks
implementation on theDelegation
class to emit "attached" blocks so they will be included with aAgentMessage
and a CAR file.
I think that would meet all of the requirements and pave the way to the future where ucanto can figure out which blocks to attach from the schema itself.
packages/interface/src/lib.ts
Outdated
@@ -704,6 +708,7 @@ export interface AgentMessage<T = unknown> | |||
invocationLinks: Tuple<Link<UCAN.UCAN<[Capability]>>> | [] | |||
receipts: Map<ToString<UCANLink>, Receipt> | |||
invocations: Invocation[] | |||
inlineLinks: IPLDBlock[] |
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.
Do we actually need this here, as in if we actually need links to them here because they will be linked from the invocation anyway.
packages/core/src/invocation.js
Outdated
}) { | ||
/** @readonly */ | ||
this.issuer = issuer | ||
/** @readonly */ | ||
this.audience = audience | ||
/** @readonly */ | ||
this.proofs = proofs | ||
/** @readonly */ | ||
this.inlineLinks = inlineLinks |
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 think you may want to do this in the Delegation
class instead of here. I would also expect some method e.g. attach(block:Block)
on the Delegation
for attaching additional blocks.
Thanks @Gozala This is super useful direction. I agree that makes more sense to add this out of the message scope. Just refactored based on review |
packages/core/src/delegation.js
Outdated
@@ -192,12 +194,20 @@ export class Delegation { | |||
Object.defineProperties(this, { data: { value: data, enumerable: false } }) | |||
return data | |||
} | |||
/** | |||
* Attach a block to the invocation encoded bytes. |
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.
* Attach a block to the invocation encoded bytes. | |
* Attach a block 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. |
Nit: I think this is more clear
@Gozala also added This way we can support both: delegation const ucan = await Delegation.delegate({
issuer: alice,
audience: bob,
capabilities: [
{
can: 'store/add',
with: alice.did(),
},
],
})
const block = await getBlock({ test: 'inlineBlock' })
ucan.attach(block) but also invocation: const add = invoke({
issuer: alice,
audience: w3,
capability: {
can: 'store/add',
with: alice.did(),
link: 'bafy...stuff',
},
proofs: [],
})
const block = await getBlock({ test: 'inlineBlock' })
add.attach(block) Later is quite important, given what I would like to do on aggregate client is sth like: const invocation = AggregateCapabilities.offer
.invoke({
issuer,
/* c8 ignore next */
audience: audience ?? servicePrincipal,
// TODO: confirm
with: resource,
nb: {
link: block.cid,
size
},
proofs
})
invocation.attach(block)
await invocation.execute(conn) This underneath will do https://github.com/web3-storage/ucanto/blob/main/packages/client/src/connection.js#L47-L50 which calls |
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 made couple of suggestions & would leave it up to you to decide whether to address them or keep things as is.
My only concern is that once invocation goes over the wire and is de-serialized on the other side it will loose attachments because replica.iterateIPLDBlocks
will not be aware of the attachments.
I think it would be a good idea to capture these links in the data model somewhere, which leaves us with two options:
- In the
iterateIPLDBlocks
traversecapabilities
(or perhapsthis.data
and include all linked blocks). - Have dedicated place for sticking attached block links so we can retain behavior of
iterateIPLDBlocks
after they cross the wire. For example we could stick{ 'ucanto/attachments': Link[] }
object intofct
fields and use it instead of traversing arbitrary dags.
I don't think we need to block this on the ☝️ but we decide to leave that out for now lets create an issue for it and maybe a code comment in attach
function to call out the limitation.
packages/core/src/delegation.js
Outdated
* @param {API.Block} block | ||
*/ | ||
attach(block) { | ||
this.attachedBlocks.set(`${block.cid}`, block) |
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.
Is there a reason to have a separate map for attached blocks as opposed to adding them to the this.blocks
? If so I think it would be a good idea to add a code comment to capture that. Otherwise I think I would just track CIDs of the attached blocks but stick actual blocks into this.blocks
.
packages/core/src/delegation.js
Outdated
* @returns {IterableIterator<API.Block>} | ||
*/ | ||
|
||
export const exportBlocks = function* (root, blocks, attachedBlocks) { |
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 think you should just amend exportDAG
to include attached blocks, I don't see a reason to have a separate function. In fact it's probably going to be confusing that exportDAG
does not contain attached blocks while iterateIPLDBlocks
does and could lead to some errors.
…h a subdag recursive function
This PR adds support to create and encode invocations adding inline blocks to the encoded bytes for transport. This enables us to add the bytes for a IPLD Block with a CAR file over the wire for projects like w3 aggregation
Example of usage delegation
invocation: