Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

Refactor volume filter function #604

Closed
debnil opened this issue Dec 5, 2020 · 7 comments
Closed

Refactor volume filter function #604

debnil opened this issue Dec 5, 2020 · 7 comments
Assignees
Labels
feature request New feature or request
Milestone

Comments

@debnil
Copy link
Contributor

debnil commented Dec 5, 2020

The volumeFilterFn within plugins/volumeFilter.go can be significantly streamlined. The below flowchart outlines its logical flow to help guide a refactor.

Volume filter function flowchart

@debnil debnil self-assigned this Dec 5, 2020
@debnil
Copy link
Contributor Author

debnil commented Dec 7, 2020

When looking at the above flowchart, we’re basically trying to answer: when should either the base or quote asset keep being sold?

  1. If the relevant cap is nil. Then, we return the operation with the same amount.
  2. The projected amount sold is within the cap’s value. Then, we return the operation with the same amount.
  3. If the mode is exact and there’s still remaining capacity. Then, we update the amount on the operation and return it.

If one of the above conditions is not met for exactly one asset, we return no operation.

@nikhilsaraf
Copy link
Contributor

Flowchart is awesome, well done! 💯

@debnil
Copy link
Contributor Author

debnil commented Dec 14, 2020

This reworked/simplified version of the above flowchart converts a buying op to a selling op and formats a base cap as a quote cap. This simplifies it down to a single workflow.

Reworked flowchart

@nikhilsaraf
Copy link
Contributor

  1. we probably want to update the op amount only in the case where there is remaining capacity (bottom right of second flowchart). In the case where there is no remaining capacity we should discard the op (i.e. don't keep selling quote).

  2. I'm not sure if we can convert a base cap to a quote cap. I think the more appropriate thing to do is to "select otb, tbb, cap from base part of inputs" or from the quote part of inputs. I'm thinking that the common format we select will not have any idea of "base" or "quote" but only has an understanding of otb, tbb, cap, and offerAmount. Any conversions/transformations/selections before that should provide this invariant. What do you think?

  3. we probably need logic (box) to convert sell offers back to buy offers. What is the best place to put this? based on the Tip below do we even need this?

  4. we also need a box to represent where and when we will update the TBB values. should we do this before converting back to a buy op or before? (i.e. what is the unit of accounting here)


Tip:

  • a "sell base" limit is a limit on the amount of base asset that is sold as a result of selling the base asset.
  • a "sell quote" limit is a limit on the amount of base asset that is sold denominated in the quote asset, i.e. the amount of quote asset that is purchased as a result of selling the base asset.
  • a "buy base" limit is a limit on the amount of base asset that is purchased as a result of buying the base asset.
  • a "buy quote" limit is a limit on the amount of base asset bought denominated in the quote asset, i.e. the amount of quote asset that is sold as a result of purchasing the base asset.

This raises the question of whether a buy operation (buying base and selling quote) will be constrained by a "sell quote" filter. I don't think so. I think it should only be affected by "buy" filters. similarly for sell operations.

(we should document this correctly in the config file and review tests in dailyVolumeByDate, added as a separate issue here so we don't forget: #623)

This may require us to have logic at the top of the flowchart where we do a passthrough if the filter type (buy/sell) is different from the offer type (buy/sell).

@debnil
Copy link
Contributor Author

debnil commented Dec 15, 2020

Revision v2

Got it, the above is helpful.

  1. That was my mistake, edited in new flowchart.
  2. Agreed, edited.
  3. I don't think we need this based on the tip. I've edited the flowchart to have the passthrough logic.
  4. I've added a box for this, we don't need conversion given the tip.

@nikhilsaraf
Copy link
Contributor

Good call on the conversion back to buy ops, doesn’t seem like it’s required.

I think we can get to the desired solution with the above proposal and can’t think of anything in particular that sticks out. Thanks for putting this together. Let’s get this converted to code! 🎉

debnil added a commit that referenced this issue Dec 17, 2020
* vf-testing-strategy

* Revert "vf-testing-strategy"

This reverts commit d0a9e4f.

* Initial commit

* Reformat return op

* Address review - nikhilsaraf

* Rename later use of offerAmount to newOfferAmount
@nikhilsaraf
Copy link
Contributor

I believe this is fully completed (unless I've missed something), reflecting by closing issue.

@nikhilsaraf nikhilsaraf added this to the v1.10.1 milestone Jan 30, 2021
@nikhilsaraf nikhilsaraf added the feature request New feature or request label Jan 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature request New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants