BE-676 | Implement InGivenOut APIs for CL pool #604
Conversation
deividaspetraitis
force-pushed
the
BE-676
branch
from
a9a8e59 to
d254c1b
Compare
| @@ -282,6 +283,9 @@ replace ( | |||
| github.com/osmosis-labs/osmosis/osmoutils => github.com/osmosis-labs/osmosis/osmoutils v0.0.12-0.20240825083448-87db4447a1ff | |||
| github.com/osmosis-labs/osmosis/v28 => github.com/osmosis-labs/osmosis/v28 v28.0.2 | |||
|
|
|||
| // Temp replacement | |||
| github.com/osmosis-labs/osmosis/v28 v28.0.2 => github.com/osmosis-labs/osmosis/v28 v28.0.0-20250123151709-6bb88cccbfe6 | |||
There was a problem hiding this comment.
Note for reviewer: we need to close osmosis-labs/osmosis#8954 first
| // - the current tick is not within the specified current bucket range | ||
| // - tick model has no liquidity flag set | ||
| // - the current sqrt price is zero | ||
| // - rans out of ticks during swap (token out is too high for liquidity in the pool) | ||
| func (r *routableConcentratedPoolImpl) CalculateTokenInByTokenOut(ctx context.Context, tokenOut sdk.Coin) (sdk.Coin, error) { |
There was a problem hiding this comment.
Note for reviewer: Would it make sense to move this method to ChainModel in Osmosis repo and let SQS just forward request there? As we are doing for example here:
// CalculateTokenInByTokenOut implements RoutablePool.
func (r *routableBalancerPoolImpl) CalculateTokenInByTokenOut(ctx context.Context, tokenOut sdk.Coin) (sdk.Coin, error) {
tokenIn, err := r.ChainPool.CalcInAmtGivenOut(sdk.Context{}.WithContext(ctx), sdk.Coins{tokenOut}, r.TokenInDenom, r.GetSpreadFactor())
if err != nil {
return sdk.Coin{}, err
}
return tokenIn, nil
}
There was a problem hiding this comment.
No, chain operates on the chain store. Here, we simulate the store by operating on the custom data representation that is ingested from chain
WalkthroughThis pull request introduces changes across multiple files, focusing on enhancing error handling and pool routing functionality. The primary modifications include updating the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (5)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
router/usecase/pools/routable_concentrated_pool.go (1)
197-204: Consider refactoring to eliminate duplicate logic between swap calculation methods.The methods
CalculateTokenOutByTokenInandCalculateTokenInByTokenOutshare similar logic structures with slight differences in variable handling and function calls. To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, consider abstracting the common logic into shared functions or components. This will reduce code duplication and make future updates easier.domain/errors.go (1)
146-150: Ensure error message covers cases where bothAmountInandAmountOutare empty.Currently, if both
AmountInandAmountOutare empty strings, the error message will not provide specific details, which could lead to confusion. Consider adding a default error message or explicitly handling this scenario.Apply this diff to handle the case when both amounts are empty:
func (e ConcentratedNotEnoughLiquidityToCompleteSwapError) Error() string { if e.AmountIn != "" { return fmt.Sprintf("not enough liquidity to complete swap in pool (%d) with amount in (%s)", e.PoolId, e.AmountIn) + } else if e.AmountOut != "" { return fmt.Sprintf("not enough liquidity to complete swap in pool (%d) with amount out (%s)", e.PoolId, e.AmountOut) + } else { + return fmt.Sprintf("not enough liquidity to complete swap in pool (%d)", e.PoolId) } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumrouter/usecase/pools/routable_concentrated_pool_test.gois excluded by!**/*_test.go
📒 Files selected for processing (3)
domain/errors.go(1 hunks)go.mod(8 hunks)router/usecase/pools/routable_concentrated_pool.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (2)
router/usecase/pools/routable_concentrated_pool.go (1)
205-312: Consider movingCalculateTokenInByTokenOuttoChainModelin the Osmosis repository.As previously suggested, it might be beneficial to move this method to the
ChainModelin the Osmosis repository and have SQS forward the request. This approach is consistent with howCalculateTokenInByTokenOutis handled inroutable_balancer_pool.go. Centralizing this logic can improve maintainability and reduce code duplication across repositories.go.mod (1)
287-287: Reminder to update the Osmosis dependency after merging PR 8954.As noted, a temporary replacement has been added for
github.com/osmosis-labs/osmosis/v28. Ensure to update the dependency to the official release version once PR 8954 is merged to maintain consistency and stability in the codebase.
| // for a given amount of token in. | ||
| return sdk.Coin{}, domain.ConcentratedNotEnoughLiquidityToCompleteSwapError{ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct the comment to reflect the accurate context of the swap operation.
The comment indicates "for a given amount of token in," which is misleading in the context of CalculateTokenInByTokenOut. This method calculates the required token in amount for a specified token out, so the comment should be updated accordingly.
Apply this diff to correct the comment:
// This happens when there is not enough liquidity in the pool to complete the swap
- // for a given amount of token in.
+ // for a given amount of token out.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // for a given amount of token in. | |
| return sdk.Coin{}, domain.ConcentratedNotEnoughLiquidityToCompleteSwapError{ | |
| // This happens when there is not enough liquidity in the pool to complete the swap | |
| // for a given amount of token out. | |
| return sdk.Coin{}, domain.ConcentratedNotEnoughLiquidityToCompleteSwapError{ |
| // ChargeTakerFeeExactIn implements domain.RoutablePool. | ||
| // Charges the taker fee for the given token in and returns the token in after the fee has been charged. | ||
| func (r *routableConcentratedPoolImpl) ChargeTakerFeeExactIn(tokenIn sdk.Coin) (tokenInAfterFee sdk.Coin) { | ||
| tokenInAfterTakerFee, _ := poolmanager.CalcTakerFeeExactIn(tokenIn, r.GetTakerFee()) |
There was a problem hiding this comment.
Handle potential errors returned by CalcTakerFeeExactIn.
In ChargeTakerFeeExactIn, the error returned by CalcTakerFeeExactIn is being ignored. Ignoring errors can lead to unintended consequences or silent failures. Consider handling the error to ensure robust error management.
Apply this diff to handle the error:
func (r *routableConcentratedPoolImpl) ChargeTakerFeeExactIn(tokenIn sdk.Coin) (tokenInAfterFee sdk.Coin, err error) {
- tokenInAfterTakerFee, _ := poolmanager.CalcTakerFeeExactIn(tokenIn, r.GetTakerFee())
+ tokenInAfterTakerFee, err := poolmanager.CalcTakerFeeExactIn(tokenIn, r.GetTakerFee())
+ if err != nil {
+ return sdk.Coin{}, err
+ }
return tokenInAfterTakerFee, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ChargeTakerFeeExactIn implements domain.RoutablePool. | |
| // Charges the taker fee for the given token in and returns the token in after the fee has been charged. | |
| func (r *routableConcentratedPoolImpl) ChargeTakerFeeExactIn(tokenIn sdk.Coin) (tokenInAfterFee sdk.Coin) { | |
| tokenInAfterTakerFee, _ := poolmanager.CalcTakerFeeExactIn(tokenIn, r.GetTakerFee()) | |
| // ChargeTakerFeeExactIn implements domain.RoutablePool. | |
| // Charges the taker fee for the given token in and returns the token in after the fee has been charged. | |
| func (r *routableConcentratedPoolImpl) ChargeTakerFeeExactIn(tokenIn sdk.Coin) (tokenInAfterFee sdk.Coin, err error) { | |
| tokenInAfterTakerFee, err := poolmanager.CalcTakerFeeExactIn(tokenIn, r.GetTakerFee()) | |
| if err != nil { | |
| return sdk.Coin{}, err | |
| } | |
| return tokenInAfterTakerFee, nil | |
| } |
| // ChargeTakerFeeExactOut implements domain.RoutablePool. | ||
| // Charges the taker fee for the given token out and returns the token out after the fee has been charged. | ||
| func (r *routableConcentratedPoolImpl) ChargeTakerFeeExactOut(tokenOut sdk.Coin) (tokenOutAfterFee sdk.Coin) { | ||
| return sdk.Coin{} | ||
| tokenOutAfterTakerFee, _ := poolmanager.CalcTakerFeeExactOut(tokenOut, r.GetTakerFee()) | ||
| return tokenOutAfterTakerFee |
There was a problem hiding this comment.
Handle potential errors returned by CalcTakerFeeExactOut.
Similarly, in ChargeTakerFeeExactOut, the error returned by CalcTakerFeeExactOut is being ignored. Proper error handling is crucial for reliable software. Consider modifying the function to handle the error appropriately.
Apply this diff to handle the error:
func (r *routableConcentratedPoolImpl) ChargeTakerFeeExactOut(tokenOut sdk.Coin) (tokenOutAfterFee sdk.Coin, err error) {
- tokenOutAfterTakerFee, _ := poolmanager.CalcTakerFeeExactOut(tokenOut, r.GetTakerFee())
+ tokenOutAfterTakerFee, err := poolmanager.CalcTakerFeeExactOut(tokenOut, r.GetTakerFee())
+ if err != nil {
+ return sdk.Coin{}, err
+ }
return tokenOutAfterTakerFee, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ChargeTakerFeeExactOut implements domain.RoutablePool. | |
| // Charges the taker fee for the given token out and returns the token out after the fee has been charged. | |
| func (r *routableConcentratedPoolImpl) ChargeTakerFeeExactOut(tokenOut sdk.Coin) (tokenOutAfterFee sdk.Coin) { | |
| return sdk.Coin{} | |
| tokenOutAfterTakerFee, _ := poolmanager.CalcTakerFeeExactOut(tokenOut, r.GetTakerFee()) | |
| return tokenOutAfterTakerFee | |
| // ChargeTakerFeeExactOut implements domain.RoutablePool. | |
| // Charges the taker fee for the given token out and returns the token out after the fee has been charged. | |
| func (r *routableConcentratedPoolImpl) ChargeTakerFeeExactOut(tokenOut sdk.Coin) (tokenOutAfterFee sdk.Coin, err error) { | |
| tokenOutAfterTakerFee, err := poolmanager.CalcTakerFeeExactOut(tokenOut, r.GetTakerFee()) | |
| if err != nil { | |
| return sdk.Coin{}, err | |
| } | |
| return tokenOutAfterTakerFee, nil | |
| } |
|
This PR implements InGivenOut APIs for CL pool, more specifically following methods:
CalculateTokenInByTokenOutChargeTakerFeeExactOutNew APIs were covered with unit tests.
Summary by CodeRabbit
Release Notes
New Features
Dependency Updates
Improvements