search

joshstevens19 / simple-uniswap-sdk

Uniswap SDK which handles the routes automatically for you, changes in trade quotes reactive subscriptions, exposure to formatted easy to understand information, bringing back the best trade quotes automatically, generating transactions for you and much more.
MIT License
190 stars 97 forks source link

Allow to override tradePath #9

Closed mathijs81 closed 3 years ago

mathijs81 commented 3 years ago

I'm trying to execute trades from WETH -> other tokens, and I really want to use the WETH that the account already owns. It seems that the automatic getTradePath then always makes the transaction between unwrapped ETH -> token. For my purpose it would be great if the tokenpair (or createFactory) has an optional parameter to force the tradepath to stay erc20ToErc20 even though one of the tokens is WETH.

joshstevens19 commented 3 years ago

so you want to be able to swap WETH > TOKEN without using the native eth when you do a swap? aka we treat WETH as a erc20 token (as it is one) and do the swap token call as swapExactTokensForTokens instead of the swapETHForExactTokens ? is that what your asking for?

joshstevens19 commented 3 years ago

Also if you were doing TOKEN > WETH you want to do the same as above with this same flag?

mathijs81 commented 3 years ago

yes exactly. I now have managed to as a workaround deliberately specify the wrong ChainId, so that the actual WETH contract address is not recognized as such and the swaps are done between the tokens only, and this is exactly what I want.

joshstevens19 commented 3 years ago

Great request, il get this done and deployed for you when Im back on my computer in 20. I be a flag as you said on the uniswap pair context I’m going to name it to something bespoke to weth as this is the only case I can see rather then overriding routes. Il let you know when I’ve done those changes.

joshstevens19 commented 3 years ago

FILE DIDNT ATTACH

Could you download this and install it on your project you now have a setting on UniswapPairSettings called useWETHAsERC20Route aka

 settings: new UniswapPairSettings({
      // if not supplied it use `0.005` which is 0.5%;
      // all figures
      slippage: 0.005,
      // if not supplied it will use 20 a deadline minutes
      deadlineMinutes: 20,
      disableMultihops: false,
      useWETHAsERC20Route: true,
      uniswapVersions: [UniswapVersion.v2, UniswapVersion.v3],
    }),

my quotes are working but I want you to do a full test aka doing a swap and everything for me as I don't have a setup that you are requesting. Let me know thanks

joshstevens19 commented 3 years ago

sorry github does not support .tgz files I pushed it to master so you can get to the tgz file that locally packaged build should have that change I said above on

https://github.com/uniswap-integration/simple-uniswap-sdk/blob/master/simple-uniswap-sdk-2.5.0.tgz

mathijs81 commented 3 years ago

It works, even without setting any different setting (e.g. the new default is to not replace WETH by 'native' ETH, is that correct)?

joshstevens19 commented 3 years ago

Oh it should only work if you set it to true. I must have a if the wrong way around or a default. The default should be how it is now and you have to define that setting for it to do what you have requested. But if it works and does the swap il check the code tomorrow to see where I’ve gone wrong and deploy it. You checked this without your override on the chain right? Can you toggle it from true and false to see if it acts differently. Il get this sorted tomorrow morning.

mathijs81 commented 3 years ago

I have my chain overridden to 1 now so everything looks like mainnet. I can't get the 'old' behavior at all anymore. Not setting the parameter, setting it to true or false ==> everything makes the trade start or end at WETH?

joshstevens19 commented 3 years ago

Kk will be able to test this out tomorrow. Main thing was that the trades work and you said that does work right? I can fix the other parts. Il do it tomorrow and send you another package for you to test to give me the all clear. 👍

Z-Hau commented 3 years ago

Hi, I was going to request this feature as pointed out by @mathijs81 too, would love to have this feature.

So I assume that after this implementation, if we want to swap from WETH(ERC20 token) > TOKEN, we just need to set "useWETHAsERC20Route: true".

While if we want to swap from unwrapped ETH > TOKEN , we just set "useWETHAsERC20Route: false"? Thank you.

P.S. Hope it works for when doing TOKEN > WETH too.

joshstevens19 commented 3 years ago

Hi yes that correct. By default it will use the native approach but if you set this flag both WETH > TOKEN and TOKEN > WETH will be a token to token exchange. Will get this done today 👍

joshstevens19 commented 3 years ago

had the config the wrong way, i updated the locally packaged tgz file again its here

https://github.com/uniswap-integration/simple-uniswap-sdk/blob/master/simple-uniswap-sdk-2.5.0.tgz

can you redownload and let me know it works for you please.

@mathijs81

joshstevens19 commented 3 years ago

also, another question would it be nicer when you did native trade quotes instead of calling it WETH on the end and out we called it ETH what I mean by this is:

you have useWETHAsERC20Route as false:

  • swapping ETH > UNI > routeText = 'ETH > USDC > USDT > UNI'
  • swapping UNI > ETH > routText = 'UNI > USDC > USDT > ETH'
  • swapping UNI > AAVE > routeText = 'UNI > WETH > USDT > AAVE' <<note this one keeps WETH in the middle routes

you have useWETHAsERC20Route as true:

  • swapping ETH > UNI > routeText = 'WETH > USDC > USDT > UNI'
  • swapping UNI > ETH > routText = 'UNI > USDC > USDT > WETH'
  • swapping UNI > AAVE > routeText = 'UNI > WETH > USDT > AAVE'

This is on the stuff in the response like routeText

Be interesting to see your thoughts on that as people who are asking for this feature @mathijs81 @Z-Hau

Z-Hau commented 3 years ago

also, another question would it be nicer when you did native trade quotes instead of calling it WETH on the end and out we called it ETH what I mean by this is:

you have useWETHAsERC20Route as false:

  • swapping ETH > UNI > routeText = 'ETH > USDC > USDT > UNI'
  • swapping UNI > ETH > routText = 'UNI > USDC > USDT > ETH'
  • swapping UNI > AAVE > routeText = 'UNI > WETH > USDT > AAVE' <<note this one keeps WETH in the middle routes

you have useWETHAsERC20Route as true:

  • swapping ETH > UNI > routeText = 'WETH > USDC > USDT > UNI'
  • swapping UNI > ETH > routText = 'UNI > USDC > USDT > WETH'
  • swapping UNI > AAVE > routeText = 'UNI > WETH > USDT > AAVE'

This is on the stuff in the response like routeText

Be interesting to see your thoughts on that as people who are asking for this feature @mathijs81 @Z-Hau

If I understand you correctly, do you mean that user will only have to use "ETH" as the input while the actual route will be determined by the flag "useWETHAsERC20Route", instead of having to use "ETH" or "WETH" in conjunction with the flag for different scenario? Sorry if I misunderstood your question.

joshstevens19 commented 3 years ago

no, i'm merely asking that on some of the objects we return we give some metadata like the route path in text like the above. Right now it will always say WETH even if it's going to convert to native because when you swap it is WETH but that uniswap toggle that to ETH for you before it finishes. This is purely for context in the object!

The user experience for the developer will be the same as described above this was purely to see if I should fix up some of these readonly properties that some dev use to show on the UI.

mathijs81 commented 3 years ago

I have no real use for the route path at this moment, so I can't give you feedback on this.

Purely from a development perspective, I think it would be cleaner to have ETH be represented more like the other tokens as well. I looked around a bit, and discovered that a number of projects are using 0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee as a 'fake' contract address for native ETH, see e.g.: https://docs.aave.com/developers/v/1.0/deployed-contracts/deployed-contract-instances#reserves-assets and https://www.npmjs.com/package/paraswap#user-content-to-get-the-rate-of-a-token-pair-using-the-api as examples

If it would be possible to implement that in this SDK too, then this new flag doesn't have to be there anymore, because the user can specify what he wants directly because the WETH address is not representing native ETH anymore?

Z-Hau commented 3 years ago

Alright, got it. During debugging, I was definitely confused by the fact that the routeText, fromToken/toToken, fromBalance are showing WETH although ETH is selected. But as mentioned by @mathijs81, no real use for my case too. But definitely is helpful or less confusing for debugging.

joshstevens19 commented 3 years ago

@mathijs81 I think that end up becoming more confusing for first-time starters with this SDK. I am happy with this flag hidden away for more advanced users. I will look into making it so if you are doing native the object response is not confusing now we have this flag. Thanks both for feedback!

can you confirm if that new package works please? @mathijs81

joshstevens19 commented 3 years ago

i posted the new package comment here - https://github.com/uniswap-integration/simple-uniswap-sdk/issues/9#issuecomment-872820020

mathijs81 commented 3 years ago

Yes it works great, like the old way without the new setting and the way I requested with useWETHAsERC20Route: true.

joshstevens19 commented 3 years ago

perfect will get this package published in a hour or so will put the version number in here when done and close it then.

joshstevens19 commented 3 years ago

Hey this is release on version 2.5.0 thanks for the feature request! 👍

joshstevens19 commented 3 years ago

Hey, guys just a heads up tomorrow I'm going to look at a nicer way to handle useWETHAsERC20Route flag, I want to create a NATIVE_ETH property you pass in instead of the WETH logic (similar to what @mathijs81) was suggesting, if you want native and if you want WETH you pass in WETH contract address, this way it's a very clear interface for devs to know how to make it work with both native + WETH.

I think I will also remove this flag as this approach seems a bit backwards (and confusing) once we clear up the way you work with native and weth in the sdk.

This is mainly due to the ability to swap WETH > ETH and the other way around, right now the library with the current approach does not support that logic, which it should!

This will also clear up the confusing @mathijs81 talked about where you get WETH balances in ETH and the other way round. So just a heads up if you do see it break on an npm install circle back to this thread. I will include the breaking changing details here and bump it to 3.0.0 once it's done!

Due to the way the docs suggest people use it aka using WETH.. I will probably depreciate WETH static class and make it point to NATIVE_ETH and then create a new WETH_CONTRACT class, I have not worked out full details yet but wanted to let you guys know.

Thanks

joshstevens19 commented 3 years ago

Breaking change in > https://github.com/uniswap-integration/simple-uniswap-sdk/releases/tag/3.0.0 think this approach works much nicer! thanks.

Z-Hau commented 3 years ago

@joshstevens19 I was trying to do ETH > WETH but I got the error "No routes found for ETH > WETH".

joshstevens19 commented 3 years ago

you got direct routes turned on? right now sdk doesn't support it to go directly it has to go to another route.. (aka ETH > USDT > WETH) that's on the to-do list as it needs a different contract call to wrap it

Z-Hau commented 3 years ago

You mean "disableMultihops" ? Nope, this is using the default value of false.

joshstevens19 commented 3 years ago

yes.. erm ok mine seems to be doing it fine

image

can you show me your code?

joshstevens19 commented 3 years ago

wait you installed the 3.0.0 sdk? or were you talking about an older version. You need the 3.0.0 sdk for that to come back with any routes.

Z-Hau commented 3 years ago

Yup, using the 3.0.0 sdk. Let me try again.

Z-Hau commented 3 years ago

image

Z-Hau commented 3 years ago

Here is my code. It stops before the "console.log(trade)".

image

Z-Hau commented 3 years ago

Forgot to mention that I am currently on Ropsten. I just tried it out on Mainnet and it works.

joshstevens19 commented 3 years ago

yes so any testnets default to direct routes it doesn't try to go down many routes.. (aka basically it uses disableMultihops in the background on anything but MAINNET) and as right now you cant do ETH > WETH directly it say that! As said its on list for direct swap just not had time to relook yet

joshstevens19 commented 3 years ago

You can raise a ticket which will probably get more priority as il see it a lot more haha!

Z-Hau commented 3 years ago

Sure, will do so. Thanks !