Page MenuHomePhabricator

[services][identity] wallet login
ClosedPublic

Authored by varun on Jun 16 2022, 12:46 PM.
Tags
None
Referenced Files
F3112147: D4285.diff
Thu, Oct 31, 4:53 PM
Unknown Object (File)
Wed, Oct 23, 2:38 PM
Unknown Object (File)
Tue, Oct 22, 11:26 AM
Unknown Object (File)
Mon, Oct 21, 6:34 AM
Unknown Object (File)
Sun, Oct 20, 7:20 PM
Unknown Object (File)
Sun, Oct 20, 7:19 PM
Unknown Object (File)
Sat, Oct 19, 8:18 AM
Unknown Object (File)
Sat, Oct 19, 3:05 AM

Details

Summary

Full implementation of wallet login. Steps:

  1. Parse SIWE message and signature from request
  2. Verify message using siwe-rs crate
  3. If verification is successful, generate a token, store it in the token table in DynamoDB, and then return it to the caller

Note that PAKE login is not covered here. It will be in a separate diff.

Test Plan

Successfully signed in with a metamask wallet

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jun 16 2022, 12:49 PM
Harbormaster failed remote builds in B9784: Diff 13543!
varun requested review of this revision.Jun 16 2022, 1:15 PM

changed the type of token in the .proto file to string, so this is the corresponding code change

New external dependencies look good

jim added inline comments.
services/identity/src/service.rs
71 ↗(On Diff #13549)

Error case isn't handled

72 ↗(On Diff #13549)

When would this be None?

73 ↗(On Diff #13549)

This is really hard to read, please refactor. The error handling in the match arms is heavily duplicated. For starters, make a subroutine that returns a Result, and in here do the if let Err(err) = result { log and break } thing once.

This revision now requires changes to proceed.Jun 17 2022, 11:53 AM

create a subroutine to improve error handling

varun marked 3 inline comments as done.

handle unhandled error case and unhandled option case

services/identity/src/service.rs
72 ↗(On Diff #13549)

oneof fields are completely optional, which is why this can be None. we are reconsidering the use of oneof across all our .proto files, but i've handled the None case in the latest revision.

services/identity/src/service.rs
87–105

Don't love how I'm repeating myself here but wasn't sure how to avoid it

tomek added inline comments.
services/identity/src/service.rs
62

What is the purpose of it?

87–105

I think you can create a function that contains the repeated logic

services/identity/src/service.rs
62

MyIdentityService doesn't implement Debug, so we have to skip self when we do tracing

This revision is now accepted and ready to land.Jun 27 2022, 2:45 PM
This revision was automatically updated to reflect the committed changes.
services/identity/src/service.rs
75–123 ↗(On Diff #13860)

Readability is not great here imo....

services/identity/src/service.rs
75–123 ↗(On Diff #13860)

Yeah, agreed. Made it a little better in a subsequent diff that handled PAKE login, and have a diff out to improve it further