Receipt writing #1

hannahhoward · 2024-07-28T20:30:24Z

Goals

Adds the ability to write receipts. This will be needed for the server.

Implementation

I had to think through how the write side of receipts is going to work since we hadn't really addressed that. I ultimately decided for writing, it is much easier to avoid really specific receipts and just fall back on an Any type in the IPLD schema, which means the result just needs a datamodel.Node()

I also started to fill out the result model a bit for this Any type -- including some functionality mirroring the Failure class in js-ucanto.

One other item I added support for is partial proof chains -- i.e. receipts/invocations where most of the blocks are NOT contained entirely within the CAR file. js-ucanto seems to support this. (though maybe not for invocations? Not sure)

Anyway, this was a fascinating exercise in learning about the ucanto library. The typing in the JS version is IMHO, wildly complicated and unveildy and the limitations of go types are quite useful in simplifying things, even if it's a bit verbose.

btw, the whole "how do we handle the result type" is really yet another case of this -- ipld/go-ipld-prime#443 -- how to deserialize schema'd types with a possibly unknown schema somewhere in the tree (but that you might have some hints about). I also noticed we still have VERY poor support in IPLD prime for combining schema trees. What you really out to be able to do all the combination with schema.Type values rather than byte reads of the schema files, which is way ineffecient. I filed an issue here: ipld/go-ipld-prime#568

I'm actually a good bit of the way through the validator package but wanted to try to ship vaguely incrementally so stopping here.

adds support for issueing receipts. Also increases API compatbility with UCanto.

alanshaw

🙌 Awesome, this looks great.

Ideally I would like to see a test that creates a message with a receipt, serializes it (to CAR) and then deserializes it and checks all the data is still present and correct.

I appreciate there is not a test that does that for invocation messages (my bad - sorry, pet project 😬).

alanshaw · 2024-07-29T08:28:20Z

core/dag/blockstore/blockstore.go

@@ -194,3 +194,21 @@ func NewBlockReader(options ...Option) (BlockReader, error) {

 	return &blockreader{keys, blks}, nil
 }
+
+func Encode(view ipld.View, bs BlockWriter) error {


Perhaps this should be a BatchPut operation that BlockWriter implements? This isn't really encoding anything, so I think if not BatchPut then it needs a rename :)

alanshaw · 2024-07-29T09:52:11Z

core/delegation/proofs.go

+}
+
+// Encode writes a set of proofs, some of which may be full delegations to a blockstore
+func (proofs Proofs) Encode(bs blockstore.BlockWriter) ([]ipld.Link, error) {


"Encode" to me is a function that transforms some data structure into another. This function is more like a utility to write a set of blocks into a blockstore as a convenience (I appreciate each block gets encoded as side effect). I'd perhaps rename to WriteInto? We have similar method naming in JS ucanto already, although it's never an instance method...

That said, it's a little confusing that this may or may not write blocks to a blockstore, depending on whether the instance is backed by an actual delegation. I can imagine it causing bugs that may be tricky to track down.

I propose removing this function and instead move the logic to where it is used, so it can be seen to be explicitly handled, I don't think there are multiple usages anyway(?)

so it's worth noting that all over the JS code this "maybe present" for proofs exists -- also for the invocatin in the receipt. I'd say see if you need it while working on the validator / server.

note that this is actually present in the JS delegation as well as receipt. I don't quite know what that implies, but it seems the goal is to be able to construct receipts and invocations without every block for every proof.

alanshaw · 2024-07-29T09:53:46Z

core/invocation/invocation.go

+	return Ran{nil, link}
+}
+
+func (r Ran) Encode(bs blockstore.BlockWriter) (ipld.Link, error) {


Similar to my previous comment, this is a "maybe write to blockstore" function - I'd move to where it is used or rename and heavily document.

alanshaw · 2024-07-29T09:55:19Z

core/ipld/view.go

@@ -7,7 +7,7 @@ import (
 // View represents a materialized IPLD DAG View, which provides a generic
 // traversal API. It is useful for encoding (potentially partial) IPLD DAGs
 // into content archives (e.g. CARs).
-type IPLDView interface {
+type View interface {


alanshaw · 2024-07-29T10:04:28Z

core/receipt/datamodel/receipt.go

-	Ok  O
-	Err X
+	Ok  *O
+	Err *X


Could you explain why this is needed please?

So, a union type per bindnode encoding is ALWAYS a pointer type. But the point just encodes whether or not it's present, which is then used to generate the keyed union. You'll note over in go-w3up you're forced to pass pointer types to the receipt. In fact, if you don't the whole code will panic. In my opinion, since no receipt that doesn't have a pointer type will EVER decode or encode properly, it perhaps makes better sense to hide the pointer requirement, which is IPLD specific, from the user.

alanshaw · 2024-07-29T10:14:25Z

core/receipt/receipt.go

+		[]byte(`type Result union {
+	  | any "ok"
+	  | any "error"
+	} representation keyed


Would be nice to pull this out as a separate file like the others. Amongst other reasons we get syntax highlighting with Rod's IPLD vscode plugin.

alanshaw · 2024-07-29T12:44:13Z

core/receipt/receipt.go

@@ -187,3 +190,167 @@ func NewReceiptReader[O, X any](resultschema []byte) (ReceiptReader[O, X], error
 	}
 	return &receiptReader[O, X]{typ}, nil
 }
+
+type UniversalReceipt Receipt[datamodel.Node, datamodel.Node]


🤷‍♀️ IDK this reads clearer to me...

Universal to me implies that there's something special going on to allow many types of receipt to all be used as one, but that's not the case here, it just could be any receipt.

Suggested change

type UniversalReceipt Receipt[datamodel.Node, datamodel.Node]

type AnyReceipt Receipt[datamodel.Node, datamodel.Node]

alanshaw · 2024-07-29T12:46:32Z

core/result/result.go

+	Error() (X, bool)
+}
+
+type UniversalResult Result[ipld.Node, ipld.Node]


Suggested change

type UniversalResult Result[ipld.Node, ipld.Node]

type AnyResult Result[ipld.Node, ipld.Node]

hannahhoward · 2024-07-30T02:17:31Z

I've addressed most PR changes @alanshaw though I haven't written a test -- but there is a seperate ticket for testing

alanshaw

LGTM

hannahhoward added 2 commits July 28, 2024 11:37

feat(receipt): add receipt creation

e41e117

adds support for issueing receipts. Also increases API compatbility with UCanto.

feat(message): add receipt encoding in messages

fa4cbd7

hannahhoward requested a review from alanshaw July 28, 2024 20:30

alanshaw requested changes Jul 29, 2024

View reviewed changes

refactor(receipt): respond to PR changes

3bfd389

alanshaw approved these changes Jul 30, 2024

View reviewed changes

alanshaw merged commit 6fb87be into main Aug 1, 2024
1 check passed

alanshaw deleted the feat/receipt-writing branch August 1, 2024 08:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Receipt writing #1

Receipt writing #1

hannahhoward commented Jul 28, 2024

alanshaw left a comment

alanshaw Jul 29, 2024

alanshaw Jul 29, 2024 •

edited

Loading

hannahhoward Jul 30, 2024

hannahhoward Jul 30, 2024

alanshaw Jul 29, 2024

alanshaw Jul 29, 2024

alanshaw Jul 29, 2024

hannahhoward Jul 30, 2024

alanshaw Jul 29, 2024

alanshaw Jul 29, 2024

alanshaw Jul 29, 2024

hannahhoward commented Jul 30, 2024

alanshaw left a comment

	type UniversalReceipt Receipt[datamodel.Node, datamodel.Node]
	type AnyReceipt Receipt[datamodel.Node, datamodel.Node]

	type UniversalResult Result[ipld.Node, ipld.Node]
	type AnyResult Result[ipld.Node, ipld.Node]

Receipt writing #1

Receipt writing #1

Conversation

hannahhoward commented Jul 28, 2024

Goals

Implementation

alanshaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanshaw Jul 29, 2024 • 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

hannahhoward commented Jul 30, 2024

alanshaw left a comment

Choose a reason for hiding this comment

alanshaw Jul 29, 2024 •

edited

Loading