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

Feature/0x V4 #44

Merged
merged 8 commits into from
Mar 15, 2024
Merged

Feature/0x V4 #44

merged 8 commits into from
Mar 15, 2024

Conversation

hubagaspar91
Copy link
Contributor

Protocol Information

Checklist:

  • Added export ./{protocolId} in src/logics/index.ts
  • Exported all Logics in src/logics/{protocolId}/index.ts
  • Ensured that all Logics have unit tests
  • Ensured that all Logics have integration tests

Additional Notes

The PR adds support for 0x swaps. Since 0x quotes are obtainable from a paid API, an API key has to be included when getting a quote or building logics.

Tests need a Github secret ZEROEX_API_KEY to be defined and contain a valid 0x API key.

I also reused the token url lists from the paraswap logic by extracting them to utils/tokens.

@zodahu zodahu requested review from Jeff-CCH and zodahu March 11, 2024 04:21
.gitignore Outdated Show resolved Hide resolved
src/logics/zeroex-v4/logic.swap-token.ts Outdated Show resolved Hide resolved
.github/workflows/e2e-test-mainnet-pb.yml Outdated Show resolved Hide resolved
src/logics/zeroex-v4/logic.swap-token.ts Outdated Show resolved Hide resolved
src/logics/zeroex-v4/logic.swap-token.ts Outdated Show resolved Hide resolved
src/logics/zeroex-v4/logic.swap-token.ts Show resolved Hide resolved
src/utils/tokens.test.ts Outdated Show resolved Hide resolved
src/utils/tokens.ts Outdated Show resolved Hide resolved
@zodahu
Copy link
Contributor

zodahu commented Mar 13, 2024

@hubagaspar91 Thank you for submitting this PR integrating 0x. Feel free to share your thoughts on the comments above!

…ase its wrapping or unwrapping WETH, added a check to getTokenList to check whether an address is a valid ETH address, as there was an error with one of the responses containing an invalid ETH address
@hubagaspar91
Copy link
Contributor Author

hubagaspar91 commented Mar 13, 2024

Implemented all the changes you guys requested.

Most changes were straightforward, I have however though about a reasonable behaviour for the quote, after changing it to the /price endpoint as requested.

The 0x price endpoint that I am using now for quote does not take a slippagePercentage param, but returns a expectedSlippage field in the following format: "-0.00003836285924298198283753149243586624". I return this slippage from the quote endpoint, rounding it with Math.ceil to integer bps, and minimizing its value in 50 bps, to make the quotes more resilient to price and liquidity changes. Let me know if this sounds okay to you, or if you'd prefer to handle this in some other way. I was also thinking, it might make sense to give a buffer to the slippage returned by the price API, even if it's over 50 bps for the same reason (for instance, by multiplying it by some arbitrary constant).

Actually, please disregard the above, there are 2 price endpoints, and the one we are using works the same as quote, so none of the above logic is needed, and slippage can be handled as usual. Made the changes in a new commit.

Also added two extra fields, which we will be using for our integration, takerAddress and skipValidation.

Also, I am not sure if you already handle this somehow somewhere, or this is the correct way to handle it, but I included logic that sets the approveTo address to undefined if it is a WETH->ETH or ETH->WETH swap, since in those cases, the swap is not routed through 0x but the tx data returned is a simple deposit/withdraw on the WETH contract that needs no approval.

@zodahu
Copy link
Contributor

zodahu commented Mar 14, 2024

Implemented all the changes you guys requested.

Most changes were straightforward, I have however though about a reasonable behaviour for the quote, after changing it to the /price endpoint as requested.

The 0x price endpoint that I am using now for quote does not take a slippagePercentage param, but returns a expectedSlippage field in the following format: "-0.00003836285924298198283753149243586624". I return this slippage from the quote endpoint, rounding it with Math.ceil to integer bps, and minimizing its value in 50 bps, to make the quotes more resilient to price and liquidity changes. Let me know if this sounds okay to you, or if you'd prefer to handle this in some other way. I was also thinking, it might make sense to give a buffer to the slippage returned by the price API, even if it's over 50 bps for the same reason (for instance, by multiplying it with some arbitrary constant).

I think this approach could also work, considering that the 0x response provides expectedSlippage. However, it seems that in the latest commit, expectedSlippage is no longer used. Instead, clients can provides slippage to the quote function. Is there a reason for this change?

@hubagaspar91
Copy link
Contributor Author

Implemented all the changes you guys requested.

Most changes were straightforward, I have however though about a reasonable behaviour for the quote, after changing it to the /price endpoint as requested.

The 0x price endpoint that I am using now for quote does not take a slippagePercentage param, but returns a expectedSlippage field in the following format: "-0.00003836285924298198283753149243586624". I return this slippage from the quote endpoint, rounding it with Math.ceil to integer bps, and minimizing its value in 50 bps, to make the quotes more resilient to price and liquidity changes. Let me know if this sounds okay to you, or if you'd prefer to handle this in some other way. I was also thinking, it might make sense to give a buffer to the slippage returned by the price API, even if it's over 50 bps for the same reason (for instance, by multiplying it with some arbitrary constant).

I think this approach could also work, considering that the 0x response provides expectedSlippage. However, it seems that in the latest commit, expectedSlippage is no longer used. Instead, clients can provides slippage to the quote function. Is there a reason for this change?

Yes, I only went for the previous solution as I got mixed up in the docs as there is also another price endpoint which does not take a slippagePercentage param.

Generally, in think this latest approach is better and more in line with how swap UIs usually work, that is, the slippage value is provided as preset/input and is validated against during execution.

@zodahu
Copy link
Contributor

zodahu commented Mar 14, 2024

Generally, in think this latest approach is better and more in line with how swap UIs usually work, that is, the slippage value is provided as preset/input and is validated against during execution.

Understood, the latest approach looks good.

@zodahu
Copy link
Contributor

zodahu commented Mar 14, 2024

Also, I am not sure if you already handle this somehow somewhere, or this is the correct way to handle it, but I included logic that sets the approveTo address to undefined if it is a WETH->ETH or ETH->WETH swap, since in those cases, the swap is not routed through 0x but the tx data returned is a simple deposit/withdraw on the WETH contract that needs no approval.

I just noticed the comment update.

I'm thinking that approveTo could directly utilize the allowanceTarget from the /quote response, since the default value for approveTo is also constants.AddressZero, which matches the value returned by the 0x API. This could simplify the process by eliminating the need to recognize wrapp and unwrap cases.

Ref: https://0x.org/docs/0x-swap-api/api-references/get-swap-v1-quote

allowanceTarget: The target contract address for which the user needs to have an allowance in order to be able to complete the swap. For swaps with "ETH" as sellToken, wrapping "ETH" to "WETH" or unwrapping "WETH" to "ETH" no allowance is needed, a null address of 0x0000000000000000000000000000000000000000 is then returned instead.

@hubagaspar91
Copy link
Contributor Author

Also, I am not sure if you already handle this somehow somewhere, or this is the correct way to handle it, but I included logic that sets the approveTo address to undefined if it is a WETH->ETH or ETH->WETH swap, since in those cases, the swap is not routed through 0x but the tx data returned is a simple deposit/withdraw on the WETH contract that needs no approval.

I just noticed the comment update.

I'm thinking that approveTo could directly utilize the allowanceTarget from the /quote response, since the default value for approveTo is also constants.AddressZero, which matches the value returned by the 0x API. This could simplify the process by eliminating the need to recognize wrapp and unwrap cases.

Ref: https://0x.org/docs/0x-swap-api/api-references/get-swap-v1-quote


allowanceTarget: The target contract address for which the user needs to have an allowance in order to be able to complete the swap. For swaps with "ETH" as sellToken, wrapping "ETH" to "WETH" or unwrapping "WETH" to "ETH" no allowance is needed, a null address of 0x0000000000000000000000000000000000000000 is then returned instead.

Yes this is also a great suggestion, will implement this too.

@hubagaspar91
Copy link
Contributor Author

hubagaspar91 commented Mar 14, 2024

@zodahu Implemented your above 2 suggested improvements in my latest commits. Added the same logic for calculating the agent for the takerAddress provided as a param for the quote method too, so it will be more straightforward to integrate quotes that rely on RFQ liquidity.

@zodahu
Copy link
Contributor

zodahu commented Mar 15, 2024

@hubagaspar91 LGTM! Thank you for this PR. We'll integrate it into the Protocolink API next week.

@zodahu zodahu merged commit e741ac8 into dinngo:master Mar 15, 2024
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