Skip to content

Conversation

@rkeene
Copy link
Member

@rkeene rkeene commented Nov 4, 2025

This change adds a waitForCertificates utility method to the KYC client to handle waiting for the certificate from the service provider.

TODO:

  1. Finish waiting
  2. Handle back-offs
  3. Handle permanent fatal errors
  4. Deserialize errors
  5. Add timeout
  6. Add signal to abort
  7. Tests

@rkeene rkeene requested review from Copilot and lucasrosa90 November 4, 2025 17:55
@rkeene rkeene self-assigned this Nov 4, 2025
@rkeene rkeene added the enhancement New feature or request label Nov 4, 2025
@rkeene
Copy link
Member Author

rkeene commented Nov 4, 2025

@copilot Can you finish the TODOs here?

Copy link

Copilot AI commented Nov 4, 2025

@rkeene I've opened a new pull request, #92, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a new waitForCertificates method to the KeetaKYCVerification class that polls for certificate availability with configurable timeout and interval parameters.

  • Introduces polling mechanism for certificate retrieval
  • Adds configurable timeout (default 10 minutes) and poll interval (default 500ms)

Comment on lines +282 to +286
for (const startTime = Date.now(); Date.now() - startTime < timeout; ) {
try {
return(await this.getCertificates());
} catch (getCertificatesError) {
/* XXX:TODO */
Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handling immediately re-throws all errors, preventing any polling behavior. The method should only re-throw non-recoverable errors and sleep before retrying on expected failures (e.g., certificate not ready yet). The pollInterval parameter should be used to implement the delay between retries.

Suggested change
for (const startTime = Date.now(); Date.now() - startTime < timeout; ) {
try {
return(await this.getCertificates());
} catch (getCertificatesError) {
/* XXX:TODO */
const startTime = Date.now();
while (Date.now() - startTime < timeout) {
try {
return(await this.getCertificates());
} catch (getCertificatesError: any) {
// Only retry on expected recoverable errors (e.g., certificate not ready yet)
// Customize this check as needed for your error types
if (
getCertificatesError?.code === 'CERTIFICATE_NOT_READY' ||
(getCertificatesError?.message && getCertificatesError.message.includes('not ready')) ||
getCertificatesError?.status === 404 // Example: 404 means not found yet
) {
// Wait before retrying
await new Promise(resolve => setTimeout(resolve, pollInterval));
continue;
}
// Non-recoverable error, re-throw

Copilot uses AI. Check for mistakes.
Comment on lines +282 to +287
for (const startTime = Date.now(); Date.now() - startTime < timeout; ) {
try {
return(await this.getCertificates());
} catch (getCertificatesError) {
/* XXX:TODO */
throw(getCertificatesError);
Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The for loop will execute continuously without any delay between iterations, potentially causing high CPU usage and unnecessary API calls. After a failed attempt, the code should wait for 'pollInterval' milliseconds before retrying.

Suggested change
for (const startTime = Date.now(); Date.now() - startTime < timeout; ) {
try {
return(await this.getCertificates());
} catch (getCertificatesError) {
/* XXX:TODO */
throw(getCertificatesError);
const startTime = Date.now();
while (Date.now() - startTime < timeout) {
try {
return(await this.getCertificates());
} catch (getCertificatesError) {
// Wait for pollInterval before retrying
await new Promise(resolve => setTimeout(resolve, pollInterval));

Copilot uses AI. Check for mistakes.
Base automatically changed from feature/deserialize-errors to main November 6, 2025 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants