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: propose admin/upload/inspect and admin/store/inspect capabilities #77

Merged
merged 6 commits into from
Sep 13, 2023

Conversation

travis
Copy link
Member

@travis travis commented Sep 8, 2023

At a high level, these are designed to enable administrators like @dchoi27 to figure out which space a given CID is associated with.

Problem CIDs come to us from a variety of sources - sometimes they are content CIDs that correspond to a particular upload/add invocation and sometimes they are "shard" CIDs that identify one of the CARs stored using store/add that make up the shards of a larger upload. admin/upload/inspect returns information about the upload and admin/store/inspect returns information about particular shards, so given an unknown CID administrators should check both.

@travis travis marked this pull request as draft September 8, 2023 20:14
w3-admin.md Outdated Show resolved Hide resolved
w3-admin.md Outdated Show resolved Hide resolved
w3-admin.md Outdated Show resolved Hide resolved
@alanshaw
Copy link
Member

Oh I should really read the PR description before reviewing the code 🤦‍♂️

I'd vote for 2 separate capabilities for querying upload/adds and store/adds!

@travis
Copy link
Member Author

travis commented Sep 11, 2023

ok cool - I will rework this with two separate capabilities!

Base automatically changed from feat/admin-protocol to main September 11, 2023 18:05
At a high level, this capability is designed to enable us to display a list of spaces a CID's content has been uploaded to. There are a few complications:

1) a CID might identify a CAR (passed as the `link` of a `store/add` invocation or an actual "upload" (passed as the `root` of an `upload/add` invocation
2) a CID may have been uploaded to more than one space
3) a CID might even have been used in both `store/add` and `upload/add` - someone might have uploaded something and then re-uploaded the CAR that was passed to `store/add`

My proposal addresses all of these, returning a list of spaces a piece of content was `upload`ed to and a list of spaces a piece of content was `store`d in along with the dates that they were uploaded.

There are a bunch of other ways we might solve this, including:

1) separate capabilities for `upload/add`ed content and `store/add`ed content (i.e. `cid/get-upload` vs `cid/get-store`)
2) returning a single list of information with an additional `type` field to differentiate `store`d content from `upload`ed content
3) returning a single list with no differentiation
4) ??

If anyone has strong opinions on how this should work I'm definitely open to considering different designs!
At a high level, these are designed to enable administrators like @dchoi27 to figure out which space a given CID is associated with.

Problem CIDs come to us from a variety of sources - sometimes they are "root" CIDs that correspond to a particular upload and sometimes they are "shard" CIDs that identify one of the CARs that make up the shards of a larger upload. `root/get` returns information about the upload and `shard/get` returns information about particular shards, so given an unknown CID administrators should check both.
@travis travis changed the title wip: propose cid/get capability feat: propose root/get and shard/get capabilities Sep 11, 2023
@travis travis marked this pull request as ready for review September 11, 2023 18:53
travis added a commit to storacha/w3up that referenced this pull request Sep 11, 2023
implementations of new capabilities proposed in storacha/specs#77
travis added a commit to storacha/w3up that referenced this pull request Sep 11, 2023
implementations of new capabilities proposed in storacha/specs#77
Copy link
Collaborator

@Gozala Gozala left a comment

Choose a reason for hiding this comment

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

I think current naming is too generic and we probably should avoid taking up so many namespaces. Furthermore I think it would be a good idea to group these capabilities under same namespace so that prefix/* delegation used.

I offered some name suggestions, but not insisting on them.

w3-admin.md Outdated Show resolved Hide resolved
w3-admin.md Outdated Show resolved Hide resolved
w3-admin.md Outdated Show resolved Hide resolved
@alanshaw
Copy link
Member

+1 I'm cool with trace/store/add and trace/upload/add as the capability names.

@travis
Copy link
Member Author

travis commented Sep 12, 2023

ok great, works for me too! I'll get this in now!

w3-admin.md Outdated
@@ -148,3 +150,44 @@ export const get = capability({
},
})
```

### `trace/upload/get`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have suggested trace/upload/add so it corresponds to upload/add, meaning it's a capability to "trace upload/add" operation. Although I realize now it goes against established add, get, list, remove pattern used in other capabilities.

However trace/upload/get also feels counter intuitive to me even though I get the logic. On one hand it would make sense to have upload/trace and store/trace, but that goes against prefix defining the ownership.

How about console/upload/inspect and console/store/inspect instead ? Alternatively it could be admin/upload/inspect or system/upload/inspect. I suppose trace/upload/inspect works too.

Copy link
Member Author

Choose a reason for hiding this comment

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

chatted offline and agreed to go with admin/{upload|store}/inspect

@travis travis changed the title feat: propose root/get and shard/get capabilities feat: propose admin/upload/inspect and admin/store/inspect capabilities Sep 12, 2023
@travis travis changed the title feat: propose admin/upload/inspect and admin/store/inspect capabilities feat: propose admin/upload/inspect and admin/store/inspect capabilities Sep 12, 2023
@travis travis merged commit 6d76805 into main Sep 13, 2023
2 checks passed
@travis travis deleted the feat/cid-lookup branch September 13, 2023 01:00
travis added a commit to storacha/w3up that referenced this pull request Sep 13, 2023
…bilities (#918)

1. implementations of new capabilities proposed in
storacha/specs#77
2. added a new `getCID` method to each of the `{Upload|Store}Table`
interfaces, and updated the in-memory implementations of each.
3. implementations of `admin/upload/inspect` and `admin/store/inspect`
invocation handlers
4. tests for all of the above
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.

3 participants