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

Rex superswap 127 #129

Closed
wants to merge 20 commits into from

Conversation

gitChimp88
Copy link
Contributor

@gitChimp88 gitChimp88 commented Jul 13, 2022

(work in progress)
Port uniswap contracts so we can use solidity 0.8
Create RexSuperSwap contract
Create test for contract

Bring RexSuperSwap contract in to this repo to begin with
Add test for RexSuperSwap to test deployment, Install uniswap sdk - There is an error for incompatible solidity versions between dependency and RexSuperSwap.
Port uniswap contracts that we need to use for super swap, add some more test boilerplate
Test passing for swap functionality, need to test balances and different tokens.
Add check for account balances in test
Add call to coingecko api to get minimum amount out of swap
Add event to indicate swap just made
@gitChimp88 gitChimp88 marked this pull request as draft July 13, 2022 13:40
fork from latest block number, change swap to be between matic and usd
// Approve the router to spend token supplied (fromBase).
TransferHelper.safeApprove(fromBase, address(swapRouter), amountIn);

console.log("balance of Maticx before swap - ", _from.balanceOf(address(this)));
Copy link
Contributor

Choose a reason for hiding this comment

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

This will come out to be 0 because you downgraded MATICx to native MATIC on line 59, to get the balance of the matic you have, do:

console.log("balance of MATIC before swap - ", address(this).balance);

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 for the tip, now changed

// Execute the swap
amountOut = swapRouter.exactInput(params);

console.log("balance of Maticx after swap - ", _from.balanceOf(address(this)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, after the swap, you won't have MATICx, you're going to have native MATIC, check balance with address(this).balance

(actually, not sure how uniswap v3 behaves, it might give you wrapped matic in which case youll check the balance as:

ERC20(_from.getUnderlyingToken()).balanceOf(address(this))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed


console.log("balance of Maticx after swap - ", _from.balanceOf(address(this)));

emit SwapJustMade(amountOut);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is needed since Uniswap v3 emits events, remove this event

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now removed


// Step 1: Get underlying tokens and verify path
address fromBase = _from.getUnderlyingToken();
address toBase = _to.getUnderlyingToken();
Copy link
Contributor

Choose a reason for hiding this comment

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

You're missing code here to handle native supertokens, see this code: https://github.com/Ricochet-Exchange/ricochet-protocol/blob/v2/contracts/REXTwoWayMarket.sol#L396

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay i've now added this check

console.log("starting balance of Maticx - ", _from.balanceOf(address(this)));

// Step 3: Downgrade
_from.downgrade(amountIn);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, you're not handling the native supertoken case where the downgrades not needed, swapping RIC to USDCx for example, RIC doens't need to be downgraded, see the code I link to in the comment above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now handling correctly


// Step 5: Upgrade and send tokens back
TransferHelper.safeApprove(address(toBase), address(_to), amountOut);
_to.upgrade(amountOut);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here with the upgrade, you have to account for native supertokens, like if they're swapping USDCx to RIC, no upgrade required on RIC, see this code snippet: https://github.com/Ricochet-Exchange/ricochet-protocol/blob/v2/contracts/REXTwoWayMarket.sol#L443,

Copy link
Contributor

Choose a reason for hiding this comment

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

You also need to check here if the _to token is MATICx, if so, you have to use upgradeByETH method, see the code snippet here: https://github.com/Ricochet-Exchange/ricochet-protocol/blob/v2/contracts/REXTwoWayMarket.sol#L434

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great thanks, now handling correctly and added the check for MATICX

Copy link
Contributor

@mikeghen mikeghen left a comment

Choose a reason for hiding this comment

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

see comments, I don't think this is set up right to handle MATICx, which downgrades to native MATIC and needs to be upgraded with upgradeByETH, also this doesn't handle native supertokens, see my comments

Logic added for native tokens downgrade and upgrade, also upgrade of maticx
add approve function for unlimited approvals rather than decimal conversions, has underlying to and from variables to deal with native tokens, add test for native token
@gitChimp88 gitChimp88 marked this pull request as ready for review August 10, 2022 15:16
@gitChimp88 gitChimp88 requested a review from mikeghen August 10, 2022 15:16
contracts/superswap/RexSuperSwap.sol Outdated Show resolved Hide resolved
@shreyaspapi
Copy link
Collaborator

@gitChimp88 I pushed the local changes I had for Ricochet-Exchange/ricochet-super-swap#2

@gitChimp88 gitChimp88 requested review from shreyaspapi and removed request for mikeghen August 26, 2022 14:15
Copy link
Collaborator

@shreyaspapi shreyaspapi left a comment

Choose a reason for hiding this comment

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

Cool this looks great! Just one lil change + Can you also add a test for RIC -> MaticX

contracts/superswap/RexSuperSwap.sol Outdated Show resolved Hide resolved
contracts/superswap/RexSuperSwap.sol Outdated Show resolved Hide resolved
@gitChimp88
Copy link
Contributor Author

Cool this looks great! Just one lil change + Can you also add a test for RIC -> MaticX

I added the test but it fails at the moment because there's no liquidity on uniswap RIC -> Matic

@gitChimp88 gitChimp88 requested a review from shreyaspapi August 29, 2022 12:16
Copy link
Collaborator

@shreyaspapi shreyaspapi left a comment

Choose a reason for hiding this comment

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

Tests looks good to me, we need to test RIC -> MATICx @mikeghen, Also can you remove the unwanted comments in the test file @gitChimp88 .

hardhat.config.ts Outdated Show resolved Hide resolved
@mikeghen
Copy link
Contributor

@gitChimp88 I just made some liquidity for RIC-MATIC on uniswap


// Upgrade if it's not a native SuperToken
if (_hasUnderlyingTo) {
if (address(_to) == superNativeToken) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@gitChimp88 We don't need this condition, here I wrongly assumed that Super Matic has underlaying token as 0x (native token). As a result the Matic <> Ric test might be failing.

Delete unneccessary if statement and different upgrade method as matic is downgraded to wrapped matic, specified specific block (which has the required liquidity for RIC)
emit ErrorOnSwap(reason);
} catch (bytes memory returnData) {
emit ReturnDataEvent(returnData);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a problem here, if the swap reverts the contract will eat the input tokens and not return them to the user, I would just not use a try catch here at all, if swapRouter.exactInput(params) reverts, revert the whole thing

Withdraw function to convert wmatic to matic so we can upgrade to super token
@shreyaspapi
Copy link
Collaborator

Closes #127

@mikeghen
Copy link
Contributor

@gitChimp88 @shreyaspapi, hows this going? I took a look at the last commit, it looks like a test still needs to done. Lmk how I can help you.

@mikeghen mikeghen closed this Sep 29, 2022
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