BE-678 | InGivenOut APIs for Cosmwasm pool #606
Conversation
|
Note for reviewer: SonarCloud is complaining since introduced wasm client mock does not have coverage. |
|
WalkthroughThis pull request introduces a new mock WASM client in the Changes
Sequence DiagramsequenceDiagram
participant Router
participant Pool
participant WasmContract
Router->>Pool: CalculateTokenInByTokenOut(tokenOut)
Pool->>WasmContract: Query token input calculation
WasmContract-->>Pool: Return calculated token input
Pool-->>Router: Return token input amount
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: 0
🧹 Nitpick comments (2)
router/usecase/pools/routable_cw_pool.go (1)
184-187: Revise Documentation for Zero FeesThe docstring states “does not charge any fee for transmuter pools,” but the code calls
poolmanager.CalcTakerFeeExactOut. If theTakerFeeis non-zero, fees will be applied. Either ensureTakerFeeis always zero for transmuter pools or update the docstring to clarify how/when fees are actually waived.domain/mocks/wasm_client.go (1)
1-116: Consider Returning Errors Instead of PanickingThe mocking approach is thorough and flexible. However, panicking on unimplemented methods can make partial testing difficult. Consider returning a descriptive error instead of panicking, to make your tests more flexible and avoid abrupt terminations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
router/usecase/pools/routable_cw_pool_test.gois excluded by!**/*_test.go
📒 Files selected for processing (3)
domain/mocks/wasm_client.go(1 hunks)router/usecase/pools/routable_concentrated_pool.go(1 hunks)router/usecase/pools/routable_cw_pool.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (3)
router/usecase/pools/routable_cw_pool.go (2)
88-96: Clarify Transmuter vs. CosmWasm Pool CheckYour docstrings mention errors for non-transmuter pools, yet the code checks only for
CosmWasmpool type. Please verify thatCosmWasmpool type is strictly the correct indicator for a transmuter pool, or introduce a dedicated transmuter check if they are distinct concepts.
98-116: Validate Sufficient Liquidity ChecksAlthough the inline comments indicate that no slippage swaps require adequate liquidity, the function defers all checks to the contract query. Confirm the external contract consistently enforces these liquidity constraints, since the docstring suggests local checks for balances but the code relies on remote contract calls.
router/usecase/pools/routable_concentrated_pool.go (1)
342-344: 🛠️ Refactor suggestionRename Parameter in “ChargeTakerFeeExactOut”
The method doc references “token out,” but the parameter is
tokenIn. This may confuse readers. Either rename the parameter totokenOutor update the doc to clarify its purpose.Proposed fix:
-func (r *routableConcentratedPoolImpl) ChargeTakerFeeExactOut(tokenIn sdk.Coin) (tokenOutAfterFee sdk.Coin) { - tokenInAfterTakerFee, _ := poolmanager.CalcTakerFeeExactOut(tokenIn, r.GetTakerFee()) - return tokenInAfterTakerFee +func (r *routableConcentratedPoolImpl) ChargeTakerFeeExactOut(tokenOut sdk.Coin) (tokenOutAfterFee sdk.Coin) { + tokenOutAfterTakerFee, _ := poolmanager.CalcTakerFeeExactOut(tokenOut, r.GetTakerFee()) + return tokenOutAfterTakerFee }Likely invalid or redundant comment.
| func (r *routableConcentratedPoolImpl) ChargeTakerFeeExactOut(tokenIn sdk.Coin) (tokenOutAfterFee sdk.Coin) { | ||
| tokenInAfterTakerFee, _ := poolmanager.CalcTakerFeeExactOut(tokenIn, r.GetTakerFee()) | ||
| return tokenInAfterTakerFee |
There was a problem hiding this comment.
Could you please link where this is being called for a CL pool? I'm not seeing where.
I believe the same problem might be present for other pools - please check, create a task, and do a follow-up if applicable
There was a problem hiding this comment.
You're right, at the moment it is just APIs, there will be a follow up PRs with code calling those, actually I have already started working on this huge PR #607 and will open follow up smaller PRs from it later.
| func (r *routableCosmWasmPoolImpl) ChargeTakerFeeExactOut(tokenOut sdk.Coin) (outAmountAfterFee sdk.Coin) { | ||
| return sdk.Coin{} | ||
| // Returns tokenInAmount and does not charge any fee for transmuter pools. | ||
| func (r *routableCosmWasmPoolImpl) ChargeTakerFeeExactOut(tokenIn sdk.Coin) (inAmountAfterFee sdk.Coin) { |
This PR implements InGivenOut APIs for Cosmwasm pool, more specifically following methods:
Additionally new APIs ( and existing ) were covered with unit tests.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates