Skip to content

feat: (levm) arithmetic operations#544

Merged
ilitteri merged 19 commits into
mainfrom
levm/feat/arithmetic_operations
Sep 25, 2024
Merged

feat: (levm) arithmetic operations#544
ilitteri merged 19 commits into
mainfrom
levm/feat/arithmetic_operations

Conversation

@belfortep

@belfortep belfortep commented Sep 24, 2024

Copy link
Copy Markdown
Contributor

Motivation

This PR adds the arithmetic operations for LEVM

Description

Adds the implementation of this group of opcodes with tests

Closes #457
Closes #458
Closes #459
Closes #460
Closes #461
Closes #462
Closes #463
Closes #464
Closes #465
Closes #466
Closes #467

@belfortep belfortep changed the title Levm/feat/arithmetic operations feat: (levm) arithmetic operations Sep 24, 2024
@belfortep belfortep marked this pull request as ready for review September 25, 2024 12:34
@belfortep belfortep requested a review from a team as a code owner September 25, 2024 12:34
Comment thread crates/levm/src/vm.rs Outdated
Comment thread crates/levm/src/vm.rs Outdated
Comment thread crates/levm/src/vm.rs Outdated
}

#[test]
fn addmod_op_for_zero() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you check that ADDMOD works correctly when adding 2^256 - 2 and 2^256 - 3 modulo 2^256 - 1?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Checked it and the code had a bug returning a wrong value, just fixed it and added a test 🫡

}

#[test]
fn mulmod_op() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's test this with big numbers that trigger the overflowing behaviour, and modules comparable to 2^256

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should we add this tests for all the opcodes that could overflow?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sure!

@iinaki iinaki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice work!

@ilitteri ilitteri left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The PR looks good and I agree with @MegaRedHand comments. I'd also add a change in the naming. Let's use the formal operand names for the operations (both for implementation and tests if needed):

  • Addition: augend + addend = sum.
  • Subtraction: minuend - subtrahend = difference.
  • Multiplication: multiplicand × multiplier = product.
  • Division: dividend ÷ divisor = quotient.
  • Modulus: dividend % divisor = remainder.
  • Exponentiation: base ^ exponent = power.

@ilitteri ilitteri left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@ilitteri ilitteri added this pull request to the merge queue Sep 25, 2024
Merged via the queue into main with commit 63c8373 Sep 25, 2024
@ilitteri ilitteri deleted the levm/feat/arithmetic_operations branch September 25, 2024 18:18
emirongrr pushed a commit to emirongrr/ethrex that referenced this pull request Nov 10, 2024
**Motivation**

This PR adds the arithmetic operations for LEVM

**Description**

Adds the implementation of this group of opcodes with tests

<!-- Link to issues: Resolves lambdaclass#111, Resolves lambdaclass#222 -->

Closes lambdaclass#457 
Closes lambdaclass#458
Closes lambdaclass#459 
Closes lambdaclass#460 
Closes lambdaclass#461
Closes lambdaclass#462
Closes lambdaclass#463 
Closes lambdaclass#464
Closes lambdaclass#465 
Closes lambdaclass#466 
Closes lambdaclass#467
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.

Opcode: SDIV Opcode: DIV Opcode: SUB Opcode: MUL Opcode: ADD

4 participants