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.

Sign up for GitHub

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

Jump to bottom

Fixes #56 : new add functions and test for the changes #60

Closed
wants to merge 1 commit into from
Closed

Fixes #56 : new add functions and test for the changes #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 52 additions & 17 deletions pkg/vm/memory/memory_value.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,6 @@ func (address *MemoryAddress) Equal(other *MemoryAddress) bool {
return address.SegmentIndex == other.SegmentIndex && address.Offset == other.Offset
}

// Adds a memory address and a field element
func (address *MemoryAddress) Add(lhs *MemoryAddress, rhs *f.Element) (*MemoryAddress, error) {
if !rhs.IsUint64() {
return nil, fmt.Errorf("field element does not fit in uint64: %s", rhs.String())
}

address.SegmentIndex = lhs.SegmentIndex
address.Offset = lhs.Offset + rhs.Uint64()
return address, nil
}

// Subs from a memory address a felt or another memory address in the same segment
func (address *MemoryAddress) Sub(lhs *MemoryAddress, rhs any) (*MemoryAddress, error) {
// First match segment index
Expand Down Expand Up @@ -194,19 +183,65 @@ func (mv *MemoryValue) Equal(other *MemoryValue) bool {
return false
}

// Adds two memory values is the second one is a Felt
// // Adds two memory values is the second one is a Felt
// func (mv *MemoryValue) Add(lhs, rhs *MemoryValue) (*MemoryValue, error) {
// var err error
// if lhs.IsAddress() {
// if !rhs.IsFelt() {
// return nil, errors.New("rhs is not a felt")
// }
// // lhs : Address, rhs : Address
// mv.address, err = mv.address.Add(lhs.address, rhs.felt)
// } else {
// if rhs.IsAddress() {
// mv.address, err = mv.address.Add(rhs.address, lhs.felt)
// } else {
// mv.felt = mv.felt.Add(lhs.felt, rhs.felt)
// }
// }

// if err != nil {
// return nil, err
// }
// return mv, nil
// }
Comment on lines +186 to +207
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we leaving this comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will erase. I tried to compare it side by side before / after the change


// Adds a memory address and a field element
func (address *MemoryAddress) Add(lhs *MemoryAddress, rhs *f.Element) (*MemoryAddress, error) {
if !rhs.IsUint64() {
return nil, fmt.Errorf("field element does not fit in uint64: %s", rhs.String())
}

address.SegmentIndex = lhs.SegmentIndex
address.Offset = lhs.Offset + rhs.Uint64()
return address, nil
}
Comment on lines +210 to +218
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change the position of this function definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change it back to where it was. I was carefully checking if there's any missing cases.


func (mv *MemoryValue) Add(lhs, rhs *MemoryValue) (*MemoryValue, error) {
var err error
if lhs.IsAddress() {
if !rhs.IsFelt() {
return nil, errors.New("rhs is not a felt")
if rhs.IsAddress() {
// lhs : Address, rhs : Address
if lhs.address.SegmentIndex != rhs.address.SegmentIndex {
return nil, errors.New("cannot add addresses from different segments")
}
mv.address.SegmentIndex = lhs.address.SegmentIndex
mv.address.Offset = lhs.address.Offset + rhs.address.Offset
} else if rhs.IsFelt() {
// lhs : Address, rhs : felt
mv.address, err = mv.address.Add(lhs.address, rhs.felt)
} else {
return nil, errors.New("invalid rhs type")
}
Comment on lines +230 to 235
Copy link
Contributor

Choose a reason for hiding this comment

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

A simple else is enough here, no need to check if it is a field element after checking if it is an address

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.

mv.address, err = mv.address.Add(lhs.address, rhs.felt)
} else {
if rhs.IsAddress() {
mv.address, err = mv.address.Add(rhs.address, lhs.felt)
} else {
// lhs : felt, rhs : Address
return nil, errors.New("invalid operation: cannot add integer to memory address")
} else if rhs.IsFelt() {
// lhs : felt, rhs : felt
mv.felt = mv.felt.Add(lhs.felt, rhs.felt)
} else {
return nil, errors.New("invalid rhs type")
Comment on lines +240 to +244
Copy link
Contributor

Choose a reason for hiding this comment

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

It is safe to go for just an else here, no need to check if it is a felt. Memory values are either one or the other. There are never the two at the same time, or none.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.

}
}

Expand Down
82 changes: 82 additions & 0 deletions pkg/vm/vm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,18 @@ func TestInferOperandSub(t *testing.T) {
assert.Equal(t, expectedOp0Cell, op0Cell)
}

/*
CaseA: Adding a MemoryAddress to an integer (already provided).
CaseB: Adding an integer to a MemoryAddress.
CaseC: Adding two MemoryAddress values from the same segment.
CaseD: Adding two MemoryAddress values from different segments (this should result in an error).
CaseE: Adding a MemoryAddress to a FieldElement (if supported).
CaseF: Adding a FieldElement to a MemoryAddress (if supported).
CaseG: Adding two FieldElement values.
CaseH: Adding a MemoryAddress to a value that exceeds the uint64 limit (should result in an error).
CaseI: Adding a FieldElement to a value that exceeds the uint64 limit (should result in an error).
CaseJ: Adding two values where one of them is neither a MemoryAddress nor a FieldElement (should result in an error).
*/
func TestComputeAddRes(t *testing.T) {
vm := defaultVirtualMachine()

Expand Down Expand Up @@ -330,6 +342,76 @@ func TestComputeAddRes(t *testing.T) {
assert.Equal(t, expected, res)
}

func TestComputeAddRes_Jake(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please follow the current style of testing, where each case gets its own function. If we want to change the style, we should do it then for the whole project to be consistent across packages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I will change it as the current style.

// Test cases
testCases := []struct {
name string
op0 *mem.MemoryValue
op1 *mem.MemoryValue
expected *mem.MemoryValue
shouldFail bool
}{
{
name: "Type A : MemoryAddressPlusInteger",
op0: mem.MemoryValueFromMemoryAddress(mem.NewMemoryAddress(2, 10)),
op1: mem.MemoryValueFromInt(15),
expected: mem.MemoryValueFromMemoryAddress(mem.NewMemoryAddress(2, 25)),
},
{
name: "Type B : MemoryAddressPlusMemoryAddress_SameSegment",
op0: mem.MemoryValueFromMemoryAddress(mem.NewMemoryAddress(2, 10)),
op1: mem.MemoryValueFromMemoryAddress(mem.NewMemoryAddress(2, 15)),
expected: mem.MemoryValueFromMemoryAddress(mem.NewMemoryAddress(2, 25)),
Comment on lines +361 to +364
Copy link
Contributor

Choose a reason for hiding this comment

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

This should crash, since address + address is not allowed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Will modify it.

},
{
name: "Type C : MemoryAddressPlusMemoryAddress_DifferentSegment",
op0: mem.MemoryValueFromMemoryAddress(mem.NewMemoryAddress(2, 10)),
op1: mem.MemoryValueFromMemoryAddress(mem.NewMemoryAddress(3, 15)),
shouldFail: true,
},
{
name: "Type D : IntegerPlusInteger",
op0: mem.MemoryValueFromInt(10),
op1: mem.MemoryValueFromInt(15),
expected: mem.MemoryValueFromInt(25),
},
{
name: "Type E : IntegerPlusMemoryAddress",
op0: mem.MemoryValueFromInt(15),
op1: mem.MemoryValueFromMemoryAddress(mem.NewMemoryAddress(2, 10)),
shouldFail: true,
Comment on lines +379 to +382
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should succeed since sum operations are commutative, addr + felt = felt + addr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay will look into it.

},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
vm := defaultVirtualMachine()

instruction := Instruction{
Res: AddOperands,
}

cellOp0 := &mem.Cell{
Accessed: true,
Value: tc.op0,
}

cellOp1 := &mem.Cell{
Accessed: true,
Value: tc.op1,
}

res, err := vm.computeRes(&instruction, cellOp0, cellOp1)
if tc.shouldFail {
require.Error(t, err)
return
}
require.NoError(t, err)
assert.Equal(t, tc.expected, res)
})
}
}

func TestOpcodeAssertionAssertEq(t *testing.T) {
vm := defaultVirtualMachine()

Expand Down
Loading