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

chore: improve error message for invalid address literal #3621

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

tserg
Copy link
Collaborator

@tserg tserg commented Sep 26, 2023

What I did

Fix #3611

How I did it

Add BadChecksumAddress exception and catch it.

How to verify it

See tests.

Commit message

chore: improve error message for address literal

Description for the changelog

Improve error mesage for address literal that is not checksummed

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2023

Codecov Report

Attention: Patch coverage is 27.27273% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 68.77%. Comparing base (b4429cf) to head (ec1c45d).

Files Patch % Lines
vyper/semantics/analysis/utils.py 11.11% 8 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #3621       +/-   ##
===========================================
- Coverage   85.19%   68.77%   -16.43%     
===========================================
  Files          92       92               
  Lines       13916    13923        +7     
  Branches     3118     3119        +1     
===========================================
- Hits        11856     9575     -2281     
- Misses       1565     3629     +2064     
- Partials      495      719      +224     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@charles-cooper
Copy link
Member

interesting .. did

foo: constant(address) = 0x6b175474e89094c44da98b954eedeac495271d0F
not need to be changed?

also, is

"Address checksum mismatch. If you are sure this is the right "
now dead code?

@tserg
Copy link
Collaborator Author

tserg commented Sep 27, 2023

interesting .. did

foo: constant(address) = 0x6b175474e89094c44da98b954eedeac495271d0F

not need to be changed?
also, is

"Address checksum mismatch. If you are sure this is the right "

now dead code?

It's actually not dead code, because validate_expected_type relies on this to throw an exception via types_from_Constant.

On second thought, I think a better way is to add an early return for Constant nodes to immediately call expected_type.validate_literal(node) in validate_expected_type. [EDIT: this still would not be sufficient on its own, because the invalid checksum address could be in a literal array]

Also, the AST test still passes because I had BadChecksumAddress inherit from InvalidLiteral. Should it inherit from VyperException instead?

@charles-cooper
Copy link
Member

discussed offline -- it's a bit weird as there are actually two (three?) different places which construct similar error messages

@anewt95

This comment was marked as off-topic.

@anewt95

This comment was marked as spam.

@charles-cooper
Copy link
Member

another issue is the literal check will fail even if the user doesn't want an address, e.x.:

vyper.exceptions.BadChecksumAddress: Address checksum mismatch. If you are sure this is the right address, the correct checksummed form is: 0x5124fcC2B3F99F571AD67D075643C743F38f1C34

  contract "tmp/repro2.vy:4", function "foo", line 4:15 
       3 def foo():
  ---> 4     x: uint8 = 0x5124fcc2b3f99f571ad67d075643c743f38f1C34
  ----------------------^
       5

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.

Wrong InvalidLiteral raise for checksum issues
4 participants