Skip to content

[SOL-100] Fix typechecking errors #92

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

sras
Copy link
Collaborator

@sras sras commented May 20, 2025

Description

Fix errors that come up when running yarn typecheck.

Copy link

Summary of Code Changes:

This pull request addresses typechecking errors encountered when running yarn typecheck. The main changes include:

  1. Adding Undefined Fields in Test Files: In the test suites (cancel-by-resolver.ts, dutch-auction.ts, fusion-swap.ts), missing properties are added to the fee objects with values set to undefined. This ensures that objects conform to the expected type definitions and satisfy the typechecker.

  2. Defining Types Manually: In tests/utils/utils.ts, dynamic type definitions derived from the FusionSwapIDL are replaced with explicit TypeScript type definitions for ReducedOrderConfig, AuctionData, and related types. This change provides clearer and more direct type annotations.

  3. Filtering Undefined Properties: The orderConfig function in utils.ts is modified to filter out undefined properties from the params object and its nested fee object before merging them with default values. This prevents undefined values from overwriting default configurations.

  4. Refactoring in createEscrow: Adjustments are made in the createEscrow method to use a renamed variable orderConfig_ to avoid shadowing and to accommodate the updated type definitions.

  5. Commenting Out Unused Code: In fusion-swap.ts, the srcRemaining property is commented out with a TODO note for confirmation and potential removal, indicating it might be unused.


Issues and Recommendations:

  1. Hardcoding Type Definitions May Lead to Inconsistencies:

    • Issue: By manually defining types like ReducedOrderConfig instead of deriving them from the FusionSwapIDL, there's a risk that these definitions may become outdated if the IDL changes in the future. This can lead to discrepancies between the actual interface and the TypeScript types, potentially causing bugs that the typechecker might not catch.

    • Recommendation: Ensure that any changes to the IDL are reflected in the manual type definitions. Consider automating the generation of TypeScript types from the IDL to maintain consistency and reduce the maintenance burden. This approach keeps the types in sync and minimizes the risk of errors due to outdated type definitions.

  2. Adding Fields with undefined Values Might Be Unnecessary or Error-Prone:

    • Issue: Inserting properties with undefined values into objects (e.g., protocolDstAcc: undefined) may not be necessary if these fields are optional in the type definitions. Including undefined properties can sometimes lead to unintended behavior, especially if the receiving code doesn't handle undefined values properly.

    • Recommendation: Instead of explicitly setting properties to undefined, omit them entirely if they are optional. Modify the type definitions to mark these fields as optional using the ? operator if not already done. This simplifies the objects and avoids potential issues with undefined values.

    Example Fix:

    // In the type definition
    type FeeConfig = {
      protocolDstAcc?: anchor.web3.PublicKey | null;
      integratorDstAcc?: anchor.web3.PublicKey | null;
      protocolFee?: number;
      integratorFee?: number;
      surplusPercentage?: number;
      maxCancellationPremium?: anchor.BN;
    };
    
    // In the test files, omit undefined fields
    fee: {
      maxCancellationPremium: defaultMaxCancellationPremium,
      // Other fields are omitted if not needed
    },
  3. Inefficient Filtering of Undefined Properties:

    • Issue: The use of Object.entries and Object.fromEntries to filter out undefined properties in the orderConfig function can be inefficient, especially if dealing with large objects. This operation creates intermediate arrays and can impact performance.

    • Recommendation: Use a more efficient method to remove undefined properties. One approach is to iterate over the keys and construct a new object without the undefined values.

    Example Fix:

    function removeUndefined(obj: any) {
      return Object.keys(obj).reduce((acc, key) => {
        if (obj[key] !== undefined) {
          acc[key] = obj[key];
        }
        return acc;
      }, {});
    }
    
    // In the orderConfig function
    const definedParams = removeUndefined(params);
    let fee;
    if (definedParams.fee) {
      fee = removeUndefined(definedParams.fee);
    }
  4. Potentially Unnecessary Code Commenting:

    • Issue: The line // srcRemaining: _srcAmount, // TODO @sras, this appear to be unused, confirm and remove in fusion-swap.ts suggests uncertainty about whether srcRemaining is needed. Leaving commented-out code can clutter the codebase and may lead to confusion.

    • Recommendation: If it's confirmed that srcRemaining is unused, remove the line entirely. If it's needed, uncomment it and ensure it serves its purpose. Keeping the code clean improves maintainability.


By addressing these issues, the codebase will be more robust, maintainable, and aligned with best practices.

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.

1 participant