Rename caps in volume filter tests (part of #522)

[debnil]

This PR renames caps in the volume filter tests, as part of #522. This renames the various fields called SellBaseAssetCap* as BaseAssetCap*. This reduces the bloat for follow-up PRs, which will add functional changes and additional tests.

[nikhilsaraf]

let's discuss in our 1-1 slot tomorrow

[nikhilsaraf]

baseAssetCapInBaseUnits should remain as BaseAssetCapInBaseUnits as well as for the other field.

these names should follow the variable names. this can be autogenerated at some point, but we can continue to do manually for now.

[nikhilsaraf]

PS: once we eliminate all direct calls to construct VolumeFilterConfig once it all goes through the raw factory method (a pending TODO), then we can un-export these two fields.

[nikhilsaraf]

@debnil this variable name change has introduced unwanted ambiguity..

amountValueUnitsBeingSold tells us that the value corresponds to the selling asset in the manageSellOffer.
offerAmountValueUnits does not tell us whether the amount corresponds to the selling asset or buying asset in the manageSellOffer.

1. need to figure out what this variable means when we use buy types, which is not super-straightforward -- may need to take a TDD approach or intrusive debugging to figure this out.
2. need to name this variable accordingly so it is super clear.

the reason we need to do the two steps above is because we unfortunately had solved the trivial case previously -- doing a "sell" type operation using a manageSellOffer where amount and price all follow the selling asset (because that's how manageSellOffer works). the hard part is to now figure out what values we will see when we invert it.

let's discuss in our 1-1 session tomorrow (Thursday) about what you end up finding from your debugging.

[debnil]

Ah, got it, ok. I'll revert these names as well.

[nikhilsaraf]

As you might have guessed, this is the main meat of the buytwap logic. Everything else we've done so far was basically just plumbing work to get us here.

I think the right thing to do is to extract the common logic from the above sell side case to simplify this. For example, things like projectedBoughtInBaseUnits should logically represent projectedExposureInBaseUnits instead so it can be reused for both buy and sell.

this basically boils down to the fact that selling XLM/USD is the same as buying USD/XLM so we don't need any if conditions or new logic, but we need some sort of "translation layer" that converts both the buy/sell forms to a more general form where we can phrase it as "exposure" to a specific asset (where exposure means the likely consumption of that asset from the balance because we give it up) vs explicitly saying amount is being bought/sold. The end result should make it "immediately obvious" what is happening in the code.

To do this it may be helpful to grok exactly what is going on in the above sell case and refactor the logic to be more generalized to both buy and sell cases. In the past, I've had success with modeling the code blocks as a flowchart with distinct logical blocks. A good separation point for logical blocks could be the if conditions, but that is of course not necessary as there may be some redundant logic there too. When you model the buy side and sell side it will be easy to identify what the overlap is.

This is one the main reasons whey we needed extensive testing of this function with numerous test cases so we know that there are no regressions when we do such a significant refactor.

does this make sense? would be useful to discuss in our 1-1 tomorrow as well.

[debnil]

Yup, it makes a ton of sense and is super helpful. Now that we've made the associated renames in this PR, I'll work on pulling out that logic in a follow-up :)

[nikhilsaraf]

lgtm, thanks! 🚢