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

feat: swap adapter #281

Draft
wants to merge 26 commits into
base: master
Choose a base branch
from
Draft

feat: swap adapter #281

wants to merge 26 commits into from

Conversation

viatrix
Copy link
Contributor

@viatrix viatrix commented Oct 23, 2024

Add SwapAdapter (ETH-> USDC, USDC->ETH)

Description

Add SwapAdapter (ETH-> USDC, USDC->ETH). It uses Uniswap. Using it will also require getting the route for the swap (paths and fees) before submitting the transaction.

Related Issue Or Context

https://github.com/ChainSafe/sprinter-api/issues/250

closes https://github.com/ChainSafe/sprinter-contracts/issues/14
resolves https://github.com/ChainSafe/sprinter-contracts/issues/13

How Has This Been Tested? Testing details.

Unit tests under forked mainnet.
For these tests to pass, add USDC_OWNER_ADDRESS to your .env. The example address that can be used is specified in .env.example. This is a sample address of an owner of USDC tokens on mainnet.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation

Checklist:

  • I have commented my code, particularly in hard-to-understand areas.
  • I have ensured that all acceptance criteria (or expected behavior) from issue are met
  • I have updated the documentation locally and in chainbridge-docs.
  • I have added tests to cover my changes.
  • I have ensured that all the checks are passing and green, I've signed the CLA bot

contracts/adapters/SwapAdapter.sol Outdated Show resolved Hide resolved
contracts/adapters/SwapAdapter.sol Outdated Show resolved Hide resolved
@viatrix viatrix changed the title Swap adapter feat: swap adapter Oct 25, 2024
contracts/adapters/SwapAdapter.sol Outdated Show resolved Hide resolved
contracts/adapters/SwapAdapter.sol Show resolved Hide resolved
contracts/adapters/SwapAdapter.sol Show resolved Hide resolved
Copy link
Contributor

@lastperson lastperson left a comment

Choose a reason for hiding this comment

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

A few changes, and I guess we've settled to make it upgradeable?

contracts/adapters/SwapAdapter.sol Outdated Show resolved Hide resolved
contracts/adapters/SwapAdapter.sol Outdated Show resolved Hide resolved
contracts/adapters/SwapAdapter.sol Outdated Show resolved Hide resolved
contracts/adapters/SwapAdapter.sol Outdated Show resolved Hide resolved
contracts/adapters/SwapAdapter.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@lastperson lastperson left a comment

Choose a reason for hiding this comment

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

swapTokens() function could return the balanceAfter instead of amountIn now. Then calling functions could just check if it is positive to decide the transfer back to the user.

contracts/adapters/SwapAdapter.sol Outdated Show resolved Hide resolved
@lastperson
Copy link
Contributor

lastperson commented Nov 8, 2024

Does keeping approvals at 0 at the end of each tx saves gas? That is how it was done before the latest change.

Will have to check after migration to hardhat.

contracts/adapters/SwapAdapter.sol Outdated Show resolved Hide resolved
contracts/adapters/SwapAdapter.sol Outdated Show resolved Hide resolved
@mpetrunic
Copy link
Member

@MakMuftic Should we move this into new repo. sprinter-contracts where hardhat will be used together with multichain deploy tool?

@MakMuftic
Copy link
Contributor

I think that makes a lot of sense, @mpetrunic, as we talked.
I am not sure if it makes more sense to merge this one and then start working on adapters repo in separation?

I know we also talked about merging all adapters into one "UX adapter," which would be upgradable and essentially an entry point for many of these flows that require an adapter. Also, it would always live at the same address, which is very convenient. @viatrix @lastperson wdyt about this?

@lastperson
Copy link
Contributor

I'll check all the adapters we have to see which ones can be merged. For instance if any adapters are holding assets then they should be much stricter in terms of upgrading. And as always we need to discourage unlimited approvals.

@mpetrunic
Copy link
Member

I'll

We were talking on devcon, I think only native adapter should stay here

@MakMuftic
Copy link
Contributor

I think we should already start extracting this to a separate repository @lastperson @viatrix,
Created a new one 👇 (I am too tired today to figure out a better name 🫠)
https://github.com/sygmaprotocol/sprinter-solidity-periphery

Will add separate issues to the repo, but some of the things we definitely want to have:

  • repo to use hardhat and multichain deployment tool
  • unify all adapters into one
  • make this adapter upgredable

Please let me know what do you think or if you have some other ideas 🙏
cc: @mpetrunic

@lastperson
Copy link
Contributor

Starting right away. We will raise questions to discuss if there are any concerns.

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.

5 participants