fix: typescript generated files errors - #475 #477
Conversation
This reverts commit c30a6fc.
📝 WalkthroughWalkthroughThis pull request introduces updates to various components of the codebase. In the Changes
Possibly related PRs
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
s2imonovic
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #477 +/- ##
=======================================
Coverage 86.22% 86.22%
=======================================
Files 9 9
Lines 559 559
Branches 129 129
=======================================
Hits 482 482
Misses 77 77 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
data/addresses.testnet.json
Outdated
| @@ -29,6 +27,13 @@ | |||
| "chain_name": "bsc_testnet", | |||
| "type": "zetaToken" | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
just to double check changes in this file as they are not related to PR?
There was a problem hiding this comment.
That also changed when running yarn generate
package.json
Outdated
| @@ -14,7 +14,7 @@ | |||
| "lint:fix": "npx eslint . --fix --ignore-pattern coverage/ --ignore-pattern coverage.json --ignore-pattern lib/ --ignore-pattern out --ignore-pattern cache_forge/", | |||
| "test": "forge clean && forge test -vvv", | |||
| "coverage": "forge coverage --no-match-coverage \"(script|test|legacy)\" --report lcov", | |||
| "typechain": "npx typechain --target ethers-v6 \"out/**/!(*.t|test).sol/!(*.abi).json\" --out-dir types", | |||
| "typechain": "npx typechain --target ethers-v6 \"out/**/!(*.t|test|IERC20|ZetaConnectorBase|ZetaErrors|ZetaNonEthInterface).sol/!(*.abi).json\" --out-dir types", | |||
There was a problem hiding this comment.
could you please add some details on why is this excluded? is it because these are imported in other contracts and it is duplicated or something else? wondering if there is some configuration on typechain to exclude duplicates automatically
There was a problem hiding this comment.
Yes, the duplicates happen because these interfaces (IERC20, ZetaConnectorBase, etc.) are imported in multiple places in our contracts.
When forge compiles, it creates JSON artifacts for these interfaces in different locations based on where they were imported from. That's why we see IERC20 both in the root directory and in the ERC20 subdirectory.
Typechain doesn't have a built-in config option to automatically detect and exclude duplicates. It just processes all JSON files that match the glob pattern.
My solution of explicitly excluding the problematic interfaces works for now, though we'll need to update it if we encounter more duplicates later.
examples:
export type { IERC20 } from "./IERC20";
export type { IERC20 } from "./ERC20/IERC20";
export type { ZetaConnectorBase } from "./ZetaConnectorBase";
export type { ZetaConnectorBase } from "./ZetaConnector.base.sol/ZetaConnectorBase";
export type { ZetaErrors } from "./ZetaErrors";
export type { ZetaErrors } from "./Zeta.non-eth.sol/ZetaErrors";
export type { ZetaNonEthInterface } from "./ZetaNonEthInterface";
export type { ZetaNonEthInterface } from "./Zeta.non-eth.sol/ZetaNonEthInterface";
There was a problem hiding this comment.
got it, previously we had v1 and v2 directories and then they got merged into 1 repo, so probably some of these can be removed in the solidity code itself, and only have them on 1 place and import from that 1 place
types/factories/index.ts
Outdated
| @@ -1,7 +1,6 @@ | |||
| /* Autogenerated file. Do not edit manually. */ | |||
| /* tslint:disable */ | |||
| /* eslint-disable */ | |||
| export * as erc20 from "./ERC20"; | |||
There was a problem hiding this comment.
just to double check, did you remove these manually or it is now working fine with yarn generate?
There was a problem hiding this comment.
that got removed when I ran yarn generate
|
@fadeev please review |
There was a problem hiding this comment.
I'm getting errors:
yarn generate
Generating protocol addresses...
An unexpected error occurred:
TypeError: ethers_1.ethers.JsonRpcProvider is not a constructor
at fetchAthensAddresses (/Users/fadeev/github.com/zeta-chain/protocol-contracts/tasks/addresses.ts:146:20)
at SimpleTaskDefinition.main [as action] (/Users/fadeev/github.com/zeta-chain/protocol-contracts/tasks/addresses.ts:306:9)
at processTicksAndRejections (node:internal/process/task_queues:105:5)
at async Environment._runTaskDefinition (/Users/fadeev/github.com/zeta-chain/protocol-contracts/node_modules/hardhat/src/internal/core/runtime-environment.ts:351:14)
at async Environment.run (/Users/fadeev/github.com/zeta-chain/protocol-contracts/node_modules/hardhat/src/internal/core/runtime-environment.ts:184:14)
at async main (/Users/fadeev/github.com/zeta-chain/protocol-contracts/node_modules/hardhat/src/internal/cli/cli.ts:322:7)
An unexpected error occurred:
TypeError: ethers_1.ethers.JsonRpcProvider is not a constructor
at fetchAthensAddresses (/Users/fadeev/github.com/zeta-chain/protocol-contracts/tasks/addresses.ts:146:20)
at SimpleTaskDefinition.main [as action] (/Users/fadeev/github.com/zeta-chain/protocol-contracts/tasks/addresses.ts:306:9)
at processTicksAndRejections (node:internal/process/task_queues:105:5)
at async Environment._runTaskDefinition (/Users/fadeev/github.com/zeta-chain/protocol-contracts/node_modules/hardhat/src/internal/core/runtime-environment.ts:351:14)
at async Environment.run (/Users/fadeev/github.com/zeta-chain/protocol-contracts/node_modules/hardhat/src/internal/core/runtime-environment.ts:184:14)
at async main (/Users/fadeev/github.com/zeta-chain/protocol-contracts/node_modules/hardhat/src/internal/cli/cli.ts:322:7)
Generating protocol addresses types...
SyntaxError: /Users/fadeev/github.com/zeta-chain/protocol-contracts/data/addresses.mainnet.json: Unexpected end of JSON input
at parse (<anonymous>)
at Object..json (node:internal/modules/cjs/loader:1718:39)
at Module.load (node:internal/modules/cjs/loader:1289:32)
at Function._load (node:internal/modules/cjs/loader:1108:12)
at TracingChannel.traceSync (node:diagnostics_channel:322:14)
at wrapModuleLoad (node:internal/modules/cjs/loader:220:24)
at Module.require (node:internal/modules/cjs/loader:1311:12)
at require (node:internal/modules/helpers:136:16)
at Object.<anonymous> (/Users/fadeev/github.com/zeta-chain/protocol-contracts/scripts/generate_addresses_types.ts:1:1)
at Module._compile (node:internal/modules/cjs/loader:1554:14)
|
@fadeev I'm getting the exact same error on the main branch. Are you sure you checked out to fix/generate-action? |
|
@s2imonovic I am, yes. 
|
|
@fadeev just have pushed small fix. please pull, run |
There was a problem hiding this comment.
approved, but would be cleaner if we can find a way in future to not to have to exclude every duplicate generated type in package.json, since the list can grow and it is not practical
potential ideas, can be tackled in follow up issue or PR:
- check if typechain has support for it
- check if some duplicates actually can be removed due to v1/v2 previous split
- write a script that would somehow handle duplicates
...
![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
🧹 Nitpick comments (3)
src/fix-duplicates.ts (3)
1-36: Good implementation, but consider adding error handlingThe script effectively handles the removal of duplicate export types from the generated index.ts file, which aligns with the PR objective of fixing TypeScript compilation issues. However, it lacks error handling for file operations.
Consider adding try-catch blocks for file operations to handle potential errors:
- const root = process.cwd(); - const indexPath = path.join(root, 'types', 'index.ts'); - let content = fs.readFileSync(indexPath, 'utf8'); + const root = process.cwd(); + const indexPath = path.join(root, 'types', 'index.ts'); + let content; + try { + content = fs.readFileSync(indexPath, 'utf8'); + } catch (error) { + console.error(`Error reading file ${indexPath}:`, error); + process.exit(1); + }And similarly for the write operation:
- fs.writeFileSync(indexPath, filteredLines.join('\n')); - console.log('Duplicate exports have been removed from index.ts'); + try { + fs.writeFileSync(indexPath, filteredLines.join('\n')); + console.log('Duplicate exports have been removed from index.ts'); + } catch (error) { + console.error(`Error writing to file ${indexPath}:`, error); + process.exit(1); + }
18-19: Improve regex pattern for more robust matchingThe current regex pattern works for basic export type statements but might not catch all variations.
Consider a more comprehensive regex that handles whitespace variations:
- const match = line.match(/export type \{ ([^}]+) \} from/); + const match = line.match(/export\s+type\s+\{\s*([^}]+)\s*\}\s+from/);
4-9: Add configurability to make the script more reusableThe script currently has hardcoded paths, which limits its reusability.
Consider accepting configuration options through command-line arguments:
+ import { program } from 'commander'; + + // Set up command-line argument parsing + program + .option('-d, --dir <directory>', 'directory containing index.ts', 'types') + .parse(process.argv); + + const options = program.opts(); + // get the project root directory const root = process.cwd(); // path to index.ts file - const indexPath = path.join(root, 'types', 'index.ts'); + const indexPath = path.join(root, options.dir, 'index.ts');This would require adding
commanderas a dependency, but makes the script more flexible for future use cases.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
data/addresses.testnet.jsonis excluded by!data/**types/index.tsis excluded by!types/**
📒 Files selected for processing (3)
lib/types.ts(1 hunks)package.json(2 hunks)src/fix-duplicates.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/types.ts
- package.json
🔇 Additional comments (1)
src/fix-duplicates.ts (1)
14-32:✅ Verification successful
Consider handling multiple exports in a single line
The current implementation assumes each export type is on its own line, but TypeScript allows multiple types to be exported in a single statement.
Verify if the generated files might contain lines like
export type { Type1, Type2 } from './module'and ensure your solution handles them correctly.
🏁 Script executed:
#!/bin/bash # Check if the types/index.ts file contains lines with multiple exported types if [ -f "types/index.ts" ]; then echo "Checking for multiple exports in a single line in types/index.ts:" grep -E "export type \{[^}]*,[^}]*\} from" types/index.ts | head -n 5 else echo "types/index.ts file not found" fiLength of output: 96
No Multiple Exports Found in Current Files
After running the verification script, the file
types/index.ts(which was expected to contain potential multiple type export statements) wasn’t found, and there’s no evidence from the repository that a line matching the patternexport type { Type1, Type2 } from './module'exists. While the current implementation only handles single export types per line, it appears to be sufficient for the generated files at the moment.Keep in mind that if future changes introduce multiline exports or single-line exports with multiple types, the duplicate removal logic in
src/fix-duplicates.tsmight need updating.
Fix TypeScript Generated Files Errors (#475)
This PR addresses various TypeScript compilation issues that have been occurring in auto-generated files across multiple PRs.
Changes
tasks/addresses.tsto be compatible with current ethers versionindex.tspackage.jsonto prevent generation of duplicate typesSummary by CodeRabbit
"USDC.SUI".index.tsfile.ethersdependency to version6.13.5for improved functionality.