Make `DeployAndVerifyAndThen` more robust against errors in etherscan verification

[maurelian]

Description

#1722 Adds source code verification on Etherscan during the deployment process. That step is wrapped in a try/catch. We should add better error handling to determine when to catch and when to continue.

Describe the solution you'd like

1. Understand the Etherscan verification API, and the different errors it returns.
2. Enumerate which errors should be caught and which should not.
3. Add logic to identify and handle those errors appropriately.

[indeavr]

There are 2 classes of errors:

1. On Submit- Failed to send contract verification request
2. On Verification - Failure during etherscan status polling. The verification may still succeed but should be checked manually

In (1) it could either be a (1.1) network issue or (1.2):

Reason: The Etherscan API responded that the address ${req.contractaddress} does not have bytecode. This can happen if the contract was recently deployed and this fact hasn't propagated to the backend yet. Try waiting for a minute before verifying your contract. If you are invoking this from a script, try to wait for five confirmations of your contract deployment transaction before running the verification subtask.

In (2) the reason is specified as text.

How do you imagin that this error handling could be made robust ?

My Propositon:

IMO the verification process should ALWAYS throw. This way we ensure that a verification error can't stay unnoticed.

• I would recomment that we split the DeployAndVerifyAndThen into separate functions - deploy & verify.

Observation: There is no logic after the DeployAndVerifyAndThen inside the deploy scripts (& no postDeployAction).

[maurelian]

Thanks for this thoughtful response and taking the time to look into the code!

For context, this issue was requested by @smartcontracts in his review of the initial PR. So it would be good to have him comment. Here are my thoughts:

How do you imagine that this error handling could be made robust ?

IMO we should:

1. enumerate the possible errors (which you've done!)
2. decide which to catch or throw on
   • IMO none of the errors you've mentioned are worth throwing on. We can always restart the verification process separately. We're very unlikely to miss a failed verification, since the deployment scripts explicitly include it as a step in the process before the upgrade is finalized).
3. Explicitly catch those error classes, so that new error classes we haven't considered will throw.

• I would recommend that we split the DeployAndVerifyAndThen into separate functions - deploy & verify.

I agree!

Observation: There is no logic after the DeployAndVerifyAndThen inside the deploy scripts (& no postDeployAction).

It'd be great to get rid of the ...AndThen/postDeployAction part of the code then. :)