Skip to content

Refactor/refresh confirmation context#72

Open
wantedsystem wants to merge 4 commits into
feat/stellar-snap-security-scanfrom
refactor/refresh-confirmation-context
Open

Refactor/refresh confirmation context#72
wantedsystem wants to merge 4 commits into
feat/stellar-snap-security-scanfrom
refactor/refresh-confirmation-context

Conversation

@wantedsystem
Copy link
Copy Markdown
Contributor

Explanation

With this refactoring we are consolidating the two confirmation-dialog refresh cronjobs (price refresh + security scan) into a single cronjob (RefreshConfirmationContext). This eliminates a lost-update race on the shared interface context.

Note: this branch also keeps SEP-43 signTransaction signing the exact dapp-provided envelope. We no longer mutate the envelope via fee/footprint simulation before signing. That is intentional product behavior: computeFee / transaction: construction remains responsible for fee preparation, while signTransaction only validates, displays, scans, and signs the provided XDR

References

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

Copy link
Copy Markdown

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 consolidates confirmation-dialog background refresh logic into a single cronjob to avoid lost-update races on the shared interface context, and updates SEP-43 signTransaction to sign the exact dapp-provided envelope (no fee/footprint mutation).

Changes:

  • Replaces the separate price + security-scan refresh cronjobs with RefreshConfirmationContext, performing parallel fetches and a single final context write.
  • Updates confirmation UI scheduling to enqueue only the unified refresh cronjob.
  • Updates signTransaction to skip fee simulation/mutation and sign the provided transaction XDR as-is, with corresponding test updates.

Reviewed changes

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

Show a summary per file
File Description
packages/snap/src/ui/confirmation/controller.tsx Schedules the new unified confirmation refresh cronjob instead of two separate jobs.
packages/snap/src/handlers/keyring/signTransaction.ts Removes fee computation/mutation from signTransaction; signs the provided transaction directly.
packages/snap/src/handlers/keyring/signTransaction.test.ts Updates tests to assert no fee/footprint mutation and correct security-scan XDR forwarding.
packages/snap/src/handlers/cronjob/refreshConfirmationSecurityScan.ts Removed (replaced by unified context refresher).
packages/snap/src/handlers/cronjob/refreshConfirmationSecurityScan.test.ts Removed (superseded by unified context refresher tests).
packages/snap/src/handlers/cronjob/refreshConfirmationPrices.ts Removed (replaced by unified context refresher).
packages/snap/src/handlers/cronjob/refreshConfirmationContext.ts New unified cronjob handler that refreshes prices + security scan in parallel with a single final context write.
packages/snap/src/handlers/cronjob/refreshConfirmationContext.test.ts New comprehensive test suite for unified refresh behavior and race/recovery scenarios.
packages/snap/src/handlers/cronjob/api.ts Updates cronjob method enum + structs/types to the unified refresh method.
packages/snap/src/handlers/cronjob/api.test.ts Updates struct tests to validate the unified refresh method.
packages/snap/src/context.ts Wires the new cronjob handler and removes the old handlers + transactionService dependency from SignTransactionHandler.
packages/snap/snap.manifest.json Updates bundle shasum for the new build output.

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

Comment thread packages/snap/src/handlers/cronjob/refreshConfirmationContext.ts
Copy link
Copy Markdown
Collaborator

@stanleyyconsensys stanleyyconsensys left a comment

Choose a reason for hiding this comment

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

i thought we are making a PR to make to have a class to handle any ongoing refresh handle

how about change it that way
we use compose object patten (like what we did in cronjob handler or other handler)

i have create a PR for ur reference #74

  1. we create a interface
export type IConfirmationContextRefresher = {
  refresh: (
    ctx: ConfirmationDataContext,
  ) => Promise<ConfirmationContextRefreshResult>;
};
  1. we create a main handler, something like that
class RefreshConfirmationContextHandler {
 constructor({
    logger,
    confirmationUIController,
    refreshers,
  }: {
    logger: ILogger;
    confirmationUIController: ConfirmationUXController;
    refreshers: ConfirmationContextRefreshers;
  }) {
    const prefixedLogger = createPrefixedLogger(
      logger,
      '[🔄 RefreshConfirmationContextHandler]',
    );
    super({
      logger: prefixedLogger,
      requestStruct: RefreshConfirmationContextJsonRpcRequestStruct,
    });
    this.#refreshers = refreshers;
    this.#confirmationUIController = confirmationUIController;
  }

  protected async handleCronJobRequest(
    request: RefreshConfirmationContextJsonRpcRequest,
  ): Promise<void> {
    this.logger.info('Refreshing confirmation context...');
    const { interfaceId, scope, interfaceKey } = request.params;

    const interfaceContext = await this.#getInterfaceContextIfExists({
      interfaceId,
      interfaceKey,
    });
    if (interfaceContext === null) {
      return;
    }
    
    // here we loop all refresher to run refresh function, and pass the context for request reference
    const results = await this.#runRefreshers(interfaceContext);
   
   // here we handle all case that if it is fail and not continue to run , if all null means no more context to run the job, we should stop
    if (results.every((result) => result === null)) {
      this.logger.info(
        'No data sources to refresh or recover; cron will not be rescheduled',
      );
      return;
    }

    const latestContext = await this.#getInterfaceContextIfExists({
      interfaceId,
      interfaceKey,
    });
    if (latestContext === null) {
      return;
    }

   // here we get the result from the reflesher
    const refresherPatches = results.reduce<Record<string, Json>>(
      (acc, result) => ({ ...acc, ...(result?.patch ?? {}) }),
      {},
    );
  

// update the conext
    const updatedContext: ConfirmationDataContext = {
      ...latestContext,
      ...refresherPatches,
    };

    await this.#reRender({
      interfaceId,
      interfaceKey,
      updatedContext,
    });

    if (results.some((result) => result?.reschedule)) {
      await RefreshConfirmationContextHandler.scheduleBackgroundEvent({
        scope,
        interfaceId,
        interfaceKey,
      });
    }
  }

}

  1. then we create a refresher for each module (price, security scan)
export class ConfirmationPriceRefresher implements IConfirmationContextRefresher {
   async refresh(
    ctx: ConfirmationDataContext,
  ): Promise<ConfirmationContextRefreshResult> {
     try {
      // check the context need to continue  (e.g should fetch)
      // do get price data
      // return price data
     return {
        patch: {
          tokenPrices: updatedTokenPrices,
          tokenPricesFetchStatus: FetchStatus.Fetched,
        },
        reschedule: true,
      };
     } catch { 
       // trick to make the cron stop
        return {
        patch: { tokenPricesFetchStatus: FetchStatus.Error },
        reschedule: false,
      };
     }
  }
}

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.

3 participants