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

fix: Adds scaling function to _readTwap #30

Merged
merged 1 commit into from
Aug 16, 2024
Merged

Conversation

jar-o
Copy link
Collaborator

@jar-o jar-o commented Jul 29, 2024

The _readTwap() function was not performing the proper scaling on the values it received from Uniswap. Aggor uses 8 decimals, and all sources, Chronicles, Chainlink, and Uniswap must conform from their defaults (which all vary) to match. Scaling is a pretty simple and normal pattern in Solidity, and we are employing it elsewhere in Aggor. It simply got missed/lost on the _readTwap() function.

Copy link

This pull request has been linked to Shortcut Story #7267: Aggor deploy by August 2nd.

@jar-o jar-o force-pushed the sc-7267-aggor-scale-patch branch 2 times, most recently from 27a080e to 67151b8 Compare July 29, 2024 17:02
Copy link

@hexonaut hexonaut left a comment

Choose a reason for hiding this comment

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

Fix looks good, but would like to see a test specifically added that would have failed under the old setup. IE it would have explicitly, incorrectly picked the lower one always even if the high oracle is reporting something way off.

Also, these are code changes and need to be re-audited.

@jar-o jar-o force-pushed the sc-7267-aggor-scale-patch branch from 395f72c to b80a638 Compare July 31, 2024 13:54
@jar-o jar-o changed the title Adds scaling function to _readTwap fix: Adds scaling function to _readTwap Aug 16, 2024
@jar-o jar-o merged commit b28a3fa into main Aug 16, 2024
3 checks passed
@pmerkleplant pmerkleplant deleted the sc-7267-aggor-scale-patch branch August 16, 2024 15:46
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