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

Restore cast script #2937

Conversation

franciszekjob
Copy link
Collaborator

@franciszekjob franciszekjob commented Feb 12, 2025

Closes #2933

Introduced changes

Restore cast script by adjusting run function

Checklist

  • Linked relevant issue
  • Updated relevant documentation
  • Added relevant tests
  • Performed self-review of the code
  • Added changes to CHANGELOG.md

@franciszekjob
Copy link
Collaborator Author

note: Please remember that this PR fixes cast scripts, so tests and linting outside of sncast crate are still failing.

@franciszekjob franciszekjob marked this pull request as ready for review February 12, 2025 16:06
Copy link
Member

@kkawula kkawula left a comment

Choose a reason for hiding this comment

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

works!

@franciszekjob franciszekjob requested a review from a team as a code owner February 16, 2025 19:42
@franciszekjob franciszekjob requested review from ddoktorski and removed request for a team February 16, 2025 19:43
Copy link
Member

@cptartur cptartur left a comment

Choose a reason for hiding this comment

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

Does test pass for this?

@franciszekjob
Copy link
Collaborator Author

Does test pass for this?

Once I changed usage of prepare_args to empty array, they didn't. Now they pass, after I've set max gas value.

@franciszekjob
Copy link
Collaborator Author

image

Copy link
Member

@cptartur cptartur left a comment

Choose a reason for hiding this comment

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

Please address the comments, otherwise we can merge

Comment on lines +337 to +366
let param_types = builder.generic_id_and_size_from_concrete(&func.signature.param_types);
let base_offset = 5;
// * Segment arena is allocated conditionally, so segment index is automatically moved (+2 segments)
// * Each used builtin moves the offset by +1
// * Line `let mut builtin_offset = 3;` in `create_entry_code_from_params`
// * TODO Where is remaining +2 in base offset coming from? Maybe System builtin and Gas builtin which seem to be always included
let segment_index = if param_types
.iter()
.any(|(ty, _)| ty == &SegmentArenaType::ID)
{
// FIXME verify this
base_offset + builtins.len() + 2
} else {
// FIXME verify this
base_offset + builtins.len()
};
let syscall_handler = SyscallHintProcessor::new(
&mut blockifier_state,
&mut context,
// This segment is created by SierraCasmRunner
Relocatable {
segment_index: segment_index
.try_into()
.expect("Failed to convert index to isize"),
offset: 0,
},
CallEntryPoint::default(),
&string_to_hint,
ReadOnlySegments::default(),
);
Copy link
Member

Choose a reason for hiding this comment

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

Add a TODO here to extract the copied code to common function.

@franciszekjob franciszekjob merged commit 07cda7e into kkawula/2862-bump-cairo-and-blockifier Feb 18, 2025
15 of 23 checks passed
@franciszekjob franciszekjob deleted the franciszekjob/2933-restore-cast-script branch February 18, 2025 09:48
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.

4 participants