-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Conversation
...c/e2e_vm_tests/test_programs/should_pass/stdlib/fixed_point_test/fixed_point_test/Cargo.toml
Outdated
Show resolved
Hide resolved
...c/e2e_vm_tests/test_programs/should_pass/stdlib/fixed_point_test/fixed_point_test/Cargo.toml
Outdated
Show resolved
Hide resolved
test/src/e2e_vm_tests/test_programs/should_pass/stdlib/logarythmic_test/Forc.toml
Outdated
Show resolved
Hide resolved
test/src/e2e_vm_tests/test_programs/should_pass/stdlib/logarythmic_test/Forc.toml
Outdated
Show resolved
Hide resolved
test/src/e2e_vm_tests/test_programs/should_pass/stdlib/u128_pow_test/Forc.toml
Outdated
Show resolved
Hide resolved
@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. 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 |
There was a problem hiding this 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.
…ixed-point-num-lib
There was a problem hiding this 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.
7e627df
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
- 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.
- 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.
- 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?
impl core::ops::Divide for UFP128
has alet mut s = self;
. individe()
. Can this not be changed to something likeref mut self
in the params instead?
As discussed previously, it's better to move this PR to the Changing this to a draft in the meantime. |
Pull request was converted to draft
I'm going to close this in favor of FuelLabs/sway-libs#32 and FuelLabs/sway-libs#31. |
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