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.

Sign up for GitHub

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

Jump to bottom

feat!: return allocated bytes in store/add receipt #1213

Merged
merged 10 commits into from
Dec 7, 2023
Merged

feat!: return allocated bytes in store/add receipt #1213

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
45b18cd
feat: return allocated bytes in store/add receipt
Dec 4, 2023
b351ab9
fix: typo
Dec 4, 2023
f8d24cd
chore: appease linter
Dec 4, 2023
07e2225
fix: tests
Dec 4, 2023
880f001
refactor: apply suggestions from code review
Dec 4, 2023
313fe50
feat: result return types for upload and store table
Dec 6, 2023
4423167
Merge branch 'feat/store-add-allocated-bytes' of github.com:web3-stor…
Dec 6, 2023
db568ce
chore: appease linter
Dec 6, 2023
7623f6a
fix: remove unneeded codes
Dec 6, 2023
089888e
fix: less churn
Dec 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 24 additions & 8 deletions packages/capabilities/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,18 +438,34 @@ export type StoreRemove = InferInvokedCapability<typeof StoreCaps.remove>
export type StoreList = InferInvokedCapability<typeof StoreCaps.list>

export type StoreAddSuccess = StoreAddSuccessDone | StoreAddSuccessUpload
export interface StoreAddSuccessDone {
status: 'done'

export type StoreAddSuccessStatusUpload = 'upload'
export type StoreAddSuccessStatusDone = 'done'

export interface StoreAddSuccessResult {
/**
* Status of the item to store. A "done" status indicates that it is not
* necessary to upload the item. An "upload" status indicates that the item
* should be uploaded to the provided URL.
*/
status: StoreAddSuccessStatusUpload | StoreAddSuccessStatusDone
/**
* Total bytes allocated in the space to accommodate this stored item.
* May be zero if the item is _already_ stored in _this_ space.
*/
allocated: number
Copy link
Contributor

@gobengo gobengo Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We may want to advertise how much we can ensure consistency of this, which I imagine might be 'not very much' unless we have transactions in our underlying db

e.g. let's say cid A is not stored in the space. Two store/add invocations get created with different invocations CIDs (e.g. different nonce/expirty) to add cid A to the space. Then those two invocations are submitted to us at the exact same second. iiuc it may be the case that both of those responses come back with allocated: size(A) (because we dont have transactions, and in this scenario both invocations calls carStoreBucket.has(link) here would have returned false.

my main concern

  • someone might use this allocated response value in some business logic without being aware of the eventual consistency lack-of-transactionality in generating this value. i.e. this could be nonzero even if technically the a different store/add invocation added the CID first (in the race condition describes above).
  • in that race condition, the same CID may have multiple store/add receipts whose results have allocated set to the size of the referent of the CID.

If we want to return this information in the result of the write operation (store/add), we probably need something like transactions to do it in a way that isn't subject to these race conditions.
But if possible it may be nice to avoid returning this value in the result of the write operation (because it almost always increases the cost of the write operation to add return values that. must also be consistent)

alternatively we ignore the above concerns but maybe it's a good idea to at least put in the jsdoc here that there a nonzero response does not necessarily indicate that the store/add is the authoritative addition of the CID to the space.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point, in hindsight this is maybe not the right solution because we want to use it in our business logic...

Copy link
Contributor

Choose a reason for hiding this comment

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

good point, so maybe we want the insert later on to have a result of being conflict or not. Only one Put should happen in dynamo successfully

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, on reflection I think we can make allocated in the response be authoratitive about the addition of the CID to the space. This is what we can do:

  • We don't need TransactWriteItems since we're not writing multiple items!
  • We can use conditional expressions to ensure writes fail:
    • Change store/add PutItem so that it fails if not a new record
    • Change store/remove DeleteItem so that it fails if record does not exist

/** DID of the space this item will be stored in. */
with: DID
/** CID of the item. */
link: UnknownLink
url?: undefined
headers?: undefined
}

export interface StoreAddSuccessUpload {
status: 'upload'
with: DID
link: UnknownLink
export interface StoreAddSuccessDone extends StoreAddSuccessResult {
status: StoreAddSuccessStatusDone
}

export interface StoreAddSuccessUpload extends StoreAddSuccessResult {
status: StoreAddSuccessStatusUpload
url: ToString<URL>
headers: Record<string, string>
}
Expand Down
2 changes: 1 addition & 1 deletion packages/eslint-config-w3up/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ module.exports = {
EXPERIMENTAL_useProjectService: true,
},
rules: {
"@typescript-eslint/no-floating-promises": "error",
'@typescript-eslint/no-floating-promises': 'error',
'@typescript-eslint/no-unused-vars': 'off',
'@typescript-eslint/ban-ts-comment': 'off',

Expand Down
2 changes: 1 addition & 1 deletion packages/filecoin-api/test/events/aggregator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -738,7 +738,7 @@ export const test = {
minPieceInsertedAt: new Date().toISOString(),
}
const putAggregateRes = await context.aggregateStore.put(
aggregateRecord,
aggregateRecord
)
assert.ok(putAggregateRes.ok)

Expand Down
6 changes: 4 additions & 2 deletions packages/upload-api/src/store/add.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export function storeAddProvider(context) {
Server.DID.parse(capability.with).did()
)
const issuer = invocation.issuer.did()
const [allocated, carIsLinkedToAccount, carExists] = await Promise.all([
const [allocated, carIsLinkedToSpace, carExists] = await Promise.all([
allocate(
{
capability: {
Expand All @@ -42,7 +42,7 @@ export function storeAddProvider(context) {
return allocated
}

if (!carIsLinkedToAccount) {
if (!carIsLinkedToSpace) {
await storeTable.insert({
space,
link,
Expand All @@ -57,6 +57,7 @@ export function storeAddProvider(context) {
return {
ok: {
status: 'done',
allocated: carIsLinkedToSpace ? 0 : size,
with: space,
link,
},
Expand All @@ -67,6 +68,7 @@ export function storeAddProvider(context) {
return {
ok: {
status: 'upload',
allocated: size,
with: space,
link,
url: url.toString(),
Expand Down
74 changes: 69 additions & 5 deletions packages/upload-api/test/handlers/store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,10 @@ export const test = {
if (!storeAdd.out.ok) {
throw new Error('invocation failed', { cause: storeAdd })
}
if (storeAdd.out.ok.status !== 'upload') {
throw new Error(`unexpected status: ${storeAdd.out.ok.status}`)
}

assert.equal(storeAdd.out.ok.status, 'upload')
assert.equal(storeAdd.out.ok.with, spaceDid)
assert.deepEqual(storeAdd.out.ok.link.toString(), link.toString())

Expand Down Expand Up @@ -177,9 +179,11 @@ export const test = {
if (!storeAdd.out.ok) {
throw new Error('invocation failed', { cause: storeAdd })
}
if (storeAdd.out.ok.status !== 'upload') {
throw new Error(`unexpected status: ${storeAdd.out.ok.status}`)
}

const url =
storeAdd.out.ok.status === 'upload' && new URL(storeAdd.out.ok.url)
const url = new URL(storeAdd.out.ok.url)
if (!url) {
throw new Error('Expected presigned url in response')
}
Expand Down Expand Up @@ -227,9 +231,11 @@ export const test = {
if (!storeAdd.out.ok) {
throw new Error('invocation failed', { cause: storeAdd })
}
if (storeAdd.out.ok.status !== 'upload') {
throw new Error(`unexpected status: ${storeAdd.out.ok.status}`)
}

const url =
storeAdd.out.ok.status === 'upload' && new URL(storeAdd.out.ok.url)
const url = new URL(storeAdd.out.ok.url)
if (!url) {
throw new Error('Expected presigned url in response')
}
Expand Down Expand Up @@ -287,8 +293,10 @@ export const test = {
}

assert.equal(storeAdd.out.ok.status, 'done')
assert.equal(storeAdd.out.ok.allocated, data.byteLength)
assert.equal(storeAdd.out.ok.with, spaceDid)
assert.deepEqual(storeAdd.out.ok.link.toString(), link.toString())
// @ts-expect-error making sure it's not an upload status
assert.equal(storeAdd.out.ok.url == null, true)

const item = await context.storeTable.get(spaceDid, link)
Expand All @@ -313,6 +321,62 @@ export const test = {
)
},

'store/add returns allocated: 0 if already added to space': async (
assert,
context
) => {
const { proof, spaceDid } = await registerSpace(alice, context)
const connection = connect({
id: context.id,
channel: createServer(context),
})

const data = new Uint8Array([11, 22, 34, 44, 55])
const link = await CAR.codec.link(data)

const { url, headers } = await context.carStoreBucket.createUploadUrl(
link,
data.length
)

// simulate an already stored CAR
const put = await fetch(url, {
method: 'PUT',
mode: 'cors',
body: data,
headers,
})
assert.equal(put.ok, true, 'should be able to upload to presigned url')

const inv0 = StoreCapabilities.add.invoke({
issuer: alice,
audience: connection.id,
with: spaceDid,
nb: { link, size: data.byteLength },
proofs: [proof],
})

const r0 = await inv0.execute(connection)

assert.equal(r0.out.ok?.status, 'done')
assert.equal(r0.out.ok?.allocated, data.byteLength)
assert.equal(r0.out.ok?.with, spaceDid)

const inv1 = StoreCapabilities.add.invoke({
issuer: alice,
audience: connection.id,
with: spaceDid,
nb: { link, size: data.byteLength },
proofs: [proof],
})

const r1 = await inv1.execute(connection)

assert.equal(r1.out.ok?.status, 'done')
assert.equal(r1.out.ok?.allocated, 0)
assert.equal(r1.out.ok?.with, spaceDid)
},

'store/add disallowed if invocation fails access verification': async (
assert,
context
Expand Down
32 changes: 18 additions & 14 deletions packages/upload-client/test/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ describe('uploadFile', () => {
}),
])

/** @type {import('../src/types.js').StoreAddSuccessUpload} */
/** @type {Omit<import('../src/types.js').StoreAddSuccessUpload, 'allocated'>} */
const res = {
status: 'upload',
headers: { 'x-test': 'true' },
Expand All @@ -62,13 +62,12 @@ describe('uploadFile', () => {

const service = mockService({
store: {
add: provide(StoreCapabilities.add, ({ invocation }) => {
add: provide(StoreCapabilities.add, ({ invocation, capability }) => {
assert.equal(invocation.issuer.did(), agent.did())
assert.equal(invocation.capabilities.length, 1)
const invCap = invocation.capabilities[0]
assert.equal(invCap.can, StoreCapabilities.add.can)
assert.equal(invCap.with, space.did())
return { ok: res }
assert.equal(capability.can, StoreCapabilities.add.can)
assert.equal(capability.with, space.did())
return { ok: { ...res, allocated: capability.nb.size } }
}),
},
upload: {
Expand Down Expand Up @@ -143,7 +142,7 @@ describe('uploadFile', () => {
}),
])

/** @type {Omit<import('../src/types.js').StoreAddSuccessUpload, 'link'>} */
/** @type {Omit<import('../src/types.js').StoreAddSuccessUpload, 'link'|'allocated'>} */
const res = {
status: 'upload',
headers: { 'x-test': 'true' },
Expand All @@ -159,6 +158,7 @@ describe('uploadFile', () => {
link: /** @type {import('../src/types.js').CARLink} */ (
capability.nb.link
),
allocated: capability.nb.size,
},
})),
},
Expand Down Expand Up @@ -229,7 +229,7 @@ describe('uploadDirectory', () => {
}),
])

/** @type {Omit<import('../src/types.js').StoreAddSuccessUpload, 'link'>} */
/** @type {Omit<import('../src/types.js').StoreAddSuccessUpload, 'link'|'allocated'>} */
const res = {
status: 'upload',
headers: { 'x-test': 'true' },
Expand All @@ -251,6 +251,7 @@ describe('uploadDirectory', () => {
link: /** @type {import('../src/types.js').CARLink} */ (
capability.nb.link
),
allocated: capability.nb.size,
},
}
}),
Expand Down Expand Up @@ -323,7 +324,7 @@ describe('uploadDirectory', () => {
}),
])

/** @type {Omit<import('../src/types.js').StoreAddSuccessUpload, 'link'>} */
/** @type {Omit<import('../src/types.js').StoreAddSuccessUpload, 'link'|'allocated'>} */
const res = {
status: 'upload',
headers: { 'x-test': 'true' },
Expand All @@ -339,6 +340,7 @@ describe('uploadDirectory', () => {
link: /** @type {import('../src/types.js').CARLink} */ (
capability.nb.link
),
allocated: capability.nb.size,
},
})),
},
Expand Down Expand Up @@ -409,6 +411,7 @@ describe('uploadDirectory', () => {
link: /** @type {import('../src/types.js').CARLink} */ (
invocation.capability.nb.link
),
allocated: invocation.capability.nb.size,
},
}
}),
Expand Down Expand Up @@ -545,7 +548,7 @@ describe('uploadCAR', () => {
}),
])

/** @type {Omit<import('../src/types.js').StoreAddSuccessUpload, 'link'>} */
/** @type {Omit<import('../src/types.js').StoreAddSuccessUpload, 'link'|'allocated'>} */
const res = {
status: 'upload',
headers: { 'x-test': 'true' },
Expand All @@ -567,6 +570,7 @@ describe('uploadCAR', () => {
link: /** @type {import('../src/types.js').CARLink} */ (
capability.nb.link
),
allocated: capability.nb.size,
},
}
}),
Expand Down Expand Up @@ -646,7 +650,7 @@ describe('uploadCAR', () => {
}),
])

/** @type {Omit<import('../src/types.js').StoreAddSuccessUpload, 'link'>} */
/** @type {Omit<import('../src/types.js').StoreAddSuccessUpload, 'link'|'allocated'>} */
const res = {
status: 'upload',
headers: { 'x-test': 'true' },
Expand All @@ -659,15 +663,15 @@ describe('uploadCAR', () => {
add: provide(StoreCapabilities.add, ({ capability, invocation }) => {
assert.equal(invocation.issuer.did(), agent.did())
assert.equal(invocation.capabilities.length, 1)
const invCap = invocation.capabilities[0]
assert.equal(invCap.can, StoreCapabilities.add.can)
assert.equal(invCap.with, space.did())
assert.equal(capability.can, StoreCapabilities.add.can)
assert.equal(capability.with, space.did())
return {
ok: {
...res,
link: /** @type {import('../src/types.js').CARLink} */ (
capability.nb.link
),
allocated: capability.nb.size,
},
}
}),
Expand Down
4 changes: 4 additions & 0 deletions packages/upload-client/test/store.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ describe('Store.add', () => {
url: 'http://localhost:9200',
link: car.cid,
with: space.did(),
allocated: car.size,
}

const service = mockService({
Expand Down Expand Up @@ -103,6 +104,7 @@ describe('Store.add', () => {
url: 'http://localhost:9400', // this bucket always returns a 400
link: car.cid,
with: space.did(),
allocated: car.size,
}

const service = mockService({
Expand Down Expand Up @@ -156,6 +158,7 @@ describe('Store.add', () => {
url: 'http://localhost:9500', // this bucket always returns a 500
link: car.cid,
with: space.did(),
allocated: car.size,
}

const service = mockService({
Expand Down Expand Up @@ -254,6 +257,7 @@ describe('Store.add', () => {
url: 'http://localhost:9200',
link: car.cid,
with: space.did(),
allocated: car.size,
}

const service = mockService({
Expand Down
3 changes: 2 additions & 1 deletion packages/w3up-client/test/capability/store.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ describe('StoreClient', () => {
it('should store a CAR file', async () => {
const service = mockService({
store: {
add: provide(StoreCapabilities.add, ({ invocation }) => {
add: provide(StoreCapabilities.add, ({ invocation, capability }) => {
assert.equal(invocation.issuer.did(), alice.agent.did())
assert.equal(invocation.capabilities.length, 1)
const invCap = invocation.capabilities[0]
Expand All @@ -27,6 +27,7 @@ describe('StoreClient', () => {
url: 'http://localhost:9200',
link: car.cid,
with: space.did(),
allocated: capability.nb.size,
},
}
}),
Expand Down
Loading
Loading