-
Notifications
You must be signed in to change notification settings - Fork 434
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
Add Validation classes and types #1851
Add Validation classes and types #1851
Conversation
1fcbfee
to
87b3af7
Compare
…MetadataContract
…if it doesn't work
Requesting an interim review before adding tests @marcocastignoli |
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 a few comments in particular for renaming and moving staff around.
After solving the comments I think we can proceed by:
- merging this back in the main PR
- create a new PR in which we adjust
Validation
to use the new types fromCompilation
- create a new PR in which we create
Validation
tests
@@ -18,7 +18,7 @@ | |||
"check": "run-s check:*", | |||
"check:eslint": "eslint . --ext .ts", | |||
"check:prettier": "prettier \"./**/*.ts\" --check", | |||
"test": "c8 --reporter=none mocha -r ts-node/register test/**/*.spec.ts --no-timeout --exit", | |||
"test": "c8 --reporter=none mocha -r ts-node/register test/**/*.spec.ts test/*.spec.ts --no-timeout --exit", |
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 do you need this? shouldn't test/*.spec.ts
be already included in the superset test/**/*.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.
Yes, but for some reason it didn't do it for me 🤷
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.
in any case we should split the test files in their own folders
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 so you mean test/Validation/xxx.spec.ts
test/Compilation/xxx.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.
Yes
packages/lib-sourcify/src/Validation/SolidityMetadataContract.ts
Outdated
Show resolved
Hide resolved
/"{\\"compiler\\":{\\"version\\".*?},\\"version\\":1}"/; | ||
const HARDHAT_OUTPUT_FORMAT_REGEX = /"hh-sol-build-info-1"/; | ||
|
||
export function createMetadataContractsFromPaths( |
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.
Regarding this, I reiterate what I said yesterday, I would like the final library to be used like this:
import {Validation, Compilation, Verification} from "@ethereum-sourcify/lib-sourcify";
const metadataContracts = Validation.createMetadataContractsFromPaths(...)
const metadataContract = new Validation.SolidityMetadataContract(...)
What do you think? If you like it, then we can rename this file as validation.ts
(like an index of this folder), in which we export everything related to validation:
- the functions already in this file
- and we import/export
SolidityMetadataContract
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.
Do we need the Validation.
prefix? There's only the SolidityMetadataContract
class. We can move this function as a static function to SolidityMetadataContract.createMetadataContractsFromPaths
.
But I also liked your version. How does it look for Compilation and Verification, let's think about that.
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.
Ok let's go with your way, it's nice. How do we do that do we declare a Validation
class or a namespace, or function?
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 can just export a default object with all the needed classes / functions in an index file
// index.ts
export default {
Validation:{},
Compilation:{},
Verification:{},
}
* Init class * Init SolidityMetadataContracts, split zipUtils and processFiles * SolidityMetadataContract: create dummy Compilation, add fetchMissing methods * Add tests for zipUtils, Fix directories being in `files` array * Add fetchUtils tests * Run tests both under test/*.spec.ts and in subfolders test/**/*.spec.ts * Remove `outputSelection` to be handled by the Compilation class * Fetch missing before creating the Compilation * Rename everything `check..` to `create...` in validation ie. SolidityMetadataContract * Try first assembling the contract without variations, only try after if it doesn't work * Rename metadata2Provided to metadataPathToProvidedFilePath
Fixes #1821
PR to be merged to the main Verification Refactoring flow. I'll be documenting the changes and rationale along the way.
validation.ts
and the way we createCheckedContract
s fromfiles
is by first splitting the files as metadata and sources. Then we take variations of all source files and from them we find the correct sources for each metadata. I decided to move the variation step intoSolidityMetadataContract
class. The downside is we might need to run "variation" and keccaking steps multiple times for each source file, when there are multiple metadata and a set of source files in the current files array. However, our API will accept a single metadata. So I don't expect this to happen in practice a lot. Even if it happens it's should be still ok, even if hashing could be a little expensive. This way it's cleaner conceptually, as all the mapping, variation etc. is within the class.JsonInput
type for Solidity butVyperJsonInput
type for VyperlegacyAssembly
compilation output. So it need compilation to have run once during verification. Therefore it should be under SolidityVerification.generateEditedContract
can be underSolidityCompilation
TODO:
async createCompilation()
checkFiles...
tocreateMetadataContractsFromFiles
"check...