-
Notifications
You must be signed in to change notification settings - Fork 112
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
fix: verification endpoint alignment with etherscan #367
base: main
Are you sure you want to change the base?
Conversation
API E2E Test Results207 tests 202 ✅ 19s ⏱️ For more details on these failures, see this check. Results for commit e3f982f. ♻️ This comment has been updated with latest results. |
Unit Test Results 4 files 264 suites 11m 45s ⏱️ Results for commit e3f982f. ♻️ This comment has been updated with latest results. |
Visit the preview URL for this PR: |
@@ -136,6 +135,12 @@ export class ContractController { | |||
ContractVerificationCodeFormatEnum.solidityJsonInput, | |||
].includes(request.codeformat); | |||
|
|||
// eslint-disable-next-line @typescript-eslint/no-var-requires | |||
const semver = require("semver"); |
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.
Why not import 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.
Import doesn't work because the semver package only functions with CommonJS modules. I found a workaround using require to properly use the object.
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.
Did you try to add types?
public optimizationUsed: string; | ||
|
||
@ApiProperty({ | ||
name: "constructorArguements", | ||
description: "Contract constructor arguments", | ||
example: | ||
"0x000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000094869207468657265210000000000000000000000000000000000000000000000", | ||
"0x000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000094869207468657265210000000000000000000000000000000000000000000000 or 000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000094869207468657265210000000000000000000000000000000000000000000000", |
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.
What's that or
for? If this is to show multiple examples just use examples
field, it's array.
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.
It shows that constructor arguments can be sent either with or without the 0x prefix, as the Etherscan API only accepts without the prefix.
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.
Then use examples
array with both of these values please with and without 0x.
zkCompilerVersion: "v1.3.14", | ||
zksolcVersion: "v1.3.14", |
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.
Does this mean that the change is breaking? If so, can we make it backward-compatible (e.g. add support for property alias)?
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.
Yes, this change is breaking. TBH I would better leave zkCompilerVersion
for consistency and ask etherscan to rename on their side, because for solc it's not solcVersion
, it's compilerversion
. If we still decide to go with zksolcVersion
then we should support both zksolcVersion
and zkCompilerVersion
to avoid breaking changes.
What ❔
Resolved incompatibilities with the explorer's verification API endpoint.
Why ❔
For contract verification through Foundry and Hardhat, the API endpoint should function in the same way as it works on Etherscan.
fixes #351
Checklist