-
Notifications
You must be signed in to change notification settings - Fork 0
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: update filecoin spec #81
Conversation
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 material changes look fine to me and I believe are as we discussed. However there are tons of grammatical errors. Perhaps we could get a gramatical automation hooked up here so I don't have to suggest these manually.
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.
There were a few inconsistencies with capabilities name changes. Per previous diagram as well as Table of Contents looks like we are aligned, but the name changes got badly propagated further in the document on the capabilities + Schema.
More specifically, Aggregator capabilities should be piece/offer and piece/accept and Dealer capabilities should be aggregate/offer and aggregate/accept like in the Table of contents. In the capabilities + schema section of the PR, we can see aggregate/offer and aggregate/accept for the aggregator and deal/offer + deal/accept for Dealer, which is not correct
Co-authored-by: Alan Shaw <[email protected]> Co-authored-by: Vasco Santos <[email protected]>
Co-authored-by: Vasco Santos <[email protected]>
Thanks for making all the fixes. I have not found a github action that can do such a thing, however I did find vscode plugin that does, I hope it will help me avoid so many grammar errors in the future. |
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.
LGTM! This is great ❤️
Left small non blocking small suggestions
w3-filecoin.md
Outdated
type PieceAcceptDetail struct { | ||
# Piece as Filecoin Piece with padding | ||
piece PieceCid | ||
# storefront responsible for invocation | ||
storefront string | ||
# grouping for joining segments into an aggregate (subset of space) | ||
piece PieceLink | ||
# Grouping for joining segments into an aggregate (subset of space) | ||
group string | ||
} |
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.
Should we do like above for Piece*
and only capture once this type?
type PieceAcceptDetail = PieceOfferDetail
w3-filecoin.md
Outdated
type AggregateAcceptDetail struct { | ||
# Contains each individual piece within Aggregate piece | ||
pieces &AggregatePieces | ||
# Piece as Aggregate of CARs with padding | ||
aggregate PieceCid | ||
# Fields to create a contract with a Storage Provider for aggregate | ||
# storefront responsible for invocation | ||
storefront string | ||
# Label is an arbitrary client chosen label to apply to the deal | ||
# @see https://github.com/filecoin-project/go-state-types/blob/ff2ed169ff566458f2acd8b135d62e8ca27e7d0c/builtin/v9/market/deal.go#L201-L221 | ||
label string | ||
aggregate PieceLink | ||
} |
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.
Should we do like above for Piece*
and only capture once this type?
type AggregateAcceptDetail = AggregateOfferDetail
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.
Just a few minor grammar issues but this LGTM.
Co-authored-by: Vasco Santos <[email protected]> Co-authored-by: Alan Shaw <[email protected]>
Merging it as talked with @Gozala |
📺 Preview
Updates capability names per diagram
@vasco-santos I also have dropped
storefront
field which seemed redundant, given thatwith
field is more credible and verifiable field containing same information. I think we have discussed this while back and if I recall correctly main hesitation on your side was that it required Storefront to delegate capability to an Aggregator. I do intend to make it non-brainer at the ucanto layer, but in the meantime I propose that we use same DID across Storefront and Aggregator which would avoid this complication. When we decide to support multiple storefronts (which I understand is no longer the immediate plan) we can make change in ucanto that would make configuring actors with delegations hassle free. If you disagree or there was some other reason that I failed to recall, we can put back those fields as necessary.