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

Feature proposal: testing replace endpoint with the same CID #124

Open
2color opened this issue Jun 21, 2022 · 3 comments
Open

Feature proposal: testing replace endpoint with the same CID #124

2color opened this issue Jun 21, 2022 · 3 comments
Labels
area/compliance check An issue having to do with an existing, or new, pinning compliance check effort/hours exp/beginner kind/enhancement

Comments

@2color
Copy link

2color commented Jun 21, 2022

It should be possible to update a request ID with the same CID using the POST https://pinning-service.example.com/pins/{requestid} endpoint, for example when wanting to change the name, meta, or origins.

Currently, web3.storage returns an error when attempting to do that: {"reason":"PSA_INVALID_DATA","details":"Existing and replacement CID are the same."}.

@SgtPooki
Copy link
Member

It sounds like what you want is to update an existing Pin. The spec actually doesn't specify an update method. The replace pin object is supposed to be sugar for "Using requestid X, delete reference to CID & instead point to this CID". See https://ipfs.github.io/pinning-services-api-spec/#tag/pins/paths/~1pins~1{requestid}/post

So technically, what web3 is doing is to spec. However, I feel like there should be an update method. Otherwise, we have to:

  1. Create an entirely new pin, with the same CID, with the newly requested data.
    • Per the spec, pinning services should allow this. requestid is the unique identifier, not CID.
    • This will give us an entirely new requestid
  2. Delete the old pin
  3. Update any references in database in your app to use the new requestid

I don't know if we want to use replacePin for that. Replace implies that the CID we're pinning is being replaced, i.e. a new CID, not the meta properties (i.e. name, meta, and origins). However, idk if the semantics of replace/update are important enough to warrant an entirely new method, or if we should just overload the replace method.

Before updating this, we should get buy-in from all pinning-services and ensure the spec is up to date.

Once we do get update functionality, we could add the following checks similar to https://github.com/ipfs-shipyard/pinning-service-compliance/blob/main/src/checks/edit/replacePin.ts

  • Name can be updated
  • meta fields can be modified (to avoid large spec rewrite, this should be a full replacement of any existing meta properties, so clients should GET, and modify, then POST)
  • origins can be suggested (ideally, the pinning provider would validate any suggested origins, and not take them on faith)

@lidel any thoughts? I also opened ipfs/pinning-services-api-spec#96 in the spec repo.

@lidel
Copy link
Member

lidel commented Jun 22, 2022

@SgtPooki
Copy link
Member

@lidel i think replace should drop the “required” for CID. As an engineer, that makes it feel like replace is requiring a new CID.

If the CID is required, and it’s the same CID, why am i sending it?

If its different, i know i need to send it... Otherwise what exactly am i trying to do?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/compliance check An issue having to do with an existing, or new, pinning compliance check effort/hours exp/beginner kind/enhancement
Projects
None yet
Development

No branches or pull requests

3 participants