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 div mod hint #127

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

coxmars
Copy link

@coxmars coxmars commented Aug 7, 2024

Hi @zmalatrax,

I've made significant progress on the DivMod hint implementation, including updates to the hintName.ts, hintSchema.ts, hintHandler.ts. The core logic is nearly complete, but a few final adjustments are still needed in divMod.ts. Also, the verified tag is working according with the contribution guidelines.

Status

  • Add div mod hint working as expected.

@coxmars coxmars changed the title Feat/div mod hint feat: Add div mod hint Aug 7, 2024
@zmalatrax zmalatrax linked an issue Aug 7, 2024 that may be closed by this pull request
@coxmars
Copy link
Author

coxmars commented Aug 8, 2024

Update 📌

Hi @zmalatrax

  • Implemented and tested the DivMod hint.

  • All related tests for DivMod have passed successfully.

  • Here are the test results:

image
  • Please review the changes and let me know if any additional modifications are needed, also can you explain me a little bit the integration test with Cairo I think it is the only I need to complete it 🫡

Copy link
Collaborator

@zmalatrax zmalatrax left a comment

Choose a reason for hiding this comment

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

great :)

src/hints/math/divMod.ts Outdated Show resolved Hide resolved
src/hints/math/divMod.ts Outdated Show resolved Hide resolved
src/hints/math/divMod.test.ts Outdated Show resolved Hide resolved
src/hints/math/divMod.test.ts Outdated Show resolved Hide resolved
src/hints/math/divMod.test.ts Outdated Show resolved Hide resolved
@zmalatrax
Copy link
Collaborator

@coxmars Looks like your typescript files aren't well formatted, we're using Trunk, I'd recommend you installing their extension if you're on VSCode or Neovim

Otherwise, you can add their CLI and run trunk fmt.

@coxmars
Copy link
Author

coxmars commented Aug 8, 2024

@coxmars Looks like your typescript files aren't well formatted, we're using Trunk, I'd recommend you installing their extension if you're on VSCode or Neovim

Otherwise, you can add their CLI and run trunk fmt.

Thanks for the feedback, I will apply the requested changes asap 🫡

@zmalatrax
Copy link
Collaborator

About the integration test, you need to add a Cairo program divmod.cairo in the cairo_programs/cairo/hints folder that makes use of the DivMod hint.

It should be a simple program focused on this hint, such as

fn main() {
    let _ = 17_u32 / 12_u32;
    let _ = 17_u32 % 12_u32;
}

@coxmars
Copy link
Author

coxmars commented Aug 9, 2024

Hi @zmalatrax I was able to solve some problems running the integration test haha, it took me a while but I got it done, unit test and integration test ready plus the feedback given, as you can see below it is working so I will open this draft PR to "Ready for review" 🫡

image

@coxmars coxmars marked this pull request as ready for review August 9, 2024 01:02
src/hints/math/divMod.ts Outdated Show resolved Hide resolved
@coxmars
Copy link
Author

coxmars commented Aug 9, 2024

Hi @zmalatrax I already changed the condition as you said and @enitrat I added a new edge case test, if you need something else to be improved just tell me, thanks 🫡

image

@coxmars
Copy link
Author

coxmars commented Aug 14, 2024

Hi @zmalatrax I applied the changes and add a few more tests ✅

image image

@coxmars
Copy link
Author

coxmars commented Aug 29, 2024

Hi @zmalatrax, I already applied the improvement, thanks 🫡

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.

feat: add DivMod hint
3 participants