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.
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.
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.
using \d might even be clearer (lots of alpha here)
There was a problem hiding this comment.
also make sure to use \. and not just . (which is "any character")
There was a problem hiding this comment.
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.
Apparently they have already extensively documented the backslash plague 😂
I'll make some updates.
There was a problem hiding this comment.
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.
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.
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.
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.
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
decimaltype, which is translated to thefixed168x10ABI type. Testing a contract function with adecimaltype argument will fail with aConversionError.How I did it
ape_ethereumplugin:Decimalas the native Python type for ABI types with "fixed" in the name.apebase:Decimalvalue. This converter will translate a "1.0" string in-place. Did not add afloatconverter, because floating-point accuracy will cause subtle errors.Tests:
Checklist: