Add support for basic arithmetic operations #36
Conversation
| Some(u256_token(84)), | ||
| ) | ||
| }) | ||
| } |
There was a problem hiding this comment.
@g-r-a-n-t regarding #37 Instead of repeating the above test for multiplication, substraction etc, we could use parameters like this to put it all in one test.
#[rstest(input, expected,
case("return_addition_u256.vy", vec![42, 42, 84]),
...
case("return_substraction_u256.vy", vec![42, 42, 0),
...
case("return_multiplication_u256.vy", vec![5, 5, 25),
...
)]
There was a problem hiding this comment.
At least, that is what I would do if this was Py-EVM / Trinity code. Interested to hear your thoughts..
There was a problem hiding this comment.
I think it looks good.
I also wonder if we could do the same thing here #29, where we're trying to automatically generate tests with macros (which doesn't work anymore) 🤔
cburgdorf
force-pushed
the
christoph/feat/arithmetic
branch
2 times, most recently
from
d409b32
to
58e5909
Compare
| typ: Type::Base(Base::U256), | ||
| }) | ||
| }, | ||
| vyp::BinOperator::Div => { |
There was a problem hiding this comment.
@g-r-a-n-t You mentioned in #24 that you would separate each arm into its own function. I was wondering if there was a strong argument in favor for that (beside just separation of concerns) because I believe it might just unnecessary bloat loc here. I can be convinced but wanted to here the reasoning :)
There was a problem hiding this comment.
I'm ok with this. Also, these functions will get even lighter with the semantic analysis pass.
|
@g-r-a-n-t There are a bunch of other binary operations and I'm happy to implement these but since #24 was just about these basic four I thought I cut it short here and let you review this as a self-contained package first. |
| case("return_addition_u256.vy", vec![42, 42], Some(84)), | ||
| case("return_subtraction_u256.vy", vec![42, 42], Some(0)), | ||
| case("return_multiplication_u256.vy", vec![42, 42], Some(1764)), | ||
| case("return_division_u256.vy", vec![42, 42], Some(1)), |
There was a problem hiding this comment.
There's stuff like division by zero that we could test here but I don't want to open a can of worms and put it in an issue for now #41
There was a problem hiding this comment.
Yeah, for now I think it's ok to put things like this off. Below are some thoughts on how I think this should be done.
My thinking right now is that we will introduce safe math functions to the runtime, where we check for things like overflow and div-by-zero. Then instead of mapping a / b to div(a', b'), we map to safe_div(a', b'), where safe_div is a function that we've defined that reverts on a zero denominator.
I think the semantic analysis pass should be responsible for determining what functions are needed during runtime and then we'll include them in the Yul contract when mapping from the Fe contract. This approach would be extended to things like maps where we have generic type params. For example, defining the contract field pub guest_book: map<address, bytes[100]> would result in two function being added to the runtime named something like map_get_address_bytes100 and map_insert_address_bytes100, which would be used in place of the expression self.guest_book[0x00] and statement self.guest_book[0x00] = some_bytes.
There was a problem hiding this comment.
Great feedback, I put it on #41
cburgdorf
force-pushed
the
christoph/feat/arithmetic
branch
3 times, most recently
from
e827371
to
53b3455
Compare
cburgdorf
force-pushed
the
christoph/feat/arithmetic
branch
from
53b3455
to
d470af9
Compare
What was wrong
As stated in #24 we need to add support for basic arithmetic operations.
How was it fixed
add,sub,mul,divto their YUL counterparts