FRC-0087: On-chain Deal Aggregation #879

aashidham · 2023-12-07T16:14:48Z

Create aggregation FRC. More edits to come before finalization.

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

small edits

willscott · 2023-12-07T17:00:34Z

FRCs/frc-0070.md

+
+Recommended API interface for the two retrieval methods mentioned above:
+
+- Retrieval endpoint for Filecoin retrieval: GET /data/<pieceCID>


Is there an example of a market implementation on filecoin that supports data at this path?

I believe boost would expose these pieces at /piece/<pieceCID>, fwiw

We should also specify the expectation for how an aggregator supporting this standard would define where it's HTTPS endpoint for the data layer is located.

there is no market impl on this today, lighthouse/spheron are supporting retrieval via Ipfs pinning instead of Filecoin, but they want to experiment retrieving from Filecoin in the near future.
Im open to aligning with boost endpoint for consistency (GET at /piece/pieceCID)

jsoares

I'm confused about the scope of this PR.

It's adding two new files (FRC-0067.md and FRC-0070.md)
FRC-0067.md seems to correspond to existing FRC-0068.md
FRC-0070.md claims to be FRC-0068 in the metadata, is using an unassigned and conflicting FIP number in filename, and has seemingly the same title as FRC-0068.

I think part of this is due to having been branched out of an older branch, but can we clean this up prior to substantive review? There should only be one RFC here, no unassigned numbers, and no recommitting of existing files. The metadata for the new RFC should match its content.

FRCs/frc-0070.md

Co-authored-by: Jorge Soares <547492+jsoares@users.noreply.github.com>

rvagg · 2023-12-17T23:24:27Z

FRCs/frc-xxxx.md

+- Retrieval endpoint for Filecoin retrieval: GET /data/<pieceCID>
+- Retrieval gateway for IPFS retrieval: https://gateway/ipfs/<pieceCID>


I think this is using the client's piece CID here, rather than the aggregated piece CID, correct? I like the use of commPc above to differentiate from commPa, perhaps that could be used here to disambiguate what this is referring to? They look very close to existing boost retrieval endpoints, with /piece/ instead of /data/ today, but those are for stored deal pieces, of commPa (I don't think we do anything fancy with /piece/ retrievals on PoDSI deals today yet anyway).

Yes this is referring to client piece CID. We had a call with Will to align on these, so that it is consistent with the piece retrieval gateway FRC.

Note that this PR is not updated (@aashidham will be updating it soon), you can see the latest draft for retrieval section here #866

Fixed here b2fcc65

rvagg · 2023-12-17T23:24:56Z

FRCs/frc-xxxx.md

+        uint64 providerId;  // The storage provider that is storing the data for the deal.
+				uint64 marketActorId; // The actor ID of the storage market in which the deal representation is stored. dealId is scoped by marketActorId


Suggested change

uint64 providerId; // The storage provider that is storing the data for the deal.

uint64 marketActorId; // The actor ID of the storage market in which the deal representation is stored. dealId is scoped by marketActorId

uint64 providerId; // The storage provider that is storing the data for the deal.

uint64 marketActorId; // The actor ID of the storage market in which the deal representation is stored. dealId is scoped by marketActorId

Fixed here 522c607

rvagg · 2023-12-17T23:25:32Z

FRCs/frc-xxxx.md

+    event SubmitAggregatorRequestWithRaaS(uint256 indexed id, bytes cid,
+																					uint256 _replication_target, uint256 _repair_threshold,
+																					uint256 _renew_threshold);


Suggested change

event SubmitAggregatorRequestWithRaaS(uint256 indexed id, bytes cid,

uint256 _replication_target, uint256 _repair_threshold,

uint256 _renew_threshold);

event SubmitAggregatorRequestWithRaaS(uint256 indexed id, bytes cid, uint256 _replication_target, uint256 _repair_threshold, uint256 _renew_threshold);

Fixed here 522c607

@honghaoq doesn't seem to be fixed yet, it's fixed in IOnchainDataAggregator but not IOffchainDataAggregator

Done 1d850a2 Good catch:)

rvagg · 2023-12-17T23:26:18Z

FRCs/frc-xxxx.md

+		/**
+     * @notice Function to submit a new file to the aggregator, specifing the raas parameters


Suggested change

/**

* @notice Function to submit a new file to the aggregator, specifing the raas parameters

/**

* @notice Function to submit a new file to the aggregator, specifing the raas parameters

Fixed here 522c607

rvagg · 2023-12-17T23:26:48Z

FRCs/frc-xxxx.md

+     */
+		function submitRaaS(bytes memory _cid, bytes memory _fetchLink,
+												 uint256 memory _replication_target, uint256 memory _repair_threshold,
+									       uint256 memory _renew_threshold);


Suggested change

*/

function submitRaaS(bytes memory _cid, bytes memory _fetchLink,

uint256 memory _replication_target, uint256 memory _repair_threshold,

uint256 memory _renew_threshold);

*/

function submitRaaS(bytes memory _cid, bytes memory _fetchLink, uint256 memory _replication_target, uint256 memory _repair_threshold, uint256 memory _renew_threshold);

Fixed here 522c607

Updated data retrieval section

willscott

This looks like a reasonable reflection of how data segment indexed data is being structured and made accessible at least in boost. I am in favor of the FIP editors merging this FRC

honghaoq · 2024-02-01T18:32:50Z

This looks like a reasonable reflection of how data segment indexed data is being structured and made accessible at least in boost. I am in favor of the FIP editors merging this FRC

Thanks @willscott, can you approve in the code review, looks like it needs 3 approvals to be able to merge

willscott · 2024-02-02T14:53:55Z

can you approve in the code review, looks like it needs 3 approvals to be able to merge

I'm not able to approve - you need two fip editors / code owners, as I understand the process.

anorth · 2024-02-05T20:48:43Z

I'm assigning number 0087 to this FRC. Please update the header and filename, and also add an appropriate line to the table in README.md

Update as per Alex's comment

honghaoq · 2024-02-08T21:18:07Z

I'm assigning number 0087 to this FRC. Please update the header and filename, and also add an appropriate line to the table in README.md

Updated: fdd0dc7
and FIP readme PR: https://github.com/filecoin-project/FIPs/pull/937/files

@anorth can you help approve it? my understanding is we need your approval so @aashidham can merge this PR, thanks

anorth · 2024-02-08T21:26:08Z

Please rename the file to frc-0087. If you are able, please also change the README in this same PR so they land together.

anorth · 2024-02-08T21:27:28Z

FRCs/frc-xxxx.md

+created: 2023-10-07
+---
+
+# FVM-Enabled Deal Aggregation


This title doesn't match the title metadata. Please make them consistent, and also with the title listed in the README

changed title metadata

added 0087 to the readme

made authors list consistent

aashidham · 2024-02-09T15:35:29Z

Title metadata, README and file name have all been altered to reflect the latest.

@honghaoq your edits were in a separate patch branch, you have privileges to make changes directly on aggregation-frc branch. I do not have privileges to merge this PR Honghao, that is in the CODEOWNERS purview.

@anorth have a look and let me know if things are good to merge

aadi and others added 8 commits April 25, 2023 23:19

deal making FRC added

cc2bc2b

added images of deal flow

e913fea

Update frc-nnnn-deal-making.md

839c7f5

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

adherence to template and more copy changes

f262cb1

Update and rename frc-nnnn-deal-making.md to 0067.md

05e3942

Rename 0067.md to frc-0067.md

5d30951

Merge branch 'filecoin-project:master' into master

1a6eba8

Create frc-0070.md

21ae7d0

aashidham requested review from momack2, arajasek, jennijuju, kaitlin-beegle, anorth, raulk and jsoares as code owners December 7, 2023 16:14

aashidham added 4 commits December 7, 2023 11:43

Update frc-0070.md

b675220

small edits

Update frc-0070.md

65a8269

Update frc-0070.md

8bc1f1a

Update frc-0070.md

1d91929

willscott reviewed Dec 7, 2023

View reviewed changes

jsoares requested changes Dec 13, 2023

View reviewed changes

FRCs/frc-0070.md Outdated Show resolved Hide resolved

FRCs/frc-0070.md Outdated Show resolved Hide resolved

aashidham and others added 4 commits December 13, 2023 11:42

Update FRCs/frc-0070.md

2fdaf3a

Co-authored-by: Jorge Soares <547492+jsoares@users.noreply.github.com>

Update FRCs/frc-0070.md

615fa0b

Co-authored-by: Jorge Soares <547492+jsoares@users.noreply.github.com>

Delete FRCs/frc-0067.md

c9c7949

Rename frc-0070.md to frc-xxxx.md

52cb20e

rvagg reviewed Dec 17, 2023

View reviewed changes

Update frc-xxxx.md

b2fcc65

Updated data retrieval section

honghaoq approved these changes Jan 19, 2024

View reviewed changes

willscott reviewed Jan 24, 2024

View reviewed changes

honghaoq and others added 4 commits January 26, 2024 02:41

Update frc-xxxx.md

1d850a2

Merge branch 'filecoin-project:master' into aggregation-frc

b272da5

Updates on some function signatures

d284dd2

Update frc-xxxx.md

01e7d7b

kaitlin-beegle approved these changes Feb 1, 2024

View reviewed changes

anorth changed the title ~~Aggregation frc~~ FRC-0087: Aggregation Feb 5, 2024

anorth changed the title ~~FRC-0087: Aggregation~~ FRC-0087: Deal Aggregation Feb 5, 2024

anorth changed the title ~~FRC-0087: Deal Aggregation~~ FRC-0087: On-chain Deal Aggregation Feb 5, 2024

Update frc-xxxx.md

fdd0dc7

Update as per Alex's comment

honghaoq approved these changes Feb 8, 2024

View reviewed changes

Update frc-xxxx.md

6f3c17e

honghaoq approved these changes Feb 8, 2024

View reviewed changes

anorth mentioned this pull request Feb 8, 2024

Update README.md #937

Closed

anorth reviewed Feb 8, 2024

View reviewed changes

aashidham added 5 commits February 9, 2024 08:21

Rename frc-xxxx.md to frc-0087.md

43a7e99

Update frc-0087.md

29712f8

changed title metadata

Update README.md

e78cded

added 0087 to the readme

Update frc-0087.md

27defc1

made authors list consistent

Update README.md

e1b903a

anorth approved these changes Feb 12, 2024

View reviewed changes

anorth merged commit 7c80fad into filecoin-project:master Feb 12, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FRC-0087: On-chain Deal Aggregation #879

FRC-0087: On-chain Deal Aggregation #879

aashidham commented Dec 7, 2023

willscott Dec 7, 2023

honghaoq Dec 7, 2023 •

edited

Loading

jsoares left a comment •

edited

Loading

rvagg Dec 17, 2023

honghaoq Dec 19, 2023 •

edited

Loading

honghaoq Jan 18, 2024 •

edited

Loading

rvagg Dec 17, 2023

honghaoq Jan 18, 2024 •

edited

Loading

rvagg Dec 17, 2023

honghaoq Jan 18, 2024

rvagg Jan 25, 2024

honghaoq Jan 26, 2024

rvagg Dec 17, 2023

honghaoq Jan 18, 2024 •

edited

Loading

rvagg Dec 17, 2023 •

edited

Loading

honghaoq Jan 18, 2024 •

edited

Loading

willscott left a comment

honghaoq commented Feb 1, 2024

willscott commented Feb 2, 2024

anorth commented Feb 5, 2024

honghaoq commented Feb 8, 2024

anorth commented Feb 8, 2024

anorth Feb 8, 2024

aashidham commented Feb 9, 2024


		Recommended API interface for the two retrieval methods mentioned above:

		- Retrieval endpoint for Filecoin retrieval: GET /data/<pieceCID>

		- Retrieval endpoint for Filecoin retrieval: GET /data/<pieceCID>
		- Retrieval gateway for IPFS retrieval: https://gateway/ipfs/<pieceCID>

		uint64 providerId; // The storage provider that is storing the data for the deal.
		uint64 marketActorId; // The actor ID of the storage market in which the deal representation is stored. dealId is scoped by marketActorId

		/**
		* @notice Function to submit a new file to the aggregator, specifing the raas parameters

FRC-0087: On-chain Deal Aggregation #879

FRC-0087: On-chain Deal Aggregation #879

Conversation

aashidham commented Dec 7, 2023

Choose a reason for hiding this comment

honghaoq Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

jsoares left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honghaoq Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

honghaoq Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honghaoq Jan 18, 2024 • 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

honghaoq Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

rvagg Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

honghaoq Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

willscott left a comment

Choose a reason for hiding this comment

honghaoq commented Feb 1, 2024

willscott commented Feb 2, 2024

anorth commented Feb 5, 2024

honghaoq commented Feb 8, 2024

anorth commented Feb 8, 2024

Choose a reason for hiding this comment

aashidham commented Feb 9, 2024

honghaoq Dec 7, 2023 •

edited

Loading

jsoares left a comment •

edited

Loading

honghaoq Dec 19, 2023 •

edited

Loading

honghaoq Jan 18, 2024 •

edited

Loading

honghaoq Jan 18, 2024 •

edited

Loading

honghaoq Jan 18, 2024 •

edited

Loading

rvagg Dec 17, 2023 •

edited

Loading

honghaoq Jan 18, 2024 •

edited

Loading