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

Fixed point numbers library #2064

Closed
wants to merge 36 commits into from
Closed

Conversation

rostyslavtyshko
Copy link
Contributor

@rostyslavtyshko rostyslavtyshko commented Jun 21, 2022

Adding fixed point numbers library akin to https://github.com/abdk-consulting/abdk-libraries-solidity

Adding basic math functionality to other connected types i.e. u8/16/32/64/128

@rostyslavtyshko rostyslavtyshko added the lib: std Standard library label Jun 21, 2022
@rostyslavtyshko rostyslavtyshko marked this pull request as draft June 21, 2022 10:02
@rostyslavtyshko rostyslavtyshko self-assigned this Jun 21, 2022
sway-lib-std/src/fixed_point.sw Outdated Show resolved Hide resolved
sway-lib-std/src/fixed_point.sw Outdated Show resolved Hide resolved
sway-lib-std/src/u128.sw Outdated Show resolved Hide resolved
sway-lib-std/src/u128.sw Outdated Show resolved Hide resolved
sway-lib-std/src/u128.sw Outdated Show resolved Hide resolved
@emilyaherbert
Copy link
Contributor

emilyaherbert commented Jun 21, 2022

@rostyslavtyshko can you clarify what was happening that was causing the related bug on this PR?

@Braqzen
Copy link
Contributor

Braqzen commented Jun 21, 2022

@rostyslavtyshko can you clarify what was happening that was causing the related bug on this PR?

Compiling Sway and the library works as expected however when you attempt to import the U128 type (for testing) and use the Expo trait with the pow() function it would complain that pow() does not exist no matter how it was imported and where it was located.

E.g. some_u128_var.pow() with the error that pow() is not found on the type.

The strange thing is that other traits defined within the same file, or other files, for the U128 have been discovered and worked as expected. The expo trait seemed to not exist within tests at all despite the library itself compiling as expected.

Sorry, got ahead of myself and edited your comment by accident

sway-lib-std/src/fixed_point.sw Outdated Show resolved Hide resolved
sway-lib-std/src/fixed_point.sw Outdated Show resolved Hide resolved
Copy link
Contributor

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

Would it be possible to make this closer to https://docs.rs/fixed/latest/fixed/ in terms of interface and naming? (Most of the changes to this PR would just be naming, since everything other than the core ops should be deferred to a future PR.)

Specifically something like https://docs.rs/fixed/latest/fixed/struct.FixedU64.html, with a hard-coded 32:32.

@rostyslavtyshko rostyslavtyshko marked this pull request as ready for review August 30, 2022 13:00
sezna
sezna previously approved these changes Sep 8, 2022
Copy link
Contributor

@Braqzen Braqzen left a comment

Choose a reason for hiding this comment

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

Seems like this PR is dependent upon #2340 so I would wait for that to go in before resolving the commented out code.

All of the logging should be removed btw. It's not too bad when testing via scripts but it should not be in the std (if there are any). The scripts should just contain the setup and assertions since there is no use in spamming logs during an automated workflow.

In the PR linked above I've mentioned that Self should be used consistently where possible as this allows the rest of the code to be more flexible. This should also be done here.

@simonr0204 simonr0204 self-requested a review September 26, 2022 15:52
simonr0204
simonr0204 previously approved these changes Sep 26, 2022
sezna
sezna previously approved these changes Sep 26, 2022
@sezna sezna enabled auto-merge (squash) September 26, 2022 16:01
simonr0204
simonr0204 previously approved these changes Sep 26, 2022
Copy link
Contributor

@Braqzen Braqzen left a comment

Choose a reason for hiding this comment

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

  1. There are various sections which are commented out. Should all of them remain commented out or are they able to be completed? Is there an issue ready to track this? We would not want this to get buried.
  2. Some of the tests do not look like tests since they simply log and do not assert anything. Logs should be removed from tests and only assertions should remain.
  3. There are various imports that use the glob operator and I'm unsure if they are needed. There's at least one which I think can be explicit, maybe more.
  4. Sway now has a prelude which means that there are certain types that do not have to be imported. I've seen some of those in this PR. Can these imports be removed (since they are unnecessary) or must they still be imported because this is part of the standard library and there's something that I'm unaware of?
  5. impl core::ops::Divide for UFP128 has a let mut s = self;. in divide(). Can this not be changed to something like ref mut self in the params instead?

@mohammadfawaz
Copy link
Contributor

As discussed previously, it's better to move this PR to the sway-libs repository. Once #2873 is resolved, we can bring back all the math libraries into sway-lib-std.

Changing this to a draft in the meantime.

@mohammadfawaz mohammadfawaz marked this pull request as draft October 5, 2022 18:10
auto-merge was automatically disabled October 5, 2022 18:10

Pull request was converted to draft

@mohammadfawaz
Copy link
Contributor

I'm going to close this in favor of FuelLabs/sway-libs#32 and FuelLabs/sway-libs#31.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib: std Standard library
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

10 participants