1. This should probably be defined in lib/types
2. It should be defined like other constants are defined. Try git grep Object.freeze

Neither of these should be async functions since they don't use the await keyword

Given this gets called every time we need to use the client, I think we should have it return the client directly. That lets us skip the second check on line 130. The same change can be made for deleteUser

What do errors look like if these promises reject? Should we wrap each call with a try-catch so we're surfacing a more clear error? It would be good to make sure we can distinguish which of these calls caused an error if one of them rejects

JSON.stringify seems like an implementation detail. I'd rather return the { userID, accessToken } object directly and let the caller stringify it if that's what they need to do

This is a ridiculous line. Object.freeze does nothing when passed a number. Before you use any library function in any language ever, you should do some research to understand what it is that you're calling

this needs to be defined in web because its type (DeviceType, which is a literal union) comes from web/protobufs/identity-structs.cjs

That doesn't explain why it "needs" to be defined in web. It sounds like you're saying it will only be used on web since the equivalent code on both native and keyserver is implented in Rust. But the constant itself seems more general, and it feels more appropriate to keep it next to the other identity-related constants.

Meanwhile, there's really not much of a downside to defining something in lib even if it's only used in web, given that all packagers these days support tree shaking.

i've defined it like the others, though

You definitely have not defined it "like the others". Please try again, and this time please understand that it's critical you learn to read and understand code before using it.

Shouldn't this be implementing IdentityServiceClient? And shouldn't IdentityServiceClient be extended to include loginPasswordUser?

If there was a stack here I could check if it's implemented in a follow-up diff, but since there's no stack I'm left to assume it's simply not done.

@palys is modifying the same code in D10584. You'll likely have rebasing issues... I wonder if it would be easier for you to just copy-paste his approach here

Separately, it looks like you're taking a different approach if the authClient is not defined when deleteUser is called. Would be good for you two to discuss which approach makes sense.

We should use this approach instead

Same here

we currently return a stringified object in commRustModule.loginPasswordUser, so i've returned the same stringified object here so we have a consistent interface to use in lib

In D10584, @tomek adds getKeyserverKeys to the IdentityServiceClient interface, but he has it return an object instead of a string. He's able to do this because of the new abstraction layer he introduces to native (IdentityServiceContextProvider).

We should take the same approach here. IdentityServiceClient should be typed to return proper objects instead of strings. The conversion on native will be handled in IdentityServiceContextProvider, and the conversion on web will be handled automatically by the proto codegen here in IdentityServiceClientWrapper.

I want to try to remove this from the API and get the key info within the context provider / client wrapper

Tried to preempt this here... we should not be using generateAndGetPrekeys here

Maybe you can introduce a diff preceding this one in the stack that implements an alternative to generateAndGetPrekeys that doesn't generate a new prekey on every login?

The call to generateAndGetPrekeys has the potential to result in new prekeys being generated. After we upload these new keys to the identity service, we need to mark them as published

(This will likely still be true after we refactor this to not use generateAndGetPrekeys)

The call to getDeviceKeyUpload has the potential to result in new one-time keys being generated. After we upload these new keys to the identity service, we need to mark them as published

(The one-time key generation should be revisited, but the above will likely still be true afterwards)

It seems like these both call CryptoModule::generateOneTimeKeys under the hood, but I can't tell what that's function doing.

It seems like it's generating one-time keys too often... a similar problem to the one I highlighted in D10808.

1. The comparison in this->keys.oneTimeKeys.size() == oneTimeKeysSize should probably not be an equality check... I imagine we want to avoid generating new one-time keys if the limit is exceeded, rather than if it's exactly met.
2. I don't have a good intuition for what this->keys.oneTimeKeys is, or why it's being checked.

Shouldn't we have consistency between native and web? I suspect that generateOneTimeKeys needs to be updated to match how retrieveAccountKeysSet handles one-time keys, but first we should figure out what this function is doing today and what this->keys.oneTimeKeys is for.