Hi there!
The PR requires the changelog to be updated in order to pass that check.

Hi @ClementWalter! Thanks for the contribution. Could you add a test with the failing case that you mentioned?

I don’t see where to actually add tests. It seems like the function is currently not tested at all

I also see that in the places where you actually test the mod builtins, you left relevant TODOs

// TODO: Import directly from common library once released
from cairo_programs.mod_builtin_feature.common.modulo import ModBuiltin, UInt384, run_mod_p_circuit

Which would actually have shown the issue. Should I update this?

@ClementWalter:

I don’t see where to actually add tests. It seems like the function is currently not tested at all

The function mod_builtin_fill_memory is covered in the following tests:

• tests::cairo_run_test::cairo_run_apply_poly
• tests::cairo_run_test::cairo_run_mod_builtin
• tests::cairo_run_test::cairo_run_mod_builtin_failure
• tests::cairo_run_test::cairo_run_apply_poly_proof
• tests::cairo_run_test::cairo_run_mod_builtin_large_batch_size
• tests::cairo_run_test::cairo_run_mod_builtin_large_batch_size_failure
• tests::cairo_run_test::cairo_run_mod_builtin_proof
• tests::cairo_run_test::cairo_run_mod_builtin_large_batch_size_proof
• vm::runners::builtin_runner::modulo::tests::test_air_private_input_all_cairo
• math_utils::tests::mul_inv_x_by_x_is_1
• tests::cairo_run_test::cairo_run_apply_poly
• tests::cairo_run_test::cairo_run_inv_mod_p_uint512
• tests::cairo_run_test::cairo_run_mod_builtin
• program_hash::tests::test_compute_program_hash_chain::case_2_field_arithmetic
• tests::cairo_run_test::cairo_run_mod_builtin_large_batch_size
• tests::cairo_run_test::cairo_run_overflowing_dict
• vm::runners::builtin_runner::output::tests::get_allocated_memory_units

A quick way of testing this is by adding a test case in vm_core that forces add_mod_ptr_n or mul_mod_ptr_n to be 0. With forcing I mean overriding one of the args. This would be enough to keep coverage at a reasonable value.

I also see that in the places where you actually test the mod builtins, you left relevant TODOs

// TODO: Import directly from common library once released
from cairo_programs.mod_builtin_feature.common.modulo import ModBuiltin, UInt384, run_mod_p_circuit
Which would actually have shown the issue. Should I update this?

I tried uncommenting those imports on main and tests passed.
For example, in cairo_programs/mod_builtin_feature/apply_poly.cairo:

// TODO: Import directly from common library once released
from cairo_programs.mod_builtin_feature.common.modulo import ModBuiltin, UInt384, run_mod_p_circuit
// from starkware.cairo.common.modulo import run_mod_p_circuit
// from starkware.cairo.common.cairo_builtins import ModBuiltin, UInt384

I tried leaving it like this:

// TODO: Import directly from common library once released
// from cairo_programs.mod_builtin_feature.common.modulo import ModBuiltin, UInt384, run_mod_p_circuit
from starkware.cairo.common.modulo import run_mod_p_circuit
from starkware.cairo.common.cairo_builtins import ModBuiltin, UInt384

And the related tests passed without errors.

Given this, I think it's not necessary to uncomment those imports for now but I will keep you updated when it would be necessary.

Hi @ClementWalter! I'm not sure that the fix in the right place.

• Right now the n==0 logic is in the fetch_builtin_params closure, which only fetches the BuiltinRunner given the BuiltinName.
• I think its better to move the n==0 logic to the fill_memory function, so that its analogous to the python implementation:

  cairo-vm/vm/src/vm/runners/builtin_runner/modulo.rs

  Lines 506 to 510 in 3de653d

  	pub fn fill_memory(
  	memory: &mut Memory,
  	add_mod: Option<(Relocatable, &ModBuiltinRunner, usize)>,
  	mul_mod: Option<(Relocatable, &ModBuiltinRunner, usize)>,
  	) -> Result<(), RunnerError> {

Would this change also fix the issue?

@JulianGCalderon it will probably work as well, as you prefer. I however don't manage to do neither make deps nor make deps-macos locally so I'm unable to push this to merge because I don't manage to run the tests locally, and so to add some

@ClementWalter could you share the error message you get when running make deps-macos? We may be able to help you with that.