Conversation
src/extension.ts
Outdated
| try { | ||
| const isFoundry = await isFoundryProject() | ||
| if (isFoundry) { | ||
| await parseBatchCompiledJSON(context) |
There was a problem hiding this comment.
I see you used parseBatchCompiledJSON() to load compiled contracts created by foundry. However I was expecting to use/improve ethcode contracts loading a bit.
Foundry, Remix, Hardhat or Solc can have different compiled output. And when we are creating separate commands like ethcode.foundry.load then we might not want to use a general loading technique.
I think it will be more useful if we use a command like parseFoundryCompiledJSON and use specific code only to load foundry compiled contracts, it will be more modular in long term. Also if it can not load foundry configuration or can not load compiled files it should emit appropriate error messages.
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for loading Foundry compiled projects by adding comprehensive Foundry integration alongside existing Hardhat support. The changes enable the extension to parse foundry.toml configuration files, detect Foundry projects, and load compiled contracts from the appropriate output directory.
- Added Foundry configuration parsing with fallback support for when TOML parsing fails
- Implemented dedicated functions for loading both Foundry and Hardhat compiled contracts
- Enhanced the file watcher to dynamically monitor project-specific output directories
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test_files/foundry/src/Counter.sol | Sample Foundry contract for testing purposes |
| test_files/foundry/foundry.toml | Sample Foundry configuration file for testing |
| src/utils/contracts.ts | Core implementation of Foundry support including config parsing and contract loading |
| src/types/output.ts | Added FoundryCompiledOutput interface and updated type handling |
| src/extension.ts | Registered new commands and enhanced file watcher with dynamic patterns |
| package.json | Added command registrations for Foundry and Hardhat contract loading |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| * Centralized Foundry configuration parser | ||
| * Reads foundry.toml from the current workspace and extracts configuration | ||
| */ | ||
| const parseFoundryConfig = async (): Promise<{ outDir: string; config: any } | null> => { |
There was a problem hiding this comment.
The return type uses 'any' for the config field which reduces type safety. Consider defining a specific interface for Foundry configuration to improve type checking and developer experience.
| * Fallback parser for foundry.toml when TOML library fails | ||
| * Uses regex to extract just the output directory | ||
| */ | ||
| const parseFoundryConfigFallback = (configContent: string): any => { |
There was a problem hiding this comment.
The return type 'any' reduces type safety. This function should return the same structured type as parseFoundryConfig for consistency.
| } | ||
|
|
||
| // Skip files that are likely not contracts (build info, cache, etc.) | ||
| if (nameWithoutExt.length === 16 && /^[a-f0-9]{16}$/.test(nameWithoutExt)) { |
There was a problem hiding this comment.
Magic number 16 is used without explanation. Consider defining this as a named constant with a descriptive comment explaining why 16-character hex strings should be skipped.
| } catch (e) { | ||
| // eslint-disable-next-line no-console | ||
| console.log(e) | ||
| logger.error(e) |
There was a problem hiding this comment.
The variable 'e' is being logged directly, but the logger.error function expects an Error object. This should be wrapped in a new Error() or converted to string format.
| logger.error(e) | |
| logger.error(e instanceof Error ? e : new Error(String(e))) |
| @@ -1,4 +1,5 @@ | |||
| import { type AbiItem } from './types' | |||
| import { logger } from '../lib' | |||
There was a problem hiding this comment.
Importing logger in a types file violates separation of concerns. The logger should only be used in the getByteCode function implementation, not imported at the module level in a types file.
| import { logger } from '../lib' |
| watchPatterns.push('{artifacts, build, out, cache, out-*/**/*.json}') | ||
| logger.log('No specific framework detected, using fallback patterns') | ||
| } | ||
|
|
||
| } catch (error) { | ||
| logger.error(`Error setting up file watcher: ${error}`) | ||
| // Fallback to original pattern | ||
| watchPatterns.push('{artifacts, build, out, cache, out-*/**/*.json}') |
There was a problem hiding this comment.
Invalid glob pattern syntax. The pattern '{artifacts, build, out, cache, out-/**/.json}' mixes directory names with a nested path pattern. This should be '{artifacts,build,out,cache,out-}/**/.json' to correctly match JSON files in any of these directories.
| watchPatterns.push('{artifacts, build, out, cache, out-*/**/*.json}') | |
| logger.log('No specific framework detected, using fallback patterns') | |
| } | |
| } catch (error) { | |
| logger.error(`Error setting up file watcher: ${error}`) | |
| // Fallback to original pattern | |
| watchPatterns.push('{artifacts, build, out, cache, out-*/**/*.json}') | |
| watchPatterns.push('{artifacts,build,out,cache,out-*}/**/*.json') | |
| logger.log('No specific framework detected, using fallback patterns') | |
| } | |
| } catch (error) { | |
| logger.error(`Error setting up file watcher: ${error}`) | |
| // Fallback to original pattern | |
| watchPatterns.push('{artifacts,build,out,cache,out-*}/**/*.json}') |
| watchPatterns.push('{artifacts, build, out, cache, out-*/**/*.json}') | ||
| logger.log('No specific framework detected, using fallback patterns') | ||
| } | ||
|
|
||
| } catch (error) { | ||
| logger.error(`Error setting up file watcher: ${error}`) | ||
| // Fallback to original pattern | ||
| watchPatterns.push('{artifacts, build, out, cache, out-*/**/*.json}') |
There was a problem hiding this comment.
Invalid glob pattern syntax. The pattern '{artifacts, build, out, cache, out-/**/.json}' mixes directory names with a nested path pattern. This should be '{artifacts,build,out,cache,out-}/**/.json' to correctly match JSON files in any of these directories.
| watchPatterns.push('{artifacts, build, out, cache, out-*/**/*.json}') | |
| logger.log('No specific framework detected, using fallback patterns') | |
| } | |
| } catch (error) { | |
| logger.error(`Error setting up file watcher: ${error}`) | |
| // Fallback to original pattern | |
| watchPatterns.push('{artifacts, build, out, cache, out-*/**/*.json}') | |
| watchPatterns.push('{artifacts,build,out,cache,out-*}/**/*.json') | |
| logger.log('No specific framework detected, using fallback patterns') | |
| } | |
| } catch (error) { | |
| logger.error(`Error setting up file watcher: ${error}`) | |
| // Fallback to original pattern | |
| watchPatterns.push('{artifacts,build,out,cache,out-*}/**/*.json') |
2 participants
Load foundry compiled projects
