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

Fix v6.0.1 #310

Merged
merged 13 commits into from
Jul 19, 2024
Merged

Fix v6.0.1 #310

merged 13 commits into from
Jul 19, 2024

Conversation

alex0207s
Copy link
Contributor

@alex0207s alex0207s commented Apr 30, 2024

  • fix typo in RFQ contract
  • add a parameter (salt) to Swap event in GenericSwap contract
  • change handling logic for makerToken as ETH in RFQ contract
  • change the input token ratio calculation method in SmartOrderStrategy contract
  • remvoe uniAgent.sol contract

Why do we need to adjust the handling logic for makerToken as ETH in the RFQ contract?

Prior to this PR, when makerToken is ETH, RFQ directly unwraps WETH into ETH. This isn't ideal, especially when RFQ is only a segment of the entire swap path. If RFQ unwraps WETH into ETH directly, it requires the subsequent swapping process to wrap ETH back into WETH, incurring additional steps and costs.

With this PR, we don't unwrap WETH directly. WETH is only unwrapped to ETH when the makerToken is ETH. This could avoid additional steps or costs associated with handling WETH.

Why do we need to adjust the calculation method for a input token ratio?

Prior to this PR, the ratio value was expressed in ten-thousandths (0.01%), which was not precise enough for some specific protocols(e.g. dsspsm).

With this PR, the ratio is now determined by both the numerator and the denominator, allowing for greater flexibility and accuracy in expressing the ratio.

Why do we remove uniAgent.sol?

Due to a backend integration issue, the current version of uniAgent.sol is not sufficient to replace GenericSwap.sol and SmartOrderStrategy.sol when the swap only occurs in the Uniswap pool.

@alex0207s alex0207s requested a review from 108356037 April 30, 2024 09:37
@NIC619
Copy link
Contributor

NIC619 commented May 2, 2024

fix handling logic for makerToken as ETH in RFQ contract

This does not feel like a "fix". This is changing how we treat WETH as maker token:

  • before this PR: ETH == WETH
    • No ETH as maker token allowed; WETH as maker token means ETH as maker token, we converts it to ETH and send it to recipient
  • after this PR: ETH != WETH
    • WETH as maker token is the same as other ERC20 tokens, we will not convert it to ETH anymore; ETH as maker token means we will convert WETH to ETH and send it to recipient

@NIC619
Copy link
Contributor

NIC619 commented May 2, 2024

Please add a readme or a natspec/comment for how we handle different maker tokens, to help others better understand how it works.

In RFQ,
- if maker token is ETH, ...
- if maker token is WETH, ...
- if maker token is normal ERC20, ...

foundry.toml Outdated Show resolved Hide resolved
contracts/RFQ.sol Outdated Show resolved Hide resolved
contracts/RFQ.sol Outdated Show resolved Hide resolved
contracts/RFQ.sol Outdated Show resolved Hide resolved
NIC619
NIC619 previously approved these changes May 6, 2024
contracts/interfaces/ISmartOrderStrategy.sol Show resolved Hide resolved
contracts/interfaces/ISmartOrderStrategy.sol Outdated Show resolved Hide resolved
contracts/SmartOrderStrategy.sol Outdated Show resolved Hide resolved
test/forkMainnet/SmartOrderStrategy/AMMs.t.sol Outdated Show resolved Hide resolved
test/forkMainnet/SmartOrderStrategy/AMMs.t.sol Outdated Show resolved Hide resolved
contracts/SmartOrderStrategy.sol Outdated Show resolved Hide resolved
@alex0207s alex0207s merged commit 3452939 into master Jul 19, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants