Skip to content
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

deduplicate Array.from(toBytes...) and more cosmetic improvements #92

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

bh2smith
Copy link
Collaborator

@bh2smith bh2smith commented Aug 9, 2024

User description

Closes #82

We use toPayload on Adapter.sign so that we are only constructing payloads in one singular place.

Also found a few other places (naming conventions) that could be cleanedup/improved.


PR Type

enhancement, bug fix


Description

  • Refactored toPayload function to handle both Hex and Uint8Array types, improving payload validation.
  • Removed redundant imports and deduplicated payload creation logic in NearEthAdapter.
  • Enhanced logging messages for better clarity and debugging.
  • Standardized variable names for consistency across the codebase.
  • Updated unit tests to reflect changes in error messages.

Changes walkthrough 📝

Relevant files
Enhancement
ethereum.ts
Refactor and improve payload handling and logging               

src/chains/ethereum.ts

  • Removed redundant toBytes and isBytes imports.
  • Added toPayload import and used it to deduplicate payload creation.
  • Improved logging messages for better clarity.
  • Simplified variable names and removed unnecessary console logs.
  • +14/-15 
    transaction.ts
    Enhance `toPayload` function for better validation             

    src/utils/transaction.ts

  • Modified toPayload function to handle both Hex and Uint8Array.
  • Improved error message for payload validation.
  • +6/-4     
    handlers.ts
    Standardize recovery data naming and payload creation       

    src/wallet-connect/handlers.ts

  • Renamed signatureRecoveryData to recoveryData for consistency.
  • Updated payload creation to use the enhanced toPayload function.
  • +5/-5     
    Tests
    utils.transaction.test.ts
    Update test for `toPayload` error message                               

    tests/unit/utils.transaction.test.ts

  • Updated test case to reflect the improved error message in toPayload.
  • +1/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @mintbase-codium-pr-agent mintbase-codium-pr-agent bot added enhancement New feature or request Bug fix labels Aug 9, 2024
    @bh2smith bh2smith merged commit 57b2110 into main Aug 9, 2024
    1 check passed
    @bh2smith bh2smith deleted the 82/dedup-payload branch August 9, 2024 14:39
    @mintbase-codium-pr-agent
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Logging Consistency
    The logging message at line 172 introduces a new format which includes both the Near account ID and the Ethereum address. Ensure that this change is consistent with the logging strategy across the entire application, especially if logs are monitored or parsed by tools.

    Error Handling
    The function toPayload throws an error for invalid input lengths. It's crucial to ensure that all calls to toPayload properly handle this exception to avoid runtime errors.

    Functionality Change
    The sign function now uses toPayload for payload processing instead of the previous manual conversion and array creation. This change should be thoroughly tested to ensure that the behavior of signing remains consistent.

    @mintbase-codium-pr-agent
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add null checks to prevent potential runtime errors in the toPayload function

    The toPayload function throws an error for invalid byte length but does not handle
    the case where toBytes might return an undefined or null value. This can lead to a
    runtime error when trying to access the length property. Add a null check before
    accessing the length property.

    src/utils/transaction.ts [15-18]

     const bytes = isBytes(msgHash) ? msgHash : toBytes(msgHash);
    -if (bytes.length !== 32) {
    -  throw new Error(`Payload must have 32 bytes: ${msgHash}`);
    +if (!bytes || bytes.length !== 32) {
    +  throw new Error(`Payload must have 32 bytes and be a valid byte array: ${msgHash}`);
     }
     
    Suggestion importance[1-10]: 8

    Why: Adding null checks enhances the robustness of the toPayload function, preventing potential runtime errors when toBytes returns an undefined or null value.

    8
    Add robust input handling to the toPayload function to prevent runtime errors

    Ensure that the toPayload function is robust against different input types and
    formats. The current implementation assumes that msgHash will always be either a Hex
    string or a Uint8Array, but does not handle cases where msgHash might not be in the
    expected format. Consider adding type checks and handling unexpected input
    gracefully.

    src/chains/ethereum.ts [240]

    -payload: toPayload(msgHash),
    +payload: toPayload(msgHash) || [],
     
    Suggestion importance[1-10]: 4

    Why: While adding robust input handling is generally a good practice, the suggested change (payload: toPayload(msgHash) || [],) does not actually improve the robustness of the toPayload function itself. It only adds a fallback which might mask underlying issues.

    4
    Maintainability
    Replace direct console.log usage with a configurable logging utility

    Replace the direct usage of console.log with a more flexible logging framework or
    utility that can be configured for different environments (development, production,
    etc.). This allows for better control over logging levels and formats, which is
    crucial for production environments where too much logging can lead to performance
    issues and log flooding.

    src/chains/ethereum.ts [97]

    -console.log(`Requesting signature from ${this.mpcContract.accountId()}`);
    +logger.info(`Requesting signature from ${this.mpcContract.accountId()}`);
     
    Suggestion importance[1-10]: 7

    Why: Using a logging utility instead of console.log improves maintainability and allows for better control over logging levels and formats, which is beneficial for production environments.

    7
    Security
    Ensure comprehensive testing of the toPayload function to handle all edge cases in the signing process

    The method sign in the NearEthAdapter class has been modified to use toPayload for
    preparing the payload. However, the change from Array.from(hashToSign) to
    toPayload(msgHash) assumes that toPayload handles all cases correctly. It's
    important to ensure that toPayload is thoroughly tested to handle all edge cases, as
    this is critical for the security and functionality of the signing process.

    src/chains/ethereum.ts [240]

    +payload: toPayload(msgHash),
     
    -
    Suggestion importance[1-10]: 6

    Why: Ensuring that toPayload is thoroughly tested is important for security and functionality. However, the suggestion does not provide a specific code improvement but rather a recommendation for testing.

    6

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    Deduplicate Payload Construction
    1 participant