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

deal making FRC added #693

Merged
merged 6 commits into from
Jul 28, 2023
Merged

Conversation

aashidham
Copy link
Contributor

No description provided.

@luckyparadise
Copy link
Collaborator

@aashidham would you like to present this FRC at our next Core Devs Call? Might be a good place to gather initial feedback and comments.

@aashidham
Copy link
Contributor Author

@aashidham would you like to present this FRC at our next Core Devs Call? Might be a good place to gather initial feedback and comments.

Absolutely! I'll follow up on Slack.

@aashidham
Copy link
Contributor Author

Following up on this -- what is pending before approval can be granted? Started a discussion for this PR to solicit more feedback: #721

@luckyparadise
Copy link
Collaborator

Hi @aashidham there is no "approval" as such as all FRCs at the moment remain as open drafts. However, if you like, I can share this widely and solicit feedback and engagement for you. Let me know thanks.

@aashidham
Copy link
Contributor Author

That would be great @luckyparadise thanks!

Reviewed for language and completeness. No substantial changes to the content or intent of the PR.

A valid client contract implementation would minimally support either the `OneToOneDealProposer` or `WildcardDealProposer` interface. Both of these inherit the `BaseDealProposer` interface.

In particular, the `OneToOneDealProposer` interface is for client contracts that have specified a single Boost SP for every deal proposal. Developers implementing this interface will likely have to reach out and coordinate with Boost SPs specified for a particular deal. Boost SPs with address `x` can then listen to `OneToOneDealProposalCreate` events with the provider field set to `x`. This flow is tremendously simplified, since it only deals with one client contract and one Boost SP per deal proposal.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think reference to "Boost" belongs in this FRC. Boost is just one implementation of deal-making.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I largely agree -- however there are some references to Boost that are useful for context. Have a look at the latest draft.

Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

Editorial 👍

Comment on lines +232 to +233
uint64 extra_params_version;
ExtraParamsV1 extra_params;
Copy link
Member

Choose a reason for hiding this comment

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

I think the idea here is to have the version of the params (currently we have only one version, namely ExtraParamsV1) in the extra_params_version field, and for the extra_params to be generic, such as bytes, and not of specific type as it is defined now.

The way it is defined now make the parameter extra_params_version redundant, no?

Comment on lines +247 to +248
uint64 extra_params_version;
ExtraParamsV1 extra_params;
Copy link
Member

Choose a reason for hiding this comment

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

@luckyparadise
Copy link
Collaborator

@kaitlin-beegle can you merge this PR and assign the FRC number?

@kaitlin-beegle
Copy link
Contributor

@kaitlin-beegle can you merge this PR and assign the FRC number?

@luckyparadise yes, but please follow up @aashidham next week and make sure they are able to follow up @nonsense's feedback.

@kaitlin-beegle kaitlin-beegle merged commit 60e86d6 into filecoin-project:master Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants