feat!: return allocated bytes in store/add receipt

[alanshaw]

This PR adds a new field to the store/add success result: allocated: number - the total bytes allocated in the space to accommodate the stored item, it may be zero if the item is already stored in this space.

This allows us to accurately report and bill for items stored in a space. Currently, store/add when the item is already stored in the space will cause a new diff to be created, effecitvely counting the same shard multiple times. This could happen because of accidentally uploading the same item, or because of a retry of a big item where some shards were successful.

The following additional changes enable this functionality:

• StoreTable and UploadTable methods now have return types that are Result<O, X> - this allows us to specify and communicate 2 errors RecordNotFound and RecordKeyConflict (explained in the following bullets).
• In the context of these 2 tables the semantics of insert have changed to be more in line with postgres/other DBs - "insert" means add to the DB or fail (with RecordKeyConflict) if an item with the same key already exists.
  • We can satisfy this constraint in dynamodb with condition expressions or ReturnValues.
• In the UploadTable I've renamed insert to upsert since the behaviour in use here is more akin to "update or insert" - aka "upsert".
• remove and get now fail with RecordNotFound if the item to delete is not available.
  • Again in dynamodb we can satisfy this constraint with condition expressions or ReturnValues

[vasco-santos]

LGTM
small things inline

[gobengo]

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.

[alanshaw]

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

[vasco-santos]

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

[alanshaw]

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

[alanshaw]

@gobengo please for another review 🙏

A few changes since you last looked:

• StoreTable and UploadTable methods now have return types that are Result<O, X> - this allows us to specify and communicate 2 errors RecordNotFound and RecordKeyConflict (explained in the following bullets).
• In the context of these 2 tables the semantics of insert have changed to be more in line with postgres/other DBs - "insert" means add to the DB or fail (with RecordKeyConflict) if an item with the same key already exists.
  • We can satisfy this constraint in dynamodb with condition expressions or ReturnValues.
• In the UploadTable I've renamed insert to upsert since the behaviour in use here is more akin to "insert or update" - aka "upsert".
• remove and get now fail with RecordNotFound if the item to delete is not available.
  • Again in dynamodb we can satisfy this constraint with condition expressions or ReturnValues

[vasco-santos]

I think there is some weirdness on the return types of errors? I don't understand why ts is not complaining

[vasco-santos]

nice, we drop a request :)

[ALIK202]

0xBa5c04Cdf4771ac6Da3f48fc368314a00DA064B7