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

chore: remove anvil because it was never used #893

Merged
merged 1 commit into from
May 22, 2024
Merged

Conversation

dinhani-cw
Copy link
Contributor

No description provided.

@dinhani-cw dinhani-cw requested a review from a team as a code owner May 22, 2024 04:28
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

2, because the PR involves removing configurations and references to an unused network (Anvil) across multiple files. The changes are straightforward and mainly involve deletions, which are typically easier to review than additions or modifications.

🧪 Relevant tests

No

⚡ Possible issues

No

🔒 Security concerns

No

Code feedback:

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible issue
Verify that no other parts of the application depend on the removed 'anvil' network configuration

Ensure that the removal of the 'anvil' network configuration does not affect any other
configurations or dependencies that might still be expecting this network to be available.
It's important to verify that no other parts of the application are using the 'anvil'
configuration, as this could lead to runtime errors.

e2e/hardhat.config.ts [31-33]

+stratus: {
+    url: "http://localhost:3000",
+    accounts: {
 
-
Suggestion importance[1-10]: 9

Why: This suggestion is crucial as it addresses the potential for runtime errors if other parts of the application still depend on the removed 'anvil' network configuration. Ensuring no dependencies remain is important for application stability.

9
Possible bug
Ensure no residual code references the removed 'Anvil' enum value

After removing the 'Anvil' enum value, ensure that no other parts of the codebase are
referencing this enum value. This includes checks, switch cases, or any logic that might
fail due to the absence of the 'Anvil' network type.

e2e/test/helpers/network.ts [3-7]

+export enum Network {
+    Stratus = "stratus",
+    Hardhat = "hardhat",
+    Unknown = "",
+}
 
-
Suggestion importance[1-10]: 9

Why: This suggestion is important because any residual references to the removed 'Anvil' enum value could lead to runtime errors or unexpected behavior. Ensuring all references are removed is essential for code correctness.

9
Maintainability
Check for any inconsistencies in scripts or CI/CD pipelines due to the removal of the 'e2e-anvil' job

Confirm that no scripts or automation tasks are left in an inconsistent state after the
removal of the 'e2e-anvil' job. This includes checking for any references or dependencies
on this specific job in other scripts or CI/CD pipelines.

justfile [179-180]

+e2e-hardhat test="":
+    #!/bin/bash
 
-
Suggestion importance[1-10]: 8

Why: This suggestion is valuable for maintaining the integrity of automation tasks and CI/CD pipelines. Ensuring that no references or dependencies on the removed 'e2e-anvil' job exist helps prevent build or deployment failures.

8
Enhancement
Add logging for unrecognized network types to aid in debugging

Consider adding a logging mechanism or a warning for cases where the network type is not
recognized. This can help in debugging and ensuring that all network types are handled
appropriately.

e2e/test/helpers/network.ts [15-16]

 default:
+    console.warn("Warning: Network type not recognized");
     return Network.Unknown;
 
Suggestion importance[1-10]: 7

Why: Adding logging for unrecognized network types is a good enhancement for debugging purposes. While not critical, it improves the maintainability and debuggability of the code.

7

@dinhani-cw dinhani-cw enabled auto-merge (squash) May 22, 2024 04:30
@dinhani-cw dinhani-cw merged commit c87603f into main May 22, 2024
26 checks passed
@dinhani-cw dinhani-cw deleted the remove-anvil branch May 22, 2024 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant