-
Notifications
You must be signed in to change notification settings - Fork 402
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
[Vyper support] Add VyperCheckedContract
in lib-sourcify
and the new endpoint /verify/vyper
in server
#1760
base: vyper-verification-main
Are you sure you want to change the base?
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 found it quite impressive how minimal the changes to lib-sourcify
are for this feature.
Some general feedback:
- Why do we need a metadata file mock for VyperCheckContract? I guess this is due to the
verifyDeployed
method which expects a metadata. This could be documented better in some way. Is it possible to get rid of it when we refactor the verification flow? - lib-sourcify README needs to be updated due to renaming of
CheckedContract
.
I also tried to verify a vyper contract on a local server instance. Unfortunately, it didn't work due to Vyper path not found
. The server logs show that vyper was downloaded. But I cannot find the binary on my machine.
2024-12-03T11:19:24.889Z [debug] [Server] Downloaded vyper - version=0.4.0 | vyperPath=/home/manuel/.vyper-bin/vyper.0.4.0.linux | platform=linux - [traceId=db3e02b8-8701-47ad-9deb-c84b6af76241]
2024-12-03T11:19:24.889Z [info] [Server] safeHandler - errorMessage=Vyper path not found | errorStack=Error: Vyper path not found
at useVyperCompiler (/home/manuel/Projects/sourcify/sourcify/services/server/dist/server/services/compiler/local/vyperCompiler.js:46:15)
creationBytecode?: string; | ||
runtimeBytecode?: string; | ||
metadataRaw!: string; | ||
compilerOutput?: CompilerOutput; |
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.
Just wondering here, is the Vyper compiler output compatible with this type?
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.
solved by create a new VyperOutput
@@ -0,0 +1,214 @@ | |||
// TODO: Handle nodejs only dependencies |
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.
IMO, we should really get rid of the code duplication we have with the compiler code. It shouldn't be too hard to move this code to a package sourcify-compilers
or similar.
You don't have to address this here, but we should do it with the Vyper milestone, IMO. Or what do you think?
Related issue: #1348
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.
Just saw you also mentioned this in the TODOs section. Then, we are sure that we should do this :)
@@ -94,7 +94,7 @@ export async function checkFiles( | |||
})); | |||
const { metadataFiles, sourceFiles } = splitFiles(parsedFiles); | |||
|
|||
const checkedContracts: CheckedContract[] = []; | |||
const checkedContracts: SolidityCheckedContract[] = []; |
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.
This function checkFiles
and I think also the whole file validation.ts
is now only meant to be used by SolidityCheckedContract
. This feels a bit inconsistent because verification.ts
is meant to be used by both. For now, I would like to have at least some documentation about this added in the lib-sourcify
README.
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.
But validation is possible only via metadata.json
, idk how we could handle this for Vyper.
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.
Okay I see the point. I think it's just important to update the documentation regarding this.
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.
Maybe we can rename this CheckFilesWithMetadata
or CheckMetadataFiles
or something like this?
compile( | ||
version: string, | ||
solcJsonInput: VyperJsonInput, | ||
forceEmscripten?: boolean, |
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 would remove this argument, as it is not supported by the Vyper compiler.
} | ||
|
||
public async recompile( | ||
forceEmscripten = false, |
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 think I would not use this argument at all as it is not used by Vyper. I think you can remove it here (even though AbstractCheckedContract declares it).
const contractPath = req.body.contractPath; | ||
const contractName = req.body.contractName; |
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 wondered why we need both here, contractPath and contractName.
I found this in the Vyper documentation on the compiler output: The contract name will always be the file name without a suffix
Also I think we don't validate the input in any way. The user could provide any arbitrary name if we don't check that it is also what the compiler returns.
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 also saw that, but I was scared of finding a programmatic way to always get rid of the suffix... We always had problems dealing with filepaths and filenames. Do you think we can safely assume to remove everything after the last dot?
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 think we need to check if the Vyper compiler accepts any arbitrary file suffix or only .vy
files.
I am just afraid here that the contractName is just accepted from the user as input and never checked against the compiler output. So the user could bring any arbitrary name into the database.
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.
Can't we just remove the contractName
input field and make use of the output of the recompilation? the output should include the correct name
services/server/src/server/controllers/verification/vyper/stateless/vyper.stateless.paths.yaml
Show resolved
Hide resolved
@@ -1052,4 +1052,66 @@ describe("/", function () { | |||
"perfect", | |||
); | |||
}); | |||
|
|||
it("should verify a vyper contract resulting in a partial match", async () => { |
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 consistency, I would prefer to move this test into a new test file vyper.stateless.spec.ts
.
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.
But all the tests regarding APIs for stateless verification are there... At this point we can split also all the other tests into their own file for consistency
services/server/test/integration/verification-handlers/verify.stateless.spec.ts
Outdated
Show resolved
Hide resolved
The main reason why we still need the metadata here is because we want to store it in RepositoryV1/V2 to align the repository fs to solidity contracts. I think @kuzdogan had this idea first. |
That's probably related to the fact that I still have to work on rewriting platform architecture management (vyper versions for x86, arm, ...) |
We discussed this during the call but to recap, the reason is all of the APIv1 is based on the existence of the metadata.json. Some examples:
So it's needed for backwards compatiility |
This PR currently:
VyperCheckedContract
andSolidityCheckedContract
.VyperCheckedContract
generates a metadata file for Vyper contracts.useVyperCompiler
, a function similar touseCompiler
which handles dynamic download for Vyper versions (need some more work for the platform architectures), and vyper compilation.verifyDeployed
now takes as parameterCheckedContract
, an abstract class extended by bothVyperCheckedContract
andSolidityCheckedContract
. It skips metadata-related functions forVyperCheckedContract
.StorageServices
(I still haven't checked everything in detail)TODO:
/verify/vyper
API errors + openapi responses