Skip to content

rETH#31

Open
julienmartinlevrai wants to merge 32 commits intomasterfrom
reth
Open

rETH#31
julienmartinlevrai wants to merge 32 commits intomasterfrom
reth

Conversation

@julienmartinlevrai
Copy link
Contributor

@julienmartinlevrai julienmartinlevrai commented Jun 23, 2022

Set to be merged after #30

@julienmartinlevrai julienmartinlevrai self-assigned this Jun 23, 2022
@julienmartinlevrai julienmartinlevrai marked this pull request as ready for review July 7, 2022 12:04
}

function testTakeLpDaiEthNoProfit() public {
function testTakeLpDaiEthNoProfit() private { // most LP tokens are getting offboarded
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note that we have these tests replaced in a pending PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You did a really great job of replacing these tests.

@julienmartinlevrai
Copy link
Contributor Author

@talbaneth thank you for your review. I will be addressing it shortly.

Copy link
Contributor

@talbaneth talbaneth left a comment

Choose a reason for hiding this comment

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

Changes ok, tests pass.

LGTM for deploying.
Don't forget to add the address to addresses.json.

Copy link
Contributor

@godsflaw godsflaw left a comment

Choose a reason for hiding this comment

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

LGTM

@talbaneth
Copy link
Contributor

I see test_bigAmtWithComplexPath is failing.
Current reth market price is 1,337.

The test is trying to take 3K reth at price of 970 reth/dai with greater than zero profit. Was still failing for 2K but succeeded at 1.5K reth.
Then I tried at 1172 reth/dai (~10% lower than current reth price), failed at 1.5K, suceeded at 1K.
So seems like the liquidity is not great, but still good enough to backstop.
For now I'll change the test to use 1K reth.

@talbaneth
Copy link
Contributor

@julienmartinlevrai @godsflaw please check and approve to merge

Copy link
Contributor Author

@julienmartinlevrai julienmartinlevrai left a comment

Choose a reason for hiding this comment

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

  • contract deployed at mainnet address 0x7cdAb0fE16efb1EFE89e53B141347D7F299d6610
  • contract code verified on Etherscan
  • verified code matches the code of this PR
  • addresses in constructor parameters correspond to the right contracts
  • no libraries
  • this contract does not use any type of auth

LGTM (I can’t approve my own PR though)

Copy link
Contributor Author

@julienmartinlevrai julienmartinlevrai left a comment

Choose a reason for hiding this comment

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

Also, tests pass.

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