[Wallet] Add support for 8 digits verification codes #5925
Conversation
| */ | ||
| getAttestationForSecurityCode(serviceURL: string, body: AttesationServiceCodeForSecurityRequest) { | ||
| const urlParams = new URLSearchParams({ ...body }) | ||
| return fetch(appendPath(serviceURL, 'get_attestations') + '?' + urlParams, { |
There was a problem hiding this comment.
Does this include any retry logic? We should add this (with exponential backoff) if not.
There was a problem hiding this comment.
I'm having a hard time to understand when do we want to do that. Basically the fact that we are not 100% sure that current validator is the one who sent the code brings some complexity here. Possible cases I see:
- Timeout - is it really worth trying it once again considering that it might not be the one we need.
4xx- we expect this to happen for some validators.5xx- something went really wrong on the attestation service side. Probably, it make sense to try it later, but how much time we want to spend on each validator in total?
Any thoughts? cc: @aslawson @timmoreton @codyborn
There was a problem hiding this comment.
Perhaps we can parallelize the requests.. if we get a 200 response from one of them we return immediately. Otherwise, for each 500 or timeout we retry a couple times before giving up.
There was a problem hiding this comment.
This would be an optimization. We do have manual retry by the user. If there is a failure and user selects "resend", it will try the existing actionable issuer first again. We allow the user to do this after 1 min, but if we receive either a timeout or a failure (other than expected failure on "already sent too many times" or "wrong code") we could resend the request within that minute.
There was a problem hiding this comment.
actually i suppose that's for the initial request for get the security code, so really only "wrong code" would be the acceptable error
There was a problem hiding this comment.
I think it would be nice if we can retry with backoff except on cases where we get 4xx -- especially 403 (security code invalid, or attempts exceeded) or 404 (attestation not found -- i.e it was a hash collision and this is the wrong service). However I feel it is an optimization if time is tight
3ec416a to
16d71d8
Compare
| */ | ||
| getAttestationForSecurityCode(serviceURL: string, body: AttesationServiceCodeForSecurityRequest) { | ||
| const urlParams = new URLSearchParams({ ...body }) | ||
| return fetch(appendPath(serviceURL, 'get_attestations') + '?' + urlParams, { |
There was a problem hiding this comment.
actually i suppose that's for the initial request for get the security code, so really only "wrong code" would be the acceptable error
ac56b17 to
46fc901
Compare
| if (!cache[issuerAddress]) { | ||
| cache[issuerAddress] = `${hashAddressToSingleDigit(issuerAddress)}` | ||
| } | ||
| return cache[issuerAddress] |
There was a problem hiding this comment.
I don't actually think this is providing that much gain, hashAddressToSingleDigit doesn't seem too computationally complex. I was thinking we could have a cache like you originally suggested that was key:digit --> List<IssuerAddresses>. That way when you receive a code, you can just check the cache for the list of issuers to send to rather than iterating through each issuer. Though, it would require more maintenance -- eg. removing entries whenever you remove entries from the existing ActiveAttestations cache
There was a problem hiding this comment.
I would not include it in the current PR. We can handle it later as an improvement to the current solution.
| // for reject and resolve to achieve what we need | ||
| async function raceUntilSuccess<T>(promises: Array<Promise<T>>) { | ||
| try { | ||
| const errors = await Promise.all( |
There was a problem hiding this comment.
We can use Promise.any and not have to switch the definitions
There was a problem hiding this comment.
Unfortunately it's not a part of https://github.com/microsoft/TypeScript/blob/master/lib/lib.es2015.promise.d.ts
cc68f9e to
2e3899a
Compare
|
I've addressed all the feedback. Lmk if there anything else, otherwise I would like to merge it tomorrow before 1.6.0 cut. Thanks! |
Description
This PR implements short verification codes. See the demo below.
To enable this feature we need all the validators to support
https, which is not the case atm, so this feature are disabled by feature flag.Short code
Essentially, short code, which user receives consist of two parts: prefix and security code (e.g.
12345678:1- prefix,2345678- security code).We use prefix to identify which issuer has sent the code. Because we can not store persistent mapping of prefix<>issuer on the client side (will be gone after reinstall), we are doing the following:
This approach allows us to scale for more than 10 actionable attestations at the same time and to avoid changing
Attestation.solcontract to store mapping prefix<>issuer in it.Caveat
We do occasionally redundant requests with security code to wrong validators and those exposing the code itself and increasing waiting time for the user. For 3 SMSs the probability of this happening is 30%.
For more information see this discussion https://discord.com/channels/600834479145353243/732977310101405736/778336853505081344
Demo
Tested
With short codes enabled/disabled.
Related issues
Backwards compatibility
Yes