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

Remove Hash Method from Request Router #120

Merged
merged 3 commits into from
Sep 19, 2024
Merged

Remove Hash Method from Request Router #120

merged 3 commits into from
Sep 19, 2024

Conversation

bh2smith
Copy link
Contributor

@bh2smith bh2smith commented Sep 19, 2024

User description

This was a temporary hack that doesn't fall into the wallet connect suite for request types. Will be implemented properly in near-safe.


PR Type

enhancement, other


Description

  • Removed the Hash type from the SessionRequestParams type definition.
  • Removed the hash method from the signMethods array.
  • Removed the case handling for the hash method in the requestRouter function.

Changes walkthrough 📝

Relevant files
Enhancement
types.ts
Remove `Hash` type and method from type definitions           

src/types.ts

  • Removed Hash type from SessionRequestParams.
  • Removed hash from the signMethods array.
  • +0/-2     
    request.ts
    Remove `hash` method handling from request router               

    src/utils/request.ts

    • Removed case handling for hash method in requestRouter.
    +0/-10   

    💡 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
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Type Consistency
    Ensure that the removal of the Hash type from SessionRequestParams does not affect other parts of the application that might still expect this type to be available.

    Logging Removal
    The removal of the hash case also eliminates a warning log about unsafe hash usage. Confirm if this warning should be preserved or logged elsewhere in the application.

    @mintbase-codium-pr-agent
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Verify that the removal of 'Hash' does not affect other parts of the codebase

    Since the 'Hash' type was removed from 'SessionRequestParams', ensure that no other
    part of the codebase relies on this type being part of 'SessionRequestParams'. This
    could potentially break functionality if 'Hash' is expected elsewhere.

    src/types.ts [245-248]

     export type SessionRequestParams =
       | EthTransactionParams[]
       | Hex
       | PersonalSignParams
    +  // Ensure no other dependencies on 'Hash'
     
    Suggestion importance[1-10]: 8

    Why: This suggestion is crucial for ensuring that the removal of 'Hash' does not introduce bugs or break functionality elsewhere in the codebase. It addresses a potential issue that could have significant impacts if overlooked.

    8
    Possible bug
    Check for any unhandled cases where 'hash' might still be used

    Ensure that the removal of the 'hash' case in 'requestRouter' does not leave any
    unhandled cases where 'hash' might still be passed to this function, potentially
    leading to unhandled requests or errors.

    src/utils/request.ts [41-44]

     switch (method) {
       case "eth_sign": {
         const [sender, messageHash] = params as EthSignParams;
         return {
    +    // Ensure no calls to 'hash' remain unhandled
    +}
     
    Suggestion importance[1-10]: 8

    Why: This suggestion is important for preventing potential bugs that could arise from unhandled cases. It ensures robustness in the request handling logic.

    8
    Enhancement
    Add a default case in the switch statement to handle unsupported methods

    Consider adding a default case in the 'switch' statement of 'requestRouter' to
    handle any unexpected or unsupported methods gracefully, especially after removing a
    case like 'hash'.

    src/utils/request.ts [41-44]

     switch (method) {
       case "eth_sign": {
         const [sender, messageHash] = params as EthSignParams;
         return {
    +  default:
    +    throw new Error(`Unsupported method: ${method}`);
    +}
     
    Suggestion importance[1-10]: 7

    Why: Adding a default case improves the robustness of the code by ensuring that unsupported methods are handled gracefully. This is a good enhancement for error handling.

    7
    Maintainability
    Document the removal of "hash" from 'signMethods' for clarity

    Consider documenting the removal of "hash" from 'signMethods' to maintain clarity
    for future developers or in case of API documentation, as this change affects the
    exposed signing methods.

    src/types.ts [255-258]

     export const signMethods = [
       "eth_sign",
       "personal_sign",
       "eth_sendTransaction",
    +  // "hash" method removed - document this change
     ]
     
    Suggestion importance[1-10]: 6

    Why: Documenting changes is important for maintainability and clarity, especially for future developers. However, this suggestion is more about improving documentation rather than addressing a critical issue.

    6

    @bh2smith bh2smith merged commit 72d718e into main Sep 19, 2024
    1 check passed
    @bh2smith bh2smith deleted the remove-hash-type branch September 19, 2024 12:09
    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.

    1 participant