Combine StablePool traits into one #30
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
woocash2
reviewed
Jul 15, 2024
helpers/stable_swap_math/mod.rs
Outdated
| /// Compute the amount of LP tokens to mint after a deposit | ||
| /// return <lp_amount_to_mint, lp_fees_part> | ||
| /// Given `deposit_amounts` user want deposit, calculates how many lpt | ||
| /// is required to be burnt. |
There was a problem hiding this comment.
minted, not burned? (also use plural? (are))
woocash2
reviewed
Jul 15, 2024
helpers/stable_swap_math/mod.rs
Outdated
| /// return <lp_amount_to_burn, lp_fees_part> | ||
| /// all amounts are in c_amount (comparable amount) | ||
| /// Given `withdraw_amounts` user want get, calculates how many lpt | ||
| /// is required to be burnt |
woocash2
reviewed
Jul 15, 2024
amm/contracts/stable_pool/lib.rs
Outdated
| /// NOTE: | ||
| /// If the pool contains a token with rate oracle, this function makes | ||
| /// a cross-contract call to the `RateProvider` contract if the cached rate is expired. | ||
| /// This means that the gas cost of the contract methods that use this function may change, |
There was a problem hiding this comment.
'vary' instead of 'change'? Feels like a better fitting word 😛
woocash2
reviewed
Jul 15, 2024
amm/traits/stable_pool.rs
Outdated
| amounts: Vec<u128>, | ||
| ) -> Result<(u128, u128), StablePoolError>; | ||
|
|
||
| /// Calculate ideal deposit amounts required | ||
| /// Estimate ideal deposit amounts required |
There was a problem hiding this comment.
I would stay with 'Calculate' here and below. 'Estimate' suggests some randomness 😛. I suppose that 'Estimate' here was supposed to account for precision errors. I think 'Calculate' is still a good word, even in presence of precision errors.
woocash2
requested changes
Jul 15, 2024
There was a problem hiding this comment.
LGTM, but I've got a few remarks:
- I don't understand the purpose of merging
StablePoolwithStablePoolView. I've got nothing against it, but I would like to understand the reason (if there's any) - You made all getters to use immutable reference, which required you to remove rate updates from them. It makes sense to have an immutable getter, but consider a scenario when the contract is not used for a long long time for whatever reason. Someone uses a getter to for instance get the mint liquidity for given amounts (they are consideing putting in the liquidity, but want to estimate lpt first). Then they will get very outdated results. Pls consider getting back to
mutin these getters which use rates. If you disagree, we can discuss that.
The reason was purely cosmetic. Imho it simplifies the contract.
The getters still use updated rates for calculations. The difference is that now the getters don't update the cached rates in the contract. |
woocash2
approved these changes
Jul 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR combines
StablePoolandStablePoolViewinto one trait. It also introduces a few less significant changes:&mut self