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

Feature: add BoundedInt trait in core::integer for min() and max() #2563

Merged

Conversation

Amxx
Copy link
Contributor

@Amxx Amxx commented Mar 20, 2023

When manipulating u256, it is sometime necessary to represent the max value, similarly to how solidity does type(uint256).max. For example, some implementation of ERC-20 do not decrease the allowance in transferFrom if the current allowance is set to this max value.

Having this maximum value accessible from the corelib would save devs the burden of re-declaring this in their files.

I'm open to many different names such as uint256_max, max_uint256. If possible, uint256::max would be nice.

Note that this is declared as a function and not as a constant because Only literal constants are currently supported.


This change is Reviewable

Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Suggestion: define a trait instead, something like

trait BoundedInt<T> {
  fn min() -> T;
  fn max() -> T;
}

implement it for u256. Then you could use BoundedInt::max(), or BoundedInt::<u256>::max() if not inferred. You will probably be able to use u256::max() in the future, though, I don't think this is currently supported.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @Amxx and @milancermak)

Copy link
Contributor

@spapinistarkware spapinistarkware 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 2 files reviewed, 2 unresolved discussions (waiting on @Amxx and @milancermak)


corelib/src/integer.cairo line 982 at r1 (raw file):

fn max_u256() -> u256 implicits(RangeCheck) nopanic {
    u256 { low: 0xffffffffffffffffffffffffffffffff_u128, high: 0xffffffffffffffffffffffffffffffff_u128 }

Line too long. Is your formatter configured correctly? WIth max line length of 100?

Copy link
Contributor Author

@Amxx Amxx left a comment

Choose a reason for hiding this comment

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

I refactored my PR to use this trait. I implemented it for u8, u16, u32, u64, u128 and u256

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @milancermak and @spapinistarkware)

Copy link
Contributor

@spapinistarkware spapinistarkware 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 r2, 1 of 1 files at r3.
Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions (waiting on @Amxx)


corelib/src/integer.cairo line 987 at r3 (raw file):

}

impl BoundedU8 of BoundedInt::<u8> {

Please add #[inline(always)] above all of the impl functions. Also remove the RangeCheck.

Copy link
Contributor

@spapinistarkware spapinistarkware 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 all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Amxx)

auto-merge was automatically disabled March 28, 2023 07:57

Head branch was pushed to by a user without write access

Conflicts:
	corelib/src/test.cairo
Copy link
Contributor Author

@Amxx Amxx 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 3 files reviewed, 1 unresolved discussion (waiting on @milancermak and @spapinistarkware)


corelib/src/integer.cairo line 987 at r3 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Please add #[inline(always)] above all of the impl functions. Also remove the RangeCheck.

done

Copy link
Contributor Author

@Amxx Amxx 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 3 files reviewed, 1 unresolved discussion (waiting on @milancermak and @spapinistarkware)


corelib/src/test.cairo line 666 at r1 (raw file):

Previously, milancermak (Milan Cermak) wrote…
    max_u256() + 1.into();

Done.

Copy link
Contributor

@spapinistarkware spapinistarkware 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 r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @milancermak)

auto-merge was automatically disabled March 28, 2023 15:28

Head branch was pushed to by a user without write access

@Amxx Amxx changed the title Feature: add integer::max_u256() to corelib Feature: add BoundedInt trait in core::integer for min() and max() Mar 28, 2023
Copy link
Contributor

@spapinistarkware spapinistarkware 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 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @milancermak)

Copy link
Contributor

@spapinistarkware spapinistarkware 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: all files reviewed, 1 unresolved discussion (waiting on @milancermak)

@starkware-libs starkware-libs deleted a comment from milancermak Mar 29, 2023
@spapinistarkware spapinistarkware added this pull request to the merge queue Mar 29, 2023
Merged via the queue into starkware-libs:main with commit e1165f1 Mar 29, 2023
@Amxx Amxx deleted the corelib/integer/max_uint256 branch March 29, 2023 08:54
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.

2 participants