-
Notifications
You must be signed in to change notification settings - Fork 59
Retry mechanism for verification in multiple explorers #487
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
base: master
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 |
amusingaxl
left a comment
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.
Hey there, I left some comments and suggestions.
| """ | ||
| Etherscan block explorer verifier implementation. | ||
| """ | ||
| import os |
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.
seems like this is unused
| """ | ||
| import sys | ||
| import json | ||
| import time |
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.
This also seems unused
scripts/verification/test_retry.py
Outdated
| import random | ||
| import unittest | ||
| from unittest.mock import patch, MagicMock | ||
| from typing import Tuple, Callable | ||
|
|
||
| from retry import retry_with_backoff, DEFAULT_MAX_RETRIES, DEFAULT_BASE_DELAY |
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.
random, MagicMock, Tuple, Callable, DEFAULT_MAX_RETRIES e DEFAULT_BASE_DELAY all seem unused
| data['libraryaddress1'] = library_address | ||
|
|
||
| # Submit verification request with retry | ||
| max_retries = 3 |
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.
Should this hardcoded value be here? Isn't it also set somewhere else?
d10c6d9 to
547d643
Compare
amusingaxl
left a comment
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.
LGTM
amusingaxl
left a comment
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.
Left some suggestions.
| import json | ||
| from typing import Dict, Any, Optional |
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.
Json, Dict and Any seems to be unused.
scripts/verification/README.md
Outdated
| # ✅ Correct way - run as module | ||
| python3 -m scripts.verification.test_retry |
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.
The test name needs updating here.
scripts/verification/verify.py
Outdated
| if library_address: | ||
| cmd.extend(["--libraries", f"src/DssExecLib.sol:DssExecLib:{library_address}"]) |
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.
Since library is present in foundry.toml, can we get rid of all library parsing/passing inside the scripts?
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.
Agreed
| # Etherscan (requires API key) | ||
| if chain_id == "1" and etherscan_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.
Awareness question - would hardcoding 1 allow for verification against Tenderly Testnets?
https://docs.tenderly.co/virtual-testnets/smart-contract-frameworks/foundry#verify-existing-contracts
From my understanding, testnets created from Mainnet will report chain ID of 1, so verify.py should work with them
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.
Yeah, as long as we don't change the chain ID for Tenderly Virtual Testnets, it's fine.
I don't think we have a use case to change it regardless.
| capture_output=True, | ||
| text=True, | ||
| check=True, | ||
| env=os.environ | {"ETH_GAS_PRICE": "0", "ETH_PRIO_FEE": "0"}, |
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.
Question - why do ETH_GAS_PRICE and ETH_PRIO_FEE need to be set?
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.
There was a bug in cast call that caused it to fail to execute if the sender didn't have enough gas to fund the call, even though it's supposed to be a read-only function.
Usually this command is executed in the same shell that executed the deployment, so those variables would most likely be set.
Not sure if they fixed it, but explicitly setting the gas price and the prio fee to zero was the workaround.
| print("No foundry.toml found", file=sys.stderr) | ||
|
|
||
| # If we get here, no library address was found | ||
| print("WARNING: Assuming this contract uses no libraries", file=sys.stderr) |
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.
Question - does it ever make sense to continue with verification when no libraries are present?
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.
-
Question - what role does this index file play?
-
Please adjust
Makefileto a correct file since currently Make cannot find./scripts/verify.py
| return chain_id | ||
|
|
||
|
|
||
| def get_library_address() -> str: |
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.
As mentioned in the other place, I believe that libraries should be handled by forge itself, and not parsed/provided to it
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.
Agreed.
| if len(contract_address) != 42: | ||
| sys.exit('Malformed address') |
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.
nit: this can check against a regular expression which is safer for address detection since it also checks hex-format
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 don't think this check is necessary.
If it's not a valid address, it will fail anyway.
This is more a guard against forgetting to set the address, or setting it so something different because of bad copy-pasta.
| delay = int(os.environ.get("VERIFY_DELAY", "5")) | ||
|
|
||
| chain_id = get_chain_id() | ||
| etherscan_api_key = os.environ.get("ETHERSCAN_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.
Could you please check for the presence of a bug caused by the presence of a non-empty ETHERSCAN_API_KEY environment variable which causes Forge to always verify against etherscan even when other verifier has been specified.
This only happens when:
ETHERSCAN_API_KEYwas set to a non-empty value;--verifier sourcify.
This combination causes forge to never verify on Sourcify.
It is caused by this line which doesn't use sourcify when ETHERSCAN_API_KEY is there.
scripts/verification/verify.py
Outdated
| if constructor_args: | ||
| cmd.extend(["--constructor-args", constructor_args]) |
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.
Spells are not supposed to have constructor args. We can safely remove this.
amusingaxl
left a comment
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.
Left some notes.
| - Etherscan: used on mainnet when `ETHERSCAN_API_KEY` is set | ||
|
|
||
| ## Notes | ||
| - Libraries: if `DssExecLib` is configured in `foundry.toml`, it is linked automatically via `--libraries`. |
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.
The CLI no longer accepts constructor args, as we're relying on foundry.toml. Please update the README do reflect that.
| export VERIFY_RETRIES=5 | ||
| export VERIFY_DELAY=5 | ||
|
|
||
| python -m scripts.verification.verify DssSpell 0xYourSpellAddress [constructorArgs] |
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.
Constructor args are not supported anymore.
| python -m scripts.verification.verify DssSpell 0xYourSpellAddress [constructorArgs] | |
| python -m scripts.verification.verify DssSpell 0x{spell_address} |
| and other contract-related data operations. | ||
| """ | ||
| import os | ||
| import re |
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.
Unused
| import re |
| # Constants | ||
| SOURCE_FILE_PATH = "src/DssSpell.sol" | ||
| LIBRARY_NAME = "DssExecLib" |
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.
Both unused in this file:
| # Constants | |
| SOURCE_FILE_PATH = "src/DssSpell.sol" | |
| LIBRARY_NAME = "DssExecLib" |
| return False | ||
|
|
||
|
|
||
| def verify_contract_with_verifiers( |
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.
Probably it's best to inject both chain_id and etherscan_api_key as parameters than to fetch them locally
| def verify_contract_with_verifiers( | |
| def verify_contract_with_verifiers( | |
| contract_name: str, | |
| contract_address: str, | |
| chain_id: str, | |
| etherscan_api_key: str, | |
| ) -> bool: |
| ) | ||
|
|
||
| # Parse command line arguments | ||
| spell_name, spell_address = parse_command_line_args() |
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.
| spell_name, spell_address = parse_command_line_args() | |
| spell_name, spell_address = parse_command_line_args() | |
| chain_id = get_chain_id() | |
| etherscan_api_key = os.environ.get("ETHERSCAN_API_KEY", "") |
| spell_name, spell_address = parse_command_line_args() | ||
|
|
||
| # Verify spell contract | ||
| spell_success = verify_contract_with_verifiers( |
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.
| spell_success = verify_contract_with_verifiers( | |
| spell_success = verify_contract_with_verifiers( | |
| contract_name=spell_name, | |
| contract_address=spell_address, | |
| chain_id=chain_id, | |
| etherscan_api_key=etherscan_api_key, | |
| ) |
| print('Could not determine action contract address', file=sys.stderr) | ||
| sys.exit(1) | ||
|
|
||
| action_success = verify_contract_with_verifiers( |
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.
| action_success = verify_contract_with_verifiers( | |
| action_success = verify_contract_with_verifiers( | |
| contract_name="DssSpellAction", | |
| contract_address=action_address, | |
| chain_id=chain_id, | |
| etherscan_api_key=etherscan_api_key, | |
| ) |
| retries = int(os.environ.get("VERIFY_RETRIES", "5")) | ||
| delay = int(os.environ.get("VERIFY_DELAY", "5")) |
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.
This function has one too many levels of abstraction going on.
Parsing environment variables and getting their default values shouldn't be part of its responsibilities.
You most likely want to inject those already parsed into the function as parameters and let the top level call deal with those.
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.
Needs to resolve the conflict.
Enhanced Contract Verification with Retry Mechanisms and Multi-Explorer Support
🎯 Summary
Enhances
scripts/verify.pyto eliminate single points of failure when verifying Sky Protocol spells. Adds robust retry mechanisms and support for multiple block explorers (Etherscan + Sourcify) with automatic fallback.🔧 Key Changes
Retry Mechanisms
Multi-Explorer Support
Enhanced Error Handling
📝 Files Changed
scripts/verify.py- Enhanced with retry mechanisms and multi-verifier supportscripts/verification/README.md- Updated documentationscripts/verification/test_retry.py- Simple test for the retry mechanism⚙️ Usage (Unchanged)
✅ Benefits
🧪 Testing
🎯 Addresses Requirements
No breaking changes - pure enhancement with full backward compatibility.