-
Notifications
You must be signed in to change notification settings - Fork 302
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
Add Integral atomic relayer (Event driven version) #674
base: master
Are you sure you want to change the base?
Add Integral atomic relayer (Event driven version) #674
Conversation
Hi All, A few notes with this PR:
Since there could be some concern that these changes may impact the Uniswap integration, I've added comments to the PR with explanations of our changes.
|
@@ -23,7 +23,7 @@ export class Oracle { | |||
): OracleObservation { | |||
const delta = blockTimestamp - last.blockTimestamp; | |||
return { | |||
blockTimestamp: state.blockTimestamp, | |||
blockTimestamp, |
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.
Here we simply matched the .ts implementation with the .sol implementation: https://github.com/Uniswap/v3-core/blob/main/contracts/libraries/Oracle.sol#L39
You can see that new observations also update the blockTimestamp in the Solidity code.
Furthermore, since Uniswap's pricing does not depend on the oracle logic, you can be rest assured this won't negatively impact the Uniswap quoting mechanics.
@@ -93,12 +93,12 @@ export class Oracle { | |||
let beforeOrAt; | |||
let atOrAfter; | |||
while (true) { | |||
i = (l + r) / 2; | |||
i = Math.floor((l + r) / 2); |
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.
This is another bug fix to the original Uniswap .ts implementation. In Solidity, division is done on integers so there's implicitly a floor. We added floor manually to match the EVM execution: https://github.com/Uniswap/v3-core/blob/main/contracts/libraries/Oracle.sol#L164
|
||
beforeOrAt = state.observations[i % cardinality]; | ||
|
||
// we've landed on an uninitialized tick, keep searching higher (more recently) | ||
if (!beforeOrAt.initialized) { | ||
if (!beforeOrAt || !beforeOrAt.initialized) { |
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.
In Solidity, accessing a member of an undefined struct won't cause any issues since the values will be returned as 0 or empty. But in Javascript, accessing a memory on a undefined object will cause the system to crash. We simply added a check that the object should be defined before accessing its member here: https://github.com/Uniswap/v3-core/blob/main/contracts/libraries/Oracle.sol#L169
@@ -150,7 +150,8 @@ export class Oracle { | |||
} | |||
|
|||
beforeOrAt = state.observations[(index + 1) % cardinality]; | |||
if (!beforeOrAt.initialized) beforeOrAt = state.observations[0]; | |||
if (!beforeOrAt || !beforeOrAt.initialized) |
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.
The change here is the same as the one in 101: https://github.com/Uniswap/v3-core/blob/main/contracts/libraries/Oracle.sol#L223
No description provided.