-
-
Notifications
You must be signed in to change notification settings - Fork 130
Show server response when Sourcify fails #199
Conversation
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'm super happy you took the time to implement this ❤️. I left a few comments!
src/verifier/SourcifyVerifier.ts
Outdated
this.logger.info(error.message); | ||
this.logger.info(`Response from ${SOURCIFY_API_URL}:`); | ||
this.logger.info(error.response.data); |
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'm thinking these should stay debug
messages?
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 some context into the PR message. I think we should return these messages to the user why the verification failed? Here is where the non 2xx
status responses end up
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 see. In that case I think we need to change a bit more about this piece of code.
I copied most of this from the Etherscan verifier. For Etherscan, failed verifications also result in a 200 status code and only unexpected errors return non 2xx, which is why it said "Failed to connect to API" rather than some more descriptive error.
I'll use the context you provided above to make some small changes to this PR so it's more with the way the EtherscanVerifier reports errors.
src/verifier/SourcifyVerifier.ts
Outdated
if (verificationStatus !== 'perfect' || verificationStatus !== 'partial') { | ||
throw new Error( | ||
`Sourcify couldn't verify the contract. Server response at ${SOURCIFY_API_URL}: \n ${JSON.stringify( | ||
res.data.result[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.
Is there any case in which result[0]
is undefined
or not JSON (causing the JSON.stringify
to throw)?
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.
For the cases when result
or result[0]
would be undefined the HTTP status should be non-2xx so it should throw inside sendVerifyRequest
. If it exists, it should always be a JSON. However undefined[0]
would also throw. How should we handle this?
I merged For the detailed responses / messages, I moved those back to "debug" logging, so the output doesn't get too cluttered. @kuzdogan could you take a look at the changes I made to see if they make sense? |
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.
Small typo, otherwise looks good!
Tested the outputs with the Superfluid contracts in the issue:
Validation error 400
:
npx truffle run --network xdai-mainnet verify TestToken@0x1BD3b6522102F9ea406807f8ecaeB2D96278a83e --verifiers=sourcify
Verifying contracts on sourcify
Verifying TestToken@0x1BD3b6522102F9ea406807f8ecaeB2D96278a83e
Fail - Unable to verify: Validation Error: address}
Failed to verify 1 contract(s): TestToken@0x1BD3b6522102F9ea406807f8ecaeB2D96278a83e
Failed verification 500
:
npx truffle run --network xdai-mainnet verify TestToken@0x1BD3b6522102F9ea406807f8ecaeB2D96278a83f --verifiers=sourcify
Verifying contracts on sourcify
Verifying TestToken@0x1BD3b6522102F9ea406807f8ecaeB2D96278a83f
Fail - Unable to verify: The deployed and recompiled bytecode don't match.}
Failed to verify 1 contract(s): TestToken@0x1BD3b6522102F9ea406807f8ecaeB2D96278a83f
No contract at address 200
:
npx truffle run --network xdai-mainnet verify TestToken@0x1F98431c8aD98523631AE4a59f267346ea31F984 --verifiers=sourcify
Verifying contracts on sourcify
Verifying TestToken@0x1F98431c8aD98523631AE4a59f267346ea31F984
Fail - Unable to verify: Chain #100 does not have a contract deployed at 0x1F98431c8aD98523631AE4a59f267346ea31F984.
Failed to verify 1 contract(s): TestToken@0x1F98431c8aD98523631AE4a59f267346ea31F984
Successful verification 200
:
npx truffle run --network mumbai verify TestToken@0x4b3f93c8a9429aec1ebc9d87fb0fb6be91905675
Verifying contracts on polygonscan
No polygonscan or testnet_polygonscan API Key provided
Verifying contracts on sourcify
Verifying TestToken@0x4b3f93c8a9429aec1ebc9d87fb0fb6be91905675
Pass - Verified: https://sourcify.dev/#/lookup/0x4b3f93c8a9429aec1ebc9d87fb0fb6be91905675
Successfully verified 1 contract(s).
Ok one more thing I noticed, can we please return the user if the contract is a "perfect" match or "partial" match? This should be in the |
Co-authored-by: Kaan Uzdoğan <kaanuzdogan@hotmail.com>
@kuzdogan Yeah it makes sense to relay that information to the user. What exactly does a "partial" match mean, when would you expect to see that? |
A "partial" match is when the "executional" part of the bytecodes match but the metadata hash in the bytecode does not match. If you are not familiar with the metadata hash, check out https://playground.sourcify.dev/?address=0x00000000219ab540356cBB839Cbe05303d7705Fa&chainId=1 The metadata hash essentially acts as a compilation fingerprint. If you change ANYTHING in the compilation the metadata hash will be different. Add a space, a comment in the source code; change file names, compiler settings etc. and the metadata will change. This is how Etherscan and other verifiers currently verify contracts but Sourcify does (and aims) perfect (full) verification. You get this information in |
@kuzdogan I just added a new commit to differentiate between perfect and partial matches. Could you double check if it works on your end? |
Thanks. I'm just thinking if we can better convey what a partial match is. But it has always been a challenge how to do it. I'll just ask @marcocastignoli to have a look in case he has a better idea. Otherwise lgtm 👍 @marcocastignoli any ideas on the latest change? |
Fixes #197
The Sourcify server response usually contains useful information to why the verification failed. It would be useful to return this to the user when the verif. fails.
Edit: Adding some context
In the case when there's an invalid address or chain the server responds with status
400
(Bad Request). Then, theaxios.post
will throw witherror.message = Request failed with status 400
.Example response with 400 Bad Request:
Similarly for
500
errors, it will fall here, when the verification simply fails.Example response 500 Internal Server Error
If there is no contract at the address, the server returns
200
but withstatus: null
.I now realize the last one is not ideal and inconsistent with the other responses.