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

Expand api #4

Merged
merged 100 commits into from
Oct 18, 2023
Merged

Expand api #4

merged 100 commits into from
Oct 18, 2023

Conversation

Oppen
Copy link
Contributor

@Oppen Oppen commented Sep 21, 2023

Feature

Expand APIs

  • Add an optional arbitrary feature for fuzzers using the arbitrary crate
  • Specialized arithmetic operations with primitive types
  • Implemented bitwise operations
  • Implemented arithmetic modulo p'

Does this introduce a breaking change?

No

@Oppen
Copy link
Contributor Author

Oppen commented Sep 29, 2023

@LucasLvy added the docs and implemented ToPrimitive and FromPrimitive. I kept implementations of From in terms of FromPrimitive for convenience of use, since the trait defines them as fallible and they always succeed.

@Oppen
Copy link
Contributor Author

Oppen commented Sep 29, 2023

I also left a TODO in the ToPrimitive implementation as I'm undecided as to whether we should consider returning negative numbers instead of failing for the higher values. Since it would be supporting a new case and it shouldn't break user code (if people expected unsigned, they would probably convert to unsigned) I think it's reasonable to postpone the decision.

@Oppen
Copy link
Contributor Author

Oppen commented Oct 2, 2023

Just fixed the failing check, forgot to run cargo fmt 🤦

crates/stark-felt/src/lib.rs Outdated Show resolved Hide resolved
crates/stark-felt/src/lib.rs Outdated Show resolved Hide resolved
crates/stark-felt/src/lib.rs Outdated Show resolved Hide resolved
crates/stark-felt/src/lib.rs Outdated Show resolved Hide resolved
crates/stark-felt/src/lib.rs Outdated Show resolved Hide resolved
crates/stark-felt/src/lib.rs Outdated Show resolved Hide resolved
crates/stark-felt/src/lib.rs Outdated Show resolved Hide resolved
crates/stark-felt/src/lib.rs Outdated Show resolved Hide resolved
@Oppen
Copy link
Contributor Author

Oppen commented Oct 4, 2023

@tdelabro @LucasLvy I'll be thinking about how to improve the arbitrary implementation. Do you mind checking the other changes in the meantime to win some time?

@Oppen
Copy link
Contributor Author

Oppen commented Oct 4, 2023

Heh, forgot to enable the feature when testing locally 🤦
It's ready for a new round @LucasLvy @tdelabro

crates/stark-felt/src/lib.rs Outdated Show resolved Hide resolved
crates/stark-felt/src/lib.rs Outdated Show resolved Hide resolved
crates/stark-felt/src/lib.rs Outdated Show resolved Hide resolved
@0xLucqs
Copy link
Collaborator

0xLucqs commented Oct 10, 2023

I think there is a problem in the features (some missing imports for some feature) @Oppen

@Oppen
Copy link
Contributor Author

Oppen commented Oct 10, 2023

I think there is a problem in the features (some missing imports for some feature) @Oppen

Yes, I mistakenly thought some super::alloc imports were redundant while fixing a different one.

(Self(FieldElement::from(&q)), Self(FieldElement::from(&r)))
}

/// Multiplicative inverse inside field.
pub fn inverse(&self) -> Option<Self> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can probably use this num-trait trait: https://docs.rs/num-traits/latest/num_traits/ops/inv/trait.Inv.html

&(self.0).representative().div_rem(&n.0.representative()).1,
))
/// Raises `self` to the power of `exponent`.
pub fn pow_felt(&self, exponent: &Felt) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

use bitvec::array::BitArray;
use num_traits::{FromPrimitive, ToPrimitive, Zero};
Copy link
Collaborator

Choose a reason for hiding this comment

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

crates/stark-felt/src/lib.rs Show resolved Hide resolved
@tdelabro
Copy link
Collaborator

@Oppen Do you want to merge it here and keep the last conversation for further PR?

@Oppen
Copy link
Contributor Author

Oppen commented Oct 17, 2023

I think that would be good. Could you extract those to an issue so we don't forget?

@AbdelStark AbdelStark merged commit ea52b64 into starknet-io:main Oct 18, 2023
2 checks passed
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.

7 participants