feat!: implement receipts #266

Gozala · 2023-03-28T08:00:49Z

Overview

Implements Receipts as per upcoming invocation spec 0.2
- Few things changed since so I need to update some code
- ⚠️ clients now return Receipt as opposed to Result type which is a breaking change.
Changes transport codecs so that they can adapt based on content-type / accept headers
- Now we have Transport.Codec as opposed to { encoder, decoder } pair which performs content type negotiation.
- ⚠️ Now client takes codec field corresponding to Transport.OutboundCodec
- ⚠️ Now server takes codec field corresponding to Transport.InboundCodec
- ⚠️ CBOR transport is gone on server we could use new Legacy codec to deal with all clients and new clients should not use CBOR.
CAR and CBOR codecs moved to @ucanto/core as they are needed for receipts. Transport still reexports them so it's not breaking change.
@ucanto/core now re-exports things that were used from multiformats to cut down deps and avoid things getting out of sync.
Handlers can no longer return falsy values, it was a best practice that now is ennfroced by the typesystem and the library. If falcy value is returned it will end up encoded as unit {} in the receipt.
CARs didn't used to contains roots in blocks, now they do because previous version complicated encode / decode of receipts for no good reason
@ucanto/core now has exports DAG module with bunch of utilities to for building and viewing DAGs encoded in CARs.

Fixes #265

packages/client/src/connection.js

gobengo · 2023-03-29T22:45:47Z

packages/client/src/connection.js

+    const receipts = []
+    for await (const invocation of workflow) {
+      const { cid } = await invocation.delegate()
+      const receipt = await Receipt.issue({


It feels weird that an error in the channel + codec working together would lead to a Receipt vs just not catching the error here.

I would be happy for it to just throw/reject when encountering unexpected errors here vs 'wrapping' something it doesn't understand (error is unknown).
In doing so, I think this takes the thrown object, throws away all properties other than 'message' and 'cause'. It would be frustrating if those properties were important to debug some mistake.

(i.e. if we wrap at all, I think we should only wrap errors from successful decodes, not errors encountered while decoding, i.e. errors where the error might have been in the decoder vs what was on the other side of the channel)

I agree this is not ideal, yet I feel very strongly that two channels for error handling is going to be a super error prone.

try { const receipt = await agent.invoke(capbility) if (receipt.out.error) { // handle error } } catch (error) { // handle error }

Perhaps we could come up with a better solution so that they are separate steps, but lets do it in a followup

packages/client/test/services/account.js

packages/server/src/handler.js

gobengo · 2023-03-29T23:06:26Z

packages/client/test/client.spec.js

+        'application/car': CAR.request,
+      },
+      decoders: {
+        'application/car+receipt': CAR.response,


These media types are not defined by IANA, yet application/vnd.ipld.car is https://www.iana.org/assignments/media-types/application/vnd.ipld.car

I don't think it's a good idea to use unregistered media types that don't have a corresponding RFC. It could collide with something made by someone else.
I think we can prototype what might be in an RFC by using the existing media types or something else starting with application/vnd

The media types here are things I'd expec t be defined independent of any implementation in something like a 'ucanto over http' spec/profile

hmm application/car had been used all over the place, but looks like IPFS gateway now sets application/vnd.ipld.car;version=1 and so should we.

Complication however here is that our codecs also used to set this as content type before this change so not recognizing it will break existing clients. I created a followup issue for this #269

gobengo · 2023-03-29T23:08:49Z

packages/core/src/dag.js

+ */
+export const iterate = function* (value) {
+  if (
+    value &&


Why not use the type checker to enforce this?
That way it doesn't silently iterate nothing if some accidentally passes undefined instead of the iterable they intended.

If unknown is desirable as type of value,
we could add an else block here that throws TypeError

hmm intention here was to pass anything and if it is an IPLDView it would iterate over it's blocks and if it isn't do nothing. It is to traverse arbitrary structures that may contain IPLDViews inside. We could instead have isIPLDView(object) predicate so it would be:

for (const [key, value] of Object.entries(something)) { if (isIPLDView(value)) { for (const block of value.iterateIPLDBlocks()) { } } }

But I find it more convenient to do this instead

for (const [key, value] of Object.entries(something)) { for (const block of DAG.iterate(value)) { } }

Added a comment to make it bit more clear.

gobengo · 2023-03-29T23:59:24Z

packages/interface/src/transport.ts

@@ -22,16 +23,20 @@ export interface EncodeOptions {
  readonly hasher?: UCAN.MultihashHasher
 }

+export interface RequestEncodeOptions extends EncodeOptions {
+  accept?: string


What is an example value?
I find myself wondering how the string values here would be different from the string values I'd pass in an http accept header.

(or e.g. if comma separated values are allowed too). comment or jsdoc example would illustrate

gobengo · 2023-03-30T00:03:44Z

packages/server/src/server.js

        capability,
-        /** @type {Error} */ (error)
+        /** @type {Error} */ (cause)


It feels brittle to type cast this here. There's no guarantee that cause matches Error afaict.

HandlerExecutionError just wraps an error and it types it's argument as Error, now it is true that handler could throw non error, but if someone wrote the handler that throws null should suffer the consequences I say.

More seriously though I don't think threading through unknown everywhere is a better approach. Perhaps better solution would be to change HandlerExecutionError so it accepts unknown as cause to avoid cast here.

gobengo · 2023-03-30T00:06:52Z

packages/server/test/handler.spec.js

  })

  const client = Client.connect({
    id: w3,
-    encoder: CAR,
-    decoder: CBOR,
+    codec: CAR.outbound,


It would be great if client could interrogate options.channel to pick a good default for this if possible. (e.g. if options.channel is an HTTP Channel, it could issue an HTTP request to the URL interrogating 'which codecs do you support' e.g. options.channel.supportedCodecs(clientSupports) to get back a supported codec intersection)

(we could ship CAR codec with client so that always works, and if someone passes more supported codecs, it works as long as the supported codecs intersection has at least 1)

Not saying we should do now, but I hope one day we can make creating ucanto clients not require extra a priori knowledge about how which codecs are required.

By reading further I think I realized that the outbound object is already set up to do something like that!
cool. I'd be down for CAR.outbound to just be a default value here (seems like it wouldn't add very much to the bundle)

I think clients should have defaults, but since ucanto client and transports are separate packages without depndencey on each other I don't want to introduce one. That said I do want to collide them into one @ucanto/agent package in the future which is where we could do such a thing.

On the negotiation thing, I also want outbound to send one request to find out about the transports supported and protocol schema. But something I hope to do in the future.

gobengo · 2023-03-30T00:13:08Z

packages/transport/src/car/request.js

+export { CAR as codec }
+
+const HEADERS = Object.freeze({
+  'content-type': 'application/car',


seems like this media type should be defined in @ipld/js-car or otherwise stored in a constant somewhere

I think adding it ot @ipld/js-car is a great idea. I'll create an issue.

gobengo · 2023-03-30T00:16:09Z

packages/transport/src/codec.js

+   * @returns {API.ReceiptResult<API.InboundAcceptCodec, API.HTTPError>} transport
+   */
+  accept({ headers }) {
+    const contentType = headers['content-type'] || headers['Content-Type']


http headers are defined as case insensitive. We should probably only check for content-type. cOnTent-TypE is also as valid as headers['Content-Type'] and it seems reasonable to expect normalization at a higher layer

You are correct but (my estimate) 90% of cases most systems will normalize them either as one or the other so it seemed like a good idea to just check the other on in case. I don't mind removing second one but it was added because we encountered it somewhere and was easier to just check both than trying to thread the changes into third party libraries.

Put it differently this appears as new code, but it is just moving existing transport code that already was doing it. I really don't want introduce another potential break here. If we decided to not check that it's fine, but lets do it in the future when we already have migrated old clients.

packages/core/src/dag.js

Co-authored-by: Benjamin Goering <171782+gobengo@users.noreply.github.com>

Gozala · 2023-03-30T15:13:31Z

packages/core/src/dag.js

I only use jsdoc tags supported by TS https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html when we adopt other tools that recognize such tags we can add tags recognized by them.

packages/core/src/invocation.js

packages/core/src/receipt.js

Gozala · 2023-03-30T15:19:38Z

packages/core/src/receipt/outcome.js

+  })
+}
+
+/**


0.2 end up inlining this block into receipt and I have branch that remove this, so I won't bother commenting

Gozala · 2023-03-30T15:20:56Z

packages/core/src/receipt/outcome.js

+  })
+}
+
+/**


As of comments plz note that interfaces classes implement have comments I just don't put duplicate them on implementation. Functions also return interfaces so they should lead to the well commented types.

Gozala · 2023-03-30T15:36:56Z

packages/server/src/server.js

        capability,
-        /** @type {Error} */ (error)
+        /** @type {Error} */ (cause)


HandlerExecutionError just wraps an error and it types it's argument as Error, now it is true that handler could throw non error, but if someone wrote the handler that throws null should suffer the consequences I say.

More seriously though I don't think threading through unknown everywhere is a better approach. Perhaps better solution would be to change HandlerExecutionError so it accepts unknown as cause to avoid cast here.

Gozala · 2023-03-30T15:39:49Z

packages/server/test/handler.spec.js

  })

  const client = Client.connect({
    id: w3,
-    encoder: CAR,
-    decoder: CBOR,
+    codec: CAR.outbound,


I think clients should have defaults, but since ucanto client and transports are separate packages without depndencey on each other I don't want to introduce one. That said I do want to collide them into one @ucanto/agent package in the future which is where we could do such a thing.

On the negotiation thing, I also want outbound to send one request to find out about the transports supported and protocol schema. But something I hope to do in the future.

Gozala · 2023-03-30T15:40:41Z

packages/transport/src/car/request.js

+export { CAR as codec }
+
+const HEADERS = Object.freeze({
+  'content-type': 'application/car',


I think adding it ot @ipld/js-car is a great idea. I'll create an issue.

Gozala · 2023-03-30T15:45:00Z

packages/transport/src/codec.js

+   * @returns {API.ReceiptResult<API.InboundAcceptCodec, API.HTTPError>} transport
+   */
+  accept({ headers }) {
+    const contentType = headers['content-type'] || headers['Content-Type']


You are correct but (my estimate) 90% of cases most systems will normalize them either as one or the other so it seemed like a good idea to just check the other on in case. I don't mind removing second one but it was added because we encountered it somewhere and was easier to just check both than trying to thread the changes into third party libraries.

Gozala · 2023-03-30T15:47:00Z

packages/transport/src/codec.js

+   * @returns {API.ReceiptResult<API.InboundAcceptCodec, API.HTTPError>} transport
+   */
+  accept({ headers }) {
+    const contentType = headers['content-type'] || headers['Content-Type']


Put it differently this appears as new code, but it is just moving existing transport code that already was doing it. I really don't want introduce another potential break here. If we decided to not check that it's fine, but lets do it in the future when we already have migrated old clients.

packages/core/src/dag.js

feat!: implement receipts

0883fb4

Gozala force-pushed the feat/just-receipt branch from d590015 to 0883fb4 Compare March 28, 2023 08:05

heyjay44 added this to the w3up phase 4 milestone Mar 28, 2023

Gozala added 2 commits March 29, 2023 02:08

implement content negotiation

cd26404

cover new transport code

ff7220c

Gozala force-pushed the feat/just-receipt branch from e303c6e to ff7220c Compare March 29, 2023 16:06

Gozala marked this pull request as ready for review March 29, 2023 16:06

Gozala added 7 commits March 29, 2023 10:07

fix client types and test

8c0c886

update validator inference test

d3225b6

update deps

0b60a4a

cover uncovered lines

5b051ef

refactor to trim deps

ffca093

only untify null and undefined

4a018d7

add code comments

fc33269

Gozala requested a review from gobengo March 29, 2023 20:41

gobengo reviewed Mar 29, 2023

View reviewed changes

packages/client/src/connection.js Show resolved Hide resolved

gobengo approved these changes Mar 30, 2023

View reviewed changes

Gozala commented Mar 30, 2023

View reviewed changes

packages/core/src/dag.js Outdated Show resolved Hide resolved

Gozala and others added 2 commits March 30, 2023 08:47

Apply suggestions from code review

1dd23f7

Co-authored-by: Benjamin Goering <171782+gobengo@users.noreply.github.com>

Update packages/core/src/dag.js

6706729

Gozala commented Mar 30, 2023

View reviewed changes

packages/core/src/dag.js Show resolved Hide resolved

Gozala commented Mar 30, 2023

View reviewed changes

packages/core/src/dag.js Show resolved Hide resolved

Gozala added 2 commits March 30, 2023 08:51

Apply suggestions from code review

cf4c8be

fix jsdoc comment

608b219

Gozala merged commit 5341416 into main Mar 30, 2023

github-actions bot mentioned this pull request Mar 30, 2023

chore: release main #270

Merged

This was referenced Apr 11, 2023

chore: release main #276

Merged

chore: release main #277

Merged

chore: release main #278

Merged

heyjay44 mentioned this pull request Apr 11, 2023

Tracking w3up work 🆙 storacha/w3up#650

Closed

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: implement receipts #266

feat!: implement receipts #266

Gozala commented Mar 28, 2023 •

edited

Loading

gobengo Mar 29, 2023

gobengo Mar 29, 2023

gobengo Mar 29, 2023

Gozala Mar 30, 2023

gobengo Mar 29, 2023

gobengo Mar 29, 2023

Gozala Mar 30, 2023

gobengo Mar 29, 2023

Gozala Mar 30, 2023

Gozala Mar 30, 2023

gobengo Mar 29, 2023

gobengo Mar 30, 2023

gobengo Mar 30, 2023

Gozala Mar 30, 2023

gobengo Mar 30, 2023

gobengo Mar 30, 2023

gobengo Mar 30, 2023 •

edited

Loading

Gozala Mar 30, 2023

gobengo Mar 30, 2023

Gozala Mar 30, 2023

gobengo Mar 30, 2023

Gozala Mar 30, 2023

Gozala Mar 30, 2023

Gozala Mar 30, 2023

Gozala Mar 30, 2023

Gozala Mar 30, 2023

Gozala Mar 30, 2023

Gozala Mar 30, 2023

Gozala Mar 30, 2023

Gozala Mar 30, 2023

Gozala Mar 30, 2023

+                })
+              }
+              /**

feat!: implement receipts #266

feat!: implement receipts #266

Conversation

Gozala commented Mar 28, 2023 • edited Loading

Overview

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gobengo Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gozala commented Mar 28, 2023 •

edited

Loading

gobengo Mar 30, 2023 •

edited

Loading