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

Migrate SafePack to Viem #53

Merged
merged 3 commits into from
Sep 17, 2024
Merged

Migrate SafePack to Viem #53

merged 3 commits into from
Sep 17, 2024

Conversation

bh2smith
Copy link
Collaborator

@bh2smith bh2smith commented Sep 17, 2024

User description

We no longer require ethers in the lib/utils directory. This is a move toward a bit more type safety, consistency and detanglement from the network client and the contract instances.

Last step to remove ethers entirely will be to replace the ethers.JsonRpcProvider with PublicClient in lib/bundler. Then we can move ethers to a dev dependency and use it for unit testing our encodings.


PR Type

Enhancement, Tests


Description

  • Migrated contract interactions from ethers to viem in src/lib/safe.ts.
  • Updated TransactionManager to use viem and improved transaction handling.
  • Added getClient utility function and updated isContract function.
  • Added tests to compare ethers and viem implementations.
  • Reformatted code and improved readability in multiple files.

Changes walkthrough 📝

Relevant files
Enhancement
send-tx.ts
Add address validation using `viem` in transaction example

examples/send-tx.ts

  • Added isAddress check from viem to validate recoveryAddress.
+6/-1     
safe.ts
Migrate contract interactions from `ethers` to `viem`       

src/lib/safe.ts

  • Replaced ethers with viem for contract interactions.
  • Introduced DeploymentData interface.
  • Added new methods for encoding and address calculations.
  • +169/-118
    tx-manager.ts
    Update TransactionManager to use viem and improve transaction handling

    src/tx-manager.ts

  • Removed entryPointAddress from TransactionManager.
  • Updated methods to use viem for contract interactions.
  • Added nonce fetching and improved transaction building.
  • +22/-24 
    util.ts
    Add `getClient` utility and update `isContract` function 

    src/util.ts

  • Added getClient function to retrieve PublicClient.
  • Updated isContract to use getClient.
  • +13/-3   
    Formatting
    bundler.ts
    Reformat provider instantiation in bundler                             

    src/lib/bundler.ts

    • Reformatted ethers.JsonRpcProvider instantiation.
    +3/-1     
    Tests
    ethers-safe.ts
    Add comparison tests for `ethers` and `viem` implementations

    tests/ethers-safe.ts

    • Added a test file to compare ethers and viem implementations.
    +234/-0 
    lib.safe.spec.ts
    Add tests for `ContractSuite` initialization and methods 

    tests/lib.safe.spec.ts

    • Added tests for ContractSuite initialization and methods.
    +68/-0   
    utils.spec.ts
    Update `isContract` test to be asynchronous                           

    tests/utils.spec.ts

    • Updated isContract test to be asynchronous.
    +3/-3     

    💡 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 Tests labels Sep 17, 2024
    @bh2smith bh2smith merged commit 5c2f528 into main Sep 17, 2024
    1 check passed
    @bh2smith bh2smith deleted the safe-viem branch September 17, 2024 10:45
    @mintbase-codium-pr-agent
    Copy link

    PR Reviewer Guide 🔍

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

    Refactoring
    The refactoring from ethers to viem introduces significant changes in contract interaction patterns. It's crucial to ensure that the new viem-based methods mirror the functionality of the ethers-based methods accurately, especially in methods like getOpHash, buildUserOp, and addressForSetup. These methods are critical for transaction processing and their correctness directly impacts the reliability of the system.

    Error Handling
    The new error handling in getDeployment function throws a generic error without specifying the chain ID. It might be beneficial to include more specific error details to aid in debugging, especially in a multi-chain environment.

    Transaction Integrity
    The new implementation in TransactionManager should be thoroughly tested to ensure that transactions are built and executed correctly using the new viem client. This includes proper handling of nonce, gas fees, and paymaster data.

    @mintbase-codium-pr-agent
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Replace the removed variable with an equivalent to maintain functionality

    Replace the removed entryPointAddress with this.safePack.entryPoint.address in the
    bundlerForChainId method to maintain functionality after the migration from Network
    to Viem.

    src/tx-manager.ts [84-87]

    +return new Erc4337Bundler(
    +  this.safePack.entryPoint.address,
    +  this.pimlicoKey,
    +  chainId
    +);
     
    -
    Suggestion importance[1-10]: 10

    Why: This suggestion is crucial for maintaining functionality after the migration from Network to Viem, ensuring that the bundlerForChainId method works correctly by using this.safePack.entryPoint.address.

    10
    Replace hardcoded chain ID with a configurable variable

    Replace the hardcoded chain ID '11155111' with a variable or a configuration setting
    to make the deployment function more flexible and maintainable.

    src/lib/safe.ts [276]

    -address: deployment.networkAddresses["11155111"] as Address,
    +address: deployment.networkAddresses[config.chainId] as Address,
     
    Suggestion importance[1-10]: 8

    Why: Replacing hardcoded values with configurable variables improves maintainability and flexibility, making the code adaptable to different environments without changes.

    8
    Error handling
    Add error handling for network request failures

    Add error handling for the await client.readContract call to manage potential
    failures or exceptions in network requests.

    src/lib/safe.ts [103-107]

     address: (await client.readContract({
       address: m4337.address,
       abi: m4337.abi,
       functionName: "SUPPORTED_ENTRYPOINT",
    -})) as Address,
    +}).catch(e => { console.error('Failed to read contract:', e); throw e; })) as Address,
     
    Suggestion importance[1-10]: 9

    Why: Adding error handling for network requests is crucial for robustness, ensuring that potential failures are managed gracefully and do not cause unexpected crashes.

    9
    Possible bug
    Convert the synchronous call to an asynchronous call to ensure proper promise handling

    Replace the synchronous call to safePack.getSetup with an asynchronous call using
    await to ensure proper handling of promises and avoid potential runtime errors due
    to unresolved promises.

    src/tx-manager.ts [59]

    -const setup = safePack.getSetup([nearAdapter.address]);
    +const setup = await safePack.getSetup([nearAdapter.address]);
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential bug by ensuring that the promise returned by safePack.getSetup is properly awaited, preventing runtime errors due to unresolved promises.

    9
    Enhancement
    Improve error handling by using a try-catch block around asynchronous operations

    Use a try-catch block around the asynchronous operations within the
    createTransactionManager method to handle potential rejections from promises,
    improving error handling and robustness of the method.

    src/tx-manager.ts [60]

    -const safeAddress = await safePack.addressForSetup(
    -  setup,
    -  config.safeSaltNonce
    -);
    +try {
    +  const safeAddress = await safePack.addressForSetup(
    +    setup,
    +    config.safeSaltNonce
    +  );
    +} catch (error) {
    +  console.error('Error creating transaction manager:', error);
    +  throw error;
    +}
     
    Suggestion importance[1-10]: 8

    Why: Adding a try-catch block around asynchronous operations improves the robustness of the code by handling potential promise rejections, which is a good practice for error management.

    8
    Type safety
    Improve type safety by specifying a more detailed type for the ABI

    Consider using a more specific type than unknown[] | ParseAbi<readonly string[]> for
    the abi field in DeploymentData to ensure type safety.

    src/lib/safe.ts [38]

    -abi: unknown[] | ParseAbi<readonly string[]>;
    +abi: ParseAbi<readonly (string | FunctionSignature)[]>;
     
    Suggestion importance[1-10]: 7

    Why: Using a more specific type for the ABI field enhances type safety, reducing the risk of runtime errors and improving code reliability.

    7
    Robustness
    Add error handling to the getClient function to manage network-related exceptions

    Ensure that the getClient function handles exceptions or errors that might occur
    when fetching the client from the network, especially since network operations are
    prone to failures.

    src/tx-manager.ts [56]

    -return Network.fromChainId(chainId).client;
    +try {
    +  return Network.fromChainId(chainId).client;
    +} catch (error) {
    +  console.error('Failed to get client:', error);
    +  throw error;
    +}
     
    Suggestion importance[1-10]: 7

    Why: Adding error handling to the getClient function is a good practice to manage potential network-related exceptions, enhancing the robustness of the code.

    7
    Code clarity
    Improve code clarity with more descriptive variable naming

    Use a more descriptive variable name than dummyClient to clarify the purpose of the
    variable in the context of the class.

    src/lib/safe.ts [47]

    -dummyClient: PublicClient;
    +readonlyClient: PublicClient;
     
    Suggestion importance[1-10]: 6

    Why: Using more descriptive variable names enhances code readability and maintainability, making it easier for other developers to understand the code's purpose.

    6

    @bh2smith bh2smith mentioned this pull request Sep 17, 2024
    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