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: added U8, U64 and U128 into U250 #415

Merged
merged 1 commit into from
Jun 10, 2023

Conversation

aymericdelab
Copy link
Contributor

very simple pr to add intoU250 implementation for u8, u64 and u128

@milancermak
Copy link
Contributor

Can you please add u16 as well so we cover all the primitive types?

Comment on lines +59 to +63
impl U8IntoU250 of Into<u8, u250> {
fn into(self: u8) -> u250 {
u250 { inner: self.into() }
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can have a single generic impl like:

Suggested change
impl U8IntoU250 of Into<u8, u250> {
fn into(self: u8) -> u250 {
u250 { inner: self.into() }
}
}
impl TIntoU250<T, impl TInto: Into<T, felt252>> of Into<T, u250> {
fn into(self: T) -> u250 {
let inner = TInto::into(self) ;
// TODO: Bounds checks
u250 { inner }
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooooh that's very cool indeed, well done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

im stuck on a this error:

error: Plugin diagnostic: Trait `core::traits::Into::<dojo_core::integer::u250, dojo_core::integer::u250>` has multiple implementations, in: "dojo_core::integer::TIntoU250", "core::traits::TIntoT"
   --> RouteAuth:38:180
                          IWorldDispatcher { contract_address: world_address }.set_entity(dojo_core::string::ShortStringTrait::new('AuthRole'), (route.target_id, route.resource_id).into(), 0_u8, array::ArrayTrait::span(@calldata));

issue is that there's already an implementation TIntoT in the corelib so both are applicable in the case of:

commands::set_entity(
      (route.target_id, route.resource_id).into(), (AuthRole { id: route.role_id })
        );

where route.target_id and route.resource_id are both u250 already

is there a way to specify which impl you want to use in this case ?

@aymericdelab aymericdelab force-pushed the feat/integer_into_u250 branch from 76a423f to 5336558 Compare June 8, 2023 21:14
@tarrencev tarrencev merged commit 7831b5c into dojoengine:main Jun 10, 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.

3 participants