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

Hacky Deployment Fetching #43

Merged
merged 1 commit into from
Sep 15, 2024
Merged

Hacky Deployment Fetching #43

merged 1 commit into from
Sep 15, 2024

Conversation

bh2smith
Copy link
Collaborator

@bh2smith bh2smith commented Sep 15, 2024

User description

The Safe Deployment network lists we outdated, causing us to fallback on v0.2.0 contracts (totally undesired - due to loss of identical & deterministic addresses on all chains). We proposed an update here safe-global/safe-modules-deployments#37

This PR just falls back on Sepolia for the deployment data (when not found in the list). This shouldn't cause problems in cases when the assets are deployed, but the user will run into issues on networks where the assets don't yet exist.

The proper solution here is proposed in #42 (the answer to life existence and everything).

In the meantime, this solution worked quite well and resulted in this transaction on Gnosis Chain!


PR Type

enhancement, bug fix


Description

  • Added support for more chain IDs in the TransactionManager in examples/load-manager.ts.
  • Enhanced deployment fetching logic in src/lib/safe.ts to include a fallback address for missing deployments.
  • Improved error handling and added comments for future improvements in deployment fetching.

Changes walkthrough 📝

Relevant files
Enhancement
load-manager.ts
Add support for more chain IDs in TransactionManager         

examples/load-manager.ts

  • Added support for additional chain IDs in the TransactionManager.
  • +1/-1     
    Bug fix
    safe.ts
    Enhance deployment fetching with fallback mechanism           

    src/lib/safe.ts

  • Modified deployment fetching logic to include a fallback address.
  • Added error handling for missing deployments.
  • Included comments and TODOs for future improvements.
  • +11/-2   

    💡 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

    Hardcoded Fallback
    The hardcoded fallback to chainId 11155111 in the absence of a deployment address could lead to unexpected behavior if the fallback chain does not have the expected deployment. This approach should be revisited for a more robust solution.

    @mintbase-codium-pr-agent
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Use a constant array for chain IDs to enhance maintainability

    Consider using a constant array for chain IDs to improve maintainability and
    readability. This allows easy modification and reuse of chain IDs across different
    parts of your application.

    examples/load-manager.ts [15]

    -for (const chainId of [1, 10, 100, 137, 11155111]) {
    +const CHAIN_IDS = [1, 10, 100, 137, 11155111];
    +for (const chainId of CHAIN_IDS) {
     
    Suggestion importance[1-10]: 8

    Why: Using a constant array for chain IDs improves maintainability and readability by allowing easy modification and reuse of chain IDs across different parts of the application. This is a good practice for managing constants.

    8
    Best practice
    Replace hardcoded chain ID with a constant to improve code clarity

    Replace the hardcoded fallback chain ID '11155111' with a constant to avoid magic
    numbers and improve code clarity. Define this constant at the top of your file or
    module.

    src/lib/safe.ts [220]

    -address = deployment.networkAddresses["11155111"];
    +const FALLBACK_CHAIN_ID = "11155111";
    +address = deployment.networkAddresses[FALLBACK_CHAIN_ID];
     
    Suggestion importance[1-10]: 7

    Why: Replacing the hardcoded chain ID with a constant avoids magic numbers and improves code clarity. This is a best practice for maintainability and readability.

    7
    Enhancement
    Implement a dynamic fallback strategy instead of using hardcoded values

    Instead of using a hardcoded fallback address, consider implementing a more robust
    error handling strategy or a configuration-based approach to manage fallbacks
    dynamically.

    src/lib/safe.ts [218-220]

    -// TODO: This is a cheeky hack. Real solution proposed in
    -// https://github.com/Mintbase/near-safe/issues/42
    -address = deployment.networkAddresses["11155111"];
    +// Implement a dynamic fallback strategy based on configuration or environment
    +address = getFallbackAddress(chainId);
     
    Suggestion importance[1-10]: 6

    Why: While the suggestion to implement a dynamic fallback strategy is valid and can enhance the robustness of the code, it requires a more complex implementation. The current comment already indicates that a real solution is proposed in an issue.

    6
    Enhance the error message for better debugging and user information

    Ensure that the error message includes all relevant information for debugging.
    Include the function name and version in the error message.

    src/lib/safe.ts [209-211]

     throw new Error(
    -  `Deployment not found for ${fn.name} version ${version} on chainId ${chainId}`
    +  `Deployment not found for function ${fn.name} with version ${version} on chainId ${chainId}. Please check your configurations or network connectivity.`
     );
     
    Suggestion importance[1-10]: 5

    Why: Enhancing the error message to include more relevant information can aid in debugging and provide better user information. However, the existing message is already quite informative, so this is a minor improvement.

    5

    @bh2smith bh2smith merged commit a268613 into main Sep 15, 2024
    1 check passed
    @bh2smith bh2smith deleted the hack-deployments branch September 15, 2024 08:53
    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