-
-
Notifications
You must be signed in to change notification settings - Fork 132
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 conversion for fixed ABI types #1946
feat: add conversion for fixed ABI types #1946
Conversation
src/ape/managers/converters.py
Outdated
def is_convertible(self, value: Any) -> bool: | ||
# Matches only string-formatted floats with an optional sign character (+/-), | ||
# leading and trailing zeros are optional. | ||
return isinstance(value, str) and re.fullmatch("[+-]?[0-9]*[.][0-9]*", value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only nit is if theres a simpler regex to run here that picks up decimal values
Or even just trying to cast to decimal and catching exceptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reworked the regex to require digits before and after the decimal, because a "." input was slipping through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using \d
might even be clearer (lots of alpha here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also make sure to use \.
and not just .
(which is "any character")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's interesting, I used \d and got a depreciation warning from the re module. I didn't look further into it, but will take another look now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently they have already extensively documented the backslash plague 😂
I'll make some updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved with ba341bb
…uts, add "is not None" check to satisfy mypy
Also, general testing advice, using a regex is usually great paired with fuzzing to find corner cases for your expression (given positive and negative examples of what you want it to handle) |
def is_convertible(self, value: Any) -> bool: | ||
# Matches only string-formatted floats with an optional sign character (+/-). | ||
# Leading and trailing zeros are required. | ||
return isinstance(value, str) and re.fullmatch(r"[+-]?\d+\.\d+", value) is not None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re.fullmatch
is the same as checking the full string? That's helpful because we do already have conversion sequences like "1.0 ether"
and "10.0 USDC"
Can you add a comment to this effect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed, re.fullmatch
will check the full string - it should be equivalent to sandwiching the above expression with ^...$
but I'm not experienced enough with regex to know for sure if that expression has edge cases.
Added a note to the comment with b31222a
import pytest | ||
|
||
|
||
def test_converting_formatted_float_strings_to_decimal(convert): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I tend to use -k
to run multiple tests and it works great if following this pattern:
test_<method-name>_<rest>`
def test_converting_formatted_float_strings_to_decimal(convert): | |
def test_convert_formatted_float_strings_to_decimal(convert): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reformatted the test names per your suggestion, and added some more to check other bad strings per @fubuloubu's comment above.
What I did
Ape has no converter to handle Vyper's
decimal
type, which is translated to thefixed168x10
ABI type. Testing a contract function with adecimal
type argument will fail with aConversionError
.How I did it
ape_ethereum
plugin:Decimal
as the native Python type for ABI types with "fixed" in the name.ape
base:Decimal
value. This converter will translate a "1.0" string in-place. Did not add afloat
converter, because floating-point accuracy will cause subtle errors.Tests:
Checklist: