Skip to content

Conversation

@ganchoradkov
Copy link
Member

@ganchoradkov ganchoradkov commented Oct 31, 2025

Description

This PR implements a resubscription mechanism to handle pending requests that haven't received responses from peer clients. The feature uses a heartbeat to periodically check for requests waiting too long and automatically resubscribes to topics to ensure message delivery.

Key changes:

  • Added requestWaitingForResult map to track pending requests with timestamps
  • Implemented heartbeat-based monitoring that resubscribes to topics for stale requests
  • Added diagnostic logging in canary tests to identify requests stuck without responses

Type of change

  • Chore (non-breaking change that addresses non-functional tasks, maintenance, or code quality improvements)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Draft PR (breaking/non-breaking change which needs more work for having a proper functionality [Mark this PR as ready to review only when completely ready])
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How has this been tested?

Checklist

  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Additional Information (Optional)

Please include any additional information that may be useful for the reviewer.


Note

Adds heartbeat-driven resubscription for requests awaiting results and tracks them across propose/request/ping flows; tests now log any pending requests.

  • Engine (sign-client)
    • Introduces requestWaitingForResult map to track RPCs awaiting responses; entries added for wc_sessionPropose, wc_sessionRequest, and wc_sessionPing, and cleared on response/error.
    • Registers heartbeat listener to periodically resubscribe to request topics if results are delayed, using RESUBSCRIBE_INTERVAL and MAX_RESUBSCRIBE_ATTEMPTS_ON_PENDING_RESULTS with debug/warn logging.
    • Imports fromMiliseconds and HEARTBEAT_EVENTS; wires heartbeat during init().
  • Constants
    • Adds RESUBSCRIBE_INTERVAL and MAX_RESUBSCRIBE_ATTEMPTS_ON_PENDING_RESULTS in constants/engine.ts.
  • Tests (canary)
    • Enhances logger helper to support log levels.
    • After flow, logs pending engine.requestWaitingForResult items for both clients if any.

Written by Cursor Bugbot for commit d8ac736. This will update automatically on new commits. Configure here.

Copilot AI review requested due to automatic review settings October 31, 2025 09:54
cursor[bot]

This comment was marked as outdated.

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 implements a resubscription mechanism to handle pending requests that haven't received responses from peer clients. The feature uses a heartbeat to periodically check for requests waiting too long and automatically resubscribes to topics to ensure message delivery.

Key changes:

  • Added requestWaitingForResult map to track pending requests with timestamps
  • Implemented heartbeat-based monitoring that resubscribes to topics for stale requests
  • Added diagnostic logging in canary tests to identify requests stuck without responses

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File Description
packages/sign-client/src/controllers/engine.ts Implements core resubscription logic with request tracking, heartbeat monitoring, and cleanup on response receipt
packages/sign-client/src/constants/engine.ts Defines configuration constants for resubscribe interval (30s) and max attempts (3)
packages/sign-client/test/canary/canary.spec.ts Adds diagnostic logging to track pending requests in canary tests and enhances log function with level parameter
Comments suppressed due to low confidence (2)

packages/sign-client/src/controllers/engine.ts:1573

  • The deleteProposal method doesn't clean up entries from requestWaitingForResult map when proposals are deleted (except for the error case on line 397). When proposals expire or are deleted through other paths (lines 569, 606, 1098, 1468, 2235, 2307, 2770, 2908), the corresponding entries remain in the map, leading to unnecessary resubscription attempts. Consider adding this.requestWaitingForResult.delete(id) in the deleteProposal method.
  private deleteProposal: EnginePrivate["deleteProposal"] = async (id, expirerHasDeleted) => {
    if (expirerHasDeleted) {
      try {
        const proposal = this.client.proposal.get(id);
        const event = this.client.core.eventClient.getEvent({ topic: proposal.pairingTopic });
        event?.setError(EVENT_CLIENT_SESSION_ERRORS.proposal_expired);
      } catch (error) {}
    }
    await Promise.all([
      this.client.proposal.delete(id, getSdkError("USER_DISCONNECTED")),
      expirerHasDeleted ? Promise.resolve() : this.client.core.expirer.del(id),
    ]);
    this.addToRecentlyDeleted(id, "proposal");
  };

packages/sign-client/src/controllers/engine.ts:1558

  • The deleteSession method doesn't clean up entries from the requestWaitingForResult map when sessions are deleted. Requests associated with the deleted topic will remain in the map and continue to trigger resubscription attempts. Consider iterating through the map and removing entries where request.topic === topic.
  private deleteSession: EnginePrivate["deleteSession"] = async (params) => {
    const { topic, expirerHasDeleted = false, emitEvent = true, id = 0 } = params;
    const { self } = this.client.session.get(topic);
    // Await the unsubscribe first to avoid deleting the symKey too early below.
    await this.client.core.relayer.unsubscribe(topic);
    await this.client.session.delete(topic, getSdkError("USER_DISCONNECTED"));
    this.addToRecentlyDeleted(topic, "session");
    if (this.client.core.crypto.keychain.has(self.publicKey)) {
      await this.client.core.crypto.deleteKeyPair(self.publicKey);
    }
    if (this.client.core.crypto.keychain.has(topic)) {
      await this.client.core.crypto.deleteSymKey(topic);
    }
    if (!expirerHasDeleted) this.client.core.expirer.del(topic);
    // remove any deeplinks from storage after the session is deleted
    // to avoid navigating to incorrect deeplink later on
    this.client.core.storage
      .removeItem(WALLETCONNECT_DEEPLINK_CHOICE)
      .catch((e) => this.client.logger.warn(e));
    // reset the queue state back to idle if a request for the deleted session is still in the queue
    if (topic === this.sessionRequestQueue.queue[0]?.topic) {
      this.sessionRequestQueue.state = ENGINE_QUEUE_STATES.idle;
    }

    await Promise.all(
      this.getPendingSessionRequests()
        .filter((r) => r.topic === topic)
        .map((r) => this.deletePendingSessionRequest(r.id, getSdkError("USER_DISCONNECTED"))),
    );

    if (emitEvent) this.client.events.emit("session_delete", { id, topic });
  };

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


export const MAX_RESUBSCRIBE_ATTEMPTS_ON_PENDING_RESULTS = 3;

export const RESUBSCRIBE_INTERVAL = 30;
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

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

The RESUBSCRIBE_INTERVAL constant lacks a unit suffix or documentation. Consider renaming it to RESUBSCRIBE_INTERVAL_SECONDS or adding a JSDoc comment to clarify it represents seconds, especially since other constants in the file use named constants from @walletconnect/time (e.g., FIVE_MINUTES, ONE_DAY).

Suggested change
export const RESUBSCRIBE_INTERVAL = 30;
export const RESUBSCRIBE_INTERVAL_SECONDS = 30;

Copilot uses AI. Check for mistakes.
Comment on lines +19 to 20
import { ISignClient } from "@walletconnect/types";

Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

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

Unused import ISignClient.

Suggested change
import { ISignClient } from "@walletconnect/types";

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants