Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fix: align aggregation namings from commp learnings #62
fix: align aggregation namings from commp learnings #62
Changes from all commits
97df283
dcef229
99cbfa4
060181a
e8e8c14
903894e
df468f1
9bc01c0
c54e73d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Give that filecoin spec refers to these actors as
Aggregators
it may be better idea to use the same term as them as opposed toStorefront
, which I suggested before I was familiar with the preexisting terminology.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.
Given all the fields that we've dropped I wonder if we should just flatten things up and have this more like
Where
bafy...offer
is CBOR of structure like: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'm hesitant to voice this opinion, yet I wonder if we should omit the
size
which I believe could be determinstically derived from thepieces
. Unless I'm mistaken existing libraries require to specifysize
(on aggregates specifically) because the remaining space could be filled with0
s. However it creates space for an error where you may have two different aggregates for exact same pieces.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.
The problem here is that this is not directly compatible with UCAN LOG. When we want to create metrics or to run consumers based on invocations/receipts of these invocations we would need to do extra ops to read block from CAR file. We could make the block as you suggest, which would probably be the exact content we send to Spade in the end, but I would still keep
link
andsize
in thenb
field so that we can easily track things from the log without a layer of indirectionThere 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.
Nit: I think "Out of band, ...." is somewhat misleading here. Perhaps document should say HTTP URLs will be issued during deal signing.
Aside: I have also been thinking that we may want to delegate UCAN that can be used for fetching aggregate pieces to the actor we sign with a deal with. That would prevent other actors from using that URL without sharing a private key or a UCAN delegation. It could also allow us to defer the pre-signed URL creation up until actor decides to fetch.
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'm also wondering if
aggreate/offer
should be delegating capability to a "broker" to issue signed URLs for the aggregate pieces. That would allow broker to do it as needed and perhaps avoid back & forth. I realize we would still need to sign the deal with our key, but if delegated signatures were an option we could probably even get away from that.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 have started a thread on this line of thought here https://filecoinproject.slack.com/archives/C05C7CUPEKX/p1687366781115129
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.
Given the discussion in the linked thread. I think we should reframe "spade-proxy" as an "agency" to which "aggregators" can submit (offer) aggregates along with all the UCAN delegations that would allow it to:
That would I think simplify things for both "aggregator" and "agency" (spade-proxy) reducing coordination between them as "agency" would be able to take the aggregate and either get it into a filecoin.
@vasco-santos I think this also could remove for mapping CAR CID <-> Piece CID, instead pieces could come with associated UCANs, allow agency to create HTTP URLs for them. Aggregator would still need to map CARs to pieces to surface corresponding filecoin state, which I think makes sense, it's like a reference to filecoin state.