feat: add tradingLimit decimal tracking #573
Conversation
There was a problem hiding this comment.
Good job! I couldn't find any issues during my review 🦾
however, I feel that this solution is overly complex for its purpose. and also I am not even sure if it achieves the intended gas savings
while packing variables into a struct might save gas in some cases (e.g. reducing storage writes from 3 to 1 when writing the entire struct in our case), the added complexity for calculating decimals seems to outweight the potential benefits at least based on my intuition
I ran a quick gas cost analysis using the simple test test_update_withL0LG_updatesActive. here are the results for the TradingLimits.update function:
- current deployed implementation: 2119 gas
- implementation in this PR: 5066 gas
As a comparison, I implemented a simpler version that uses int256 for netFlows, and the same test consumed 951 gas. while this implementation is likely far from perfect and it's untested beyond a single gas cost check, I believe it would not be more expensive to properly implement that than the current proposal.
given the added complexity and the relatively minor gas cost implications, I would strongly prefer going with a simpler implementation.
| if ((config.flags & L0) > 0) { | ||
| int256 netflow0WithDecimals = int256(self.netflow0) * DECIMAL_ONE + int256(self.netflowDecimals); | ||
| int256 limit0WithDecimals = int256(config.limit0) * DECIMAL_ONE; | ||
| if (-1 * limit0WithDecimals > netflow0WithDecimals || netflow0WithDecimals > limit0WithDecimals) { |
There was a problem hiding this comment.
Nit, I would prefer the order to be not changed for readability:
if (netflow0WithDecimals < -1 * limit0WithDecimals || netflow0WithDecimals > limit0WithDecimals)
| @@ -110,6 +123,9 @@ library TradingLimits { | |||
| if (config.flags & LG == 0) { | |||
| self.netflowGlobal = 0; | |||
| } | |||
| if (config.flags & (L0 | LG) == 0) { | |||
There was a problem hiding this comment.
I think it would make more sense to reset the decimals for every reset for simplicity, considering it will be called infrequently and only downside in losing some decimals. wdyt?
|
@bowd Baran and I did some additional gas comparison testing yesterday this is what we found: In the test, we performed a swap with limits only configured on one of the two assets. For this asset, all three limits are configured. Result: This Implementation: Uint256 implementation: Conclusion: |
Description
This PR adds decimal tracking to our existing TradingLimits library.
Before, the trading limit state was stored in a Struct with five variables.
Together these 5 take up 208bits (32 + 32 + 48 + 48 + 48). One storage slot takes up 256 bits, meaning the current state does not fill up the entire slot and leaves space for another 48 bits.
The basic idea of this Pr is adding a single int48 that stores the current decimal part of all limit netflows.
Since an int48 can store a max/min of 140737488355327/-140737488355328 our netflows will only have 14 decimal places.
For our netflows, the actual current netflow is
netflow*1e14 + netflowDecimals. Therefore, the verify function was updated to check whether the limits are surpassed on the decimal places.e.g
In this scenario, the actual limits with decimals would look like this:
When L0 is configured, the decimals are reset when timestep0 seconds have passed.
Open Question:
Currently, our trading limits do not account for amounts smaller than 1e4/-1e4.
We could round them to 1e4/-1e4, similar to what we did previously for the unit part.
In both worlds, the gas costs for trading an amount less than 1e4 will be much higher than any benefit from
manipulating the net flow / bypassing the limits.
Other changes
Tested