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

Adapter on Create #37

Merged
merged 1 commit into from
Sep 13, 2024
Merged

Adapter on Create #37

merged 1 commit into from
Sep 13, 2024

Conversation

bh2smith
Copy link
Collaborator

@bh2smith bh2smith commented Sep 13, 2024

User description

instead of passing the accountId (which seems to have CORS issues because its a cookie) we build the adapter and try to construct the Safe. Not clear yet if this is going to work.


PR Type

Enhancement


Description

  • Integrated NearEthAdapter in the transaction example script (examples/send-tx.ts).
  • Refactored TransactionManager to use NearEthAdapter instead of nearAccount and mpcContractId (src/tx-manager.ts).

Changes walkthrough 📝

Relevant files
Enhancement
send-tx.ts
Integrate NearEthAdapter in transaction example script     

examples/send-tx.ts

  • Added NearEthAdapter initialization.
  • Replaced nearAccount with nearAdapter in TransactionManager creation.
  • Removed mpcContractId from TransactionManager creation.
  • +5/-4     
    tx-manager.ts
    Refactor TransactionManager to use NearEthAdapter               

    src/tx-manager.ts

  • Removed mpcContractId and nearAccount from TransactionManager config.
  • Added nearAdapter to TransactionManager config.
  • Updated TransactionManager.create to use nearAdapter.
  • +7/-13   

    💡 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 the enhancement New feature or request label Sep 13, 2024
    @bh2smith bh2smith merged commit ba88ea2 into main Sep 13, 2024
    1 check passed
    @bh2smith bh2smith deleted the adapter-pre-create branch September 13, 2024 16:49
    @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

    Missing Error Handling
    The new NearEthAdapter initialization does not include error handling. If the adapter or the MpcContract fails to initialize, it could cause unhandled promise rejections or runtime errors.

    Logging Sensitive Information
    The console log statement at line 50 might expose sensitive account details in the logs, which could be a security risk if logs are not properly secured or if they are accessible by unauthorized parties.

    @mintbase-codium-pr-agent
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Error handling
    Implement error handling for asynchronous operations to manage exceptions effectively

    Add error handling for the await operations within the create method to manage
    exceptions from failed promises.

    src/tx-manager.ts [40-59]

    -const adapter = config.nearAdapter;
    -const provider = new ethers.JsonRpcProvider(config.ethRpc);
    -const safePack = await ContractSuite.init(provider);
    -const bundler = new Erc4337Bundler(config.erc4337BundlerUrl, await safePack.entryPoint.getAddress());
    -const setup = await safePack.getSetup([adapter.address]);
    -const safeAddress = await safePack.addressForSetup(setup, config.safeSaltNonce);
    +try {
    +  const adapter = config.nearAdapter;
    +  const provider = new ethers.JsonRpcProvider(config.ethRpc);
    +  const safePack = await ContractSuite.init(provider);
    +  const bundler = new Erc4337Bundler(config.erc4337BundlerUrl, await safePack.entryPoint.getAddress());
    +  const setup = await safePack.getSetup([adapter.address]);
    +  const safeAddress = await safePack.addressForSetup(setup, config.safeSaltNonce);
    +} catch (error) {
    +  console.error('Failed to create TransactionManager:', error);
    +  throw error;
    +}
     
    Suggestion importance[1-10]: 10

    Why: Implementing error handling for asynchronous operations is essential for managing exceptions effectively, which significantly improves the code's reliability.

    10
    Possible issue
    Add default values or error handling for environment variables to prevent potential runtime errors

    Consider handling the case where process.env.ETH_RPC or
    process.env.ERC4337_BUNDLER_URL might be undefined to prevent runtime errors.

    examples/send-tx.ts [27-28]

    -ethRpc: process.env.ETH_RPC!,
    -erc4337BundlerUrl: process.env.ERC4337_BUNDLER_URL!,
    +ethRpc: process.env.ETH_RPC || 'default_eth_rpc_url',
    +erc4337BundlerUrl: process.env.ERC4337_BUNDLER_URL || 'default_bundler_url',
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime error by providing default values for environment variables, which is crucial for robust code execution.

    9
    Provide a fallback for safeSaltNonce to ensure it always has a valid value

    Ensure that options.safeSaltNonce is defined or provide a default value to avoid
    potential issues if it's missing.

    examples/send-tx.ts [30]

    -safeSaltNonce: options.safeSaltNonce,
    +safeSaltNonce: options.safeSaltNonce || 'default_nonce',
     
    Suggestion importance[1-10]: 8

    Why: Providing a fallback for safeSaltNonce ensures that the code does not fail if the value is missing, improving robustness.

    8

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

    Successfully merging this pull request may close these issues.

    1 participant