User Details
- User Since
- Nov 30 2021, 11:14 AM (68 w, 4 d)
Fri, Mar 24
sorry, requesting review again because i decided to just add the thread safety changes to this diff....
return early when we can
sorry addressing feedback now
make promises map thread-safe
i also missed jon's feedback from earlier so i'll address those comments now
yeah fair i should just split this out into two diffs
address last piece of feedback
remove unnecessary include
Wed, Mar 22
nice! just one nit
just a couple nits
Mon, Mar 20
Thu, Mar 16
just one nit inline
Wed, Mar 15
i'm a little confused why we're moving config.rs and constants.rs into this cargo project. they seem pretty specific to the identity service. if we decide we want to store the secret key in a different location, it feels awkward that we'd have to modify a library that the identity service consumes rather than the service's own config
Tue, Mar 14
jon’s suggestion makes sense to me
Mon, Mar 13
yeah let's de-dupe and remove deviceEd25519PublicKey. one other nit inline
Fri, Mar 10
i frankly don't think this adds much clutter at all... also it's more tedious for me to switch between diffs to see how these dependencies are being used. just looking at this diff, i have no idea what any of these dependencies are for besides the one that @jon annotated, and even then i still have to look at another diff to see what handle_pake_to_grpc_err looks like. the lack of context in this diff makes it difficult to determine if a given dependency is appropriate or propose an alternative.
Thu, Mar 9
i don't love adding the dependencies in a separate diff, would like to see them added when they're actually being used
Wed, Mar 8
the set of RPCs that devices will call is different than the set that ashoat's keyserver can call today. also, the actual messages are slightly different, as the linear issue i linked explains. we think it makes more sense to just have a separate service for devices, rather than introduce a bunch of conditional logic in the current service
how about identity.general? identity.client feels weird
Tue, Mar 7
oh nvm, i’ll delete my comment
remove "// eslint-disable-next-line no-unused-vars"
Mon, Mar 6
Sat, Mar 4
Fri, Mar 3
map camelCase JSON to snake_case struct
Please wait for CI before landing
please address the typos and import consolidation before landing, i'm fine with deferring the error logging/Status stuff and creating a linear task for it
back to your queue to address feedback on previous revision
LGTM
I'm good with backlogging my suggestions, just want to make sure we call them all out specifically in the linear task so we don't lose track of small things
Let's capitalize Identity Service everywhere in the .proto file for consistency. otherwise looks good
address feedback
address feedback
address feedback
Realizing my comment from last time probably doesn't make sense to Flow
address feedback