Skip to content

Conversation

Agusx1211
Copy link
Member

The completed/canceled signature requests are kept in the database for (by default) 24 hours, afterwards they are prunned.

This allows for listening for completion / cancelation.

@Agusx1211 Agusx1211 requested review from a team as code owners April 17, 2025 11:09
@Agusx1211 Agusx1211 requested review from Copilot and removed request for a team April 17, 2025 11:09
Copy link
Contributor

@Copilot 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.

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

packages/wallet/wdk/src/sequence/types/signatureRequest.ts:41

  • [nitpick] Consider renaming 'scheduledpruning' to 'scheduledPruning' for better consistency with camelCase naming conventions.
scheduledpruning: number

@Agusx1211 Agusx1211 requested a review from Copilot April 17, 2025 11:10
Copy link
Contributor

@Copilot 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 pull request adds functionality to allow listening for completed and cancelled signature requests while retaining them for a configurable period before pruning. Key changes include:

  • Introducing a union type for BaseSignatureRequest to represent pending versus completed/cancelled states.
  • Replacing direct delete operations with cancel/complete methods when updating signature request status.
  • Adding a Janitor module to periodically prune outdated signature requests based on a configurable interval.

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/wallet/wdk/src/sequence/types/signatureRequest.ts Introduces the union type BaseSignatureRequest with additional fields for completed/cancelled requests.
packages/wallet/wdk/src/sequence/transactions.ts Updates signature deletion to use the new 'complete' method.
packages/wallet/wdk/src/sequence/signatures.ts Refactors methods to use getBase(), replace delete with cancel for config update logic, and adds a new prune method.
packages/wallet/wdk/src/sequence/manager.ts Renames the deleteSignatureRequest method to cancelSignatureRequest and configures dbPruningInterval.
packages/wallet/wdk/src/sequence/janitor.ts Adds the Janitor module with randomized scheduling for signature pruning.
packages/wallet/wdk/src/sequence/handlers/* Updates handler methods to use the new BaseSignatureRequest type.
packages/wallet/wdk/src/dbs/signatures.ts Adjusts the Generic class to use BaseSignatureRequest for type consistency.

@Agusx1211 Agusx1211 requested a review from Copilot April 21, 2025 15:57
Copy link
Contributor

@Copilot 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 introduces support for listening to completed and canceled signature requests by updating signature states rather than immediately deleting them. Key changes include:

  • Updating the BaseSignatureRequest type to handle additional states ('cancelled' and 'completed') with a scheduled pruning time.
  • Refactoring signature handling in the Signatures module and across related handlers to replace deletion with state updates and scheduling for pruning.
  • Adding a new Janitor module to periodically prune outdated signature requests and modifying manager options to support the new pruning interval.

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/wallet/wdk/src/sequence/types/signature-request.ts Extended the signature request types to support 'completed' and 'cancelled' statuses with scheduled pruning.
packages/wallet/wdk/src/sequence/transactions.ts Replaced deletion of a signature with marking it as complete to support listening for state changes.
packages/wallet/wdk/src/sequence/signatures.ts Refactored the functions to update signature status, added a new helper to retrieve requests, and introduced a prune mechanism.
packages/wallet/wdk/src/sequence/manager.ts Updated the manager to support cancelling a signature request and configured a pruning interval.
packages/wallet/wdk/src/sequence/janitor.ts Added a new module for scheduled pruning of signature requests.
Other handler and database files Updated signature request types across various handlers and DB modules to use the new BaseSignatureRequest type.

@Agusx1211 Agusx1211 requested a review from Copilot April 23, 2025 14:06
Copy link
Contributor

@Copilot 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 introduces support for listening for completed and canceled signature requests by extending the signature request types and updating the signature lifecycle methods. Key changes include:

  • Adding a new 'logging-out' status and guard checks for wallet logout.
  • Updating signature request types to include 'cancelled' and 'completed' statuses with pruning scheduling.
  • Introducing new complete/cancel methods in the signatures module along with a Janitor module for periodic pruning.

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/wallet/wdk/src/sequence/wallets.ts Added logout state guard and status update during logout initiation.
packages/wallet/wdk/src/sequence/types/wallet.ts Extended wallet status types to include 'logging-out'.
packages/wallet/wdk/src/sequence/types/signature-request.ts Converted signature request type to a union to support multiple statuses.
packages/wallet/wdk/src/sequence/transactions.ts Updated transaction methods to complete signature requests instead of deleting.
packages/wallet/wdk/src/sequence/signatures.ts Added helper method and updated complete, request, and cancel flows.
packages/wallet/wdk/src/sequence/manager.ts Renamed deleteSignatureRequest to cancelSignatureRequest; added Janitor.
packages/wallet/wdk/src/sequence/janitor.ts Introduced a new cleanup module for pruning expired signature requests.
packages/wallet/wdk/src/sequence/handlers/* Updated various handlers to use the new BaseSignatureRequest type.
packages/wallet/wdk/src/dbs/signatures.ts Updated to extend Generic<BaseSignatureRequest, 'id'>.
packages/wallet/wdk/src/dbs/index.ts Adjusted exports for the updated signatures module.
Comments suppressed due to low confidence (1)

packages/wallet/wdk/src/sequence/signatures.ts:35

  • In the 'complete' method, non-config-update requests are deleted (line 170) and then the code calls 'set' to update the record. This deletion followed by an update may lead to unexpected behavior; consider updating the record directly instead of deleting it.
await this.shared.databases.signatures.del(requestId)

private startPruning() {
// Random interval between 10-20 minutes
const interval = (10 + Math.random() * 10) * 60 * 1000
this.pruneInterval = setInterval(() => this.prune(), interval)
Copy link
Preview

Copilot AI Apr 23, 2025

Choose a reason for hiding this comment

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

[nitpick] The Janitor class starts a periodic pruning interval but does not provide a method to clear this interval when the Janitor is no longer needed. Consider adding a disposal method to clean up the interval to avoid potential memory or resource leaks.

Copilot uses AI. Check for mistakes.

@Agusx1211 Agusx1211 merged commit c768e57 into master Apr 23, 2025
3 checks passed
@Agusx1211 Agusx1211 deleted the listen-cancel-complete-signatures branch April 23, 2025 14:15
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant