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

Add templates it includes slippage param for uniswap. #23

Merged
merged 12 commits into from
Nov 11, 2023

Conversation

wshino
Copy link
Contributor

@wshino wshino commented Nov 7, 2023

  • Implement templates including slippage param
  • Add mocking unit testing

(bool success, , ,) = core.handleEmailOp(emailOp);
vm.stopPrank();

assertTrue(success, "emailOp failed");
Copy link
Member

Choose a reason for hiding this comment

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

I think a good way to test would be depositing exactly 0.2 WETH to the wallet, and then assert WETH value went to 0 and DAI value increased to 0.2*PRICE. Can we mock the uniswap methods return a fake price for WETH/DAI and check actual balances at the end of the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @saleel for you review🙏
I thought it would be nice to be able to do that as well. However, this is a process in ISwapRouter.interactInputSingle, so I could only mockCall it this time. Of course, integration testing can be used to fork the actual uniswap and see the value change.
If I find a better way, I will update the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saleel Or maybe we can use fork testing feature same as integration testing, but it might cause some slowdown.
@SoraSuegami What is your opinion?

Copy link
Member

Choose a reason for hiding this comment

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

I see ok, I think we can leave it like this in unit tests then and do proper checks in integration tests. I think its better not to make unit tests slow/complex.
Maybe you want to add one integration test with slippage/sqrt?

Copy link
Collaborator

@SoraSuegami SoraSuegami Nov 9, 2023

Choose a reason for hiding this comment

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

I think its better not to make unit tests slow/complex.

I agree with that point.
Please let me know if you need a new email from my email address for the new integration test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saleel @SoraSuegami Thank you for your reply,

@saleel Yes I'd like to implement a few integration testing with slippage/sqrt as you said.

@SoraSuegami Could you please new emails like below:

Swap 0.2 ETH to DAI with 0.5 slippage
Swap 200 DAI to USDC under 79244154773652573370267 sqrt price limit

@@ -23,7 +23,7 @@ contract UniswapExtension is Extension {

Copy link
Member

Choose a reason for hiding this comment

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

We can call this defaultSlippage? or defaultSlippagePoints

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saleel I think it's easier to understand. Let me update then.

@SoraSuegami
Copy link
Collaborator

@wshino
Thank you for your updates!
Could you split the word of "sqrt price limit" in the subject templates into separated values in the array, i.e., defining [..., "sqrt", "price", "limit"] instead of [..., "sqrt price limit"]?
It is not a critical issue but the template words should be split by space.

@SoraSuegami
Copy link
Collaborator

Thank you!
Please add your commits to the audit-fix branch as well.

@SoraSuegami SoraSuegami merged commit 23209ab into feat/v1 Nov 11, 2023
3 of 4 checks passed
@saleel saleel deleted the feat/slippage-templates-for-uniswap branch November 11, 2023 07:09
@saleel saleel restored the feat/slippage-templates-for-uniswap branch November 11, 2023 07:11
@saleel saleel deleted the feat/slippage-templates-for-uniswap branch November 11, 2023 07:14
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.

3 participants