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

Moar Viem #48

Merged
merged 2 commits into from
Sep 16, 2024
Merged

Moar Viem #48

merged 2 commits into from
Sep 16, 2024

Conversation

bh2smith
Copy link
Collaborator

@bh2smith bh2smith commented Sep 16, 2024

PR Type

Enhancement, Tests


Description

  • Removed ethers-multisend dependency and replaced its functionality with local implementations.
  • Added new multisend functionality in src/lib/multisend.ts with encodeMetaTx and encodeMulti functions.
  • Updated various modules to use the new local MetaTransaction type and multisend functions.
  • Added tests for the new multisend functionality to ensure correct encoding of transactions.

Changes walkthrough 📝

Relevant files
Enhancement
index.ts
Update exports and remove `ethers-multisend` dependency   

src/index.ts

  • Removed export of MetaTransaction from ethers-multisend.
  • Updated exports for Network, BaseTx, SignRequestData, and populateTx.
  • +0/-1     
    multisend.ts
    Implement multisend functionality with encoding utilities

    src/lib/multisend.ts

  • Added new file for multisend functionality.
  • Implemented encodeMetaTx and encodeMulti functions.
  • Defined constants for multisend contract addresses.
  • +59/-0   
    safe.ts
    Update imports for `MetaTransaction` in safe module           

    src/lib/safe.ts

    • Updated imports to use MetaTransaction from local types.
    +6/-2     
    tx-manager.ts
    Use local multisend implementation in transaction manager

    src/tx-manager.ts

  • Updated import for encodeMulti to use local multisend implementation.
  • +2/-2     
    types.ts
    Define `OperationType` enum and `MetaTransaction` interface

    src/types.ts

    • Added OperationType enum.
    • Added MetaTransaction interface.
    +12/-0   
    util.ts
    Update import for `MetaTransaction` in utility module       

    src/util.ts

    • Updated import for MetaTransaction to use local types.
    +1/-2     
    Tests
    lib.multisend.spec.ts
    Add tests for multisend encoding functionality                     

    tests/lib.multisend.spec.ts

  • Added tests for encodeMulti function.
  • Verified encoding of multiple transactions.
  • +22/-0   
    Dependencies
    package.json
    Remove `ethers-multisend` dependency from package.json     

    package.json

    • Removed ethers-multisend dependency.
    +1/-2     

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

    @bh2smith bh2smith changed the title Moar Viem Viem: Multisend Sep 16, 2024
    @bh2smith bh2smith merged commit adcfb30 into main Sep 16, 2024
    1 check failed
    @bh2smith bh2smith deleted the moar-viem branch September 16, 2024 12:33
    @mintbase-codium-pr-agent mintbase-codium-pr-agent bot added enhancement New feature or request Tests labels Sep 16, 2024
    @mintbase-codium-pr-agent mintbase-codium-pr-agent bot changed the title Viem: Multisend Moar Viem Sep 16, 2024
    @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

    Hardcoded Values
    The contract addresses MULTISEND_141 and MULTISEND_CALLONLY_141 are hardcoded. Consider making these configurable or retrieving them from a more dynamic source to improve flexibility and maintainability.

    Error Handling
    The encodeMulti function does not handle errors or invalid inputs. It assumes all transactions are valid and will encode them regardless. Consider adding input validation and error handling to prevent potential runtime errors.

    @mintbase-codium-pr-agent
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a check for empty transactions array to prevent errors

    Consider checking for empty transactions array in encodeMulti function to prevent
    errors or unexpected behavior when encoding an empty batch.

    src/lib/multisend.ts [46-47]

    +if (transactions.length === 0) throw new Error("No transactions provided");
     const encodedTransactions =
    -  "0x" + transactions.map(encodeMetaTx).map(remove0x).join("");
    +  `0x${transactions.map(encodeMetaTx).map(remove0x).join("")}`;
     
    Suggestion importance[1-10]: 9

    Why: Adding a check for an empty transactions array is crucial to prevent potential runtime errors or unexpected behavior, significantly improving the robustness of the function.

    9
    Maintainability
    Extract address selection logic to improve function readability

    Refactor the encodeMulti function to separate concerns by extracting the conditional
    logic for address selection into a separate function.

    src/lib/multisend.ts [40-44]

    -multiSendContractAddress: string = transactions.some(
    -  (t) => t.operation === OperationType.DelegateCall
    -)
    -  ? MULTISEND_141
    -  : MULTISEND_CALLONLY_141
    +multiSendContractAddress: string = selectMultiSendAddress(transactions)
     
    +function selectMultiSendAddress(transactions: readonly MetaTransaction[]): string {
    +  return transactions.some((t) => t.operation === OperationType.DelegateCall)
    +    ? MULTISEND_DELEGATECALL_ADDRESS
    +    : MULTISEND_CALLONLY_ADDRESS;
    +}
    +
    Suggestion importance[1-10]: 8

    Why: This refactoring separates concerns and makes the encodeMulti function more readable and maintainable by extracting the address selection logic into a separate function.

    8
    Improve variable naming for clarity

    Consider using a more descriptive variable name instead of MULTISEND_141 and
    MULTISEND_CALLONLY_141 to improve code readability and maintainability.

    src/lib/multisend.ts [13-14]

    -const MULTISEND_141 = "0x38869bf66a61cF6bDB996A6aE40D5853Fd43B526";
    -const MULTISEND_CALLONLY_141 = "0x9641d764fc13c8B624c04430C7356C1C7C8102e2";
    +const MULTISEND_DELEGATECALL_ADDRESS = "0x38869bf66a61cF6bDB996A6aE40D5853Fd43B526";
    +const MULTISEND_CALLONLY_ADDRESS = "0x9641d764fc13c8B624c04430C7356C1C7C8102e2";
     
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code readability and maintainability by using more descriptive variable names, which helps future developers understand the purpose of these constants.

    7
    Best practice
    Use template literals for string construction

    Use template literals for constructing hex strings to enhance readability and reduce
    potential errors from manual string concatenation.

    src/lib/multisend.ts [47]

    -"0x" + transactions.map(encodeMetaTx).map(remove0x).join("");
    +`0x${transactions.map(encodeMetaTx).map(remove0x).join("")}`;
     
    Suggestion importance[1-10]: 6

    Why: Using template literals enhances readability and reduces potential errors from manual string concatenation, although the improvement is minor.

    6

    @Markeljan
    Copy link

    Moar VIEM!! 🔥 🙌

    @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.

    2 participants