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

feat: add recursive dependency search for imported contracts #25

Merged

Conversation

rkolpakov
Copy link
Contributor

@rkolpakov rkolpakov commented Oct 24, 2023

Added possibility to verify contracts if they have dependencies. Used a recursive search over the repo on github to cover complex contracts e.g. EasyTrack.

@TheDZhon
Copy link
Contributor

TheDZhon commented Oct 24, 2023

Thank you for proposing this change 🙏

Let me shed a bit of light here 🤔
So the issue arose with the brownie verification tooling (i.e., they rewrite the imports on the sources submission)

See the attached pics:
photo_2023-10-24_19-52-46
photo_2023-10-24_19-52-45

I suggest NOT enabling recursive ('fuzzy') search by default but instead utilizing it only if the enforcement flag is provided within the config. Also, the flag might be omitted to preserve backward compatibility, falling back to the previous (strict search) behavior.

wdyt, @rkolpakov and @mymphe?

@arwer13
Copy link
Contributor

arwer13 commented Oct 24, 2023

I'd prefer it enabled by flag (NOT default) as well. Although I cannot come up with an example when enabling this by default would lead to a bad outcome, I'd like to see more strictness in such a bedrock tool.

@mymphe
Copy link
Contributor

mymphe commented Oct 26, 2023

Agree with @arwer13. I think this feature is only useful for certain scenarios and should be explicitly enabled. Overall, a tool such as Diffyscan should be as dumb as possible, imo. Making it too smart might lead to critical false positives.

utils/explorer.py Outdated Show resolved Hide resolved
utils/arguments.py Outdated Show resolved Hide resolved
utils/github.py Outdated Show resolved Hide resolved
Copy link
Contributor

@arwer13 arwer13 left a comment

Choose a reason for hiding this comment

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

Thanks for the code and fixes! Almost there from my point of view, left just a pair of minor doc-related comments.

README.md Outdated Show resolved Hide resolved
utils/arguments.py Outdated Show resolved Hide resolved
@rkolpakov rkolpakov force-pushed the feature/recursive-dependency-resolving branch from 3ea2f18 to f761f76 Compare January 24, 2024 12:39
@TheDZhon TheDZhon merged commit e64a006 into lidofinance:main Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contracts verified with brownie don't preserve relative paths for imports
4 participants