Improve state mismatch issues in StripeAttest #5627
Conversation
mats-stripe
force-pushed
the
mats/attestation_improvements
branch
from
b45c7d8 to
a499cec
Compare
mats-stripe
force-pushed
the
mats/attestation_improvements
branch
from
dfe6012 to
09901b9
Compare
| } else { | ||
| // Reset and retry | ||
| resetKey() | ||
| try await self.attest() |
There was a problem hiding this comment.
The one thing I wasn't totally sure of is if we should re-attest here, since the client has keys that the server doesn't have. Curious what you think @davidme-stripe @tillh-stripe
There was a problem hiding this comment.
resetKey() resets successfullyAttested to false, so we can probably skip attest() and just call _assert(isRetry: true).
There was a problem hiding this comment.
Oh duh! Will remove this line
| let linkDefaultOptIn = (response["link_default_opt_in"] as? String).flatMap { LinkDefaultOptIn(rawValue: $0) } | ||
| let linkEnableDisplayableDefaultValuesInECE = response["link_enable_displayable_default_values_in_ece"] as? Bool ?? false | ||
| let linkShowPreferDebitCardHint = response["link_show_prefer_debit_card_hint"] as? Bool ?? false | ||
| let attestationStateSyncEnabled = response["link_mobile_attestation_state_sync_enabled"] as? Bool |
There was a problem hiding this comment.
PR for this is here https://git.corp.stripe.com/stripe-internal/pay-server/pull/1208789
| // Server has attestation but client doesn't know - update client | ||
| successfullyAttested = true |
There was a problem hiding this comment.
This feels like a band-aid, since attest would set successfullyAttested = true. Do you strongly feel that we need it?
(If you think so, let's also instrument it, so that we know if this ever occurs.)
There was a problem hiding this comment.
I think this is reasonable, as it will let the client assert when the server thinks it has successfully attested. No super strong opinions on where we go with this though.
I can add logs for the two cases here!
There was a problem hiding this comment.
Added logs here ec4645e
| } else { | ||
| // Reset and retry | ||
| resetKey() | ||
| try await self.attest() |
There was a problem hiding this comment.
resetKey() resets successfullyAttested to false, so we can probably skip attest() and just call _assert(isRetry: true).
mats-stripe
changed the title
mats-stripe
force-pushed
the
mats/attestation_improvements
branch
from
79b07b2 to
eb64767
Compare
Summary
This makes a few tweaks to the
StripeAttestclient to better handle client and server state mismatches. There are two scenarios this addresses:I'm adding a feature flag
link_mobile_attestation_state_sync_enabledto control this new behavior, as we don't know yet if it is a risky change.I'm also adding new analytic events for those two scenarios,
Motivation
More details here https://docs.google.com/document/d/1NMQoDpCOv4dvYjMmwfrsezGhc---wKy2eFkeH_DBjAQ/edit?usp=sharing
Testing
Unit tests added
Changelog
N/a