Skip to content
This repository was archived by the owner on Feb 25, 2023. It is now read-only.

Conversation

@vic-en
Copy link
Contributor

@vic-en vic-en commented Nov 16, 2020

No description provided.

const tokenAmountIn = new uni.TokenAmount(tIn, amountIn)
const result = uni.Router.swapCallParameters(
uni.Trade.exactIn(route, tokenAmountIn),
{ ttl: 50, recipient: wallet.address, allowedSlippage: new uni.Percent('1', '100') }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would be better values for the ttl (time to live) and the "allowedSlippage"?

Copy link
Contributor

Choose a reason for hiding this comment

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

ttl:Uniswap defines this as "how long the swap is valid until it expires, in seconds." I think we should make ttl an constant equal to 0 that is specified in uniswap.js.

allowedSlippage: I think we should make this a constant equal to 0 in uniswap.js as well. Since we set slippage buffer through the Hummingbot client, I don't think we need to add more slippage buffer here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There shouldn't be problem with setting allowedSloppage to 0.
However, I believe there would be a problem with setting ttl to 0. I believe the swap wouldn't be executed. I'd test to confirm this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry that was a typo - for ttl I think a value like 60 seconds would be a good initial value.

Copy link
Contributor

@fengtality fengtality left a comment

Choose a reason for hiding this comment

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

See requested changes - let's discuss the price one during 1:1 tmrw

const tokenAmountIn = new uni.TokenAmount(tIn, amountIn)
const result = uni.Router.swapCallParameters(
uni.Trade.exactIn(route, tokenAmountIn),
{ ttl: 50, recipient: wallet.address, allowedSlippage: new uni.Percent('1', '100') }
Copy link
Contributor

Choose a reason for hiding this comment

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

ttl:Uniswap defines this as "how long the swap is valid until it expires, in seconds." I think we should make ttl an constant equal to 0 that is specified in uniswap.js.

allowedSlippage: I think we should make this a constant equal to 0 in uniswap.js as well. Since we set slippage buffer through the Hummingbot client, I don't think we need to add more slippage buffer here.

@vic-en vic-en requested a review from fengtality November 19, 2020 13:02
// network defaults to KOVAN
const providerUrl = process.env.ETHEREUM_RPC_URL
this.network = process.env.ETHEREUM_CHAIN
this.chainID = uni.ChainId.KOVAN
Copy link
Contributor

Choose a reason for hiding this comment

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

This line isn't needed since it's defined below

var tOut = await uni.Fetcher.fetchTokenData(this.chainID, tokenOut)

try {
pair = await uni.Fetcher.fetchPairData(tIn, tOut)
Copy link
Contributor

Choose a reason for hiding this comment

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

indent 2 spaces

route = new uni.Route([pair], tIn, tOut)
}
catch(err){
console.log('Trying alternative/indirect route.')
Copy link
Contributor

Choose a reason for hiding this comment

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

similar to above, indent 2 spaces

Copy link
Contributor

@fengtality fengtality left a comment

Choose a reason for hiding this comment

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

buy doesn't work and throws an Operation Error - problems seems to reside in the swapExactOut` function

@fengtality
Copy link
Contributor

I added a commit which:

  • Addresses https://github.com/CoinAlpha/hummingbot/issues/2656: moves the Balancer and Uniswap addresses into their respective files, so we can delete the EXCHANGE_PROXY and UNISWAP_ROUTER constants in the Gateway installation script.
  • Uses the number of swaps utilized by Balancer to set the gas limit dynamically. The Balancer price and trade functions now have an optional parameter maxSwaps that sets the number of swaps used.

Smaller traders would use a small maxSwaps number because they care more about gas costs, while larger traders would use a high maxSwaps because they want the best price and don't care about gas as much. We should factor this calculation into the client profitability calculation as well:

const GAS_BASE = 200000;
const GAS_PER_SWAP = 100000;
const gasLimit = GAS_BASE + MAX_SWAPS * GAS_PER_SWAP

@fengtality
Copy link
Contributor

Sorry @vic-en for trampling all over your PR... I should have created a separate branch for the last 2 commits. I don't think they affected the Uniswap part of the code, but feel free to roll them back if you want.

I think the buy function in Uniswap may still throw an error, though I was testing using Postman and not with the client.

@vic-en
Copy link
Contributor Author

vic-en commented Nov 20, 2020

Sorry @vic-en for trampling all over your PR... I should have created a separate branch for the last 2 commits. I don't think they affected the Uniswap part of the code, but feel free to roll them back if you want.

I think the buy function in Uniswap may still throw an error, though I was testing using Postman and not with the client.

I figured it. I'd push a commit to address that.

@fengtality fengtality merged commit b3ef56e into development Nov 20, 2020
@fengtality fengtality deleted the feat/uniswap_gateway branch November 20, 2020 18:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants