we can reuse these on web, too, so moving them out of keyserver
Details
Details
able to log in to staging identity service from my local keyserver after clearing the metadata table
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| lib/flow-typed/npm/@commapp/olm_vx.x.x.js | ||
|---|---|---|
| 1 ↗ | (On Diff #36343) | copied this file from web |
| lib/utils/olm-utils.js | ||
| 64 ↗ | (On Diff #36343) | Will update this comment |
| 85 ↗ | (On Diff #36343) | I think this is fine, but I changed this line from throw new ServerError to throw new Error. wanted to call it out since it might be hard to tell. this is the only line of code i modified |
| keyserver/src/utils/olm-utils.js | ||
|---|---|---|
| 23 ↗ | (On Diff #36343) | This can be moved to lib as well. |
| 71–107 ↗ | (On Diff #36343) | I think that we could move to lib any function from this file that doesn't touch Rust/Database ops/networking. One thing that might block us is that a call to olm.init() isn't straightforward on web (see web/olm-utils.js file). @varun could you create a follow-up task to move all functions from this file that don't touch Rust/Database ops/networking to lib and seek for places in web that they could be used and link this task prior to landing? |
| keyserver/src/utils/olm-utils.js | ||
|---|---|---|
| 23 ↗ | (On Diff #36343) | Why does it need to be moved? It's only used in keyserver, it looks like |
| 71–107 ↗ | (On Diff #36343) | I'm not sure we need a task for this... we can move things to lib as needed, when we identify cases where code can be shared. I don't think we need to preemptively move things to lib... feels like overengineering, and could lead to eg. bloating our native bundle (I think some upcoming Metro release introduces tree shaking, but not sure it's there yet) |
| lib/utils/olm-utils.js | ||
| 85 ↗ | (On Diff #36343) | This is risky... ServerError is special-cased in keyserver code here and here Seeing as this function gets called from responders, I think it's important to make sure we throw ServerError in those contexts. I think we have two options:
|
| lib/utils/olm-utils.js | ||
|---|---|---|
| 85 ↗ | (On Diff #36343) | Option 2 looked better but was a little awkward to implement since retrieveAccountKeysSet is a callback function. I updated fetchCallUpdateOlmAccount to re-throw any error from the callback function as a ServerError. Verified with this gist: https://gist.github.com/vdhanan/5949c0a4f0501d38a953774f81328916 I was able to see ServerError: invalid_prekey in the logs after applying this patch |
Comment Actions
Seems reasonable
| keyserver/src/updaters/olm-account-updater.js | ||
|---|---|---|
| 50 ↗ | (On Diff #36498) | Better to use getMessageForException here |