-
Notifications
You must be signed in to change notification settings - Fork 186
evm: use proxy etherscan by default for typegen cli #425
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
Conversation
evm/evm-typegen/src/main.ts
Outdated
| ) | ||
| .option('--etherscan-api-key <key>', 'etherscan API key') | ||
| .option( | ||
| '--etherscan-chain-id <id>', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably want to this param to simply be called --chain-id.
it doesn't make sense anymore to have the param prefixed with etherscan, since the user isn't exposed to anything related to it default anymore.
npx @subsquid/evm-typegen@latest ./src/abis/ 0x123...000 --chain-id 1that said, I think we should also remove the prefixes from the other params as well for consistency, but explain in the addHelpText the CLI Ethers-compatible and show an example how to use it with Ethers' URL and API key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, but I think it would be better do duplicate params for backwards compatibility and add deprecation warning. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added --chain-id param
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better do duplicate params for backwards compatibility
we've added the --etherscan params very recently, I don't think it's necessary. @abernatskiy @belopash @mo4islona opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can do without --etherscan params. We just need to release a major version cause it's a breaking change, but I think that's fine.
These params would be useful for users who don't want to rely on our infra for any reason, but such users can just fetch ABI with cURL and use typegen on the file. So I think deprecating and removing this function is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abernatskiy I did it like '--etherscan-chain-id <id>, --chain-id <id>' to support both. ok or better to remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abernatskiy I did it like '--etherscan-chain-id , --chain-id ' to support both
if --etherscan-chain-id produces a warning
Parameter --etherscan-chain-id is deprecated. Please use --chain-id instead
it is a great but temporary option allowing gradual user migration
But ofc it should throw an error if both specified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mo4islona @iankressin @abernatskiy Approved?
|
If you are working in this direction then it should be not It's annoying to search for the chainId every time. I don't remember any chainId except mainnet (because it is 1, lol) |
|
Also, I'm kinda skeptical to etherscan proxy idea in general. The one can abuse our proxy url and make service unavailable. We won't be able to do anything since there is no token. Like simple cron that contantly consumes full rps limit by requesting random addresses |
We do have caching and ip blacklisting which mitigates that to a certain extent. Also this service is not a mission critical component, and there's ways around it if we experience down time. I don't think it's a problem for the time being. |
|
@shestakoven the PR is missing the changelog Check out the Pull Request section of CONTRIBUTING.md |
It's not the problem but just one of the reasons why it is doubtful. What are you doing here is basically transforming the whole tool for a specific use case but instead you can simply create an alias via package.json scripts and specify URL |
evm/evm-typegen/src/main.ts
Outdated
| ): Promise<any> { | ||
| let api = config.api + (config.api.endsWith('/') ? '' : '/') + 'api' | ||
| let api: string | ||
| if (config.api === PROXY_ETHERSCAN ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I provide API token, it will try to use it with our proxy service but should fallback to default etherscan API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
that would still require an Ethers' API key, which is the source of the bad UX |
as for me it's more annoying to find how to pass it correctly: eth, ethereum, mainnet, eth-mainnet? everyone uses their own slugs and this is really annoying. |
so why not to support all of them as other tools do? |
because we'll need a resolver for all the chains from slug to chain-id, because etherscan api uses chain-id |
and what is the problem? |
|
Here is a lsit of supported chains I think we can ask AI to generate alises for them. Also supporting aliases doesn't mean that we don't support chain Ids, we should support both |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request enhances the EVM typegen CLI tool to use an SQD Etherscan proxy service by default, adds support for named chain identifiers, and deprecates the --etherscan-chain-id option in favor of --chain-id.
Changes:
- Switches default Etherscan API endpoint to SQD proxy service when no API key is provided
- Adds comprehensive named chain identifier support (e.g., "ethereum", "polygon", "arbitrum") for ~70+ chains
- Deprecates
--etherscan-chain-idoption in favor of--chain-idwith backward compatibility
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| evm/evm-typegen/src/main.ts | Updates option handling, API endpoint selection logic, and URL normalization; adds deprecation warning for old option |
| evm/evm-typegen/src/chainIds.ts | New file containing comprehensive chain ID mappings and parsing logic with fuzzy matching for suggestions |
| evm/evm-typegen/package.json | Adds fastest-levenshtein dependency for fuzzy string matching |
| common/config/rush/pnpm-lock.yaml | Updates lock file with new dependencies and Solana package version upgrades |
| common/changes/@subsquid/evm-typegen/use-proxy-etherscan-for-typegen_2026-01-24-18-35.json | Changelog entry documenting the three main changes |
Files not reviewed (1)
- common/config/rush/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.