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

feat: ucanto invocation router with bare minimal store add invocation #4

Merged
merged 2 commits into from
Nov 14, 2022

Conversation

vasco-santos
Copy link
Contributor

@vasco-santos vasco-santos commented Nov 11, 2022

This PR adds ucanto invocation router with store/add invocation.

It integrates following AWS Resources in ApiStack:

  • S3 Bucket to store CAR files (carpark)
    • we could have re-used the current bucket w3up-cars, but it is in wrong region, so let's just be coherent with CF naming and use carpark-prod-0 ? :)
  • Dynamo DB with store_table to track mapping between Users and stored CARs
    • keeping same namings as current schema. We will need to perform a migration, so you might decide to rename things. Let's consider it later on new PRs

Notes:

  • previous services folder renamed into API, so that we can use service folder per ucanto framework current used names
  • uses hardcoded service DID, we need in follow up PR to wire it up with env var - also see authority.js + config.js in w3up repo
  • we add info to store_table like previously done in w3up repo. Probably worth considering as follow up to add a column that tracks wether the CAR already made it to the bucket. On S3 Put Event, we would update that boolean. Otherwise, we have things in the DB that we don't have immediate visibility if the given CARs ever made it

Follow up needs (will create issues):

  • handle account from store/add
  • wire up service DID with secrets

@vasco-santos vasco-santos force-pushed the feat/post-handler-using-ucanto-server branch 14 times, most recently from e9e5124 to ac9e58c Compare November 11, 2022 17:19
@vasco-santos vasco-santos marked this pull request as ready for review November 11, 2022 17:21
@vasco-santos vasco-santos force-pushed the feat/post-handler-using-ucanto-server branch 4 times, most recently from 7bdca7d to 09f69bd Compare November 14, 2022 13:58
})

// TODO: Need to set ENV for dbEndpoint...
test.skip('ucan-invocation-router', async (t) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@olizilla should we just drop this test to the lambda directly?

It is quite hard to inject the needed resources here... I would suggest follow up PR with more integration kind of tests, where we run the API Stack with all routes and perform tests using HTTP Calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

AYE

Comment on lines +29 to +32
// Only use capability account for now to check if account is registered.
// This must change to access account/info!!
// We need to use https://github.com/web3-storage/w3protocol/blob/9d4b5bec1f0e870233b071ecb1c7a1e09189624b/packages/access/src/agent.js#L270
const account = capability.with
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will handle account/agent stuff in follow up PR

@vasco-santos vasco-santos force-pushed the feat/post-handler-using-ucanto-server branch from 09f69bd to 32ce987 Compare November 14, 2022 14:01
@vasco-santos vasco-santos force-pushed the feat/post-handler-using-ucanto-server branch from 32ce987 to 85fe048 Compare November 14, 2022 14:12
payloadCID: 'string',
applicationDID: 'string',
origin: 'string',
size: 'number',
Copy link
Contributor

Choose a reason for hiding this comment

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

is it size in bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

*/
const storeTable = new Table(stack, 'store_table', {
fields: {
uploaderDID: 'string',
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Is this the account DID or the agent DID?
  • camelCase?

* Force name of bucket to be "car-park-prod-0" in prod.
*/
const ingestBucketCdkConfig =
stack.stage === 'prod' ? { cdk: { bucket: { bucketName: 'car-park-prod-0' } } } : {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of our per env config is in config.js. We should try and be consistent

const ingestBucketCdkConfig =
stack.stage === 'prod' ? { cdk: { bucket: { bucketName: 'car-park-prod-0' } } } : {}

const carStore = new Bucket(stack, 'carStore', {
Copy link
Contributor

Choose a reason for hiding this comment

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

in this file we have

  • kebab-case
  • camelCase
  • snake_case

for resource ids. plz can we not.

permissions: [storeTable, carStore],
environment: {
STORE_TABLE_NAME: storeTable.tableName,
CAR_STORE_BUCKET_NAME: carStore.bucketName
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CAR_STORE_BUCKET_NAME: carStore.bucketName
STORE_BUCKET_NAME: carStore.bucketName

import * as CBOR from '@ucanto/transport/cbor'

import getServiceDid from '../authority.js'
import { create as createCarStore } from '../buckets/car-store.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make the export from car-store be createCarStore

const AWS_REGION = process.env.AWS_REGION || 'us-west-2'

/**
* AWS API Gateway handler for POST / with ucan invocation router.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* AWS API Gateway handler for POST / with ucan invocation router.
* AWS HTTP Gateway handler for POST / with ucan invocation router.

Comment on lines 42 to 48
signingOptions: {
region: AWS_REGION,
secretAccessKey: AWS_SECRET_ACCESS_KEY,
accessKeyId: AWS_ACCESS_KEY_ID,
sessionToken: AWS_SESSION_TOKEN,
bucket: bucketName,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

pass in a UrlSigner or similar to follow the pattern of holding the state in an object that we set in StoreTable and CarStore

}
})
const response = await server.request({
// @ts-ignore - type is Record<string, string|string[]|undefined>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// @ts-ignore - type is Record<string, string|string[]|undefined>
// @ts-expect-error - type is Record<string, string|string[]|undefined>

also i think we should review the type sig that ucanto server is using here. that seems like a reasonable thing to pass in.

* Bind a link CID to an account
*
* @param {object} data
* @param {string} data.accountDID
Copy link
Contributor

Choose a reason for hiding this comment

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

is really string type?

Copy link
Contributor

@olizilla olizilla left a comment

Choose a reason for hiding this comment

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

Good start. Lots of nitpicks that we can iterate on together. GOOOOO VASCO!

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.

2 participants