To be fully correct, these should also be added to the DeviceRow struct in device_list.rs.
But as they're only internal helper counters, I'm wondering if that makes sense (gives no real benefit).
But at least a comment should be added to the struct to indicate they exist but are not handled there.

Maybe we should add a retry counter to these loops? see mod batch_operations in comm_lib/database.rs

Ideally, we could extract sth like transact_write_helper similar to batch write, but this is a follow-up, not sure if worth doing now

The AWS guide here https://aws.amazon.com/blogs/database/implement-resource-counters-with-amazon-dynamodb/ (Approach 1 - Atomic counters)
says that this might be unreliable due to transient AWS errors.
Not sure how it behaves in transactions, though. Hopefully it fails the whole transaction

I guess we have two separate transactions because we want to track counts of content and notif keys separately.

Alternative approach would be

1. Prepare put requests for content and notifs
2. Map them into tuples ("content/notif", PutRequest)
3. Merge these tuples together
4. Batch them into 98 chunks (100 - 2)
5. For each chunk,
   • count let content_count = tuples.iter().filter(|v| v.0 == "content").count();
   • count let notif_count = tuples.iter().filter(|v| v.0 == "notif").count();
   • requests: let requests = tuples.into_iter().map(|v| v.1).collect::<Vec<_>>();
   • single transaction:
     • requests
     • update_content_otk_operation
     • update_notif_otk_operation

Having single (and less transient-error-prone) DDB transaction at a cost of code complexity

Same as above - we have helpers for that

Builder pattern helps to avoid some memory operations and simplify code ;)

1. I know this is ugly, I haven't found any nice enough way of dealing with DDB error handling
2. Maybe it's possible to do sth like this

fn is_transaction_canceled(err: &DynamoDBError, reason: &str) -> bool {
  match err {
    DynamoDBError::TransactionCanceledException(
              TransactionCanceledException {
                cancellation_reasons: Some(reasons),
                ..
              },
            ) if reasons
              .iter()
              .any(|reason| reason.code() == Some(reason)) => true,
    _ => false
}

It's good to add .projection_expression(devices_table::ATTR_CONTENT_OTK_COUNT) to avoid unnecessarily getting other fields.

Otherwise, you can just use db_client.get_device_data() (it's in device_list.rs)

We have dedicated helper struct that does this (device_list.rs)

Simplify

Unfortunately this is the correct way now, because there's a problem with TryFromAttribute for numbers - https://linear.app/comm/issue/ENG-4658/improve-database-type-conversions-for-numbers

nit

One thing that came to my mind - we should not allow uploading more than 98 keys in total.
The client should batch keys into chunks, not server. But I guess we never exceed this threshold anyway (have even lower limits).

If ever uploading more at once, the UploadOneTimeKeys RPC should return the keys that were successfully uploaded (or inverse - failed). And this would be too much complexity. This way, the caller will know which keys were successfully uploaded.
Example:
Uploading 250 keys is 3 transactions. Let's say 1st succeeds but 2nd fails. Some keys are uploaded, but others not

Also, this limit will let us avoid splitting into chunks server-side.

We don't need this anymore, that was to monitor the previous inelegant solution

Same as above

olm only remembers the last 100 one-time keys so clients will never upload more than 100

From the aws docs:

Don't group operations together in a transaction if it's not necessary. For example, if a single transaction with 10 operations can be broken up into multiple transactions without compromising the application correctness, we recommend splitting up the transaction. Simpler transactions improve throughput and are more likely to succeed.

Following this principle, I think we should avoid combining content and notif updates in a single transaction

good point. going to just add a comment for now, since i can't think of a good reason to add these fields to the struct

i'll add a basic retry counter that returns an error after the 8th try. i'll also leave a todo comment to introduce and use a transact write helper

i think it should fail the whole transaction

I think we can assume that the list of OTKs that the client provides is sorted (i think oldest to newest, but need to confirm this). so if the client provides 150 content otks, we can just throw away the first 50 in the list since the olm account will have thrown them away already

And this would be too much complexity

Did you mean wouldn't be too much complexity?

olm only remembers the last 100 one-time keys so clients will never upload more than 100

*should never, i suppose it's possible though

Ah I understand your concern now. Let's just immediately fail any request that comes in (register, upload_one_time_keys) that has more than 98 keys total so that we can do everything in a single transaction and avoid the scenario where, say, 99 content keys are uploaded but 0 notif keys are

After thinking about this a little more, the alternative you propose seems like the right way to go (except we don't need chunks since we'll just fail the request immediately if the caller provides more than 49 content or notif otks)

transact_items() expects a single TransactWriteItem object, so we have to use set_transact_item() since we have potentially 100 items here