feat: access-api forwards store/ and upload/ invocations to upload-api #334

gobengo · 2023-01-11T04:51:22Z

Motivation:

make it so access-api can proxy store and upload invocations to upload-api #331

…env var is set

…d-upload-invocations-to-upload-api

gobengo · 2023-01-12T01:16:47Z

packages/access-api/test/store-list.js

+        // which will allow the access-api ucanto server to accept
+        // invocations where aud=web3storageDid
+        DID: web3storageDid,
+        PRIVATE_KEY: privateKeyFromEnv ?? process.env.PRIVATE_KEY,


some of this extra stuff is in here because I wanted to support running the tests with the production private key, which works with a non-error result. But if you don't pass that in, the assertions shift slightly to ensuring that it's an error result of the kind that would happen only if the server signer is wrong

Note how this test was run with WEB3_STORAGE_PRIVATE_KEY:

bengo@bengo ~/protocol.ai/w3protocol/packages/access-api ⚡ DID="$PROD_DID_KEY" WEB3_STORAGE_PRIVATE_KEY="$PROD_PRIVATE_KEY" pnpm test -- -f 'upload-api' > @web3-storage/[email protected] test /Users/bengo/protocol.ai/w3protocol/packages/access-api > pnpm build && mocha --bail --timeout 10s -n no-warnings -n experimental-vm-modules "-f" "upload-api" > @web3-storage/[email protected] build /Users/bengo/protocol.ai/w3protocol/packages/access-api > scripts/cli.js build proxy store/list invocations to upload-api ✔ forwards store/list invocations with aud=did:key (648ms) ✔ forwards invocations with aud=did:web:web3.storage (460ms) ✔ errors when a bad delegation is given as proof (320ms) Store.all ✔ proxies store/add to upload-api (331ms) ✔ proxies store/remove to upload-api (306ms) ✔ proxies store/list to upload-api (385ms) Upload.all ✔ proxies upload/add to upload-api (303ms) ✔ proxies upload/remove to upload-api (287ms) ✔ proxies upload/list to upload-api (342ms) 9 passing (3s)

Gozala

Added bunch of Nits, which you can completely ignore or address before or after landing. Only thing I do wish is that we had a test(s) where we spin up local store and upload services and route to those in test so that we have passing cases that don't require actual w3up service and all worked in offline mode.

That said it's probably tricky to do it without moving bunch of code from w3infra here so I think it's good enough for now.

Gozala · 2023-01-12T04:38:03Z

packages/access-api/src/config.js

@@ -1,5 +1,7 @@
 import { Signer } from '@ucanto/principal/ed25519'
 import * as DID from '@ipld/dag-ucan/did'
+// eslint-disable-next-line no-unused-vars
+import * as dagUcan from '@ipld/dag-ucan'


Nit: We re-export all of the types from the dag-ucan in the ucanto interface & it would be better to use those instead, so we don't have a chance of having two diff versions of dag-ucan in the tree.

Thanks, I updated to that, and will prefer importing from ucanto libs in the future instead of dag-ucan.

import { UCAN } from '@ucanto/interface'; import { DID } from '@ucanto/core';

fcc80ea
9559d0e

Gozala · 2023-01-12T04:41:24Z

packages/access-api/src/service/index.js


 /**
 * @param {import('../bindings').RouteContext} ctx
 * @returns {import('@web3-storage/access/types').Service}
 */
 export function service(ctx) {
  return {
+    ...UploadApiProxyService.create({


Nit: I think ...uploadProvider(), ...storeProvider() would be more aligned with other stuff already in use. Also I think it would be a good idea to make fetch optional so we only need to pass it when overriding default.

i agree and will do as fast follow #338

packages/access-api/src/service/upload-api-proxy.js

Gozala · 2023-01-12T05:11:27Z

packages/access-api/src/service/upload-api-proxy.js

+/**
+ * @implements {Pick<Map<dagUcan.DID, iucanto.Connection<any>>, 'get'>}
+ */
+class AudienceConnections {


Nit: This whole class seems like incidental complexity, things would be a whole lot simpler with just config object and static function for get method.

P.S.: I know that's not a popular opinion, yet I like to share it in a hope to bring peers to a true side :P

Is this like what you had in mind? b46dd85

P.S.: I know that's not a popular opinion, yet I like to share it in a hope to bring peers to a true side :P

Eh, I don't disagree necessarily. good suggestion.

Not exactly, sorry for not been very clear. In linked version you still have audienceConnections which has get function. When I criticize classes what I mean is:

Separate data part which is basically a state from the behavior which are all the methods

That way it does not matter how you constructed state, you can just pass it into a function and it will compute whatever. Separating data and behavior de-tangles two allowing you to store data in e.g. indexDB and then apply function after reading it from indexDB without having to built a class first. Not the case here but complex class heirerchies also often entangle initialization order.

I think there are only two valid reasons to resort to a classes with methods:

When you have to deal with open-ended variant space. In other sometimes you have many different state options but only one of them will be used in practice. In this scenario dispatching from one function introduces unnecessary coordination and defining interface is better option.

When your methods are simply a convenience sugar for the static functions. I do it all the time to allow method chaining and remove need for importing lot of dependencies. In this case they don't tangle anything they are nothing but a sugar

I would argue use of classes with methods for any other reason just introduces incidental complexity and gets between you and data nothing else.

@Gozala ah I understand what you mean now.

Separating data and behavior de-tangles two allowing you to store data in e.g. indexDB and then apply function after reading it from indexDB without having to built a class first.

def have run into that. That helped me understand.

packages/access-api/src/service/upload-api-proxy.js

packages/access-api/test/helpers/context.js

packages/access-api/test/upload-api-proxy.test.js

packages/access-api/test/store-list.js

Gozala · 2023-01-12T06:00:18Z

That said it's probably tricky to do it without moving bunch of code from w3infra here so I think it's good enough for now.

Created this issue to that effect storacha/w3infra#127

gobengo · 2023-01-12T17:41:07Z

cc @hugomrdias

hugomrdias · 2023-01-13T14:57:32Z

packages/access-api/src/config.js

+ * @template {UCAN.DID} ConfigDID
+ * @template {UCAN.DID} [ID=UCAN.DID]


whats the difference here between these?

There is none. Great catch :) ty

packages/access-api/src/service/upload-api-proxy.js

hugomrdias · 2023-01-13T15:04:03Z

packages/access-api/src/service/upload-api-proxy.js

+// eslint-disable-next-line no-unused-vars
+import * as dagUcan from '@ipld/dag-ucan'
+import { DID } from '@ucanto/core'
+import * as HTTP from '@ucanto/transport/http'
+// eslint-disable-next-line no-unused-vars
+import * as Store from '@web3-storage/capabilities/store'
+// eslint-disable-next-line no-unused-vars
+import * as Upload from '@web3-storage/capabilities/upload'


can we please not do these type only imports, dag-ucan can be imported from ucanto interface and caps types can be imported inline from caps types export. Or just create a types.ts file

i got rid of them and will avoid it in the future too

hugomrdias · 2023-01-13T15:09:59Z

packages/access-api/src/service/upload-api-proxy.js

+/**
+ * @template {Ucanto.Capability} C
+ * @template [Success=unknown]
+ * @template {{ error: true }} [Failure={error:true}]
+ * @callback InvocationResponder
+ * @param {Ucanto.Invocation<C>} invocationIn
+ * @param {Ucanto.InvocationContext} context
+ * @returns {Promise<Ucanto.Result<Success, Failure>>}
+ */


should this just be Ucanto ServiceMethod ?

The only difference is the defaults on Success and Failure. I don't need those so I removed this. Good find. I didn't know all the types we had at our disposal but after this PR I've definitely gotten more familiar with them.
fa1245e

hugomrdias · 2023-01-13T15:11:32Z

packages/access-api/src/service/upload-api-proxy.js

+ * @template {Record<string, any>} T
+ * @param {object} options
+ * @param {Ucanto.Signer} [options.signer]
+ * @param {Pick<Map<dagUcan.DID, Ucanto.ConnectionView<T>>, 'get'>} options.connections


what is this type? what is it picking ?

it was { get: (audience: Ucanto.UCAN.DID) => ConnectionView<T> }

I got rid of this in favor of a record/obj with no functions (which iirc Irakli had also preferred). 08981d8#diff-5fb7be1f94b33171518ab23f095f24d6e1e025282c99e8131c58e9db6c2caa4eL17

hugomrdias · 2023-01-13T16:03:05Z

packages/access-api/src/service/upload-api-proxy.js

+export class UploadApiProxyService {
+  /** @type {StoreService} */
+  store
+  /** @type {UploadService} */
+  upload
+
+  /**
+   * @param {object} options
+   * @param {Ucanto.Signer} [options.signer]
+   * @param {typeof globalThis.fetch} options.fetch
+   */
+  static create(options) {
+    const proxyOptions = {
+      signer: options.signer,
+      connections: {
+        ...audienceConnections,
+        ...(options.fetch && { fetch: options.fetch }),
+      },
+    }
+    return new this(
+      createProxyStoreService(proxyOptions),
+      createProxyUploadService(proxyOptions)
+    )
+  }
+
+  /**
+   * @protected
+   * @param {StoreService} store
+   * @param {UploadService} upload
+   */
+  constructor(store, upload) {
+    this.store = store
+    this.upload = upload
+  }
+}


having an hard time following this

cant we just to the following ? all the types should match and its easier to understand

/** * @param {object} options * @param {typeof globalThis.fetch} options.fetch */ function createUploadProxy(options) { /** @type {Record<Ucanto.DID, Ucanto.ConnectionView<import('@web3-storage/access/types').Service>>} */ const connections = { 'did:web:web3.storage': createUcantoHttpConnection({ audience: 'did:web:web3.storage', url: new URL('https://up.web3.storage'), fetch: options.fetch }), 'did:web:staging.web3.storage': createUcantoHttpConnection({ audience: 'did:web:staging.web3.storage', url: new URL('https://staging.up.web3.storage'), fetch: options.fetch }) } return { store: { add: Server.provide(Store.add, async ({capability, invocation}) => { const conn = connections[invocation.audience.did()] return conn.execute(invocation) }) }, .... // all the other handlers } }

I iterated towards this in two ways:

connections is now just an object like you illustrate here no get method

unbundling the class with store + upload into separate functions that build each

I don't believe that the way you're doing conn.execute(invocation) will actually work. I wish it did. I tried that first. but had to try some various things to avoid making ucanto changes.

@Gozala and I discussed a few ways that ucanto could make this easier, but also decided to do something that works with ucanto as is, ship that, and decde after that whether to push some of those tools (that are now in access-api/src/ucanto/proxy.js) down into ucanto itself.

why does it not work ? this was the promise of "lets make a single endpoint because ucanto makes this super simple" this is not super simple

…tore/upload service types from the capability defs

…api-proxy

…ceMethod with defaults

…yProxy (resolves #339 ]

hugomrdias · 2023-01-14T11:00:56Z

i was hoping for another round of review

hugomrdias · 2023-01-16T16:18:48Z

Upon for testing this should work as expected

/**
 * @template {API.Ability} A
 * @template {API.URI} R
 * @template {API.Caveats} C
 * @param {API.CapabilityParser<API.Match<API.ParsedCapability<A, R, API.InferCaveats<C>>>>} capability
 * @param {import('../bindings').RouteContext} ctx
 */
function createProxyProvider(capability, ctx) {
  const provider = Server.provide(
    capability,
    async ({ capability, invocation }) => {
      /** @type {import('@ucanto/interface').ConnectionView<import('@web3-storage/access/types').Service>} */
      const connection = connect({
        // This should be the same did:web as access api
        id: ctx.signer,
        encoder: CAR,
        decoder: CBOR,
        channel: HTTP.open({
          // this is the only new global env var
          url: new URL('https://staging.up.web3.storage'),
          method: 'POST',
        }),
      })

      const inv = ucanto.invoke({
        issuer: ctx.signer,
        // This should be the same did:web as access api
        audience: ctx.signer,
        capability,
        proofs: [invocation],
        expiration: invocation.expiration,
        facts: invocation.facts,
        nonce: invocation.nonce,
        notBefore: invocation.notBefore,
      })
      const [result] = await connection.execute(
        /** @type {API.ServiceInvocation<{can: A, with: R, nb: C}, import('@web3-storage/access/types').Service>} */ (
          inv
        )
      )
      return result
    }
  )

  return provider
}

In the service definition (src/service/index.js) we only need:

/**
 * @param {import('../bindings').RouteContext} ctx
 * @returns {import('@web3-storage/access/types').Service}
 */
export function service(ctx) {
  return {
    store: {
      add: createProxyProvider(Store.add, ctx),
    },
    .....
  }
}

and here https://github.com/web3-storage/w3protocol/blob/b77337692d9e4580031c429c429d4055d6f6ebff/packages/access-client/src/types.ts#L67 we need to add

store: {
    add: ServiceMethod<StoreAdd, StoreAddResponse, Failure>
  }

🤖 I have created a release *beep* *boop* --- ## [4.3.0](access-api-v4.2.0...access-api-v4.3.0) (2023-01-17) ### Features * access-api forwards store/ and upload/ invocations to upload-api ([#334](#334)) ([b773376](b773376)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- ## [2.2.0](capabilities-v2.1.0...capabilities-v2.2.0) (2023-01-30) ### Features * access-api forwards store/ and upload/ invocations to upload-api ([#334](#334)) ([b773376](b773376)) * **capabilities:** implement access/authorize and ./update caps ([#387](#387)) ([4242ce0](4242ce0)), closes [#385](#385) * embedded key resolution ([#312](#312)) ([4da91d5](4da91d5)) * update @ucanto/* to ~4.2.3 ([#405](#405)) ([50c0c80](50c0c80)) * update access-api ucanto proxy to not need a signer ([#390](#390)) ([71cbeb7](71cbeb7)) ### Bug Fixes * fix client cli service did resolve ([#292](#292)) ([6be9608](6be9608)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- ## [9.3.0](access-v9.2.0...access-v9.3.0) (2023-01-30) ### Features * access-api forwards store/ and upload/ invocations to upload-api ([#334](#334)) ([b773376](b773376)) * access-api handling store/info for space not in db returns failure with name ([#391](#391)) ([9610fcf](9610fcf)) * update @ucanto/* to ~4.2.3 ([#405](#405)) ([50c0c80](50c0c80)) * update access-api ucanto proxy to not need a signer ([#390](#390)) ([71cbeb7](71cbeb7)) ### Bug Fixes * remove unecessary awaits ([#352](#352)) ([64da6e5](64da6e5)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

#334) Motivation: * #331

🤖 I have created a release *beep* *boop* --- ## [4.3.0](access-api-v4.2.0...access-api-v4.3.0) (2023-01-17) ### Features * access-api forwards store/ and upload/ invocations to upload-api ([#334](#334)) ([6be7217](6be7217)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- ## [2.2.0](capabilities-v2.1.0...capabilities-v2.2.0) (2023-01-30) ### Features * access-api forwards store/ and upload/ invocations to upload-api ([#334](#334)) ([6be7217](6be7217)) * **capabilities:** implement access/authorize and ./update caps ([#387](#387)) ([ebe1032](ebe1032)), closes [#385](#385) * embedded key resolution ([#312](#312)) ([45f367d](45f367d)) * update @ucanto/* to ~4.2.3 ([#405](#405)) ([ec39443](ec39443)) * update access-api ucanto proxy to not need a signer ([#390](#390)) ([163fb74](163fb74)) ### Bug Fixes * fix client cli service did resolve ([#292](#292)) ([45e7ad4](45e7ad4)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- ## [9.3.0](access-v9.2.0...access-v9.3.0) (2023-01-30) ### Features * access-api forwards store/ and upload/ invocations to upload-api ([#334](#334)) ([6be7217](6be7217)) * access-api handling store/info for space not in db returns failure with name ([#391](#391)) ([665dac9](665dac9)) * update @ucanto/* to ~4.2.3 ([#405](#405)) ([ec39443](ec39443)) * update access-api ucanto proxy to not need a signer ([#390](#390)) ([163fb74](163fb74)) ### Bug Fixes * remove unecessary awaits ([#352](#352)) ([2e8c1a1](2e8c1a1)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

working test in packages/access-api/test/upload.test.js

0f79d89

gobengo linked an issue Jan 11, 2023 that may be closed by this pull request

make it so access-api can proxy store and upload invocations to upload-api #331

Closed

gobengo temporarily deployed to dev January 11, 2023 04:52 — with GitHub Actions Inactive

gobengo added 3 commits January 10, 2023 21:28

make store/list proxy test pass by overriding type on issuer

70a7576

upload-api-proxy tests ensure no error if a WEB3_STORAGE_PRIVATE_KEY …

ab27309

…env var is set

add upload-api-proxy test ensuring irrelevant invocations are rejected

658b8f1

gobengo temporarily deployed to dev January 11, 2023 23:25 — with GitHub Actions Inactive

gobengo added 2 commits January 11, 2023 15:32

rm eslint-disables at top of upload-api-proxy

6a633c6

rename test/upload.test.js to test/store-list.js

fc665af

gobengo temporarily deployed to dev January 11, 2023 23:35 — with GitHub Actions Inactive

gobengo added 5 commits January 11, 2023 15:54

add test upload-api-proxy can get all caps from Store.all

f0ac9ad

test parserCapabilities

ddb1351

initial Store.all test not working

a692aec

test can proxy Store.all

d596bc1

serve proxies Upload.all

fa6a437

gobengo temporarily deployed to dev January 12, 2023 00:59 — with GitHub Actions Inactive

gobengo added 2 commits January 11, 2023 17:08

cleanup

69ee9de

Merge branch 'main' into 331-make-it-so-access-api-can-proxy-store-an…

88a1c07

…d-upload-invocations-to-upload-api

gobengo temporarily deployed to dev January 12, 2023 01:09 — with GitHub Actions Inactive

gobengo marked this pull request as ready for review January 12, 2023 01:10

gobengo commented Jan 12, 2023

View reviewed changes

access-api uses UploadApiProxyService without options.signer

2395049

gobengo temporarily deployed to dev January 12, 2023 01:21 — with GitHub Actions Inactive

Gozala approved these changes Jan 12, 2023

View reviewed changes

gobengo mentioned this pull request Jan 12, 2023

Unify UCAN API endpoints #325

Open

3 tasks

hugomrdias self-requested a review January 12, 2023 17:41

access-api/src/config imports from ucanto libs, not dag-ucan

fcc80ea

gobengo mentioned this pull request Jan 12, 2023

refactor UploadApiProxyService #338

Open

1 task

gobengo temporarily deployed to dev January 13, 2023 01:29 — with GitHub Actions Inactive

hugomrdias suggested changes Jan 13, 2023

View reviewed changes

gobengo added 2 commits January 13, 2023 12:07

upload-api-proxy infers service type from capabilities module

3f50270

simplify upload-api-proxy and remove redundancies by e.g. inferring s…

08981d8

…tore/upload service types from the capability defs

gobengo temporarily deployed to dev January 13, 2023 21:02 — with GitHub Actions Inactive

remove duplicate @template in config configureVerifier

8b03b1b

gobengo temporarily deployed to dev January 13, 2023 22:59 — with GitHub Actions Inactive

minimize type-only imports from @web3-storage/capabilities in upload-…

94614d0

…api-proxy

gobengo temporarily deployed to dev January 13, 2023 23:07 — with GitHub Actions Inactive

access-api remove InvocationResponder type as it is essentially Servi…

fa1245e

…ceMethod with defaults

gobengo temporarily deployed to dev January 13, 2023 23:16 — with GitHub Actions Inactive

decompose UploadApiProxyService into createUploadProxy and createStor…

3a46454

…yProxy (resolves #339 ]

gobengo temporarily deployed to dev January 13, 2023 23:31 — with GitHub Actions Inactive

tests work offline. upload-api urls can be overridden via env var

394662b

gobengo temporarily deployed to dev January 14, 2023 01:54 — with GitHub Actions Inactive

gobengo merged commit b773376 into main Jan 14, 2023

gobengo deleted the 331-make-it-so-access-api-can-proxy-store-and-upload-invocations-to-upload-api branch January 14, 2023 02:03

This was referenced Jan 14, 2023

chore(main): release capabilities 2.2.0 #299

Merged

chore(main): release access-api 4.3.0 #345

Merged

chore(main): release access 9.3.0 #346

Merged

it-dag-house mentioned this pull request Mar 23, 2023

chore(main): release access-api 11.0.0-rc.0 #620

Closed

gobengo added a commit that referenced this pull request Apr 11, 2023

feat: access-api forwards store/ and upload/ invocations to upload-api (

6be7217

#334) Motivation: * #331

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: access-api forwards store/ and upload/ invocations to upload-api #334

feat: access-api forwards store/ and upload/ invocations to upload-api #334

gobengo commented Jan 11, 2023

gobengo Jan 12, 2023

Gozala left a comment

Gozala Jan 12, 2023

gobengo Jan 12, 2023

gobengo Jan 12, 2023

Gozala Jan 12, 2023

gobengo Jan 12, 2023

Gozala Jan 12, 2023

gobengo Jan 13, 2023

Gozala Jan 13, 2023

gobengo Jan 13, 2023

Gozala commented Jan 12, 2023

gobengo commented Jan 12, 2023

hugomrdias Jan 13, 2023

gobengo Jan 13, 2023

gobengo Jan 13, 2023

hugomrdias Jan 13, 2023

gobengo Jan 14, 2023

hugomrdias Jan 13, 2023

gobengo Jan 13, 2023

hugomrdias Jan 13, 2023

gobengo Jan 13, 2023

hugomrdias Jan 13, 2023

gobengo Jan 13, 2023

hugomrdias Jan 14, 2023

hugomrdias commented Jan 14, 2023

hugomrdias commented Jan 16, 2023

		* @template {UCAN.DID} ConfigDID
		* @template {UCAN.DID} [ID=UCAN.DID]

feat: access-api forwards store/ and upload/ invocations to upload-api #334

feat: access-api forwards store/ and upload/ invocations to upload-api #334

Conversation

gobengo commented Jan 11, 2023

Choose a reason for hiding this comment

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

Gozala commented Jan 12, 2023

gobengo commented Jan 12, 2023

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

hugomrdias commented Jan 14, 2023

hugomrdias commented Jan 16, 2023