-
Notifications
You must be signed in to change notification settings - Fork 180
feat(breaking): add support for hint accessible scopes #2042
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(breaking): add support for hint accessible scopes #2042
Conversation
077b47d
to
a61564e
Compare
Hi @enitrat! I don't think I get the benefit of this change. Could you expand on this? Or maybe practical example? What's the use case of this change, apart from specifying where the alias points to (if I understood correctly)? |
@FrancoGiachetta the The problem is that, when executing a hint, it is not known whether a constant is in scope or not. Let's say I have three files a.cairo
b.cairo
c.cairo
When executing the hint inside Now, the problem is that the current design of the hint executor does not enable differentiating between the constants that are effectively in scope of the hints (in our case, only Thus, it is impossible to work only with the constant name ( What this PR does is:
|
For example here cairo-vm/vm/src/hint_processor/builtin_hint_processor/math_utils.rs Lines 526 to 527 in 077b47d
"UPPER_BOUND" from the accessible constants.
|
Oh, I see. What I don't get now is, what is the issue with the full path? What is it that are you unable to do with the full path? |
If I have two variable named MY_CONST that exists in two different full files and I import one of them, there's a conflict because inside the hint I do not know which one is available in the current scope |
Hi @enitrat! Sorry for the delay. We've been discussing this with the team. This PR addresses two issues:
The latter is more straightforward, and thus could me moved to another PR. Could you split this in two different pull requests? Accessible ScopesI have the following setup (similar to yours): a.cairo: const CONST_VAR = 3; b.cairo: const CONST_VAR = 4;
const CONST_BIZ = 5; c.cairo: from a import CONST_VAR
from b import CONST_BIZ
func main() {
alloc_locals;
local foo: felt;
%{
ids.foo = ids.CONST_VAR
%}
assert foo = CONST_VAR;
return ();
} When I try to implement the hint in our VM, I have the following information: exec_scopes = ExecutionScopes { ... }
hint_data = HintProcessorData {
code: "ids.foo = ids.CONST_VAR",
ap_tracking: ...
ids_data: {
"foo": HintReference { ... },
}
}
constants = {
"__main__.main.SIZEOF_LOCALS": 0x1,
"a.CONST_VAR": 0x3,
"b.CONST_VAR": 0x4,
"b.CONST_BIZ": 0x5,
} Even if I had accessible scopes for the current hint, I wouldn't be able to resolve to which constant does |
Hey @JulianGCalderon, here's how we can know what's in scope and what is not only based on the information made available in this PR: // Process constants and make them accessible in Python hints
// Constants are in the form {"module.name": value} and are accessible if their module
// is in the hint_accessible_scopes
for (full_name, value) in constants {
let parts: Vec<_> = full_name.split('.').collect();
let const_name = parts.last().unwrap_or(&"").to_string();
let module_path = parts[..parts.len() - 1].join(".");
// Check if constant is directly accessible from current scope
if hint_accessible_scopes.iter().any(|scope| module_path == *scope) {
py_ids_dict
.borrow_mut(py)
.items
.insert(const_name.to_string(), value.to_biguint().into_bound_py_any(py)?.into());
continue;
}
// Check if constant is accessible through an alias in any accessible scope
for scope in hint_accessible_scopes {
let alias_path = format!("{}.{}", scope, const_name);
if let Some(identifier) = identifiers.get(&alias_path) {
if let Some(destination) = &identifier.destination {
if let Some(resolved_value) = constants.get(destination) {
py_ids_dict.borrow_mut(py).items.insert(
const_name.to_string(),
resolved_value.to_biguint().into_bound_py_any(py)?.into(),
);
break;
}
}
}
}
}
So to answer this, I think it's sufficient. I will proceed in splitting the PR in 2 |
@enitrat I get the idea of the code snippet. I have some questions:
Maybe I'll understand the snippet if I have more context, could you share the data that is stored in your hint processor? |
Indeed, we make the program identifiers accessible in a hint by dumping them in the |
I'm trying to think of alternate ways to support this. This is a big change on the API and we would like to exhaust all other possibilities before modifying the hint processor. Have you tried storing the hint accessible scopes inside of the hint processor? As an example, for the Cairo 1 hint processor, we store all the hint data inside of the hint processor instead of receiving it in
|
Not sure how i'd do that, every hint has its own accessible scopes, they're not shared among all hints 🤔
I think this is what i'm doing by storing them in the HintProcessorData ? |
The Cairo 1 hint representation is much different from the Cairo 0 hint representation. For this reason, instead of relying on the information passed to the Something like this: struct HintProcessorImplementation {
/// stores a vector of hints for each instruction index
hints: HashMap<usize, Vec<Hint>>,
...
} Relevant links: This is kind of a workaround to the actual problem (not knowing accessible scopes), but it may work. Let me know what you think. |
Yeah, i'm not a huge fan, I think that the best place for this information to live is currently inside the HintProcessorData. It does create some API changes so I understand the reluctance but I think it's the most logical place for it to be. I also don't really see any obvious workaround that would have cause minimum changes on that topic |
Hey @enitrat, we would like to move forward with this PR.
Thanks! |
Hi @JulianGCalderon here's the PR for identifiers destinations: |
update changelog
b70b814
to
42820a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small typo
Co-authored-by: Julian Gonzalez Calderon <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds accessible scopes in hint and improves alias identifiers resolution
Description
This PR enhances the Cairo VM by adding support for accessible scopes in hints.
Key Changes:
Accessible Scopes in Hints:
Impact:
Also, related to #2041: this solves my problem as by making a
destination
field we can resolve the alias.Checklist