Conversation
johngrantuk
commented
Feb 23, 2023
johngrantuk
commented
- Refactor stable pools to reuse same logic and maths where possible.
- Remove PhantomStable maths as it is no longer needed (replaced by just Stable math)
- Should make it easier to do maths adjustments as mentioned here
brunoguerios
left a comment
brunoguerios
left a comment
There was a problem hiding this comment.
I must say it's a bit hard to follow all those changes 😅
I added comments on some items that stood out, but big changes between classes I'd have to open both and compare side by side to be able to provide more relevant feedback.
In any way, extending/reusing classes make a lot of sense and it's nice to see that kind of change in the code. 😃
| "eslint": "^7.32.0", | ||
| "eslint-plugin-mocha-no-only": "^1.1.1", | ||
| "eslint-plugin-prettier": "^3.4.1", | ||
| "ethers": "^5.7.2", |
There was a problem hiding this comment.
Considering we already have @ethersproject imports, do we really need ethers? I wasn't able to find where you're using it.
| ); | ||
| } | ||
|
|
||
| _exactTokenInForTokenOut( |
There was a problem hiding this comment.
Was that simply being duplicated from PhantomStablePool? Is that why we were able to simply remove them?
| return ZERO; | ||
| } | ||
| handleScalingAndFeeTokenIn( | ||
| swapFee: BigNumber, |
There was a problem hiding this comment.
Is this part of an interface? Why do we need swapFee if it's not being used?
| poolPairData: PhantomStablePoolPairData, | ||
| amount: OldBigNumber | ||
| handleScalingAndFeeTokenOut( | ||
| swapFee: BigNumber, |
| this.amp = parseFixed(amp, MetaStablePool.AMP_DECIMALS); | ||
| this.swapFee = parseFixed(swapFee, 18); | ||
| this.totalShares = parseFixed(totalShares, 18); | ||
| super(id, address, amp, swapFee, totalShares, tokens, tokensList); |
There was a problem hiding this comment.
Nice that we're able to reuse other classes! 👍
| }); | ||
| }); | ||
| }); | ||
| // context('swaps', () => { |
There was a problem hiding this comment.
Do we still need this commented test? Should we remove it?
2 participants
