A signature of the returned keys changes - the new one is more convenient to use with Olm. Additionally, this diff introduces more efficient validation.
Depends on D10665
Details
Details
Check if fetching keys returns correct values on native and web.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/identity-service/identity-service-context-provider.react.js | ||
|---|---|---|
| 97 ↗ | (On Diff #35728) | Should we always validate responses from the identity service? Wondering if we've discussed a "best practice" for this, and whether it's being done in all cases |
| 100 ↗ | (On Diff #35728) | Can you explain why this is necessary? Is there a better alternative, such as adding a typehint to the resultObject definition on line 78? |
Comment Actions
Some notes about convention so please address comments but overall LGTM
| lib/types/identity-service-types.js | ||
|---|---|---|
| 5–6 ↗ | (On Diff #35728) | can be merged |
| 43 ↗ | (On Diff #35728) | I have mixed feelings about this. It assumes that oneTimeKey is mandatory - while in proto file it is optional. We should I think check it before and throw an error before that oneTimeKey is missing and you can not create a session rather than a validation error - because that is not true in some sense. |
| native/identity-service/identity-service-context-provider.react.js | ||
| 97 ↗ | (On Diff #35728) | I think it makes a lot of sense on the web because we get responses directly to the JS side, but on the native, it first goes through statically typed Rust and any issues with type should probably cause panic or err on Rust side, like here, but we then return this value as string so maybe it make some sense? And we also already have validators for the web we can make it more "symmetric". If we agree to add tcomb validation also here I will add this to the methods I implemented while adding those to this interface. |
| 100 ↗ | (On Diff #35728) |
Personally, I don't like it, because in line 78 we don't know it yet, we can truly say which type it is after validation, this is "lying for a moment" - but not feel strongly about this and this is only my personal opinion I think we have convention like here (I've seen it multiple times in the codebase) which at a logical point makes the most sense and avoids explicit any-casting |
| lib/types/identity-service-types.js | ||
|---|---|---|
| 43 ↗ | (On Diff #35728) | Makes sense - I'll update the validation |
| native/identity-service/identity-service-context-provider.react.js | ||
| 97 ↗ | (On Diff #35728) | Adding validation on native makes the code easier to reason about - we don't need to check the Rust implementation to make sure that the shape is ok (we're sending a plain string, so it's like casting through any). Overall, the more validation we have, the better - it's a lot easier to deal with inconsistencies when they are caught on validation than when they are caught later in a random place in the logic. |
| 100 ↗ | (On Diff #35728) | It looks like we can use assertWithValidator function that will either throw or return the correct type. |