Skip to content
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

Merged

Conversation

kuzdogan
Copy link
Member

@kuzdogan kuzdogan commented Jan 9, 2025

Fixes #1821

PR to be merged to the main Verification Refactoring flow. I'll be documenting the changes and rationale along the way.

  • Current validation.ts and the way we create CheckedContracts from files 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 into SolidityMetadataContract 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.
  • Note: We have JsonInput type for Solidity but VyperJsonInput type for Vyper
  • Methods related to finding auxdatas require the legacyAssembly compilation output. So it need compilation to have run once during verification. Therefore it should be under SolidityVerification. generateEditedContract can be under SolidityCompilation

TODO:

  • Leave outputSelection empty and handle that in Compilation
  • Do fetchMissing in async createCompilation()
  • rename checkFiles... to createMetadataContractsFromFiles
    • Remove everything name after "checked... "check...
  • Try first assembling the contract without variations, only try after if it doesn't work

@kuzdogan kuzdogan changed the title Init class Add Validation classes and types Jan 10, 2025
@kuzdogan kuzdogan force-pushed the verification-flow-validation-classes branch from 1fcbfee to 87b3af7 Compare January 10, 2025 10:22
@kuzdogan kuzdogan marked this pull request as ready for review January 13, 2025 15:48
@kuzdogan
Copy link
Member Author

Requesting an interim review before adding tests @marcocastignoli

Copy link
Member

@marcocastignoli marcocastignoli left a 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 from Compilation
  • 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",
Copy link
Member

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?

Copy link
Member Author

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 🤷

Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

/"{\\"compiler\\":{\\"version\\".*?},\\"version\\":1}"/;
const HARDHAT_OUTPUT_FORMAT_REGEX = /"hh-sol-build-info-1"/;

export function createMetadataContractsFromPaths(
Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member Author

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?

Copy link
Member

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:{},
}

@kuzdogan kuzdogan merged commit e80c655 into verification-flow-refactoring-main Jan 15, 2025
6 checks passed
@kuzdogan kuzdogan deleted the verification-flow-validation-classes branch January 15, 2025 13:22
kuzdogan added a commit that referenced this pull request Feb 28, 2025
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: COMPLETED
Development

Successfully merging this pull request may close these issues.

2 participants