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: persist ucan invocation car to s3 and replicate to r2 #83

Merged
merged 4 commits into from
Dec 13, 2022

Conversation

vasco-santos
Copy link
Contributor

@vasco-santos vasco-santos commented Dec 9, 2022

Per last night call, persisting CAR files of UCAN invocations handled by the service into S3 with replication to R2.

Implementation details:

  • An abstraction store for the bucket was created with same pattern as we use for Dudewhere + Carpark
  • persistence happens when ucan route handler was successfully executed
  • create ucan-invocation stack
    • it includes S3 UCAN Bucket and its function to send notifications of written files to EventBus, so that Replicator can kick in
    • future: will include the Kinesis stream, as well as its consumer functions
  • replicator stack updated with tracking events for UCAN bucket and replicating them into R2 bucket with new bucket

TODO:

  • create R2 buckets
  • set env variables in seed.run

@seed-deploy seed-deploy bot temporarily deployed to pr83 December 9, 2022 09:55 Inactive
@seed-deploy
Copy link

seed-deploy bot commented Dec 9, 2022

View stack outputs

@vasco-santos vasco-santos requested review from alanshaw and olizilla and removed request for alanshaw December 9, 2022 10:03
@vasco-santos vasco-santos changed the title feat: persist ucan invocation car to r2 feat: persist ucan invocation car to s3 and replicate to r2 Dec 9, 2022
@vasco-santos vasco-santos force-pushed the feat/persist-ucan-invocation-car-to-r2 branch from f663ccf to 428bf65 Compare December 9, 2022 14:34
@seed-deploy seed-deploy bot temporarily deployed to pr83 December 9, 2022 14:34 Inactive
@vasco-santos vasco-santos force-pushed the feat/persist-ucan-invocation-car-to-r2 branch from 428bf65 to f4a9b03 Compare December 9, 2022 14:36
@seed-deploy
Copy link

seed-deploy bot commented Dec 9, 2022

Stack outputs updated

@seed-deploy seed-deploy bot temporarily deployed to pr83 December 9, 2022 14:38 Inactive
@vasco-santos vasco-santos mentioned this pull request Dec 12, 2022
1 task
@seed-deploy
Copy link

seed-deploy bot commented Dec 12, 2022

Stack outputs updated

/**
* @param {import('@serverless-stack/resources').StackContext} properties
*/
export function UcanInvocationStack({ stack, app }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: perhaps this is the ucan-archive-stack ?

Suggested change
export function UcanInvocationStack({ stack, app }) {
export function UcanArchiveStack({ stack, app }) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is not archive in reality. We do other kind of post processing on it. I like invocation because of being a spec term https://github.com/ucan-wg/spec#29-invocation that is what we are doing.


const eventBusName = 'event-bus-arn'

test('notifies event bus when an .idx file is added to the satnav bucket', async t => {
Copy link
Contributor

Choose a reason for hiding this comment

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

copy/pasta test names

// @ts-expect-error different type interface in AWS expected request
await persistUcanInvocation(request, ucanStore)

const requestCar = await CAR.codec.decode(request.body)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to test we can round trip the ucan by fetching it from the bucket and parsing it? there is a base64 decode step that gives me pause

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah adding. That is the aws base64 thing, so adding tests as I agree we should validate things

put: async (carCid, bytes) => {
const putCmd = new PutObjectCommand({
Bucket: bucketName,
Key: carCid,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should match the carpark keys here and /${carCid}/${carCid}

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.

comments inline

@seed-deploy seed-deploy bot temporarily deployed to pr83 December 12, 2022 12:59 Inactive
@seed-deploy seed-deploy bot temporarily deployed to pr83 December 12, 2022 13:22 Inactive
@seed-deploy seed-deploy bot temporarily deployed to pr83 December 12, 2022 13:25 Inactive
@vasco-santos
Copy link
Contributor Author

#86 (review) handled in 0d55985

@olizilla
Copy link
Contributor

The latest discussion landed on "store the invocation CAR as is and store keys for the block cids that point to the car they are in like /blockCID/carCID. The values can be empty like we do in DUDEWHERE. This means we store the exact invocation bytes and can also find specific proof blocks quickly if we need to.

@alanshaw @vasco-santos what do you think?

@olizilla
Copy link
Contributor

This also sounds like something we could add in later if storing the invocations as cars is useful anyway

t.is(s3Response.$metadata.httpStatusCode, 200)

// @ts-expect-error AWS types with readable stream
const bytes = (await s3Response.Body.toArray())[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this wild magic? Why would a toArray method stick all the bytes in the first element of an array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://nodejs.org/api/stream.html#readabletoarrayoptions

It is kind of the easier way of obtaining the contents of a stream. And for a test purpose seems good

@vasco-santos vasco-santos merged commit 6140f85 into main Dec 13, 2022
@vasco-santos vasco-santos deleted the feat/persist-ucan-invocation-car-to-r2 branch December 13, 2022 09:41
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