feature: implement remote pinning API #3588

yusefnapora · 2021-03-10T23:19:32Z

This is an implementation of the ipfs.pin.remote API as well as I can gather from reading the tests in interface-ipfs-core.

I haven't figured out how to actually run the tests yet, so we'll see 😄

Leaving as a draft for now, since I probably missed some stuff and am out of steam for the day. Also it's depending on an unpublished npm package (the js-ipfs-pinning-service-client I wrote earlier) so we should sort that out.

cc @Gozala :)

- on my machine, at least :)

yusefnapora · 2021-03-12T15:55:32Z

Hey @Gozala & @alanshaw, I think this is basically ready to review, but there are a couple things that I'd like to sort out before merging.

First is the js-ipfs-pinning-service-client package, which isn't published to NPM and is just living in a github repo under my personal username. Would this belong in js-ipfs as one of the subpackages? I can pull it into this PR if so. Otherwise I guess it should at least get moved to the ipfs github org or similar and published to npm.

Second, I didn't implement saving / loading pinning service configs to the main IPFS config, although the config service instance does get passed through to the right spot, so it should't be too hard to wire up. The cli commands (js-ipfs pin remote add, etc) are also unimplemented, but maybe that could wait for a separate PR.

Anyway, things seem to have reached "works on my machine" status, so it would be great if someone else could check it out and kick the tires a bit.

lidel

quick drive-by comments :)

Perhaps as a part of this PR you could add a simple browser example under examples/browser-remote-pinning with:

input for CID (or a file picker, so user can ipfs.add a file and fill CID field that way)
inputs for pinning service Endpoint and Key
"pin!" button

It would make testing against different services easier + will be a useful artifact for the community that wants to add support for remote pinning to their dapp.

First is the js-ipfs-pinning-service-client package, which isn't published to NPM and is just living in a github repo under my personal username. Would this belong in js-ipfs as one of the subpackages?

I don't think it should be part of js-ipfs, it will be useful for people who don't run IPFS node at all (ipfs/pinning-services-api-spec#43)
We already have go counterpart at https://github.com/ipfs/go-pinning-service-http-client, so perhaps move it to https://github.com/ipfs/ and then publish to NPM, so I can add it to https://github.com/ipfs/pinning-services-api-spec#client-libraries :)

yusefnapora · 2021-03-22T21:14:08Z

@lidel, that's a good idea about the example. I haven't tested this in browsers yet, so that will be good proof that it works :) I'll try to add the example in tomorrow.

I agree that the pinning HTTP client is useful outside of js-ipfs, so it makes sense to put it into the ipfs github org. I'd like to add some tests to the client first, but that shouldn't take long. Then someone could review it and pull it into the ipfs org and do the npm thing. I'll send a flare up tomorrow or Wednesday for review :)

Gozala

Thanks @yusefnapora again for driving this effort, I am very excited about this making it's way to js-ipfs.

I did a first pass and left some notes. Overall it looks good but there are few things we'd need to figure out before we can move this forward

Separate generated pinning service client. I am not sure if having it in separate package provides enough value to justify added overhead of coordinating changes.
Generated client pulls in different http library that would increase bundle size in browser and probably increase a maintenance burden. I think we would want to reuse existing http code used in rest of the code base instead (and that may imply not generating, but writing client code). However ultimately it's @achingbrain call.
Currently things are not persisted across sessions, which I think we should add before landing this.
We already have interface and type definitions in place (see https://github.com/ipfs/js-ipfs/blob/master/packages/ipfs-core-types/src/pin/remote.ts) and it would be good to utilize them instead of duplicate them. Specifically @implements {API} allows us to ensure that same API is implanted across HTTP client and js-ipfs. If we need to update types that's fine, but we should try to reuse and comply to them unless there's a good reason not to.
I think we would prefer to not introduce another code style. I hate been this person as I've personally been pushing to revisiting current dependency injection style used to plain functions wrapped in class based sugar approach outlined here Incidental Complexity: Dependency injection is getting in the way #3391. For the reference remote pinning service API on IPFS client follows it https://github.com/ipfs/js-ipfs/blob/master/packages/ipfs-http-client/src/pin/remote/index.js

I realize there's a lot here, but I'm available to help with moving certain pieces.

Gozala · 2021-03-22T23:55:18Z

packages/ipfs-core/package.json

@@ -89,6 +89,7 @@
    "it-first": "^1.0.4",
    "it-last": "^1.0.4",
    "it-pipe": "^1.1.0",
+    "js-ipfs-pinning-service-client": "github:yusefnapora/js-ipfs-pinning-service-client#874c6ec7875ec2b3039c2361638c54158e7e4874",


I think we would want to migrate it into this monorepo possibly as a separate package, but only if having just pinning service client on it's own provides some value. If it needs to be used in conjunction with js-ipfs anyway it's probably best to just fold it under ipfs-core to reduce maintenance overhead.

I've also skimmed over https://github.com/yusefnapora/js-ipfs-pinning-service-client/blob/master/generated/src/ApiClient.js, it generates code that pulls in separate http library that we would most likely want to avoid as it would further increase bundle size and would be more maintenance burden.

packages/ipfs-core/src/components/pin/remote.js

yusefnapora · 2021-03-26T16:58:08Z

@Gozala thanks for all the review feedback 👍

I'm fine with refactoring to fit the current style; I was kind of confused by it when I first started hacking, but I think I'm getting it now 😄

I also kind of hate the generated client for the same reasons you're raising... plus I just don't really like the code it generates and would be happy to ditch it. The HTTP API itself is pretty simple, so it's not hard to write a client using any HTTP request library. I'll plan on just doing that, and I can just leave the js client on my github org for now. We can always move it to shipyard or something if people start using it in other projects.

I'll plan to hack on that and implement the config persistence, and I'll ping you next week sometime for another review pass.

Gozala

Thanks @yusefnapora for all the work that went into it. I think it's mostly there, I made bunch of minor comments to improve things here and there. There is a thing however that I think need to be addressed

Unless I've missed something, it appears to me that the way things are setup ipfs.pin.remote.* calls will fail in a next sessions, because config isn't going to be loaded.

And a before thing that I think would be really nice to have, but doesn't need to block this:

pin/remote/client.js seems unnecessary and only complicates things at this point. I think you end up with it because client used to be a separate lib, now that it's not the case it would make more sense to just have that logic in add/rm/ls/* and just pull endpoint/key instead.

Gozala · 2021-03-30T00:37:14Z

packages/ipfs-core/src/components/index.js

@@ -86,7 +89,8 @@ class IPFS {
    })
    const resolve = createResolveAPI({ ipld, name })
    const pinManager = new PinManagerAPI({ repo, dagReader })
-    const pin = new PinAPI({ gcLock, pinManager, dagReader })
+    const pinRemote = new PinRemoteAPI({ swarm, config, peerId })
+    const pin = new PinAPI({ gcLock, pinManager, dagReader, pinRemote })


Since pinRemote is not used here and is only created to be passed to PinAPI I would suggest instead passing config, swarm and peerId and construct PinRemoteAPI in PinAPI instead.

Gozala · 2021-04-19T21:21:27Z

packages/ipfs-core/src/components/config.js

+ * @typedef {Object} RemotePinningServiceConfig
+ * @property {Object} API
+ * @property {string} API.Endpoint
+ * @property {string} API.Key


go-ipfs settled on omitting endpoint keys when printing config so that they aren't easy to leak (See https://github.com/ipfs/go-ipfs/pull/7661/files#r537641281)

We should at least do the same, although @lidel suggested something smarter is needed in the long term, not sure what exactly he meant but it might be worth doing that here.

The smarter thing to do is to move all secrets to separate files outside the JSON config.
The only secret right now is Identity.PrivKey this PR adds second type of secret under Pinning.RemoteServices[*].API.Key.
I may be less work to move secrets outside, than to sanitize the output of ipfs config and make sure ipfs config replace does not forget the secrets.

Gozala · 2021-04-19T21:28:17Z

packages/ipfs-core/src/components/pin/remote/service.js

+   * @param {Credentials & AbortOptions} credentials
+   */
+  async function add (name, credentials) {
+    await loadConfig()


NIT (Feel free to disregard): I would very much prefer if the state was encapsulated by something like a PinningConfig such that this would be more in the lines of:

const services = await pinningConfig.use() const service = services[name]

Gozala · 2021-04-19T21:40:35Z

packages/ipfs-core/src/components/pin/remote/service.js

+      throw new Error('parameter "name" is required')
+    }
+    await loadConfig()
+    delete clients[name]


This probably not big deal here, but concurrency implications of this make me unease. E.g. concurrent add & rm calls may end up in a race condition where one may undo result of the other. However probability seems low enough that adding task queuing is probably not worth it 😳

Gozala · 2021-04-19T21:44:42Z

packages/ipfs-core/src/components/pin/remote/service.js

+   * List the configured remote pinning services.
+   *
+   * @param {{stat: ?boolean} & AbortOptions} opts
+   * @returns {Promise<Array<RemotePinService> | Array<RemotePinServiceWithStat>>} - a Promise resolving to an array of objects describing the configured remote pinning services. If stat==true, each object will include more detailed status info, including the number of pins for each pin status.


I think you should be able to type it as:

Suggested change

* @returns {Promise<Array<RemotePinService> | Array<RemotePinServiceWithStat>>} - a Promise resolving to an array of objects describing the configured remote pinning services. If stat==true, each object will include more detailed status info, including the number of pins for each pin status.

* @returns {Promise<Array<Options['stat'] extends true ? RemotePinServiceWithStat : RemotePinService>>} - a Promise resolving to an array of objects describing the configured remote pinning services. If stat==true, each object will include more detailed status info, including the number of pins for each pin status.

Gozala · 2021-04-20T00:03:23Z

packages/ipfs-core/src/components/pin/remote/client.js

+    const total = body.count
+    let yielded = 0
+    while (true) {
+      if (body.results.length < 1) {


This seems redundunt if it's empty next loop will be noon and next if clause will return and if doesn't it's probably better to fail.

Gozala · 2021-04-20T00:12:10Z

packages/ipfs-core/src/components/pin/remote/client.js

+        throw new Error('pin failed: ' + JSON.stringify(pinResponse.info))
+      }
+
+      await delay(pollIntervalMs)


I wonder if we should make pollInterval part of the optional add options so it could be configured if necessary to longer / shorter interval.

Gozala · 2021-04-20T00:16:55Z

packages/ipfs-core/src/components/pin/remote/rmAll.js

+   */
+  async function rmAll (options) {
+    const { service } = options
+    const svc = serviceRegistry.serviceNamed(service)


Unless I'm missing something this will return null unload config was loaded which may not be the case here.

Gozala · 2021-04-20T00:17:36Z

packages/ipfs-core/src/components/pin/remote/ls.js

+   */
+  async function * ls (options) {
+    const { service } = options
+    const svc = serviceRegistry.serviceNamed(service)


Unless I'm missing something this will return null unload config was loaded which may not be the case here.

Gozala · 2021-04-20T00:17:48Z

packages/ipfs-core/src/components/pin/remote/rm.js

+   */
+  async function rm (options) {
+    const { service } = options
+    const svc = serviceRegistry.serviceNamed(service)


Unless I'm missing something this will return null unload config was loaded which may not be the case here.

lidel · 2021-04-30T01:16:20Z

packages/ipfs-core/src/components/pin/remote/client.js

+  }
+
+  async function awaitPinCompletion (pinResponse) {
+    const pollIntervalMs = 100


https://github.com/ipfs/go-ipfs/blob/v0.8.0/core/commands/pin/remotepin.go#L224:

Suggested change

const pollIntervalMs = 100

const pollIntervalMs = 500

BigLep · 2021-05-03T14:32:12Z

@yusefnapora : what's the plan for this PR? Will you be working land it? My goal is to understand the next steps. Thanks!

lidel · 2021-05-10T14:15:01Z

Closing until protocol/web3-dev-team#58 is picked up by one of project teams

brickpop · 2022-05-16T12:19:28Z

Any chance that the PR is ever considered? Pinning data on a remote node is very important for us
Thank you

yusefnapora added 4 commits March 10, 2021 12:56

stub out remote pin api

7291eab

implement remote pinning API

9c9a1e7

implement timeouts for remote pin methods

e3d9683

fix pin.remote.ls

93cb6ee

This comment has been minimized.

Sign in to view

yusefnapora added 7 commits March 11, 2021 11:24

add pin.remote tests to tests-interface-core.js

bdb3d1e

fix lint issues

2e232b4

mv remote/index.js remote.js

a1f7d8f

use commit hash for pinning client dependency

a7b6e84

make interface-core tests pass

0047a58

- on my machine, at least :)

bump client dependency

a2e9846

use default status "pinned" if none provided in ls

cd57853

yusefnapora mentioned this pull request Mar 12, 2021

use js-ipfs remote pinning api yusefnapora/minty#3

Closed

yusefnapora marked this pull request as ready for review March 12, 2021 15:55

BigLep added the status/in-progress In progress label Mar 15, 2021

lidel reviewed Mar 19, 2021

View reviewed changes

Gozala suggested changes Mar 23, 2021

View reviewed changes

lidel mentioned this pull request Mar 24, 2021

Remote pinning service implementation in JS-IPFS protocol/web3-dev-team#58

Open

yusefnapora added 9 commits March 26, 2021 15:08

wip - refactor code style

2bb4779

wip - more refactoring

4ce386f

remove pinning client dependency

2808e3e

fix lint issues

a64118e

fix timeout for remote service stat

0fa8549

add Pinning.RemoteServices to IpfsConfig

1648b22

refactor class into lambda factory fn

c01fe6c

rm unused type imports

47713cc

move serviceNamed fn outside of pin.remote.service api object

da1ef9d

consolidate input validation

b8634c3

Gozala suggested changes Apr 20, 2021

View reviewed changes

lidel mentioned this pull request Apr 21, 2021

Proposal: Gateway++ Phase 1 protocol/web3-dev-team#100

Closed

achingbrain mentioned this pull request Apr 23, 2021

Remote services missing from ipfs-core ? #3626

Closed

lidel suggested changes Apr 30, 2021

View reviewed changes

lidel mentioned this pull request May 5, 2021

Document need for /wss on delegates list ipfs/pinning-services-api-spec#77

Open

lidel closed this May 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: implement remote pinning API #3588

feature: implement remote pinning API #3588

yusefnapora commented Mar 10, 2021

This comment has been minimized.

yusefnapora commented Mar 12, 2021

lidel left a comment

yusefnapora commented Mar 22, 2021

Gozala left a comment

Gozala Mar 22, 2021

yusefnapora commented Mar 26, 2021 •

edited

Loading

Gozala left a comment

Gozala Mar 30, 2021

Gozala Apr 19, 2021

lidel Apr 30, 2021

Gozala Apr 19, 2021

Gozala Apr 19, 2021

Gozala Apr 19, 2021

Gozala Apr 20, 2021

Gozala Apr 20, 2021

Gozala Apr 20, 2021

Gozala Apr 20, 2021

Gozala Apr 20, 2021

lidel Apr 30, 2021

BigLep commented May 3, 2021

lidel commented May 10, 2021

brickpop commented May 16, 2022

	* @returns {Promise<Array<RemotePinService> \| Array<RemotePinServiceWithStat>>} - a Promise resolving to an array of objects describing the configured remote pinning services. If stat==true, each object will include more detailed status info, including the number of pins for each pin status.
	* @returns {Promise<Array<Options['stat'] extends true ? RemotePinServiceWithStat : RemotePinService>>} - a Promise resolving to an array of objects describing the configured remote pinning services. If stat==true, each object will include more detailed status info, including the number of pins for each pin status.

feature: implement remote pinning API #3588

feature: implement remote pinning API #3588

Conversation

yusefnapora commented Mar 10, 2021

This comment has been minimized.

yusefnapora commented Mar 12, 2021

lidel left a comment

Choose a reason for hiding this comment

yusefnapora commented Mar 22, 2021

Gozala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yusefnapora commented Mar 26, 2021 • edited Loading

Gozala left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigLep commented May 3, 2021

lidel commented May 10, 2021

brickpop commented May 16, 2022

yusefnapora commented Mar 26, 2021 •

edited

Loading