kkrt-labs / cairo-vm-ts

A typescript implementation of the Cairo VM
Apache License 2.0
20 stars 13 forks source link

feat: add TestLessThanOrEqualAddress #122

Closed zmalatrax closed 1 month ago

zmalatrax commented 3 months ago

Estimated time: 0.5d Lifespan: 1d

How-to implement a hint on the Cairo VM TS available here

Similar to the TestLessThanOrEqual hint, but expecting Relocatable instead of Felt. Rust implementation reference of TestLessThanOrEqual

renzobanegass commented 3 months ago

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

Hey, I'm Renzo, a Software Engineer with over 5 years of experience, I recently started contributing to Web3 as a member of Dojo Coding, I have contributed in languages like Cairo and Rust already.

How I plan on tackling this issue

  1. Review Documentation and References
  2. Implement the Hint in Cairo VM TS Referencing the Rust implementation and the guide on implementing hints, create the new hint.
  3. Add Unit Tests
  4. Create Integration Test in Cairo
  5. Run Tests and Verify Implementation
  6. Submit PR
zmalatrax commented 3 months ago

Sure, assigned

renzobanegass commented 3 months ago

Hi, I have a question about the implementation of the hint, as there's no example for this one,

export const testLessThanOrEqualAddress = (
  vm: VirtualMachine,
  lhs: ResOperand,
  rhs: ResOperand,
  dst: CellRef
) => {
  const lhsValue = vm.getResOperandValue(lhs);
  const rhsValue = vm.getResOperandValue(rhs);
  const result = new Felt(BigInt(lhsValue <= rhsValue));
  vm.memory.assertEq(vm.cellRefToRelocatable(dst), result);
};

This implementation is quite similar to how the TestLessThanOrEqual hint would be, what I'm not sure about is, the result should be a Relocatable? If so, how can I do that conversion? If not, should the inputs be a Relocatable? I didn't quite get that part. Thank you!

zmalatrax commented 3 months ago

Hi, I have a question about the implementation of the hint, as there's no example for this one,

Hi, indeed missed providing the right one, here is a proper reference.

This hint was actually introduced with Cairo 2.7.0 and the multi_pop_back()/multi_pop_front() methods in this PR.

I'm currently bumping the Cairo compiler submodule to the right version so you can write and compile a Cairo program using this hint.

This implementation is quite similar to how the TestLessThanOrEqual hint would be, what I'm not sure about is, the result should be a Relocatable? If so, how can I do that conversion? If not, should the inputs be a Relocatable? I didn't quite get that part. Thank you!

Yes the inputs should be Relocatable, while the result is a Felt. It means that lhsValue and rhsValue extracted from lhs and rhs are expected to be Relocatable rather than Felt as in TestLessThanOrEqual.

The current method handling ResOperand is getResOperandValue() and only extracts Felt and not Relocatable.

I'll update the API so that you can properly implement the TestLessThanOrEqualAddress :)

zmalatrax commented 3 months ago

@renzobanegass All good now, you can use vm.getResOperandRelocatable() to extract a Relocatable from a ResOperand :)

renzobanegass commented 3 months ago

Great, I'll get to it then!

renzobanegass commented 3 months ago

Does this implementation look better?

export const testLessThanOrEqualAddress = (
  vm: VirtualMachine,
  lhs: ResOperand,
  rhs: ResOperand,
  dst: CellRef
) => {
  const lhsValue = vm.getResOperandRelocatable(lhs);
  const rhsValue = vm.getResOperandRelocatable(rhs);

  const isLessThanOrEqual = lhsValue.segmentId < rhsValue.segmentId || 
                            (lhsValue.segmentId === rhsValue.segmentId && lhsValue.offset <= rhsValue.offset);

  const result = new Felt(isLessThanOrEqual ? 1n : 0n);

  vm.memory.assertEq(vm.cellRefToRelocatable(dst), result);
};
zmalatrax commented 3 months ago

Does this implementation look better?

Open a PR, I'll make a review, it'll be much easier