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

TODO #5785

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

TODO #5785

wants to merge 1 commit into from

Conversation

liorgold2
Copy link
Collaborator

@liorgold2 liorgold2 commented Jun 12, 2024

This change is Reviewable

commit-id:ed05f465
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 7 unresolved discussions (waiting on @liorgold2)


crates/cairo-lang-sierra/src/extensions/modules/bounded_int.rs line 202 at r1 (raw file):

        BoundedIntDivRemAlgorithm::try_new(&lhs_range, &rhs_range)
            .ok_or(SpecializationError::UnsupportedGenericArg)?;
        // TODO: Add a require that rhs_range.upper >= 2 (also assumed below).

seems to be done right under.


crates/cairo-lang-sierra/src/extensions/modules/bounded_int.rs line 466 at r1 (raw file):

            .ok_or(SpecializationError::UnsupportedGenericArg)?;
        let prime: BigInt = Felt252::prime().to_bigint().unwrap();
        // The following is not really necessary for this function. Once we support ranges

Discussed f2f - adding a block here is just an extra safety measure - can be removed in the future once these ranges actually exist.


crates/cairo-lang-sierra/src/extensions/modules/circuit.rs line 145 at r1 (raw file):

                droppable: false,
                storable: false,
                zero_sized: false, // TODO: Should be true?

it isn't storable - so shouldn't actually affect anything.


crates/cairo-lang-sierra/src/extensions/modules/circuit.rs line 217 at r1 (raw file):

// Represents the action of adding two fields elements in the circuits builtin.
// TODO: Can we share code between gates? (including inv?)

done.
https://reviewable.io/reviews/starkware-libs/cairo/5893


crates/cairo-lang-sierra/src/extensions/modules/circuit.rs line 1416 at r1 (raw file):

                .to_usize()
                .ok_or(SpecializationError::UnsupportedGenericArg)?;
            // TODO: panic if idx is already in inputs.

done right under.


crates/cairo-lang-sierra-to-casm/src/invocations/circuit.rs line 510 at r1 (raw file):

        assert guarantee = *(rc96++);
    }
    // TODO: Check why the automatic cost verification is not working (?). Is it becaues rc96 is like

done.


crates/cairo-lang-sierra-type-size/src/lib.rs line 105 at r1 (raw file):

            | CoreTypeConcrete::Circuit(CircuitTypeConcrete::MulModGate(_))
            | CoreTypeConcrete::Circuit(CircuitTypeConcrete::SubModGate(_)) => continue,
            // TODO: What's the difference between using continue and setting their size to 0?

makes sure it is not storable.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 8 unresolved discussions (waiting on @liorgold2)


crates/cairo-lang-sierra/src/extensions/modules/circuit.rs line 135 at r1 (raw file):

            .to_usize()
            .ok_or(SpecializationError::UnsupportedGenericArg)?;
        // TODO: Refactor TypeInfo{...} into a function and use in the other 4 gates.

done.

Copy link
Collaborator Author

@liorgold2 liorgold2 left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @orizi)


crates/cairo-lang-sierra/src/extensions/modules/circuit.rs line 1296 at r1 (raw file):

    outputs_tuple_ty: &ConcreteTypeId,
) -> Result<CircuitInfo, SpecializationError> {
    // TODO: Consider moving the validate_output_tuple logic here.

Is this done?

@ilyalesokhin-starkware
Copy link
Contributor

crates/cairo-lang-sierra/src/extensions/modules/circuit.rs line 1296 at r1 (raw file):

Previously, liorgold2 wrote…

Is this done?

yes

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.

None yet

3 participants