fix!: update aggregate spec in client and api #824

vasco-santos · 2023-06-29T11:10:12Z

Updates client and api based on storacha/specs@63846e5

It also starts relying on the https://github.com/web3-storage/data-segment/ lib for the tests by generating commP of randomly generated CARs. In next iteration, tests should also include commP of commPs builder (needs storacha/data-segment#8)

Client API for submitting offer was changed to receive piece instead of computing it. Main reason for this is that there is a need to build it outside while keeping track of sizes. It could also happen here and explode if not, even though this is an aggregate offer call and not really the builder. So I would love feedback on API surface preferences. This will not be used by end users as will be abstracted within our services. The API will still need to validate if given commP of commPs is good for the given offer.

BREAKING CHANGE: aggregate capabilities now have different nb properties and aggregate client api was simplified

alanshaw · 2023-06-29T13:21:16Z

packages/aggregate-api/src/aggregate/offer.js

@@ -99,13 +84,13 @@ export const claim = async (

 /**
 * @param {Server.API.Link<unknown, number, number, 0 | 1>} offerCid
- * @param {Server.API.Invocation<Server.API.Capability<"aggregate/offer", `did:${string}:${string}` & `did:${string}` & Server.API.Phantom<{ protocol: "did:"; }> & `${string}:${string}` & Server.API.Phantom<{ protocol: `${string}:`; }>, Pick<{ offer: Server.API.Link<unknown, number, number, 0 | 1>; commitmentProof: Server.API.Link<unknown, number, number, 0 | 1>; size: number & Server.API.Phantom<{ typeof: "integer"; }>; }, "offer" | "commitmentProof" | "size"> & Partial<Pick<{ offer: Server.API.Link<unknown, number, number, 0 | 1>; commitmentProof: Server.API.Link<unknown, number, number, 0 | 1>; size: number & Server.API.Phantom<{ typeof: "integer"; }>; }, never>>>>} invocation
+ * @param {Server.API.Invocation<Server.API.Capability<"aggregate/offer", `did:${string}:${string}` & `did:${string}` & Server.API.Phantom<{ protocol: "did:"; }> & `${string}:${string}` & Server.API.Phantom<{ protocol: `${string}:`; }>, Pick<{ offer: Server.API.Link<unknown, number, number, 0 | 1>; piece: Server.Schema.InferStruct<{ link: Server.Schema.Schema<Server.API.Link<unknown, number, number, 0 | 1>, any>; size: Server.Schema.NumberSchema<number & Server.API.Phantom<{ typeof: "integer"; }>, unknown>; }>; }, "offer" | "piece"> & Partial<Pick<{ offer: Server.API.Link<unknown, number, number, 0 | 1>; piece: Server.Schema.InferStruct<{ link: Server.Schema.Schema<Server.API.Link<unknown, number, number, 0 | 1>, any>; size: Server.Schema.NumberSchema<number & Server.API.Phantom<{ typeof: "integer"; }>, unknown>; }>; }, never>>>>} invocation


Woah...maybe just pass the blocks iterator to the function instead?

This type is pretty insane, could we maybe have some types defined in .ts file and referenced here instead ? Also I think some of the stuff is redundunt there. And that thecond type parameter is probably just Server.API.DID.

Also after second look, I think passing blocks iterator is probably a way to go here indeed.

alanshaw · 2023-06-29T13:25:25Z

packages/aggregate-api/src/offer/arrange.js

@@ -14,14 +14,14 @@ export const provide = (context) =>
 * @returns {Promise<API.UcantoInterface.Result<API.OfferArrangeSuccess, API.OfferArrangeFailure>>}
 */
 export const claim = async ({ capability }, { arrangedOfferStore }) => {
-  const commitmentProof = capability.nb.commitmentProof
+  const pieceLink = capability.nb.pieceLink


Can we not suffix with types? piece: Link is ok? Why is the size not being passed in this instance?

The size is only needed on the aggregate offer for SP validation. Here is when there was already a resolution and we just want to create the receipt with final result. For searching we do not need the size (and I don't even know if by the state we get from the Oracle we would have access to it easily).

piece is Object with link and size everywhere else. That is the reasoning to name it this way here as we decided in storacha/specs#62

I got to say pieceLink also bothered me, I was not going to say anything but since @alanshaw did how about we rename piece object into something else ? I think few cases we considered pieceInfo ?

Naming is so hard 😭 and it's made harder by the fact that piece CID does not fully identify the piece without the size....

alanshaw · 2023-06-29T13:32:46Z

packages/aggregate-client/src/types.ts

+ * [Piece CID](https://spec.filecoin.io/systems/filecoin_files/piece/) of some
+ * content.
+ */
+export type PieceCID = ReturnType<typeof CommP.toCID>


Is there not an exported type for this? Or at least can we not define like CARLink?

Call this PieceLink to be consistent with CARLink?

I think we want to be consistent with filecoin namings. I stole this from @Gozala 's PR #803 but I actually wondered if data-segment lib could actually export the type

Yeah it should absolutely export a type, I'll do that

Gozala · 2023-06-30T05:49:18Z

packages/capabilities/src/aggregate.js

+ * @see https://github.com/ipfs/go-cid/blob/829c826f6be23320846f4b7318aee4d17bf8e094/cid.go#L104
+ */
+const FilCommitmentUnsealed = 0xf101
+


Nit: data-segment should probably expose these constants so they can be imported as opposed to copied around

I will wait for that to make it to next release

Gozala · 2023-06-30T05:56:51Z

packages/capabilities/src/aggregate.js

@@ -29,25 +38,33 @@ export const offer = capability({
    offer: Schema.link(),


Nit: I am no longer sure spade-proxy needs this. is it going to do anything with that data ? According to the comment above it will validate the offer, but do we actually expect it will go ahead and recalculate aggregate piece CID ?

spade-proxy can validate commP of commPs. Anyway, the offer is needed to get the inline block

Gozala · 2023-06-30T06:00:34Z

packages/aggregate-client/src/types.ts

+ * [Piece CID](https://spec.filecoin.io/systems/filecoin_files/piece/) of some
+ * content.
+ */
+export type PieceCID = ReturnType<typeof CommP.toCID>


Yeah it should absolutely export a type, I'll do that

Gozala · 2023-06-30T06:04:44Z

packages/aggregate-client/src/aggregate.js

@@ -50,12 +44,12 @@ export async function aggregateOffer(
 * Get details of an aggregate.
 *
 * @param {import('./types').InvocationConfig} conf - Configuration
- * @param {import('@ucanto/interface').Link<unknown, number, number, 0 | 1>} commitmentProof
+ * @param {import('@ucanto/interface').Link<unknown, number, number, 0 | 1>} subject


Suggested change

* @param {import('@ucanto/interface').Link<unknown, number, number, 0 | 1>} subject

* @param {import('@ucanto/interface').UnknownLink} subject

There is an UnknownLink type that covers version CID 0 as well.

Gozala · 2023-06-30T06:14:35Z

packages/aggregate-api/src/offer/arrange.js

@@ -14,14 +14,14 @@ export const provide = (context) =>
 * @returns {Promise<API.UcantoInterface.Result<API.OfferArrangeSuccess, API.OfferArrangeFailure>>}
 */
 export const claim = async ({ capability }, { arrangedOfferStore }) => {
-  const commitmentProof = capability.nb.commitmentProof
+  const pieceLink = capability.nb.pieceLink


I got to say pieceLink also bothered me, I was not going to say anything but since @alanshaw did how about we rename piece object into something else ? I think few cases we considered pieceInfo ?

Naming is so hard 😭 and it's made harder by the fact that piece CID does not fully identify the piece without the size....

Gozala · 2023-06-30T06:20:37Z

packages/aggregate-api/src/aggregate/offer.js

@@ -99,13 +84,13 @@ export const claim = async (

 /**
 * @param {Server.API.Link<unknown, number, number, 0 | 1>} offerCid
- * @param {Server.API.Invocation<Server.API.Capability<"aggregate/offer", `did:${string}:${string}` & `did:${string}` & Server.API.Phantom<{ protocol: "did:"; }> & `${string}:${string}` & Server.API.Phantom<{ protocol: `${string}:`; }>, Pick<{ offer: Server.API.Link<unknown, number, number, 0 | 1>; commitmentProof: Server.API.Link<unknown, number, number, 0 | 1>; size: number & Server.API.Phantom<{ typeof: "integer"; }>; }, "offer" | "commitmentProof" | "size"> & Partial<Pick<{ offer: Server.API.Link<unknown, number, number, 0 | 1>; commitmentProof: Server.API.Link<unknown, number, number, 0 | 1>; size: number & Server.API.Phantom<{ typeof: "integer"; }>; }, never>>>>} invocation
+ * @param {Server.API.Invocation<Server.API.Capability<"aggregate/offer", `did:${string}:${string}` & `did:${string}` & Server.API.Phantom<{ protocol: "did:"; }> & `${string}:${string}` & Server.API.Phantom<{ protocol: `${string}:`; }>, Pick<{ offer: Server.API.Link<unknown, number, number, 0 | 1>; piece: Server.Schema.InferStruct<{ link: Server.Schema.Schema<Server.API.Link<unknown, number, number, 0 | 1>, any>; size: Server.Schema.NumberSchema<number & Server.API.Phantom<{ typeof: "integer"; }>, unknown>; }>; }, "offer" | "piece"> & Partial<Pick<{ offer: Server.API.Link<unknown, number, number, 0 | 1>; piece: Server.Schema.InferStruct<{ link: Server.Schema.Schema<Server.API.Link<unknown, number, number, 0 | 1>, any>; size: Server.Schema.NumberSchema<number & Server.API.Phantom<{ typeof: "integer"; }>, unknown>; }>; }, never>>>>} invocation


This type is pretty insane, could we maybe have some types defined in .ts file and referenced here instead ? Also I think some of the stuff is redundunt there. And that thecond type parameter is probably just Server.API.DID.

Also after second look, I think passing blocks iterator is probably a way to go here indeed.

BREAKING CHANGE: aggregate capabilities now have different nb properties and aggregate client api was simplified

🤖 I have created a release *beep* *boop* --- ## [7.0.0](capabilities-v6.0.1...capabilities-v7.0.0) (2023-07-06) ### ⚠ BREAKING CHANGES * aggregate capabilities now have different nb properties and aggregate client api was simplified ### Bug Fixes * update aggregate spec in client and api ([#824](#824)) ([ebefd88](ebefd88)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- ## 1.0.0 (2023-07-11) ### ⚠ BREAKING CHANGES * aggregate capabilities now have different nb properties and aggregate client api was simplified ### Features * w3 aggregate protocol client and api implementation ([#787](#787)) ([b58069d](b58069d)) ### Bug Fixes * update aggregate spec in client and api ([#824](#824)) ([ebefd88](ebefd88)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- ## 1.0.0 (2023-07-11) ### ⚠ BREAKING CHANGES * aggregate capabilities now have different nb properties and aggregate client api was simplified ### Features * w3 aggregate protocol client and api implementation ([#787](#787)) ([b58069d](b58069d)) ### Bug Fixes * aggregate api test link comparison type ([#816](#816)) ([81bdf1c](81bdf1c)) * update aggregate spec in client and api ([#824](#824)) ([ebefd88](ebefd88)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: Vasco Santos <[email protected]>

vasco-santos force-pushed the fix/update-aggregate-spec-in-client-and-api branch 3 times, most recently from 5ee4294 to 7344322 Compare June 29, 2023 12:33

vasco-santos marked this pull request as ready for review June 29, 2023 12:35

vasco-santos requested a review from Gozala June 29, 2023 12:54

alanshaw reviewed Jun 29, 2023

View reviewed changes

Gozala approved these changes Jun 30, 2023

View reviewed changes

vasco-santos added 2 commits July 6, 2023 17:33

fix!: update aggregate spec in client and api

004b973

BREAKING CHANGE: aggregate capabilities now have different nb properties and aggregate client api was simplified

chore: address review comments

1859b4a

vasco-santos force-pushed the fix/update-aggregate-spec-in-client-and-api branch from 21c06f3 to 1859b4a Compare July 6, 2023 15:33

vasco-santos merged commit ebefd88 into main Jul 6, 2023

vasco-santos deleted the fix/update-aggregate-spec-in-client-and-api branch July 6, 2023 15:37

This was referenced Jun 29, 2023

chore(main): release aggregate-client 1.0.0 #822

Merged

chore(main): release aggregate-api 1.0.0 #821

Merged

chore(main): release capabilities 7.0.0 #829

Merged

This was referenced Jul 11, 2023

fix: upgrade deps web3-storage/dealer#13

Merged

aggregate capabilities update on spec namings alignment #823

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: update aggregate spec in client and api #824

fix!: update aggregate spec in client and api #824

vasco-santos commented Jun 29, 2023 •

edited

Loading

alanshaw Jun 29, 2023

Gozala Jun 30, 2023

alanshaw Jun 29, 2023

vasco-santos Jun 29, 2023

Gozala Jun 30, 2023

alanshaw Jun 29, 2023

vasco-santos Jun 29, 2023

Gozala Jun 30, 2023

Gozala Jun 30, 2023

vasco-santos Jul 6, 2023

Gozala Jun 30, 2023

vasco-santos Jul 6, 2023

Gozala Jun 30, 2023

Gozala Jun 30, 2023

Gozala Jun 30, 2023

Gozala Jun 30, 2023

		@@ -29,25 +38,33 @@ export const offer = capability({
		offer: Schema.link(),

	* @param {import('@ucanto/interface').Link<unknown, number, number, 0 \| 1>} subject
	* @param {import('@ucanto/interface').UnknownLink} subject

fix!: update aggregate spec in client and api #824

fix!: update aggregate spec in client and api #824

Conversation

vasco-santos commented Jun 29, 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

Choose a reason for hiding this comment

vasco-santos commented Jun 29, 2023 •

edited

Loading