-
Notifications
You must be signed in to change notification settings - Fork 13
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
Oracle price fetching refactoring #512
base: main
Are you sure you want to change the base?
Conversation
'ETH-A': { | ||
nextUnitPrice: '1712.2106886', | ||
currentUnitPrice: '1712.2106886', | ||
nextPriceChange: new Date('2022-09-09T10:00:00.000Z'), | ||
nextUnitPrice: '1602.1803800999999', | ||
currentUnitPrice: '1602.1803800999999', | ||
nextPriceChange: new Date('2022-09-14T13:00:00.000Z'), | ||
}, | ||
'ETH-B': { | ||
nextUnitPrice: '1602.1803800999999', | ||
currentUnitPrice: '1602.1803800999999', | ||
nextPriceChange: new Date('2022-09-14T13:00:00.000Z'), | ||
}, | ||
'ETH-C': { | ||
nextUnitPrice: '1208.0159951', | ||
currentUnitPrice: '1227.067888375', | ||
nextPriceChange: new Date('2022-06-13T11:00:00.000Z'), | ||
nextUnitPrice: '1602.1803800999999', | ||
currentUnitPrice: '1602.1803800999999', | ||
nextPriceChange: new Date('2022-09-14T13:00:00.000Z'), |
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.
@KirillDogadin-std Why was those values different? Those collateral types has the same oracle (confirmed via calling MCD_SPOT.ilks
), so it should return the same values. Moreover, executing just one test passes as expected:
npm run test -- --grep "Sound values are extracted"
Any idea on which of the tests introduces side effects/modifies them?
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.
within several minutes: i did not find the source of side effect:
- checked if we
poke
the oracle - does not seem so - checked if we overwrite the price - does not seem so
- checked if there's additional modifier for determination of unitPrice - does not seem source
I would have to look longer/deeper to determine the reason
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.
Thanks, I think it might be because something else is executed at the time of this test, block number changed and it somehow affects the numbers (but I still can't make sense of it). Will also look into it
core/src/constants/COLLATERALS.ts
Outdated
{ | ||
slotAddress: '0x3', | ||
format: ['uint128 isCurrentUnitPriceValid', 'uint128 currentUnitPrice'], | ||
}, | ||
{ | ||
slotAddress: '0x4', | ||
format: ['uint128 isNextUnitPriceValid', 'uint128 nextUnitPrice'], | ||
}, |
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.
@KirillDogadin-std What do you think about this format?
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.
just from looking at the selected code piece i did not figure out why you need format
/ for what purpose it's used.
Nothing against the datastructure, but perhaps a clearer key name can be an option?
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.
It's used by ABI coder. It's basically a type + variable name. See examples on how hex being decoded
here https://docs.ethers.io/v5/api/utils/abi/coder. Basically it allows us to define all those variables in unified format. But I might change it to better suit overwrite
functionality that haven't yet addressed
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.
if it remains, i would go with something like abiCoderFormat
Closes #513
Checklist:
#
)