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

Make it easier to test hints implemented out of the cairo-vm repository #1595

Open
odesenfans opened this issue Feb 1, 2024 · 2 comments
Open
Labels
enhancement New feature or request

Comments

@odesenfans
Copy link
Contributor

Hello,

We're currently developing some hints for a project. These hints live in a different Git repository. We developed hints for the bootloader in a fork of cairo-vm so I hadn't noticed yet, but it is surprisingly way more difficult to write unit tests out of the repository. Example:

If we take a hint

%{
    a = ids.a
%}

We can easily write a unit test for this in the cairo-vm repository like this:

let mut vm = vm!();
vm.segments = segments![((1, 0), 42)];
vm.run_context.fp = 1;
let mut exec_scopes = ExecutionScopes::new();
let ids_data = ids_data!["a"];

hint(&mut vm, &mut exec_scopes, &ids_data).unwrap();

let a: u64 = exec_scopes.get("a").unwrap();
assert_eq!(a, 42);

But out of tree, things become more complex / impossible.

// let mut vm = vm!();   // vm! is private
let mut vm = VirtualMachine::new(false);   // but easily replaced by a one-liner, so this is fine

// vm.segments = segments![((1, 0), 42)];   // segments! is also private
vm.add_memory_segment();
vm.add_memory_segment();
vm.insert_value(Relocatable::from((1, 0), 42);  // A bit more verbose, but OK

// vm.run_context.fp = 1;
// Incrementing FP is impossible because `run_context` is `pub(crate)`.

// let ids_data = ids_data!["a"];   // ids_data is private as well
let ids_data = HashMap::from([("a".to_string(), HintReference::new_simple(-1))];

So we're in a situation where AP/FP manipulations are impossible because of VM members being pub(crate) and the utils macros are not public so they need to be reimplemented manually.

The first point is the one that actually is blocking, so I would like your opinion on the matter. What's a good solution there? Making VM fields pub instead of pub(crate)? Adding accessors? Another solution I have not thought of?

Describe the solution you'd like
Ideally, it should be possible to write unit tests the same way independently of where the implementation lives.

Additional context
This is related to the implementation of hints for the bootloader and Starknet OS.

@odesenfans odesenfans added the enhancement New feature or request label Feb 1, 2024
@fmoletta
Copy link
Contributor

fmoletta commented Feb 1, 2024

For the first point, the VirtualMachine methods set_ap, set_fp and set_pc are public, they are hidden from the docs but they should be safe to use in tests.
And I agree that some testing macros could be made public under the test_utils feature (particularly the segments and memory macros as their usage is not tied to knowing how the vm works internally)

@odesenfans
Copy link
Contributor Author

The problem with segments! is that it requires vm.segments to be public as well. I can make a PR and we can discuss it there if this suits you.

odesenfans added a commit to Moonsong-Labs/cairo-vm that referenced this issue Feb 5, 2024
odesenfans added a commit to Moonsong-Labs/cairo-vm that referenced this issue Feb 5, 2024
odesenfans added a commit to Moonsong-Labs/cairo-vm that referenced this issue Feb 5, 2024
odesenfans added a commit to Moonsong-Labs/cairo-vm that referenced this issue Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants