Skip to content

Conversation

rjan90
Copy link
Collaborator

@rjan90 rjan90 commented Oct 7, 2025

On top of #680

  • Adapt to PDP v2.2.0 breaking changes for dataset creation
  • Re-enable and update createDataSetCmd to use combined dataset creation + piece addition flow

rjan90 added 2 commits October 7, 2025 12:46
chore(pdp): M3 Calibnet contract addresses
chore: update ABIs and ABIgen with make
@rjan90 rjan90 changed the title chore: fix breaking changes chore: fix breaking changes due to PDP v2.2.0 Oct 7, 2025
@rjan90 rjan90 requested a review from Copilot October 8, 2025 05:58
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses breaking changes introduced in PDP v2.2.0 by updating the codebase to accommodate API changes in the PDP contract. The main changes involve removing the deprecated createDataSet() function, updating the addPieces() method signature, and simplifying the calculateProofFee() function call.

  • Removed createDataSet() functionality as it no longer exists in PDP v2.2.0
  • Updated addPieces() to include the required listener address parameter
  • Simplified calculateProofFee() call by removing the gas fee estimate parameter

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
tasks/pdp/task_prove.go Updates calculateProofFee call to match simplified v2.2.0 signature
pdp/handlers.go Disables createDataSet endpoint and updates addPieces with listener address
cmd/pdptool/main.go Comments out createDataSet CLI command due to API removal

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

"strconv"
"strings"

"github.com/ethereum/go-ethereum/accounts/abi/bind"
Copy link

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

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

[nitpick] The new import for bind is only used once in the file. Consider evaluating if this import adds significant overhead or if there's a more lightweight alternative for the single bind.CallOpts{} usage.

Copilot uses AI. Check for mistakes.

fix: remove redundnat .json files and add comment on where these ABIs can be found
@rjan90 rjan90 force-pushed the phi/PDP-breaking-changes branch 2 times, most recently from 9b1157e to d896afc Compare October 8, 2025 06:53
Resolve PDP v2.2.0 breaking changes
@rjan90 rjan90 force-pushed the phi/PDP-breaking-changes branch from cfb6e10 to ef848cf Compare October 8, 2025 07:01
@rjan90 rjan90 marked this pull request as ready for review October 8, 2025 07:01
@rjan90 rjan90 requested a review from a team as a code owner October 8, 2025 07:01
@rjan90 rjan90 requested a review from rvagg October 8, 2025 07:02
Copy link
Contributor

@LexLuthr LexLuthr left a comment

Choose a reason for hiding this comment

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

If we plan to preserve pdptool then we should update it or rip the binary completely instead of leaving the dead code.

// Datasets are now created implicitly when adding the first piece via addPieces
// To create a dataset, use the add-pieces command with your first piece
/* DISABLED - createDataSetCmd removed in PDP v2.2.0
var createDataSetCmd = &cli.Command{
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably update this command instead of deleting it as there is no way to create dataSet now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense, I have updated the createDataSetCmd in 7666fb4.

rjan90 added 2 commits October 8, 2025 09:29
…ece addition flow

Re-enable and update createDataSetCmd to use combined dataset creation + piece addition flow
…ist.

fix: lint error by adding the createDataSetCmd back to the commands list.
@rvagg
Copy link
Member

rvagg commented Oct 8, 2025

Sadly this isn't going to work and it's going to require more surgery than this.

handleGetDataSetCreationStatus requires a data set id and for it to exist in pdp_data_sets, which will fail (chicken and horse).

There's 2 paths here:

  1. Data set doesn't exist, first pieces should create it
  2. Data set exists, add pieces to that data set

In #678 I suggested that we should keep the POST /pdp/data-sets (handleCreateDataSet) to create and add pieces, but the alternative is to delete that and use a data set ID of 0 to indicate "not created yet" and use POST /pdp/data-sets/0/pieces (this is the one that leads to handleAddPieceToDataSet) to do both jobs - and the fact that it's 0 is a special case that causes it to skip the "do I have this?" check and instead flip into "create+add" mode. Otherwise if it's a valid ID that I have in my db then it's just "add" mode.

I don't really mind which way, perhaps it's a little cleaner if we just delete handleCreateDataSet and go with 0 rather than the path I outlined in #678 and @virajbhartiya started on in #687.

So I think that @virajbhartiya you should start on this branch and make it work with new data sets, where 0 is a special case. Trace through handleAddPieceToDataSet and figure out where it's going to fail with 0.

But you also need to get an end-to-end setup going so you can try this out with the new contracts that it's supposed to work with and Synapse that it's supposed to interact with so we're not just guessing that it might work.

@rjan90
Copy link
Collaborator Author

rjan90 commented Oct 8, 2025

So I think that @virajbhartiya you should start on this branch and make it work with new data sets, where 0 is a special case. Trace through handleAddPieceToDataSet and figure out where it's going to fail with 0.

But you also need to get an end-to-end setup going so you can try this out with the new contracts that it's supposed to work with and Synapse that it's supposed to interact with so we're not just guessing that it might work.

Yes, please feel free to take over this @virajbhartiya. I know @TippyFlitsUK can be helpful in updating his PDP SP to whatever branch if that helps you get a E2E setup you can test against, just give him a nudge in this Slack thread once you have the updates landed in, and tell him which branch to update to.

@rvagg
Copy link
Member

rvagg commented Oct 8, 2025

Better not point an existing SP using our current contracts to this branch, it needs to be a fresh SP without any existing state wrt data sets cause this is a fresh deploy of PDPVerifier etc.

@rvagg
Copy link
Member

rvagg commented Oct 8, 2025

Which is why I suggested Viraj use https://www.notion.so/filecoindev/FWSS-End-to-end-Testing-Setup-265dc41950c1808082e8c5f9940a04c9#265dc41950c1808082e8c5f9940a04c9

alternatively, if @TippyFlitsUK wants to set up yet another SP to help that's fine, but I'd prefer devs be able to do their own end-to-end locally so we're not just guessing and/or waiting for others.

@rjan90 rjan90 force-pushed the phi/update-contract-calibnet branch from e188e1f to ec0db84 Compare October 9, 2025 07:31
@rjan90
Copy link
Collaborator Author

rjan90 commented Oct 10, 2025

Support create and upload will be fixed in #692. Closing this PR

@rjan90 rjan90 closed this Oct 10, 2025
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.

3 participants