-
Notifications
You must be signed in to change notification settings - Fork 180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Verify, Echo] Fallback to .org #1016
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left mostly minor questions, otherwise looks good!
case dataTaskError(Error) | ||
case noResponse | ||
case badStatusCode(Int) | ||
case responseDataNil | ||
case jsonDecodeFailed(Error, Data) | ||
|
||
public static func ==(lhs: HTTPError, rhs: HTTPError) -> Bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I wonder if you could use if case let
pattern matching instead of conforming to Equatable. When I'm not sure how to write it I use this website https://fuckingifcaseletsyntax.com/ :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the enum still has to be equitable for this, no? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily if case let
works in same way as switch
, and you don't need Equatable for the switch.
return try await originVerifier.verifyOrigin(assertionId: assertionId) | ||
do { | ||
return try await originVerifier.verifyOrigin(assertionId: assertionId) | ||
} catch { | ||
if (error as? HTTPError) == .couldNotConnect && !fallback { | ||
fallback = true | ||
originVerifier.verifyHostFallback() | ||
return try await verifyOrigin(assertionId: assertionId) | ||
} | ||
throw error | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here could we move the implementation from the client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@llbartekll done
it's still in the client 😅
@llbartekll done |
Description
Resolves # (issue)
How Has This Been Tested?
Due Dilligence