-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
feat(forge verify-bytecode
): support alternative block explorers + predeploys
#8510
feat(forge verify-bytecode
): support alternative block explorers + predeploys
#8510
Conversation
…ture compatibility for a preflight check of `verify_bytecode`
See: #8482 (comment), current PR does not address specific issue but is a strict improvement I feel. |
@zerosnacks @mattsse @mds1 Try blockscout to get creation data of OP VS This breaks the fork simulation to get the runtime code, as the nonce is different from the actual contract creator. Verifying creation code still works though. |
Ref #8482 (comment), adding the Moreover, adding the What we could do is |
Precendence: user provideded > explorer
@mds1 replying to #8510 (comment).
Yeah that's mostly right.
Except this part. We don't default to etherscan as this would require the user passing an etherscan API key even if they're using blockscout.
By skipping immutables, do you mean we don't check the immutables that are embedded in the runtime code? |
@yash-atreya on @mds1's comment:
This is something we've since chatted about internally and believe it'll be a very useful feature. Skipping immutable checking is super useful for checking the runtime code without the constructor args being needed.
Exactly. |
@yash-atreya that all sounds good!
Got it, in general I think that's the better and stronger approach. To take a step further, you can do: if no constructor arguments are passed AND no API key is passed (so forge has no constructor args), then it skips checking immutables. That would help cases like #8617. In this case you would also want to include a message in the output indicating this difference |
@yash-atreya cool, we will look to test this asap 👍🏻 |
…ytecode_hash is None. *required changing test suites
forge verify-bytecode
): support alternative block explorersforge verify-bytecode
): support alternative block explorers + predeploys
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
pending conflict
@yash-atreya @klkvr There seems to be a test failing that's preventing a merge here. |
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.
send it
Motivation
Resolves #8482 (excl. interaction error depending on #8549)
Adds
--verifier
and--verifier-url
argumentsSolution
forge verify
(excluding Sourcify, to be implemented in a follow-up) but opens the door for its future implementation.verify-bytecode
into the existing verification abstraction by adding averify_bytecode
method to be called on the verifier. This should make it easier to maintain.verify
command fromlib
into its own module.creation | runtime
) by using--ignore
.forge verify-bytecode
): support alternative block explorers + predeploys #8510 (comment)