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

feat: w3 aggregation #51

Merged
merged 31 commits into from
May 10, 2023
Merged

feat: w3 aggregation #51

merged 31 commits into from
May 10, 2023

Conversation

vasco-santos
Copy link
Contributor

@vasco-santos vasco-santos commented Mar 1, 2023

w3-filecoin.md Outdated Show resolved Hide resolved
w3-filecoin.md Outdated Show resolved Hide resolved
w3-filecoin.md Outdated Show resolved Hide resolved
@alanshaw
Copy link
Member

alanshaw commented Mar 6, 2023

I was reading through this and noted down a slightly simplier version (IMO! haha)

https://hackmd.io/ZjfqlXaPRtO39rVzpOqy_A?view

I don't really like using "filecoin" everywhere, this is actually a service for submitting aggregates to another service that makes deals with Filecoin storage providers. It's pretty far removed.

@vasco-santos
Copy link
Contributor Author

vasco-santos commented Mar 6, 2023

@alanshaw using aggregateCid for initial request is a good approach. We need to define the schema inside only.

I am not sure aggregate is a good naming though. filecoin maybe is not either.

My main concerns at the moment, are the pull based logic Spade is requiring. We would need to keep state in the proxy which I would avoid to the bare minimum and not have state there. This is, w3filecoin should be pulled if Spade will always work like this

@alanshaw
Copy link
Member

alanshaw commented Mar 6, 2023

My main concerns at the moment, are the pull based logic Spade is requiring. We would need to keep state in the proxy which I would avoid to the bare minimum and not have state there. This is, w3filecoin should be pulled if Spade will always work like this

It is only the list of aggregates that spade hasn't pulled yet...🤷‍♂️ I'm kinda ok with that...

I am not sure aggregate is a good naming though.

...but, why? 🙈

w3-filecoin.md Outdated Show resolved Hide resolved
w3-filecoin.md Outdated Show resolved Hide resolved
w3-filecoin.md Outdated Show resolved Hide resolved
w3-filecoin.md Outdated Show resolved Hide resolved
w3-filecoin.md Outdated Show resolved Hide resolved
w3-filecoin.md Outdated Show resolved Hide resolved
w3-filecoin.md Outdated Show resolved Hide resolved
w3-filecoin.md Outdated Show resolved Hide resolved
w3-filecoin.md Outdated Show resolved Hide resolved
w3-filecoin.md Outdated Show resolved Hide resolved
@vasco-santos vasco-santos force-pushed the feat/filecoin-spec branch 2 times, most recently from f9a673e to 635884e Compare March 27, 2023 12:00
@vasco-santos vasco-santos marked this pull request as ready for review March 27, 2023 15:10
@vasco-santos vasco-santos changed the title feat: filecoin spec feat: aggregate spec Mar 27, 2023
@vasco-santos vasco-santos changed the title feat: aggregate spec feat: w3 aggregation Mar 27, 2023
@heyjay44 heyjay44 mentioned this pull request Mar 27, 2023
23 tasks
Copy link
Collaborator

@Gozala Gozala left a comment

Choose a reason for hiding this comment

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

Submitted a feedback inline, tldr;

  1. I think we should frame it as a following steps
    1. Storefront offers broker an aggregate
    2. Broker queues the offer and returns effect that when performed will review the offer
    3. Broker reviews an offer and issues receipt for either accepting (success) or denying (failure) the offer.
      • Once offer is accepted broker takes care of arranging & renewing deals
    4. Storefront can query state of the aggregate deals using aggregate/get

w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Show resolved Hide resolved
w3-aggregation.md Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
@heyjay44 heyjay44 added this to the w3up phase 4 milestone Mar 29, 2023
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@Gozala Gozala left a comment

Choose a reason for hiding this comment

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

Provided more feedback and suggestions.

@vasco-santos vasco-santos added the P0 Critical: Tackled by core team ASAP label Apr 27, 2023
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
]
```

Each entry of the decoded offers block, has all the necessary information for a Storage Provider to fetch and store a CAR file. The `link` field has the CAR CID, while the `commitmentProof` field has the required `proof` bytes by Storage Providers (for example, `commP`). The `src` field of each piece MUST be set to a (alphabetically sorted) list of URLs from which it can be fetched. The `size` field MUST be set to the byte size of the CAR file.
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to specify that the src field is optional (allowing it to be provided when the deal is signed)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here 055603e

w3-aggregation.md Outdated Show resolved Hide resolved
@vasco-santos vasco-santos requested a review from alanshaw May 4, 2023 07:06
@@ -179,7 +179,7 @@ Invoking `aggregate/offer` capability submits an aggregate to a broker service f
]
```

Each entry of the decoded offers block, has all the necessary information for a Storage Provider to fetch and store a CAR file. The `link` field has the CAR CID, while the `commitmentProof` field has the required `proof` bytes by Storage Providers (for example, `commP`). The `src` field of each piece MUST be set to a (alphabetically sorted) list of URLs from which it can be fetched. The `size` field MUST be set to the byte size of the CAR file.
Each entry of the decoded offers block, has all the necessary information for a Storage Provider to fetch and store a CAR file. The `link` field has the CAR CID, while the `commitmentProof` field has the required `proof` bytes by Storage Providers (for example, `commP`). The `size` field MUST be set to the byte size of the CAR file. The `src` field of each piece MUST be set to a (alphabetically sorted) list of URLs from which it can be fetched. Note that `src` field is optional and can be provided in a different part of the flow such as when deal is signed or through a previously agreed API.

Choose a reason for hiding this comment

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

Hey folks, this is concerning. The byte/payload size of the CAR file is not something ♠️ cares about nor can validate in any shape or form. Let's remove this and other payload size mentions from the spec 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ribasushi good callout. It is true that Spade does not care about this and won't validate.

We can validate this in our side if we think that is important (perform HEAD requests to the src URLs). Main goal of having the size in an invocation is that it allows us to track metrics for data that we are requesting to Spade over time. All our metrics are being built around the information within invocations.

With that in mind, is there any strong opinion against keeping size?

Choose a reason for hiding this comment

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

You can keep it on your end, but there is very strong desire on the side of ♠️ to never receive/parse/store this information, and thus never be able to return it back to you on request.

So having something as a MUST when on my end I have MUST DISCARD is... odd ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, spade side won't need to do anything at it, even at the spade-proxy side. Just our UCAN Stream log processor will inspect these UCAN invocations and compute metrics over time from them.

w3-aggregation.md Outdated Show resolved Hide resolved
w3-aggregation.md Outdated Show resolved Hide resolved
@vasco-santos vasco-santos merged commit a4b56e5 into main May 10, 2023
@vasco-santos vasco-santos deleted the feat/filecoin-spec branch May 10, 2023 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 Critical: Tackled by core team ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants