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

Verification flow verification classes #1879

Conversation

marcocastignoli
Copy link
Member

@marcocastignoli marcocastignoli commented Jan 30, 2025

This PR implements the new Verification class defined in #1823.

TODO:

  • Implement calling matchWithRuntimeBytecode and matchWithCreationTx in parallel
  • Fix should verify a contract with viaIR:true, optimizer disabled, and compiler <0.8.21
  • Implement Vyper constructor argument transformation test
  • Implement custom SourcifyLibError, with custom code/message
  • Include all transformations functions inside the Verification class
  • Move transformations into Verification/Transformations.ts
  • Do not include any dependency from the old ./src/lib folder

Sorry, something went wrong.

@marcocastignoli marcocastignoli marked this pull request as ready for review February 5, 2025 07:00
Copy link
Contributor

@manuelwedler manuelwedler left a comment

Choose a reason for hiding this comment

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

I only had a glimpse on the tests. Not sure if I can take a closer look before my vacation.

- Extracted transformation functions from the Verification class to a separate module
- Added a new SolidityBugType enum to handle specific compilation scenarios
- Improved error handling for RPC unavailability
@marcocastignoli
Copy link
Member Author

Should we wait merging because you also want to check the tests? @kuzdogan

@kuzdogan kuzdogan self-assigned this Feb 13, 2025
@marcocastignoli
Copy link
Member Author

marcocastignoli commented Feb 13, 2025

Sorry, something went wrong.

Copy link
Member

@kuzdogan kuzdogan left a comment

Choose a reason for hiding this comment

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

Some small and some major things. I haven't looked at the tests yet to not make this review too large. I'll review the tests as the final next step.

My comments about the transformations array merging might be confusing. We can do a short call about that.

I'm reviewing this a bit thoroughly as this is an important piece of code but otherwise great work!

Copy link
Member

@kuzdogan kuzdogan left a comment

Choose a reason for hiding this comment

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

Couple issues. I didn't really check how comprehensive these tests are and if more are needed. Also there are a lot of potential for refactoring. For the sake of moving forward I'm leaving these out for this PR.

- Update source file reading to support nested directory structures
- Clarify error message for bytecode matching with no auxdata
- Improve test description for bytecode matching scenario
Copy link
Member

@kuzdogan kuzdogan left a comment

Choose a reason for hiding this comment

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

LGTM!

LFG!

@marcocastignoli marcocastignoli merged commit 337fd91 into verification-flow-refactoring-main Feb 26, 2025
6 checks passed
@marcocastignoli marcocastignoli deleted the verification-flow-verification-classes branch February 26, 2025 17:36
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.

None yet

3 participants