Makes sense
- Feed Queries
- All Stories
- Search
- Feed Search
- Transactions
- Transaction Logs
All Stories
Oct 10 2023
Rebase
Update isInvalidSidebarSource to pass unit tests
Rebase
Rather than a script, it would make my life a little easier if we could put this is in keyserver/src/database/migration-config.js – that way it should automatically run when I deploy the keyserver next
Rebase
In D9413#276917, @michal wrote:
- Don't we need to update the command in the docker file?
@michal pointed out that doing this will later cause a circular dependency, so I'm keeping it in message-utils
I agree we do not need to call revalidateAccountPrekeys anymore from getOlmSessionInitializationDataResponder. However, I think we still need to do it in registerOrLogin, so that we make sure the prekey that we pass when calling the identity service is initialized (in case it is the first call) and not expired (in case the keyserver has not been touched in some time).
In D9355#276898, @michal wrote:If I understand correctly this will try to remove S3 blobs that might not exist (for holders can exist without having a blob uploaded and the logic will run for them too). Is this expected?
I noticed that when Jon was first working on this, he copy-pasted some of @marcin's functions and kept the same name. Since that point, it appears that the copy-pasted functions have diverged. I think we should look to deduplicate them, or maybe have one call the other. We should certainly rename them to have different names if we are going to keep both.
In D9351#276883, @michal wrote:LGTM, potentially we could add these optimisations:
- Return impl Iterator<> instead of Vec
Ensure in Objective-C code that messageInfosArray is not optional.
Update isInvalidSidebarSource
- Make messageInfosArray not optional since it is actually not
- Use flatMap
Remove NotificationsCryptoModule instantiation. Return opaque type containing crypto module state accessible only from NotificationsCryptoModule implementation file.
Thanks for the test videos
Seems reasonable, could be good to test it with an ENS name being mentioned as well to cover all grounds
- Don't we need to update the command in the docker file?
- This way cargo run -- server --http-port 123 doesn't work, only cargo run -- --http-port 123 server works, which feels weird. We could potentially add global = true to all args but that also has it's problems
If I understand correctly this will try to remove S3 blobs that might not exist (for holders can exist without having a blob uploaded and the logic will run for them too). Is this expected?
Attaching videos here since it's the last diff in the stack:
LGTM, potentially we could add these optimisations:
- Return impl Iterator<> instead of Vec
- Return &'self str in blobs_to_find_holders
but I haven't analysed it very thoroughly so there might be lifetime/ergonomics issues
Rebase
Changing this diff to upgrade Lottie packages instead. Way better idea than forcing a Yarn resolution that may break things in the future
Upgrade Lottie instead
Looks fine assuming that we always want to rotate both even if only one actually needs rotation. Make sure you get @varun acceptance before landing since I don't have enough context to review publishNewPrekeys
Shouldn't we also handle processUpdatesActionType action?
Looks good to me but letting others review as well