Add timeout option for signClient.request

[junyulah]

Is your feature request related to a problem? Please describe.

The default timeout for request sign is 5 minutes in hard code way, but in some scenarios, we need to customize the timeout value. One possible scenario is the wallet sign is an async process and may take more than 5 minutes. It's a better way to expose this option to Dapps.

Describe the solution you'd like

• add timeout option in params

  public request: IEngine["request"] = async <T>(params: EngineTypes.RequestParams) => {
    this.isInitialized();
    await this.isValidRequest(params);
    const { chainId, request, topic, timeout } = params;
    const id = await this.sendRequest(topic, "wc_sessionRequest", { request, chainId });
    const { done, resolve, reject } = createDelayedPromise<T>(timeout);
    this.events.once<"session_request">(engineEvent("session_request", id), ({ error, result }) => {
      if (error) reject(error);
      else resolve(result);
    });
    return await done();
  };

• support timeout option in createDelayedPromise

export function createDelayedPromise<T>(timeout?: number) {
  timeout = toMiliseconds(timeout || FIVE_MINUTES);

[bkrem]

Hi @junyulah,

Thanks for raising this point regarding customisable timeouts for sign requests, and your detailed suggestion for the changed implementation!

One possible scenario is the wallet sign is an async process and may take more than 5 minutes.

We’re curious about your use case here, could you expand on the scenario where 5 mins is insufficient to respond to the sign request?

[junyulah]

Hi @bkrem,

Thanks for the response. Currently we are developing a MPC wallet, so to sign a transaction in our wallet will need multiple signers to sign it. And it's possible our users will need more than 5 minutes to contact different signers to sign the transaction. In our real testing scenarios, it's possible to take even longer than half hour. Get back to this timeout issue, it would be a great help if we can customize the timeout option, so the Dapps can have the flexibility to set timeout to support our MPC signing scenarios.

[bkrem]

Thanks for the context 💯

As a follow-up question: How does the dapp know which wallets need 30mins (or more) and which are fine with the 5min default in your scenario? Would you distinguish by the peer's (i.e. wallet's) metadata or just up the timeout in general for all requests?

The team is aware and we'll discuss at our Sign sync early next week, so hoping we can get back to you after that regarding this suggestion 👍

[junyulah]

Hi @bkrem, thanks for the quick response. The follow-up question is a great question! Currently we haven't go that far, we are still developing the wallet. For our first step, we are working with a partner team which is working on a Dapp, that Dapp will the set timeout to a longer time. Like you said, there are many options for our partner team to do, but sounds like using the metadata of our wallet to distinguish requests is a better solution, but up the timeout in general is also acceptable in our first step.

Very excited to hear your team will discuss this, hope to hear your conclusions soon.

[bkrem]

Hi @junyulah,

So the team went over this suggestion and it looks like this will already be covered by an upcoming implementation change (optional expiry values for wc_sessionRequest) outlined in this PR:
https://github.com/WalletConnect/walletconnect-docs/pull/315/files

This hasn't been agreed yet, but the way this would likely be implemented in the JS SignClient is similar to what you suggested: an additional expiry/timeout parameter (presumably the duration in seconds) which the client then internally converts to a UNIX timestamp as outlined in the spec PR.

Please let us know if you see issues here from your side.

cc @ganchoradkov

[bkrem]

Hi @junyulah,

Good news! This was shipped as part of https://github.com/WalletConnect/walletconnect-monorepo/releases/tag/2.2.1

Please see the PR linked above until the docs have been properly updated to reflect the new optional expiry parameter.

Let us know if you see any issues with usage 🙏

Closing.