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

feat: Zero and One traits #4254

Merged
merged 13 commits into from
Oct 26, 2023
Merged

Conversation

enitrat
Copy link
Contributor

@enitrat enitrat commented Oct 13, 2023

Continues the work started in #3891
Closes #4236

  • Created a num directory with traits.cairo file
  • Added a One trait, and implementations for uints, ints and felt252
  • Added a Zero trait, and implementations for uints, ints and felt252
  • Made the Zeroable trait generically implemented from the Zero trait
  • Made the Oneable trait generically implemented from the One trait

Questions:

  • Should I split them into num/traits/one.cairo and num/traits/zero.cairo?
  • Should I bring other numeric traits inside this dir

Starknet tests are failing but I'm not sure why.


This change is Reviewable

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 6 files reviewed, 2 unresolved discussions (waiting on @enitrat)


corelib/src/num/traits.cairo line 6 at r1 (raw file):

    /// Returns the multiplicative identity element of Self, 0.
    #[inline(always)]
    fn zero() -> T;

doesn't do anything here.
and generally avoid unless you have a specific issue (for example, making a current test contract be less efficient)

Suggestion:

trait Zero<T> {
    /// Returns the multiplicative identity element of Self, 0.
    fn zero() -> T;

corelib/src/num/traits.cairo line 15 at r1 (raw file):

impl Felt252Zero of Zero<felt252> {
    #[inline(always)]
    fn zero() -> felt252 {

this is already inlined by heuristic.

Suggestion:

    fn zero() -> felt252 {

@enitrat
Copy link
Contributor Author

enitrat commented Oct 17, 2023

Removed #[inline(always)] for zero() and one() functions

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.

:lgtm:

Reviewed 5 of 6 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @enitrat)

a discussion (no related file):
@yuvalsw for 2nd eye.


Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 6 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @enitrat)

a discussion (no related file):
Please split into num/traits/one.cairo and num/traits/zero.cairo as suggested, and re-export them in num/traits.cairo (e.g. use one::One;).



corelib/src/math.cairo line 132 at r2 (raw file):

}

impl TOneableImpl<T, +One<T>, +Drop<T>> of Oneable<T> {

I think you should +Copy here too, we don't want any such impl to be consuming.


corelib/src/zeroable.cairo line 13 at r2 (raw file):

}

impl ZeroableImpl<T, +Zero<T>, +Drop<T>> of Zeroable<T> {

same here


corelib/src/num/traits.cairo line 3 at r2 (raw file):

// === Zero ===

trait Zero<T> {

please doc. Same for One


corelib/src/num/traits.cairo line 9 at r2 (raw file):

    fn is_zero(self: @T) -> bool;
    /// Returns whether self is not equal to 0, the multiplicative identity element.
    fn is_non_zero(self: @T) -> bool;

Suggestion:

    /// Returns the additive identity element of Self, 0.
    fn zero() -> T;
    /// Returns whether self is equal to 0, the additive identity element.
    fn is_zero(self: @T) -> bool;
    /// Returns whether self is not equal to 0, the additive identity element.
    fn is_non_zero(self: @T) -> bool;

corelib/src/num/traits.cairo line 12 at r2 (raw file):

}

impl Felt252Zero of Zero<felt252> {

Please move the impls to the module where the type is defined and not here. E.g. for uints move to integer.cairo.

Code quote:

impl Felt252Zero of Zero<felt252> {

@enitrat
Copy link
Contributor Author

enitrat commented Oct 18, 2023

Applied suggestions.

  • Split into zero and one files
  • Moved impls to respective files integer.cairo for uints/ints, lib.cairo for felt252
  • Added doc for Zero and One traits
  • Added a requirement for Copy<T> on generic impls

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.

Reviewed 3 of 7 files at r3, all commit messages.
Reviewable status: 4 of 8 files reviewed, 8 unresolved discussions (waiting on @enitrat and @yuvalsw)


corelib/src/lib.cairo line 8 at r3 (raw file):

};
use serde::Serde;
use num::traits::{Zero, One};

remove this use - as this exports this as core::One - which we shouldn't.

@enitrat
Copy link
Contributor Author

enitrat commented Oct 19, 2023

Removed import of num::traits::{Zero,One} in lib.cairo
Referred to the full path of num::traits::Zero and num::traits::One in the impl for felt252 in lib.cairo instead

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.

Reviewed 1 of 2 files at r4, all commit messages.
Reviewable status: 5 of 8 files reviewed, 8 unresolved discussions (waiting on @enitrat and @yuvalsw)

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 7 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @enitrat)


corelib/src/integer.cairo line 7 at r4 (raw file):

use array::ArrayTrait;
use array::SpanTrait;
use num::traits::{Zero, One};

this also re-exports it from here. Better fully qualify in the usage instead.


corelib/src/lib.cairo line 211 at r4 (raw file):

impl Felt252Zero of num::traits::Zero<felt252> {

please move these to felt252.cairo (create it). Felt stuff should be in a dedicated module. We can't yet move items that are here as it would break existing code, but new items should be there.
Thanks!

@enitrat
Copy link
Contributor Author

enitrat commented Oct 19, 2023

  • Removed import of {Zero,One} in integer.cairo and used full paths
  • Moved Felt252{Zero,One} to felt252.cairo

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.

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @enitrat)

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @enitrat)


corelib/src/integer.cairo line 2330 at r5 (raw file):

    #[inline(always)]
    fn is_zero(self: @u8) -> bool {
        *self == num::traits::Zero::zero()

would be slightly better to use the specific impl directly and not the trait.

Suggestion:

*self == U8Zero::zero()

@enitrat
Copy link
Contributor Author

enitrat commented Oct 19, 2023

would be slightly better to use the specific impl directly and not the trait.

Done.

Also, I noticed that I was importing One in math.cairo and Zero in Zeroable. For consistency with previous reviews, I used explicit full paths instead.

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Good catch, thanks. Hope this is the last iteration :)

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @enitrat)


corelib/src/math.cairo line 143 at r6 (raw file):

        num::traits::One::is_non_one(@self)
    }
}

same in zeroable

Suggestion:

impl TOneableImpl<T, impl OneImpl: num::traits::One<T>, +Drop<T>, +Copy<T>> of Oneable<T> {
    fn one() -> T {
        OneImpl::one()
    }
    #[inline(always)]
    fn is_one(self: T) -> bool {
        OneImpl::is_one(@self)
    }
    #[inline(always)]
    fn is_non_one(self: T) -> bool {
        OneImpl::is_non_one(@self)
    }
}

@enitrat
Copy link
Contributor Author

enitrat commented Oct 19, 2023

Done, should be all good now, thanks for taking the time!

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

LGTM, but CI fails. Please check and fix.

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @enitrat)

@enitrat
Copy link
Contributor Author

enitrat commented Oct 19, 2023

CI fails after moving felt impls to felt_252.cairo. Because these impls are not part of the prelude, they're not found by the generic TZeroableImpl - and thus Zeroable<felt252> can't be used as it used to before.
The following code is erroneous:

    #[inline(always)]
    fn is_zero(self: EthAddress) -> bool {
        self.address.is_zero()
    }

-->

Method `is_zero` not found on type "core::felt252". Did you import the correct trait and impl?

One solution would be this

use felt_252::{Felt252Zero};

    //...
    #[inline(always)]
    fn is_zero(self: EthAddress) -> bool {
        zeroable::TZeroableImpl::<felt252>::is_zero(self.address)
    }

But I'm not sure if it's the best approach. Another approach would be to re-export them in lib.cairo (I don't think we want that either.)

A third approach would be using impl alias in the felt_252.cairo file to declare Felt252Zeroable from TZeroableImpl<felt252> and refer to this one.

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

The third approach SG. Also note:

  1. you must rename felt252.cairo to e.g. felt_252.cairo. Otherwise there would be a conflict with the type name.
  2. You need to add felt_252 as a module in lib.cairo.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @enitrat)

@enitrat
Copy link
Contributor Author

enitrat commented Oct 23, 2023

What I ended up doing for ClassHash, ContractAddress, EthAddress is:

  • Replaced Zeroable<ContractAddress> impls by Zero<ContractAddress> - it will automatically generate an implementation for the Zeroable trait through TZeroableImpl.

  • Used impl aliases to instantiating the generic TZeroable with the concrete type

  • Explicitly used Felt252Zero in class_hash, contract_address, and eth_address files with:

    #[inline(always)]
    fn is_zero(self: EthAddress) -> bool {
        felt_252::Felt252Zero::is_zero(self.address)
    }

The only concern I have is that the Zero trait becomes practically unusable because:

  • To use .is_zero() on a felt252, I need to import Felt252Zero
  • If I import Felt252Zero, then I think an impl for zeroable::TZeroableImpl<felt252>; is created
  • Since Zeroable is included in the prelude, then both Zeroable and Zero can be applied to the felt252 type, which causes the following conflict.
Ambiguous method call. More than one applicable trait function with a suitable self type was found: Zero::is_zero and Zeroable::is_zero. Consider adding type annotations or explicitly refer to the impl function.

I don't think the approach is right, and I do think that we should rethink how Zeroable / Zero are used - ideally, we can perhaps don't include Zeroable in the prelude and only use the Zero trait instead?
In the current state of things, I would be against merging this PR until we find a satisfying solution.

@enitrat enitrat force-pushed the feat/zero-one-traits branch from 82178e8 to 849c327 Compare October 23, 2023 04:49
Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

That is true, but as you realised, it's currently impossible to remove it from the prelude. We are working on solutions to do that. For now we have some workarounds.

What I meant is to have the generic impl (TZeroableImpl) "hidden" (in a module), and instantiate impl aliases of it with the concrete relevant types, each in its relevant module. A similar approach is used in into_felt252_based::HashImpl, you can take a look there.
This will mean that Zeroable will not be implemented (in context) for any type that implements Zero, but only for the types for which there is an impl alias with them as the generic argument.
For now, there will indeed be ambiguity between Zeroable and Zero for the types already implementing Zeroable, so you can't use Zero for such types. What would be possible is to use it in generic implementations, e.g. fn something<T, +Zero<T>>(t: T) { ... t.is_zero() ... }.

I feel like it's not super clear what needs to be done here, so let me try to elaborate:

  1. Put the generic impl of Zeroable in an inline module.
  2. Have impl aliases with the relevant type for all the types that already implement Zeroable (like you did).
  3. Leave Zero as is.
  4. Same for One.

Reviewed 2 of 5 files at r8, 4 of 4 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @enitrat)

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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @enitrat)


corelib/src/zeroable.cairo line 24 at r9 (raw file):

        ZeroImpl::is_non_zero(@self)
    }
}

as this currently causes Zeroable to be available whenever Zero is available, so is_zero is always ambiguous - we can do this instead:

(this would provide backwards compatility for only what we already provided without causing collisions in the general case)

Do the same for Oneable.
(just saw yuval wrote the same thing, but this is some more info, so leaving here)

Suggestion:

mod zero_based {
impl ZeroableImpl<T, impl ZeroImpl: num::traits::Zero<T>, +Drop<T>, +Copy<T>> of Zeroable<T> {
    fn zero() -> T {
        ZeroImpl::zero()
    }
    #[inline(always)]
    fn is_zero(self: T) -> bool {
        ZeroImpl::is_zero(@self)
    }
    #[inline(always)]
    fn is_non_zero(self: T) -> bool {
        ZeroImpl::is_non_zero(@self)
    }
}
}
/// In the files this originally existed in:
impl U8Zeroable = zero_based::ZeroableImpl<u8, integer::U8Zero>;
impl U16Zeroable = zero_based::ZeroableImpl<u16, integer::U16Zero>;
impl U32Zeroable = zero_based::ZeroableImpl<u32, integer::U32Zero>;
impl U64Zeroable = zero_based::ZeroableImpl<u64, integer::U64Zero>;
impl U128Zeroable = zero_based::ZeroableImpl<u128, integer::U128Zero>;
impl U256Zeroable = zero_based::ZeroableImpl<u256, integer::U256Zero>;

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r10, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @enitrat)


.tool-versions line 0 at r10 (raw file):
this is unrelated.


corelib/src/integer.cairo line 2332 at r10 (raw file):

// Zero trait implementations
impl U8Zero of num::traits::Zero<u8> {

BTW, you can use the same method for these impls. Impl it once, generically and hidden in an inner module here, and then have impl aliases. It would be shorter.

Same for One.


corelib/src/math.cairo line 131 at r10 (raw file):

}

impl TOneableImpl<T, impl OneImpl: num::traits::One<T>, +Drop<T>, +Copy<T>> of Oneable<T> {

do the same for oneable. "Hidden" generic impl plus impl aliases.


corelib/src/zeroable.cairo line 28 at r10 (raw file):

}

impl ContractAddressZeroable = zeroable::zero_based::ZeroableImpl<felt252, felt_252::Felt252Zero>;

Remove, I see it's already in contract_address.cairo, and correctly (here you accidentally use felt252).

Code quote:

impl ContractAddressZeroable = zeroable::zero_based::ZeroableImpl<felt252, felt_252::Felt252Zero>;

corelib/src/starknet/class_hash.cairo line 40 at r10 (raw file):

}

impl ContractAddressZeroable = zeroable::zero_based::ZeroableImpl<ClassHash, ClassHashZero>;

Suggestion:

ClassHashZeroable

corelib/src/starknet/contract_address.cairo line 42 at r10 (raw file):

}

impl ContractAddressZeroable = zeroable::zero_based::ZeroableImpl<ContractAddress,ContractAddressZero>;

Suggestion:

ContractAddress, ContractAddre

corelib/src/felt_252.cairo line 33 at r8 (raw file):

}

impl Felt252Zeroable = zeroable::TZeroableImpl<felt252>;

An impl for felt existed , use an impl alias instead here.

Copy link
Contributor Author

@enitrat enitrat left a comment

Choose a reason for hiding this comment

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

Sorry, I was still working on it but got interrupted before pushing the commit with one and fixing what was previously wrong - should be good now.

Last problem is the failing CI on contract class tests, which I think is due to the change in sierra code induced by the changes in the corelib. Should I update the test fixtures as well?

Reviewable status: 10 of 14 files reviewed, 7 unresolved discussions (waiting on @yuvalsw)


.tool-versions line at r10 (raw file):

Previously, yuvalsw wrote…

this is unrelated.

Done.


corelib/src/integer.cairo line 2332 at r10 (raw file):

Previously, yuvalsw wrote…

BTW, you can use the same method for these impls. Impl it once, generically and hidden in an inner module here, and then have impl aliases. It would be shorter.

Same for One.

I don't think I can because I cannot create generic numeric literals. Or I didn't understand what you suggested, but I don't see how I can create a generic impl for Zero and One.


corelib/src/math.cairo line 131 at r10 (raw file):

Previously, yuvalsw wrote…

do the same for oneable. "Hidden" generic impl plus impl aliases.

Done.


corelib/src/zeroable.cairo line 28 at r10 (raw file):

Previously, yuvalsw wrote…

Remove, I see it's already in contract_address.cairo, and correctly (here you accidentally use felt252).

Done.


corelib/src/starknet/class_hash.cairo line 40 at r10 (raw file):

}

impl ContractAddressZeroable = zeroable::zero_based::ZeroableImpl<ClassHash, ClassHashZero>;

Done.


corelib/src/starknet/contract_address.cairo line 42 at r10 (raw file):

}

impl ContractAddressZeroable = zeroable::zero_based::ZeroableImpl<ContractAddress,ContractAddressZero>;

Done.


corelib/src/felt_252.cairo line 33 at r8 (raw file):

Previously, yuvalsw wrote…

An impl for felt existed , use an impl alias instead here.

These are Zero and One impls, not Zeroable/Oneable. Oneable didn't exist for felt, Zeroable did but under the zeroable.cairo file

Copy link
Contributor

@yuvalsw yuvalsw 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 r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @enitrat)


.tool-versions line at r10 (raw file):

Previously, enitrat (Mathieu) wrote…

Done.

I still see it.


corelib/src/integer.cairo line 2332 at r10 (raw file):

Previously, enitrat (Mathieu) wrote…

I don't think I can because I cannot create generic numeric literals. Or I didn't understand what you suggested, but I don't see how I can create a generic impl for Zero and One.

Ah, you are right, it's not currently possible.

Copy link
Contributor Author

@enitrat enitrat left a comment

Choose a reason for hiding this comment

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

Should be good now, sorry for the troubles - I finally understood how reviewable works which makes thing easier indeed.

I still have this CI problem I raised above.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yuvalsw)


.tool-versions line at r10 (raw file):

Previously, yuvalsw wrote…

I still see it.

Done. For some reason it reappeard 🤔

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Yes, please update the expected test files with: CAIRO_FIX_TESTS=1 cargo test -p starknet.
We'll review the changes to make sure they look good.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @enitrat)


.tool-versions line at r10 (raw file):

Previously, enitrat (Mathieu) wrote…

Done. For some reason it reappeard 🤔

I still see it :(

Copy link
Contributor Author

@enitrat enitrat left a comment

Choose a reason for hiding this comment

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

Done.

Reviewable status: 13 of 28 files reviewed, 1 unresolved discussion (waiting on @orizi and @yuvalsw)


.tool-versions line at r10 (raw file):

Previously, yuvalsw wrote…

I still see it :(

Done. I triple checked this time! It seems that scarb auto-generates it

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 15 of 15 files at r12, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @enitrat)

@yuvalsw
Copy link
Contributor

yuvalsw commented Oct 26, 2023

Seems like you need to rebase.

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.

Reviewed 2 of 7 files at r3, 1 of 2 files at r4, 1 of 5 files at r8, 1 of 4 files at r9, 3 of 7 files at r10, 4 of 4 files at r11, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @enitrat)

@enitrat enitrat force-pushed the feat/zero-one-traits branch from 5fb0afd to 510a242 Compare October 26, 2023 13:31
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.

Reviewed 2 of 15 files at r13, all commit messages.
Reviewable status: 15 of 28 files reviewed, all discussions resolved (waiting on @yuvalsw)

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 15 of 15 files at r13, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @enitrat)

@yuvalsw yuvalsw added this pull request to the merge queue Oct 26, 2023
Merged via the queue into starkware-libs:main with commit d205edc Oct 26, 2023
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.

feat: public Zero and Onetraits
3 participants