native invocation of keyserver endpoint supporting sign in with ethereum. linear task
Details
Details
new stuff to test:
- i recommend you run this on the simulator, with your phone nearby open to an ethereum wallet.
i've verified all the autolinking from comm -> wallet -> comm works during the signing process so we can treat physical device/sim as the same. if you want to run on physical device you'll have to tweak your network.json for your macbook's keyserver
- connect wallet by tapping QR code, scan QR using physical device and connect
- tap again to sign a message
- proceed to be authenticated
existing stuff we need to verify still works:
- log in with existing account
- test creating account without SIWE
- login in with new account
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- derek/siwe-types
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Lots of issues here
| keyserver/src/creators/account-creator.js | ||
|---|---|---|
| 97 ↗ | (On Diff #18443) | How does this work if the user does an old-school username/password registration? Can you amend your Test Plan to cover testing that? |
| keyserver/src/responders/user-responders.js | ||
| 5 ↗ | (On Diff #18443) | You should never submit a diff with commented-out code |
| 297–301 ↗ | (On Diff #18443) | You should never submit a diff with commented-out code |
| 354 ↗ | (On Diff #18443) | I'm confused, why are we pulling out watchedIDs here? Does createAccount really not need it? If createAcount doesn't need it, why does siweResponder need it? |
| 366–418 ↗ | (On Diff #18443) | Feels like you copy-pasted this code. Can you factor it out instead? |
| 383 ↗ | (On Diff #18443) | You should not be submitting code with $FlowFixMes |
| keyserver/src/responders/user-responders.js | ||
|---|---|---|
| 354 ↗ | (On Diff #18443) | createAccount doesn't need it because there are no watched threads if you are signing up. logging in, on the other hand, there may be threads you are watching, hence why siweResponder needs it |
Comment Actions
This diff is way too large and should've been broken up. You're still in a PR mindset. I wish we had time to do it right but I know we'll miss our goals if I don't relax expectations
i've verified all the autolinking from comm -> wallet -> comm works during the signing process so device/sim are the same, you'd have to tweak your network.json for your macbook's keyserver
Struggling to follow this sentence
| keyserver/src/responders/user-responders.js | ||
|---|---|---|
| 354 ↗ | (On Diff #18443) | Got it, can you add a code comment? |
| 366–418 ↗ | (On Diff #18443) | This wasn't addressed |
| 294 ↗ | (On Diff #18493) | Why are you doing this here, it makes no sense. There is no "old version" of this API, you are literally introducing the API right now |
| lib/reducers/data-loaded-reducer.js | ||
| 20 ↗ | (On Diff #18493) | If this replaces logInActionTypes for SIWE we are going to need to add it to a WHOLE lot of other places, git grep logInActionTypes.success please |
| lib/types/account-types.js | ||
| 163–165 ↗ | (On Diff #18493) | Shouldn't this just be SIWEResult = RegisterResult? Arguably you don't even need the type alias Question – why isn't it something like RegisterResult | LogInResult? |
| native/account/siwe-panel.react.js | ||
| 47 ↗ | (On Diff #18493) | Can you remind me what "mocked" means in this context |
| native/utils/url-utils.js | ||
| 43 ↗ | (On Diff #18493) | Can you add a comment (probably will need multiple lines) explaining what's going on here Remember max 80 char width lines |
| keyserver/src/responders/user-responders.js | ||
|---|---|---|
| 294 ↗ | (On Diff #18493) | this is the old login API, although the way the diff broke it down makes it seem like this is new. if you look above, loginResponder became loginQueries |
| lib/reducers/data-loaded-reducer.js | ||
| 20 ↗ | (On Diff #18493) | good catch - thinking about it, do you foresee any problems using loginActionTypes here instead? |
| native/account/siwe-panel.react.js | ||
| 47 ↗ | (On Diff #18493) | it's an outdated line, i thought i removed it in this diff but it's in the diff higher in the stack |
Comment Actions
i'm familiar with the stack process now, going forward will break up diffs more sensibly
i've verified all the autolinking from comm -> wallet -> comm works during the signing process so device/sim are the same, you'd have to tweak your network.json for your macbook's keyserver
Struggling to follow this sentence
the only difference between sim and physical device is on sim, you scan the QR, and physical device will deep link you to rainbow and back to comm between every interaction (connecting wallet, signing message). i've tested it on the physical device, and it successfully deep links back and forth, so we can move forward with testing treating them the same
Comment Actions
the only difference between sim and physical device is on sim, you scan the QR, and physical device will deep link you to rainbow and back to comm between every interaction (connecting wallet, signing message). i've tested it on the physical device, and it successfully deep links back and forth, so we can move forward with testing treating them the same
This sentence reads cleanly to me but I'm still a bit confused. Who handles the simulator differently? Where does the QR code appear?
Can you share a video of the QR code experience?
| keyserver/src/responders/user-responders.js | ||
|---|---|---|
| 354 ↗ | (On Diff #18443) | I think this wasn't addressed? |
| 294 ↗ | (On Diff #18493) | Ah, you're totally right... sorry for my brusque comment |
| lib/reducers/data-loaded-reducer.js | ||
| 20 ↗ | (On Diff #18493) | Yeah don't think that will work, siweAction can return the result of a log in or a register depending on the scenario. I think you need to do what I suggested above:
It should be possible to get that done in <2 hours I think, you just need to go through each occurence and add a || siweActionTypes.success. Encourage you to get on that ASAP |
| lib/reducers/data-loaded-reducer.js | ||
|---|---|---|
| 20 ↗ | (On Diff #18493) | thoughts on abstracting it to a util function isSuccessfulAuthType? this would help accommodate something like apple auth, unless that would fall under register/login action |
| lib/reducers/data-loaded-reducer.js | ||
|---|---|---|
| 20 ↗ | (On Diff #18493) | Yeah that makes sense to me |
Comment Actions
@ashoat addressed the comments & modified all reducers in https://phab.comm.dev/D5851
Comment Actions
I know we discussed updating this diff in place, but as I've continued working it's been getting VERY unwieldy and frustrating.
Between the merge conflicts, the wide surface area the diff covers, the increased scope (validating nonce, message, etc), the things that need to be moved around, the changes that should be in D5851 instead, etc: I think it REALLY makes sense to break this diff down and put up a new stack.
I've been working with this code for hours and I'm confident I'll be able to move MUCH faster that way and review will go MUCH smoother.
I'm fully cognizant that I'm going against what was agreed upon earlier, so want to make it clear that I've thought this through and am not making this decision lightly. I want to finish this work as soon as possible and that's how I, personally, will be able to do it the fastest.
| native/utils/url-utils.js | ||
|---|---|---|
| 43–49 ↗ | (On Diff #18618) | Interesting... it's definitely been a problem for me. My SIWE testing instructions explicitly have the user patch in the production URL. I've had to test on prod while developing this basically |
| native/utils/url-utils.js | ||
|---|---|---|
| 43–49 ↗ | (On Diff #18618) | huh weird, I'm using simulator instead of physical device so that might be why |
| native/utils/url-utils.js | ||
|---|---|---|
| 43–49 ↗ | (On Diff #18618) | How are you able to test using an Ethereum wallet on simulator? |
| native/utils/url-utils.js | ||
|---|---|---|
| 43–49 ↗ | (On Diff #18618) | I'm using the QR code workflow instead. I'll make sure to test the physical device workflow as well. |