BE-677 | InGivenOut APIs for Alloyed pool #605
Conversation
|
WalkthroughThe pull request introduces a systematic modification across multiple files in the domain and router packages, focusing on changing the Changes
Sequence DiagramsequenceDiagram
participant Caller
participant Pool
Caller->>Pool: ChargeTakerFeeExactOut(tokenIn)
Pool-->>Caller: tokenInAfterFee
Possibly related PRs
Suggested reviewers
Poem
✨ 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: 4
🧹 Nitpick comments (2)
router/usecase/pools/routable_cw_alloy_transmuter_pool.go (2)
132-133: Clarify zero-taker-fee behavior in doc comment.
The doc comment says the pool does not charge a fee, but this implementation still callsCalcTakerFeeExactOut. Ifr.TakerFeeis non-zero, a fee is charged. It may be worth clarifying that any fee is zero only ifTakerFeeis zero.
134-135: Parameter name and method name are conceptually mismatched.
The method is namedChargeTakerFeeExactOutbut its parameter istokenIn. Consider clarifying which side of the swap the fee is calculated upon for consistent semantics.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
router/usecase/pools/routable_cw_alloy_transmuter_pool_test.gois excluded by!**/*_test.go
📒 Files selected for processing (6)
domain/mocks/pool_mock.go(1 hunks)domain/routable_pool.go(1 hunks)router/usecase/pools/routable_concentrated_pool.go(1 hunks)router/usecase/pools/routable_cw_alloy_transmuter_pool.go(3 hunks)router/usecase/pools/routable_cw_orderbook_pool.go(1 hunks)router/usecase/pools/routable_result_pool.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (5)
router/usecase/pools/routable_cw_alloy_transmuter_pool.go (3)
233-236: Doc comment is correct and well-structured.
The explanation of normalization factors and formula is thorough and matches the code that follows.
237-263: Looks good, recommend boundary tests.
CalcTokenInAmtproperly checks zero normalization factors and rate limiters before computing the amount. Consider adding explicit unit tests for edge cases (e.g., zerotokenOut.Amount, maximumtokenOut.Amount) to ensure correctness under boundary conditions.
92-106: Verify correctness of parameter usage.
InsideCalculateTokenInByTokenOut, the call toCalcTokenInAmt(tokenIn, r.TokenOutDenom)passestokenInas the first argument, butCalcTokenInAmtexpects atokenOut sdk.Coin. The mismatch in naming can cause confusion or logical errors.Run the following script to locate all references of
CalculateTokenInByTokenOutand confirm they are passing the correct “token out” coin:domain/routable_pool.go (1)
65-65: Parameter rename aligns with the rest of the PR changes.
Renaming the parameter fromtokenOuttotokenInis consistent with the new design, but the method nameChargeTakerFeeExactOutcan be slightly confusing now. If feasible, clarify in the interface doc that “ExactOut” refers to the swap mode, not the parameter name.domain/mocks/pool_mock.go (1)
204-205: Ensure consistent interpretation of the fee addition.Here, you’re adding the fee to the original token amount, which is typical for an “exact-out” scenario (the cost to the user is increased). Confirm that the rest of the codebase aligns with this design for exact-out fees.
| @@ -224,7 +224,7 @@ func (r *routableConcentratedPoolImpl) ChargeTakerFeeExactIn(tokenIn sdk.Coin) ( | |||
|
|
|||
| // ChargeTakerFee 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) { | |||
| func (r *routableConcentratedPoolImpl) ChargeTakerFeeExactOut(tokenIn sdk.Coin) (tokenInAfterFee sdk.Coin) { | |||
There was a problem hiding this comment.
Implement or remove the placeholder logic.
Currently, this function always returns an empty coin and does not charge the taker fee. If it’s meant to implement exact-out taker fee logic, consider basing it on your mock or adding a dedicated helper, for example:
- return sdk.Coin{}
+ takerFee := r.TakerFee.Mul(tokenIn.Amount.ToLegacyDec())
+ tokenInAfterFee = tokenIn.Add(sdk.NewCoin(tokenIn.Denom, takerFee.TruncateInt()))
+ return tokenInAfterFeeCommittable suggestion skipped: line range outside the PR's diff.
|
Note for reviewer: Sonar is complaining about code coverage since this PR touches a few other places in the code. |
This PR implements InGivenOut APIs for Alloyed pool, more specifically following methods:
CalculateTokenInByTokenOutChargeTakerFeeExactOutNew APIs were covered with unit tests. Additionally it updates
ChargeTakerFeeExactOutAPIs for other pools by renaming parameter fromtokenOuttotokenInsince taker fees are calculated always fortokenIn.Summary by CodeRabbit
Refactor
ChargeTakerFeeExactOutacross multiple pool implementationstokenOuttotokenInin various pool-related interfaces and structsNew Features
CalculateTokenInByTokenOutmethod for Alloy Transmuter poolCalcTokenInAmtmethod to support token calculationsThese changes appear to be part of a broader refactoring effort to standardize token fee calculation methods across different pool types.