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

Extend fixedpoint library to support negative numbers #44

Closed
dpaiton opened this issue Mar 26, 2024 · 4 comments
Closed

Extend fixedpoint library to support negative numbers #44

dpaiton opened this issue Mar 26, 2024 · 4 comments
Labels
good first issue Good for newcomers

Comments

@dpaiton
Copy link
Contributor

dpaiton commented Mar 26, 2024

Several motivators for this:

@dpaiton dpaiton added the good first issue Good for newcomers label Mar 26, 2024
@dpaiton dpaiton changed the title extend fixedpoint math library to support negative numbers Extend fixedpoint math library to support negative numbers Mar 26, 2024
@dpaiton dpaiton changed the title Extend fixedpoint math library to support negative numbers Extend fixedpoint library to support negative numbers Mar 26, 2024
@dpaiton
Copy link
Contributor Author

dpaiton commented Mar 29, 2024

Tasks:

  • Change the FixedPoint struct to I256 instead of U256
  • Change try_from type import err
  • Change hardcoded u256 in to_scaled_string
  • Check bounds in exp.
  • Change conversions to and from FixedPoint to properly handle negative values
  • Verify that remaining math fns are fine
  • Write negative tests

@dpaiton dpaiton self-assigned this Mar 29, 2024
@jalextowle
Copy link
Contributor

I thought about this more last night, and I came away with a slightly different perspective on this. From a priorities standpoint, I see this as a “Nice to Have.” I don’t see this as a blocker for implementing targeted likings or other features, and I wouldn’t consider this to be as urgent as those features. I’m happy to talk through how we deal with negative numbers in the other optimization flows. It’s easier than it sounds, and we’ve gotten quite far without this kind of thing.

I say all this, but if implementing this were super quick (like a few hours), I’d say go for it. I’m skeptical that it will be that easy since we need to preserve 1:1 parity with solidity’s fixed point numbers, which also wrap “uint256” and don’t support negative numbers.

@dpaiton
Copy link
Contributor Author

dpaiton commented Mar 29, 2024

This makes sense to me. I agree with your assessment that the tech debt is acceptable. Let's delay for now.

After an initial survey of the problem, I think it would be relatively easy to make the conversion if you're willing to live with the loss of range (just go straight to signed). I'm not sure that we really need the uint range, so lets consider this when we pick the task back up. The amount of work increases significantly if we're going to keep the internal representation uint but still support signed operations. It's also worth noting that the present implementation of Hyperdrive and FixedPoint jump back-and-forth between i256 and u256 so I think in the end we don't have that big range anyways.

@dpaiton dpaiton removed their assignment Mar 29, 2024
@ryangoree ryangoree transferred this issue from delvtech/hyperdrive May 1, 2024
@dpaiton
Copy link
Contributor Author

dpaiton commented Oct 2, 2024

resolved by #186

@dpaiton dpaiton closed this as completed Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants