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.

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

feat: add TestLessThanOrEqualAddress #122

Open
Tracked by #90
zmalatrax opened this issue Aug 6, 2024 · 8 comments · May be fixed by #132
Open
Tracked by #90

feat: add TestLessThanOrEqualAddress #122

zmalatrax opened this issue Aug 6, 2024 · 8 comments · May be fixed by #132
Assignees
Labels
good first issue Good for newcomers

Comments

@zmalatrax
Copy link
Collaborator

zmalatrax commented Aug 6, 2024

Estimated time: 0.5d
Lifespan: 1d

  • Implement the TestLessThanOrEqualAddress hint.
  • Add unit tests, testing the parsing and the logic of the hint.
  • Add a Cairo program using the hint, used as an integration test .

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

@zmalatrax zmalatrax mentioned this issue Aug 6, 2024
@zmalatrax zmalatrax added the good first issue Good for newcomers label Aug 6, 2024
@renzobanegass
Copy link

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
Copy link
Collaborator Author

Sure, assigned

@renzobanegass
Copy link

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@renzobanegass
Copy link

Great, I'll get to it then!

@renzobanegass
Copy link

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
Copy link
Collaborator Author

Does this implementation look better?

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

@renzobanegass renzobanegass linked a pull request Aug 7, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
Status: 🏗️ In progress
Development

Successfully merging a pull request may close this issue.

2 participants