test: StableMath rounding
#526
Conversation
| assertLe( | ||
| invariant, | ||
| invariantRoundedDown, | ||
| "Output should be less than or equal to the rounded down mock value." | ||
| ); | ||
| assertLe(invariant, invariantRoundedUp, "Output should be less than or equal to the rounded up mock value."); |
There was a problem hiding this comment.
mhmhmh, how come? raw is < roundDown & raw < roundUp?
There was a problem hiding this comment.
In the StableMath functions, the protocol aims for computing a value either as small as possible or as large as possible by means of rounding; in order to achieve this, it may use different rounding directions during the calculations. Then, the objective of the test case is to verify that the implemented rounding permutation behaves more effectively than other solutions (e.g., always rounding down or always rounding up, which are the permutations that RoundingMock offers).
| // BUG: Revise rounding in `computeBptOutGivenExactTokensIn()` | ||
| // assertLe(bptOutGivenExactTokensIn, bptOutGivenExactTokensInRoundedUp, "Output should be less than or equal to the rounded up mock value."); |
There was a problem hiding this comment.
pinging @jubeira here 👀
There was a problem hiding this comment.
Interesting finding.
At first glance the current rounding directions make sense; I don't see anything that is clearly wrong. I'd need to take a deeper look.
There was a problem hiding this comment.
Thanks for putting this together @0xJabberwock, interesting approach.
Overall the tests make sense. I'd need to take a deeper look at the code to understand why some of the test cases are not passing, but if the tests are correct we might find something.
My main concern with this is that we need to copy the code to the mock to be able to override the rounding directions. In principle I don't see an easy way around this, but I do think it could become a maintenance issue. E.g. if I modify stable math, the tests will still pass and won't catch potential mistakes that I might be introducing unless I also modify the mock, which defeats part of the purpose of the tests. Do you think there's a way to mitigate this problem?
| balances[tokenIndexIn] -= tokenAmountIn; | ||
| } | ||
|
|
||
| return balances[tokenIndexOut] - finalBalanceOut - 1; |
There was a problem hiding this comment.
Why -1? Is that a round down?
There was a problem hiding this comment.
I haven't found an explicit explanatory comment regarding this line in StableMath (from which the logic was copied and pasted). It might have to do with the Newton-Raphson approximation.
In the context of the whole computation, it may work as a rounding mechanism given that it subtracts in computeOutGivenExactIn() but adds in computeInGivenExactOut().
There was a problem hiding this comment.
It makes sense. Exact_In wants to round down the tokenOut result to ensure it's in the vault's favor, and Exact_Out wants to round up the amount of tokens in, so the user pays 1 extra token to account for rounding errors.
I think that's unrelated to Newton-Raphson since it's just a numeric method for iteratively achieving a function result. Instead, it seems to be related to the fact that with a limited number of decimals, rounding issues occur, and we need to make sure the vault doesn't pay for it. (By the way, sorry if I'm being obvious, hahaha)
| balances[tokenIndexOut] += tokenAmountOut; | ||
| } | ||
|
|
||
| return finalBalanceIn - balances[tokenIndexIn] + 1; |
There was a problem hiding this comment.
Same, why +1? Is it rounding up?
There was a problem hiding this comment.
Same answer as in #526 (comment).
| // BUG: Revise rounding in `computeBptOutGivenExactTokensIn()` | ||
| // assertLe(bptOutGivenExactTokensIn, bptOutGivenExactTokensInRoundedUp, "Output should be less than or equal to the rounded up mock value."); |
There was a problem hiding this comment.
Interesting finding.
At first glance the current rounding directions make sense; I don't see anything that is clearly wrong. I'd need to take a deeper look.
test:
|
| Severity Level | Results | |
|---|---|---|
| Contracts | Critical High Medium Low Note Total |
1 1 0 10 36 48 |
| Dependencies | Critical High Medium Low Note Total |
0 0 0 0 0 0 |
For more details view the full report in OpenZeppelin Code Inspector
There was a problem hiding this comment.
Juani: My main concern with this is that we need to copy the code to the mock to be able to override the rounding directions. In principle I don't see an easy way around this, but I do think it could become a maintenance issue.
We are unlikely to change StableMath at this point, but we could at least add a note in the StableMath.sol header noting that there is a mock, and any changes need to be propagated there.
My concern with the approach is that it is a fairly blunt instrument. Many functions have multiple operations with rounding in different directions that influence/reinforce each other. If a function has three operations (down,up,down), we're testing 1) all up; 2) all down; 3) native implementation. So, (down,down,down), (up,up,up), and (down,up,down) -- but not (up,down,up), (up,up,down), (down, up, up), (down,down,up), or (up,down,down). Maybe one of those combinations is actually best.
The most general would be a more white box approach where each function took an array of rounding inputs according to the number of rounding operations, and roundingMock direction was set before each operation, instead of once at the beginning (e.g., the mulDown,divUp functions could all have a rounding direction parameter, vs. reading a global variable). The test functions could then be called with all 2^n combinations
|
After af3778f, both On a similar note, I've seen that Apart from that, currently |
@0xJabberwock Hardhat :) |
Description
Since a test framework for the rounding in the math libraries was deemed necessary, I introduce in this PR a
RoundingMockwhich emulates the pertinentFixedPointfunctions with the possibility of setting the rounding direction via parameter.By taking advantage of
RoundingMockin the context ofStableMathMock, mock functions with a configurable rounding permutation are made possible; for them to be meaningful, their logic should be equivalent to the base ones'.StableMathTesttests the rounding of the functions of interest by comparing their base result with their mock result, so it is assertable whether the rounding direction permutation is actually favoring the protocol, not so much or not at all. This methodology has proven to be effective, as a few rounding bugs have been encountered (the resolution of which is out of the scope of this PR).Besides, I refactored the
StableMathMockandFixedPointMockso their naming and arrangement matched the base contracts'.Type of change
Checklist:
main, or there's a description of how to mergeIssue Resolution
Closes #523.