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

Safe Message Decoding #55

Merged
merged 5 commits into from
Sep 19, 2024
Merged

Safe Message Decoding #55

merged 5 commits into from
Sep 19, 2024

Conversation

bh2smith
Copy link
Collaborator

@bh2smith bh2smith commented Sep 19, 2024

User description

Implementation of useDecodeSafeMessage hook from safe-wallet-web in viem.

This will be used as part of the upcoming wallet connect request router.


PR Type

Enhancement, Dependencies


Description

  • Implemented decodedSafeMessage function in src/lib/safe-message.ts for decoding and hashing messages.
  • Added utility functions to support EIP-712 typed data and hex message decoding.
  • Updated package.json to include @safe-global/safe-gateway-typescript-sdk and semver dependencies.

Changes walkthrough 📝

Relevant files
Enhancement
safe-message.ts
Implement Safe Message Decoding and Hashing Functions       

src/lib/safe-message.ts

  • Implemented decodedSafeMessage function for decoding and hashing
    messages.
  • Added utility functions for generating SafeMessage typed data and
    hashes.
  • Included support for EIP-712 typed data and hex message decoding.
  • +146/-0 
    Dependencies
    package.json
    Add Safe Gateway SDK and Semver Dependencies                         

    package.json

  • Added @safe-global/safe-gateway-typescript-sdk and semver
    dependencies.
  • Updated dev dependencies with @types/semver.
  • +4/-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
    Copy link

    PR Reviewer Guide 🔍

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

    Error Handling
    The function getDecodedMessage at lines 93-100 attempts to decode a hex message and silently fails without logging or rethrowing the error. Consider adding error handling or logging to help with debugging and maintainability.

    Type Safety
    The function generateSafeMessageTypedData at lines 52-77 uses a cast as Address which may lead to runtime errors if address.value is not a valid Address. Consider adding runtime validation to ensure type safety.

    @mintbase-codium-pr-agent
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add error handling for missing chainId in SafeInfo

    Consider adding a specific error handling for the case when chainId is not provided
    in the SafeInfo object. This is similar to the handling for the missing version.
    This will prevent runtime errors when chainId is undefined and BigInt(chainId) is
    called.

    src/lib/safe-message.ts [55-56]

     if (!version) {
       throw Error("Cannot create SafeMessage without version information");
     }
    +if (!chainId) {
    +  throw Error("Cannot create SafeMessage without chainId information");
    +}
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime error by adding a check for the chainId property, similar to the existing check for version. This is crucial for preventing unexpected crashes when chainId is undefined.

    9
    Ensure type safety for address.value conversion to Address

    Replace the direct type assertion address.value as Address with a safer
    type-checking approach to avoid potential runtime errors if address.value is not of
    type Address.

    src/lib/safe-message.ts [62]

    -const verifyingContract = address.value as Address;
    +const verifyingContract = typeof address.value === 'string' ? address.value : throw new Error("Invalid address value");
     
    Suggestion importance[1-10]: 8

    Why: This suggestion improves type safety by ensuring that address.value is a string before using it. This can prevent potential runtime errors due to incorrect type assumptions.

    8
    Performance
    Optimize chainId parsing by using parseInt

    Instead of using Number(BigInt(chainId)) for converting chainId to a number,
    directly parse it as an integer if it's a string to avoid unnecessary conversion to
    BigInt and then to Number.

    src/lib/safe-message.ts [66]

    -chainId: Number(BigInt(chainId)),
    +chainId: parseInt(chainId, 10),
     
    Suggestion importance[1-10]: 7

    Why: This suggestion optimizes the parsing of chainId by using parseInt directly, which is more efficient than converting to BigInt and then to Number. This improves performance slightly.

    7
    Maintainability
    Improve error handling in getDecodedMessage to aid debugging

    In the getDecodedMessage function, ensure that the catch block logs the error or
    handles it more explicitly rather than just failing silently. This will help in
    debugging issues related to decoding failures.

    src/lib/safe-message.ts [95-99]

     try {
       return fromHex(message, "string");
     } catch (e) {
    -  // the hex string is not UTF8 encoding so return the raw message.
    +  console.error("Decoding failed:", e);
    +  return message; // return the raw message
     }
     
    Suggestion importance[1-10]: 6

    Why: This suggestion enhances maintainability by logging errors in the catch block, aiding in debugging issues related to decoding failures. However, it is a minor improvement.

    6

    @bh2smith bh2smith merged commit 4342da4 into main Sep 19, 2024
    1 check passed
    @bh2smith bh2smith deleted the safe-message branch September 19, 2024 10:07
    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