op-deployer: add command to verify contracts on etherscan#14359
op-deployer: add command to verify contracts on etherscan#14359bitwiseguy wants to merge 12 commits intodevelopfrom
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #14359 +/- ##
============================================
+ Coverage 46.45% 90.56% +44.10%
============================================
Files 1032 117 -915
Lines 88560 5543 -83017
============================================
- Hits 41143 5020 -36123
+ Misses 44346 518 -43828
+ Partials 3071 5 -3066
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| Settings struct { | ||
| // Remappings of the contract imports | ||
| Remappings json.RawMessage `json:"remappings"` | ||
| Remappings []string `json:"remappings"` |
There was a problem hiding this comment.
This change made it easier to construct the etherscan verification request
|
|
||
| optimizer = true | ||
| optimizer_runs = 999999 | ||
| use_literal_content = true |
There was a problem hiding this comment.
This forced just build to include the source code in the artifacts, which we need for contract verification
There was a problem hiding this comment.
It feels like a smell that we need to make this change, but it should be ok. An alternative is to create a [verify] profile here that adds this setting. If we keep this, we should add a comment explaining this so contract devs know why it's here
But we are already compiling the code with forge so should be able to just get the required info more easily. For example forge build --build-info --build-info-path cache generates a JSON file in the cache dir containing all build info including the source code, and forge verify-contract --show-standard-json-input spits out the standard JSON file that you can submit directly to etherscan
Are we strongly against shelling out to forge instead here? A few thoughts about this PR:
- I worry that this is a lot of code to maintain and we are duplicating functionality that the foundry team has spent a lot of effort on to make robust, so this is likely to become flaky
- We also need to basically duplicate
etherscan.gofor each block explorer we want to support, blockscout being the other main one. They do all have very similar APIs though so we can reuse a lot of code - Forge can infer constructor args based on the initcode it fetches from the block explorer, which is usually correct and avoids having to implement and maintain
constructors.go
There was a problem hiding this comment.
Are we strongly against shelling out to forge instead here?
I was attempting to preserve the "stand-alone binary without external dependencies" property of op-deployer. I don't feel strongly about that, but was trying to follow the existing pattern. Seems like there was a lot of effort put into implementing other forge functionality in go. Is there any difference in the forge verify-contract feature compared to the other forge features that are implemented in go, or are you more asking about the op-deployer design as a whole and why it doesn't shell out to forge in all cases?
There was a problem hiding this comment.
Forge can infer constructor args based on the initcode it fetches from the block explorer, which is usually correct and avoids having to implement and maintain constructors.go
I imagine we can do the same in go and make constructors.go much simpler. I wasn't aware that constructor args could be derived that way, therefore I went with the existing complex solution. If we stick with a go implementation for contract verification, perhaps the entire constructors.go file could just be:
func inferConstructorArgs(deployedAddress common.Address, compiledBytecode string) (string, error) {
// Get deployed bytecode from chain
deployedCode, err := client.CodeAt(context.Background(), deployedAddress, nil)
if err != nil {
return "", err
}
// Remove "0x" prefix if present
compiledBytecode = strings.TrimPrefix(compiledBytecode, "0x")
deployedHex := hex.EncodeToString(deployedCode)
// Find where compiled bytecode ends in deployed bytecode
if !strings.Contains(deployedHex, compiledBytecode) {
return "", fmt.Errorf("compiled bytecode not found in deployed bytecode")
}
// Extract constructor args (everything after the compiled bytecode)
constructorArgs := strings.TrimPrefix(deployedHex, compiledBytecode)
return constructorArgs, nil
}There was a problem hiding this comment.
Is there any difference in the
forge verify-contractfeature compared to the other forge features that are implemented in go, or are you more asking about theop-deployerdesign as a whole and why it doesn't shell out to forge in all cases?
Just asking about the verify-contract feature, not op-deployer as a whole. The rationale here is that contract verification has been a pain point for foundry users for a long time (see how many foundry issues there are for unable to verify), so I worry that over time we will encounter similar issues if we try reimplementing verification. However if you have an approach that works, and we can simplify some of it as discussed in the below paragraph, then I'm ok starting with this approach to avoid needing forge as a dep, and we can always change if it ends up being problematic
Re constructors.go, it's a bit more complex than your snippet because you need to fetch the initcode (creation code) not the deployed code. You can see forge's implementation here, notably the part where they fetch creation code using this Get Contract Creator and Creation Tx Hash Etherscan API endpoint. (Fetching it yourself gets tricky because you have you have to binary search over all blocks then trace the transaction where it was deployed, so I recommend using the etherscan endpoint)
But yes we can definitely simplify constructors.go significantly using that approach!
There was a problem hiding this comment.
@mds1 - I spoke with @mslipper yesterday. I am going to refactor this pr so that we shell out to forge verify-contract as you suggested instead of reimplementing that functionality in go (and having to deal with the continued maintenance for that feature). I will reach back out when the refactor is complete
There was a problem hiding this comment.
Ok sounds great, thank you for the update!
There was a problem hiding this comment.
This is the most useful file to unit test. It will help us detect drift with the smart contract source code and the solidity function calls made here.
It would also be nice to detect updates in smart contract constructor args compared to what is expected here, but not sure how to do that yet. Perhaps when the contracts are deployed we store the encoded constructor args in the state.json file (could be a mapping of address --> string encoded args). That way we don't have to reconstruct the args during op-deployer verify runtime. Would make the verification less error-prone
6caa487 to
a67339c
Compare
| @@ -0,0 +1,323 @@ | |||
| package verify | |||
There was a problem hiding this comment.
Let's chat about this live next week. I see why you need to do this but it's a lot of complexity that I fear will become very brittle as the codebase evolves.
There was a problem hiding this comment.
Yea agreed on the complexity and brittleness as downsides. See this comment for an alternative approach where we always store constructor args at the time the contract is deployed, and therefore don't have to regenerate them later. For that solution, I think we'd have to update the solidity scripts to return the constructor args for each contract as script outputs
6f7f3d0 to
b9226fe
Compare
|
How we looking here @bitwiseguy? |
|
Closing in favor of #14633 |
Overview
Closes #13544
Provides a user-friendly command to verify L1 contracts on etherscan. The following information is used for verification:
l1ContractLocatorfield in the state.json.Usage
superchain,implementations,opchain)Testing
Manually tested by running the following sequence:
ProxyAdmin,SuperchainConfig, andProtocolVersionsby just adding an extra function to each one calledfakeFunctionso that the bytecode was unique and the contract was not automatically verified by etherscan.just build- compile the contractsContract addresses used for this test: