-
Notifications
You must be signed in to change notification settings - Fork 479
Pre 0.4.11 cbor auxdata #2277
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
Pre 0.4.11 cbor auxdata #2277
Conversation
@@ -202,7 +266,7 @@ export class SolidityCompilation extends AbstractCompilation { | |||
public async compile(forceEmscripten = false) { | |||
const contract = | |||
await this.compileAndReturnCompilationTarget(forceEmscripten); | |||
this._metadata = JSON.parse(contract.metadata.trim()); | |||
this._metadata = JSON.parse(contract.metadata || '{}'.trim()); |
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.
We have one place (maybe even more?) in the server where we expect metadata to be present:
https://github.com/ethereum/sourcify/blob/fb6387a9b948f0fa44fd13c2c71a4abc53bc8ea4/services/server/src/server/services/storageServices/SourcifyDatabaseService.ts#L344
We should also adapt 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.
Besides this, you need braces here:
this._metadata = JSON.parse(contract.metadata || '{}'.trim()); | |
this._metadata = JSON.parse((contract.metadata || '{}').trim()); |
d1558ee
to
838a275
Compare
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 it would be nice to also have tests that show that versions 0.4.9 and 0.4.0 verify correctly. Best would be integration tests in the server.
@@ -202,7 +267,7 @@ export class SolidityCompilation extends AbstractCompilation { | |||
public async compile(forceEmscripten = false) { | |||
const contract = | |||
await this.compileAndReturnCompilationTarget(forceEmscripten); | |||
this._metadata = JSON.parse(contract.metadata.trim()); | |||
this._metadata = JSON.parse((contract.metadata || '{}').trim()); |
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 still needs to be fixed (as in my comment earlier):
@@ -41,6 +42,11 @@ export class SolidityCompilation extends AbstractCompilation { | |||
public compilationTarget: CompilationTarget, | |||
) { | |||
super(jsonInput); | |||
// https://github.com/ethereum/solidity/releases/tag/v0.4.9 | |||
// pre-0.4.9 doesn't have "filename" or "path" in the compiler output | |||
if (semver.lt(this.compilerVersion, '0.4.9')) { |
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 we have a test that proves that these versions verify correctly?
Our getter functions for contract output in AbstractCompilation
depend on the assumption that there is a path:
My guess is that the output is structured differently for these Solidity versions.
I was trying to add tests for different compiler versions but somehow solcjs is not working on my setup. Pushing it here to see how it behaves on CI. Can you also check if it works on Linux? My compilations get stuck here Note: I have a |
I didn't look too deeply to understand what's going on, but it's not working on my machine either. I cannot tell if it is indeed stuck on the line you referenced above. I get the following logs:
Note the "invalid asm.js" log, and note that the log "Local compiler - Compilation done " is 3 minutes after the one before but it says At the end the tests fail without properly terminating node due to reaching the heap limit (port is still bound when trying to run again):
I think there might be an issue with the solcjs version. If the usual solc executable is not available on https://binaries.soliditylang.org/, we should maybe add a different source for downloading it (GitHub?). |
Since this is looking more complicated than it seems, I'd remove the latest tests (rebase HEAD~1 to fb3e01d) and open a separate new issue for pre v0.4.11 verification and all issues regarding solcjs failing in server context but working in lib-sourcify. lmk if this sounds ok Regarding your comment: https://github.com/ethereum/sourcify/pull/2277/files#r2256775854 I'm not sure if I get it but I feel we should make the metadata field nullable and handle accordingly? |
If I got it right, these problems can crash the server. Can we try to identify which version is the earliest working, and disable verification for any version lower than this, e.g. by just throwing an error from
I agree that we should make it nullable. You could also add a schema change for this here. The problem in my comment is that the referenced line in SourcifyDatabase assumes that |
Ok this has gotten reeaaaaly complicated. While trying to figure out which version actually works for verification I noticed that contracts with 0.4.10 also can't be verified because they don't have This seems to contradict our database schema. But I actually couldn't point which fields cause this. The only difference between a 0.4.11 and 0.4.10 contracts compiler artifacts (because it fails with Query for contracts with empty abi or userdoc or devdoc compiler versions-- Query for contracts with empty abi or userdoc or devdoc compiler versions
SELECT
cd.chain_id,
ENCODE(cd.address, 'hex') AS address,
cc.compilation_artifacts,
cc.compiler,
cc.version
FROM compiled_contracts cc
JOIN verified_contracts vc ON cc.id = vc.compilation_id
JOIN contract_deployments cd ON vc.deployment_id = cd.id
WHERE cc.compilation_artifacts->'userdoc' = 'null' -- change here
limit 5; Also somehow we verified a few contracts with 0.4.10. Here are the two: Query for contracts with earlier compiler versions-- Contracts with earlier compiler versions
SELECT
cd.chain_id,
cd.address,
cc.compiler,
cc.compilation_artifacts->'userdoc' as userdoc,
cc.compilation_artifacts->'devdoc' as devdoc,
cc.compilation_artifacts->'storageLayout' as storageLayout,
cc.compilation_artifacts->'abi' as abi,
cc.version
FROM compiled_contracts cc
JOIN verified_contracts vc ON cc.id = vc.compilation_id
JOIN contract_deployments cd ON vc.deployment_id = cd.id
WHERE cc.version ilike '%0.4.10%'
limit 100; So what am I doing?
|
878898c
to
f354b9d
Compare
f354b9d
to
e974f39
Compare
Ok this is the final version, can you please review @manuelwedler The added test with 0.4.11 is still failing on my local environment but runs on the CI. Somehow there's an issue with running the solc-json specifically on the |
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.
Looks good to me :)
The test runs fine on my machine, let's keep it with the comment.
Also thanks for the type fixes. I like it when we include smaller fixes like this in PRs.
Merged staging to resolve conflicts |
Fixes #2217
It wasn't possible to verify contracts that have a cborAuxdata and solc <=0.4.11 because the
.auxdata
field in legacyAssembly was just introduced on 0.4.12. This handles the cases when the compiler is <=0.4.11 and also <0.4.7 when metadata was first introduced.Also handles when contractIdentifier path was missing before solc 0.4.9 and when there was no metadata 09692c0
Adds tests for the cases