this update reflects the changes to InboundKeysForUserResponse made in D10560.
the codegen had a hard time with the oneOf's, so had to make some manual changes to the .flow file
Details
Details
- Reviewers
ashoat bartek - Commits
- rCOMM9fa7751477f9: [web] update grpc-web codegen
called getInboundKeysForUser on my local identity service from root.js and got back a response containing a wallet address and social proof.
called it again and successfully got a username + keys for a username/password user.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| web/protobufs/identity-auth-structs.cjs.flow | ||
|---|---|---|
| 35 ↗ | (On Diff #35448) | changed the return type from string to string | void to match identity-auth-structs.cjs |
| 37–38 ↗ | (On Diff #35448) | these were missing originally so i had to manually add them. confirmed they exist in identity-auth-structs.cjs |
| 56 ↗ | (On Diff #35448) | the ts codegen made this non-optional originally. i changed it to optional, matching the code in the identity-auth-structs.cjs |
| 249 ↗ | (On Diff #35448) | codegen made this optional but i don't think it should be. should i change it? |
Comment Actions
I'm a bit worried that the custom changes here will break the codegen verification script @bartek is working on in ENG-6383. Setting him as blocking to make sure there aren't any conflicts. If the oneof approach we're taking here results in too much codegen weirdness, we could consider option 1 of ENG-6372
(Sidenote – I really think the codegen updates should be included in the diff that updates the proto going forward. In this case, the codegen gave us some surprises that might force us to go rethink the approach in the proto file. ENG-6383 should help with this.)
| web/protobufs/identity-auth-structs.cjs.flow | ||
|---|---|---|
| 32 ↗ | (On Diff #35449) | Why are there three cases? |
| 56 ↗ | (On Diff #35448) | It's a bit inconsistent that getUsername can either be null or string, but this can either be undefined or string |
| 249 ↗ | (On Diff #35448) | Hmmm yeah that's weird... based on the proto, I'm not sure why it should be optional |
Comment Actions
As long as we don't modify .cjs files manually (non-flow), this doesn't conflict with ENG-6383.
| web/protobufs/identity-auth-structs.cjs.flow | ||
|---|---|---|
| 32 ↗ | (On Diff #35449) | from identity-auth-structs.cjs: proto.identity.auth.Identity.IdentityInfoCase = {
IDENTITY_INFO_NOT_SET: 0,
USERNAME: 1,
ETH_IDENTITY: 2
}; |
| 56 ↗ | (On Diff #35448) | i agree. i'm going to remove the has... and clear... methods here, update this type to export type IdentityObject = {
username: string,
ethIdentity: ?EthereumIdentityObject,
}and also change the get methods to getUsername(): string; getEthIdentity(): ?EthereumIdentity; This will simplify things a bit and matches the generated code in identity-auth-structs.cjs better |
| 249 ↗ | (On Diff #35448) | i'm going to update this as well to better match the code in identity-auth-structs.cjs. |
| web/protobufs/identity-auth-structs.cjs.flow | ||
|---|---|---|
| 40–41 | ended up keeping these methods because EthIdentity can be undefined per the structs.cjs file | |
| 55 | this matches the structs.cjs file more precisely, since it can be an EthereumIdentityObject or undefined | |
| 247 | identity can be an IdentityObject or undefined per the structs file, so changed this type accordingly | |
| web/protobufs/identity-auth-structs.cjs.flow | ||
|---|---|---|
| 54 | A bit confused why username isn't optional given its a oneof. Does this get set to the empty string for an ethereum identity, or is it set to the wallet address? And does the identity-auth-structs.cjs file make sure that username is always set? | |
| web/protobufs/identity-auth-structs.cjs.flow | ||
|---|---|---|
| 54 | the protobuf response from the identity service won't contain a username at all if it's a wallet user. identity-auth-structs.cjs sets it to an empty string, though, which is why it's not optional in this type | |