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.

Sign up for GitHub

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

Jump to bottom

Improve handling of "version" string from Etherscan #545

Merged
merged 3 commits into from
Feb 7, 2024
Merged

Improve handling of "version" string from Etherscan #545

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cf981c7
Improve handling of "version" string from Etherscan
ggrieco-tob Feb 7, 2024
1bc8da8
Update etherscan.py
ggrieco-tob Feb 7, 2024
ad9bf30
fixed lint
ggrieco-tob Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion crytic_compile/platform/etherscan.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,9 @@ def _convert_version(version: str) -> str:
Returns:
str: converted version
"""
return version[1 : version.find("+")]
if "+" in version:
return version[1 : version.find("+")]
return version
Copy link

Choose a reason for hiding this comment

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

The _convert_version function now handles version strings without a + character by returning the entire string if the + character is not found. This change correctly addresses the issue described in the PR objectives. However, the function unnecessarily slices the version string from index 1, which could remove the first character of the version string unintentionally. This could lead to incorrect version handling if the version string does not start with 'v' or if the character is essential.

-        return version[1 : version.find("+")]
+        return version[: version.find("+")]
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if "+" in version:
return version[1 : version.find("+")]
return version
if "+" in version:
return version[: version.find("+")]
return version

Copy link
Member

@elopez elopez Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do versions with no "+" also have no "v" in the beginning? With this change, versions without "+" won't have the first character removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was not really necessary (as the code worked with the v), but I pushed a quick fix for it anyway.



def _sanitize_remappings(
Expand Down