chore: fix review comments #592
Conversation
📝 WalkthroughWalkthroughIntroduces a new orchestration to fetch contract sources from Etherscan or Blockscout, normalize single vs multi-file formats, flatten/reorder sources, remove duplicate headers, and save comparable outputs for two contract addresses. Adds utilities for declaration parsing, file lookup, and file I/O, plus an exported main function to coordinate the process. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Caller
participant C as fetchAndFlattenContract
participant D as fetchContractSource
participant E as Etherscan API
participant B as Blockscout API
participant F as Flatten/Reorder Utils
participant FS as File System
U->>C: fetchAndFlattenContract(oldAddr, newAddr, network)
activate C
Note over C,D: Fetch old contract
C->>D: get source (oldAddr, network)
alt explorerType == "etherscan"
D->>E: GET contract source
E-->>D: ContractSource (single/multi)
else explorerType == "blockscout"
D->>B: GET contract source
B-->>D: ContractSource (normalized)
end
D-->>C: ContractSource (old)
Note over C,D: Fetch new contract
C->>D: get source (newAddr, network)
alt explorerType == "etherscan"
D->>E: GET contract source
E-->>D: ContractSource (single/multi)
else explorerType == "blockscout"
D->>B: GET contract source
B-->>D: ContractSource (normalized)
end
D-->>C: ContractSource (new)
Note over C,F: Normalize and flatten
C->>F: detect format, clean headers
C->>F: flatten multi-file or pass-through single-file
F-->>C: flatOld, flatNew
Note over C,F: Reorder multi-file to match single-file declaration order (if needed)
C->>F: extractDeclarationOrder(single)
C->>F: reorderMultiFileToMatchSingleFile(multi, order)
F-->>C: reordered content(s)
C->>FS: saveContractToFile(old)
C->>FS: saveContractToFile(new)
FS-->>C: saved paths
C-->>U: Completion + summary paths
deactivate C
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/contractDiff/contractDiffTool.ts (2)
16-16: Remove non-null assertion operator to prevent runtime failures.Using
process.env.ETHERSCAN_API_KEY!with the non-null assertion operator is unsafe. If the environment variable is not set,apiKeyEnvwill beundefinedat runtime, bypassing TypeScript's type safety. This could lead to silent failures or confusing errors when the API is called.Apply this diff to handle undefined API keys properly:
eth_mainnet: { name: "Ethereum Mainnet", apiUrl: "https://api.etherscan.io/v2/api?chainid=1", - apiKeyEnv: process.env.ETHERSCAN_API_KEY!, + apiKeyEnv: process.env.ETHERSCAN_API_KEY, explorerType: "etherscan", },Apply similar changes to lines 22, 28, 34, 40, and 46. The validation at line 237-239 should then properly catch missing API keys.
Also applies to: 22-22, 28-28, 34-34, 40-40, 46-46
224-242: API key validation is ineffective with current type assertions.The check
!config.apiKeyEnvat line 237 is currently ineffective becauseapiKeyEnvis assigned using the non-null assertion operator (!) in the config. This means TypeScript treats it as always defined, even whenprocess.env.ETHERSCAN_API_KEYis undefined.Once you remove the non-null assertions from lines 16, 22, 28, 34, 40, and 46 (as suggested in the previous comment), this validation will work correctly and catch missing API keys before attempting to call the API.
🧹 Nitpick comments (4)
scripts/contractDiff/contractDiffTool.ts (4)
272-285: Consider stronger typing for sources parameter.The
sourcesparameter is typed asany, which reduces type safety. Consider defining a more specific type for the sources structure.Add a type definition at the top of the file:
type SourceFiles = Record<string, { content: string } | string>;Then update the function signature:
-function findFileByContractName(sources: any, contractName: string): string | null { +function findFileByContractName(sources: SourceFiles, contractName: string): string | null {
335-365: Consider logging parsing errors instead of silent fallback.The catch block at lines 359-361 silently returns the original
sourceCodewithout any indication that parsing failed. This could mask issues during contract processing.Apply this diff to add error logging:
return cleanDuplicateHeaders(allCode); } catch (error) { + console.warn(`⚠️ Failed to parse multi-file format, using as-is: ${error instanceof Error ? error.message : error}`); return sourceCode; }
375-431: Consider logging reordering errors for better debugging.Similar to
flattenSourceCode, the catch block at lines 428-430 silently returns the original code without logging. This makes it difficult to debug when reordering fails.Apply this diff:
return cleanDuplicateHeaders(reorderedCode); } catch (error) { + console.warn(`⚠️ Failed to reorder multi-file format, using as-is: ${error instanceof Error ? error.message : error}`); return multiFileCode; }
467-526: Consider making output directory configurable.The
OUTPUT_DIRis hardcoded at line 472. While this works for the current use case, making it configurable would improve flexibility.Add an optional parameter:
export async function fetchAndFlattenContract( oldAddress: string, newAddress: string, network: string, + outputDir: string = "./contract-diffs" ) { - const OUTPUT_DIR = "./contract-diffs" try { console.log("\n📥 Fetching OLD implementation..."); const oldContract = await fetchContractSource(oldAddress, network);And update the function calls to use
outputDir:- const oldPath = saveContractToFile(oldFlattened, oldFile, OUTPUT_DIR); - const newPath = saveContractToFile(newFlattened, newFile, OUTPUT_DIR); + const oldPath = saveContractToFile(oldFlattened, oldFile, outputDir); + const newPath = saveContractToFile(newFlattened, newFile, outputDir);Otherwise, the orchestration logic is well-structured and handles different file format combinations correctly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
scripts/contractDiff/contractDiffTool.ts(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
scripts/**
⚙️ CodeRabbit configuration file
Review the Hardhat scripts for best practices.
Files:
scripts/contractDiff/contractDiffTool.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: slither
- GitHub Check: test
🔇 Additional comments (5)
scripts/contractDiff/contractDiffTool.ts (5)
106-163: LGTM: Blockscout integration is well-structured.The function properly handles Blockscout's response format, including multi-file contracts, and converts it to a unified ContractSource interface. Error handling covers both 404 cases and general failures appropriately.
174-213: LGTM: Etherscan integration follows best practices.The function correctly handles Etherscan's API responses with proper error checking for unverified contracts and API failures.
251-263: LGTM: Declaration extraction is correctly implemented.The regex pattern properly matches contract, interface, library, and abstract contract declarations, and the exec() loop correctly extracts all declarations in order.
294-325: LGTM: Duplicate header removal is correctly implemented.The function properly preserves the first SPDX license identifier and pragma statement while removing subsequent duplicates, which is essential for valid Solidity compilation.
442-455: LGTM: File saving logic is appropriate for a script.The function correctly handles directory creation and file writing. Using synchronous operations is acceptable in this script context.
2 participants
Fix @skosito's review comments
closes: #589 (review)
Summary by CodeRabbit