-
Notifications
You must be signed in to change notification settings - Fork 0
Implement waitForCertificates helper with polling, back-off, timeout, and abort support #92
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
base: feature/kyc-client-wait-for-certificate
Are you sure you want to change the base?
Implement waitForCertificates helper with polling, back-off, timeout, and abort support #92
Conversation
- Implemented proper waiting logic with configurable poll interval - Added back-off handling using server-provided retryAfter values - Added detection and handling of permanent fatal errors (non-retryable) - Added error deserialization and classification - Added timeout functionality with configurable timeout parameter - Added AbortSignal support for cancellation - Added comprehensive tests covering immediate success, delayed success, timeout, and abort scenarios Co-authored-by: rkeene <5068442+rkeene@users.noreply.github.com>
- Added KeetaKYCAnchorClientGetCertificateSuccessResponse type alias for better readability - Refactored error message checking into isPermanentError() helper method for more maintainable error classification - Fixed linting issues with unused test variables Co-authored-by: rkeene <5068442+rkeene@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
src/services/kyc/client.ts
Outdated
| /** | ||
| * Helper method to sleep for a given duration while respecting abort signals | ||
| */ | ||
| private async sleepWithAbort(ms: number, signal?: AbortSignal): Promise<void> { |
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.
@copilot Instead of creating this as a method in this class, create an src/lib/utils/asleep.ts with an exported function named asleep() with the same parameters (remember to use JSDoc to document them) -- also with a note that it should be replaced when the @keetanetwork/keetanet-client KeetaNet.lib.Utils.Helper.asleep method is updated to support AbortSignal
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.
Created src/lib/utils/asleep.ts with the asleep() function that supports AbortSignal. Added JSDoc noting it should be replaced when @keetanetwork/keetanet-client KeetaNet.lib.Utils.Helper.asleep is updated. Commit: 97ed650
src/services/kyc/client.ts
Outdated
| if (signal?.aborted) { | ||
| throw(new Error('Certificate wait aborted')); | ||
| } |
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.
@copilot This check is completely unneeded because it's the first thing done within the loop immediately following (if the timeout is negative then a timeout error instead of aborted error is fine)
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.
Removed the initial abort check before the loop. The loop now handles abort checking at its start. Commit: 97ed650
src/services/kyc/client.ts
Outdated
| */ | ||
| private isPermanentError(errorMessage: string): boolean { | ||
| // Patterns that indicate permanent failures | ||
| const permanentErrorPatterns = [ |
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.
@copilot Instead of an error pattern, use the retryable property of the error -- this property might need to be set in the appropriate circumstances
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.
Updated to use the error.retryable property instead of pattern matching. Set CertificateNotFoundError.retryable = true and removed the isPermanentError() method. Commit: 97ed650
src/services/kyc/client.ts
Outdated
| const waitTime = Math.max(result.retryAfter, pollInterval); | ||
|
|
||
| // Check if waiting would exceed the timeout | ||
| if (Date.now() - startTime + waitTime >= timeout) { |
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.
@copilot We can just poll one interval more than needed rather than add this
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.
Removed the timeout check before waiting. The loop now allows one additional poll attempt if needed. Commit: 97ed650
- Created src/lib/utils/asleep.ts with asleep() function for AbortSignal-aware sleep - Removed initial abort check before loop (redundant with loop check) - Removed timeout check before waiting (allows one extra poll attempt) - Updated getCertificates to use KeetaAnchorError.fromJSON for proper error deserialization - Updated waitForCertificates to use error.retryable property instead of pattern matching - Set CertificateNotFoundError.retryable = true for proper retry handling - Removed sleepWithAbort and isPermanentError helper methods - Replaced sleepWithAbort calls with asleep utility Co-authored-by: rkeene <5068442+rkeene@users.noreply.github.com>
Co-authored-by: rkeene <5068442+rkeene@users.noreply.github.com>
src/services/kyc/client.ts
Outdated
| if (isPermanentError) { | ||
| this.logger?.error(`Permanent error fetching certificates for request ${this.id}: ${errorMessage}`); | ||
| // Check if this is a KeetaAnchorError with retryable property | ||
| if (getCertificatesError instanceof KeetaAnchorError) { |
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.
@copilot Should use KeetaAnchorError.isInstance()
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.
Changed to use KeetaAnchorError.isInstance() for proper type checking. Commit: af7f72a
Co-authored-by: rkeene <5068442+rkeene@users.noreply.github.com>
Implementation complete! All code review feedback has been addressed.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.