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

fix[ux]: fix validation for abi_encode() method_id kwarg #4369

Merged
merged 5 commits into from
Dec 6, 2024

Conversation

charles-cooper
Copy link
Member

code like in the test case (abi_encode(..., method_id=b"123")) will panic due to the assertion in _parse_method_id(), since the original typecheck only checked that the method_id is <=4 bytes. this adds a rule to validate that bytestring method ids are exactly 4 bytes directly in abi_encode()'s typecheck routine.

What I did

fix #4354

How I did it

How to verify it

Commit message

Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)

Description for the changelog

Cute Animal Picture

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

code like in the test case (`abi_encode(..., method_id=b"123")`) will
panic due to the assertion in `_parse_method_id()`, since the original
typecheck only checked that the method_id is <=4 bytes. this adds a rule
to validate that bytestring method ids are exactly 4 bytes directly in
`abi_encode()`'s typecheck routine.
Copy link
Collaborator

@pcaversaccio pcaversaccio left a comment

Choose a reason for hiding this comment

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

While I was reviewing the PR, I thought of the case where method_id should be None and why x"" or b"" is disallowed. Opinions?

vyper/builtins/functions.py Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having a test for the empty case x"" is always something sane to do IMO.

@cyberthirst
Copy link
Collaborator

lgtm, let's just please merge https://github.com/charles-cooper/vyper/pull/51/files

@charles-cooper charles-cooper self-assigned this Dec 5, 2024
@charles-cooper charles-cooper changed the title fix[ux]: fix validation for abi_encode method_id kwarg fix[ux]: fix validation for abi_encode() method_id kwarg Dec 6, 2024
@charles-cooper charles-cooper merged commit c8691ac into vyperlang:master Dec 6, 2024
157 checks passed
@charles-cooper charles-cooper deleted the fix/ux/method-id branch December 6, 2024 02:12
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.

vyper.exceptions.CodegenPanic: unhandled exception , parse_Call
3 participants