feat: Solvers clean #116
feat: Solvers clean #116
Conversation
kinrezC
requested review from
clemlak,
kinrezC,
Autoparallel and
Alexangelj
and removed request for
clemlak and
kinrezC
There was a problem hiding this comment.
Okay so new pair solver does the job for computing allocate/deallocate amounts? thats good
There's some issues with imports, formatting, styling, and natspec
Clement needs to review and sign off. Solvers are kind of not in scope of the audit because they are used as lens contracts, but they still should be as clean as the rest of the codebase
|
I also feel like this should be a PR outside of the audit fixes. I imagine we will continue to edit the solvers as we need, so these will continue to be "unaudited", but i'll leave this up to @clemlak to decide on what to do |
|
Sure yeah review all you need. |
Alexangelj
added
💡 feature
|
I agree with Alex, maybe we should merge this into another branch later because the |
| function computePrice(ConstantSumParams memory params) pure returns (uint256) { | ||
| return params.price; | ||
| } |
There was a problem hiding this comment.
Not sure if this one is needed? I understand that it might be for consistency regarding the other strategies but I think this one could just be an exception.
There was a problem hiding this comment.
It's not really but makes the api the same between them all. I think general is better than bespoke so i advocate for keeping it.
There was a problem hiding this comment.
I'm fine with this, but if it lives in the ConstantSumMath.sol and it's not invoked anywhere in e.g. solver, or strategy it doesn't really do anything, so I'm not sure what to do with this at the moment.
| // These are same for allocation and deallocation | ||
| // delta y is 0 | ||
| function PrepareAllocationDeltaGivenDeltaX( | ||
| uint256 poolId, | ||
| uint256 deltaX | ||
| ) public view returns (bytes memory) { | ||
| ConstantSumParams memory params = getPoolParams(poolId); | ||
| uint256 deltaL = deltaX.mulWadDown(params.price); | ||
| return abi.encode(deltaX, 0, deltaL); | ||
| } | ||
|
|
||
| function PrepareAllocationDeltaGivenDeltaY( | ||
| // uint256 poolId, | ||
| uint256 deltaY | ||
| ) public pure returns (bytes memory) { | ||
| return abi.encode(0, deltaY, deltaY); | ||
| } |
There was a problem hiding this comment.
NatSpec could be improved and the name of the functions should use camel case.
There was a problem hiding this comment.
I changed some of the natspec but i am unsure if i am most familiar with the conventions you would like to adhere to.
src/PairSolver.sol
Outdated
There was a problem hiding this comment.
I like the idea of a PairSolver.
src/lib/StrategyLib.sol
Outdated
| function encodeAllocationDeltasGivenDeltaX( | ||
| uint256 deltaX, | ||
| uint256 reserveX, | ||
| uint256 reserveY, | ||
| uint256 liquidity | ||
| ) pure returns (bytes memory) { | ||
| uint256 deltaY = computeDeltaYGivenDeltaX(deltaX, reserveX, reserveY); | ||
| uint256 deltaL = computeDeltaLGivenDeltaX(deltaX, liquidity, reserveX); | ||
| return abi.encode(deltaX, deltaY, deltaL); | ||
| } | ||
|
|
||
| function encodeAllocationDeltasGivenDeltaY( | ||
| uint256 deltaY, | ||
| uint256 reserveX, | ||
| uint256 reserveY, | ||
| uint256 liquidity | ||
| ) pure returns (bytes memory) { | ||
| uint256 deltaX = computeDeltaXGivenDeltaY(deltaY, reserveX, reserveY); | ||
| uint256 deltaL = computeDeltaLGivenDeltaY(deltaY, liquidity, reserveY); | ||
| return abi.encode(deltaX, deltaY, deltaL); | ||
| } | ||
|
|
||
| function encodeAllocationDeltasGivenDeltaL( | ||
| uint256 deltaL, | ||
| uint256 reserveX, | ||
| uint256 reserveY, | ||
| uint256 liquidity | ||
| ) pure returns (bytes memory) { | ||
| uint256 deltaX = computeDeltaXGivenDeltaL(deltaL, reserveX, liquidity); | ||
| uint256 deltaY = computeDeltaYGivenDeltaL(deltaL, reserveY, liquidity); | ||
| return abi.encode(deltaX, deltaY, deltaL); | ||
| } |
There was a problem hiding this comment.
I'm not sure if these functions should stay here, maybe we can have them in the PairStrategy contract directly.
There was a problem hiding this comment.
Would moving them change the api at all? If not i think moving them is fine.
There was a problem hiding this comment.
These should for sure go in PairStrategy, clement is right
| @@ -91,7 +90,7 @@ contract LogNormalSwapTest is LogNormalSetUp { | |||
|
|
|||
| console2.log(invariant); | |||
|
|
|||
| uint256 amountOut = preReserves[0] - rx; | |||
| uint256 amountOut = rX - rx; | |||
There was a problem hiding this comment.
We might want to rename these variables, that's not super explicit haha.
There was a problem hiding this comment.
Agreed could be good.
| /** | ||
| * @title Pair strategy base contract for DFMM. | ||
| * @notice This abstract contract defines the basic behavior of | ||
| * a two-token strategy for DFMM. It is meant to be inherited by | ||
| * a concrete strategy implementation. | ||
| * @author Primitive | ||
| */ |
There was a problem hiding this comment.
Do you want to change it? Not sure what the "right" natspec is. I don't really have any opinions on solidity but all the kits work uses this. I'm more of the opinion on RERO then high modernism.
There was a problem hiding this comment.
I guess it's time to finish our style guide 👀
|
Closed in favor of #130 |
Closes #115
I'm sure this can be cleaner but this does what I want.