-
Notifications
You must be signed in to change notification settings - Fork 262
Implement buy twap strategy (closes #522) #537
Conversation
Implementation questions:
Let me know if there are any other changes I should make. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@debnil Will dig into this in more detail later today.
This also needs the walkthrough guide for buy Twap like we have for the others. Can you copy from sell twap and make necessary modifications? (Mostly replacing sell with buy) — feel free to do as a separate commit if you want
Thanks! And sounds good, will do that in the morning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments inline. pending files besides that:
- add
sample_buytwap.cfg
- add
buy_twap.md
- upload log file of output run as described in the issue
for both the above you'll just need to copy paste from the sell version and replace sell with buy everywhere in the files. you may need to force add the cfg
file to be committed with git, not sure.
sdex, | ||
orderConstraints, | ||
ieif, | ||
assetQuote, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the parameters passed in are already correct.
can you add a comment above the buySideStrategy variable:
// switch sides of base/quote here for buy side
plugins/buyTwapStrategy.go
Outdated
levelProvider, | ||
config.PriceTolerance, | ||
config.AmountTolerance, | ||
false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking we would need to set this to true
because it's a buy side level provider, but I could be wrong.
Was the pricing correct for you when you ran this strategy on testnet? If so then maybe it's correct to keep this as false
for this specific strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't run this yet because of the database config issue, but looking at the definitions, I think you're correct. Will confirm post-successful run.
plugins/buyTwapStrategy.go
Outdated
config.AmountTolerance, | ||
false, | ||
) | ||
// switch side of base/quote here for the delete side |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be deleteSideStrategy := makeDeleteSideStrategy(sdex, assetBase, assetQuote)
and we don't need the comment of switch sides..
on here.
instead we can add a comment saying
// use assetBase as param to assetBase argument since the delete strategy is
// on the sell side so the params should line up correctly with the arguments
@debnil I've fixed the config file issue so when you merge the master branch it should work without any config issues |
@nikhilsaraf I've made the requested changes and added the files. Run log is below. I'm not totally certain how to check if this has the desired price levels or behavior. I'll add the run log for
|
Sell TWAP run log here. I do see some differences, but should we be swapping the base and quote assets in the config file, in addition to the code changes we've made?
|
Logfile when changing the last boolean input parameter for
|
@nikhilsaraf After running
|
This PR implements the buy twap strategy and closes #522.