BE-675 | InGivenOut APIs for balancer, stableswap, transmuter pools #603
Conversation
Implements InGivenOut APIs for balancer, stableswap, transmuter pools
| @@ -4,6 +4,7 @@ package types | |||
| // candidate pool to be used for routing. | |||
| type CandidatePool struct { | |||
| ID uint64 | |||
| TokenInDenom string | |||
There was a problem hiding this comment.
Note: Adding new TokenInDenom field at this point led to more elegant and simpler to understand implementation than alternative appoach by simply renaming existing field to be used for either ExactIn or ExactOut.
There was a problem hiding this comment.
Please add a note that only one must be set at a time
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces comprehensive changes across multiple files in the router and domain layers, focusing on enhancing token routing and pool calculation capabilities. The modifications primarily involve adding new methods to routable pool implementations, updating method signatures to include token input denominations, and standardizing JSON struct tags. These changes aim to provide more flexible and explicit handling of token calculations, fees, and pool routing logic. Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (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: 13
🧹 Nitpick comments (2)
router/usecase/pools/routable_balancer_pool.go (1)
38-46: Enhance error handling in CalculateTokenInByTokenOut.Consider wrapping the error with additional context to aid in debugging.
func (r *routableBalancerPoolImpl) CalculateTokenInByTokenOut(ctx context.Context, tokenOut sdk.Coin) (sdk.Coin, error) { tokenIn, err := r.ChainPool.CalcInAmtGivenOut(sdk.Context{}, sdk.Coins{tokenOut}, r.TokenInDenom, r.GetSpreadFactor()) if err != nil { - return sdk.Coin{}, err + return sdk.Coin{}, fmt.Errorf("failed to calculate token in amount: %w", err) } return tokenIn, nil }router/usecase/pools/routable_cw_transmuter_pool.go (1)
76-95: LGTM! Implementation correctly follows transmuter pool logic.The implementation properly validates pool type and balance before performing a no-slippage swap.
Fix typo in comment.
There's a typo in the comment: "Esnure" should be "Ensure".
- // Esnure that the pool is concentrated + // Ensure that the pool is concentrated
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
pools/usecase/pools_usecase_test.gois excluded by!**/*_test.gorouter/usecase/pools/routable_concentrated_pool_test.gois excluded by!**/*_test.gorouter/usecase/pools/routable_cw_alloy_transmuter_pool_test.gois excluded by!**/*_test.gorouter/usecase/pools/routable_cw_orderbook_pool_test.gois excluded by!**/*_test.gorouter/usecase/pools/routable_cw_transmuter_pool_test.gois excluded by!**/*_test.gorouter/usecase/pools/routable_pool_test.gois excluded by!**/*_test.gorouter/usecase/quote_test.gois excluded by!**/*_test.go
📒 Files selected for processing (15)
domain/mocks/pool_mock.go(4 hunks)domain/mocks/pools_usecase_mock.go(1 hunks)domain/routable_pool.go(1 hunks)ingest/types/candidate_route.go(1 hunks)pools/usecase/pools_usecase.go(2 hunks)router/usecase/pools/pool_factory.go(5 hunks)router/usecase/pools/routable_balancer_pool.go(3 hunks)router/usecase/pools/routable_concentrated_pool.go(4 hunks)router/usecase/pools/routable_cw_alloy_transmuter_pool.go(4 hunks)router/usecase/pools/routable_cw_orderbook_pool.go(4 hunks)router/usecase/pools/routable_cw_pool.go(4 hunks)router/usecase/pools/routable_cw_transmuter_pool.go(3 hunks)router/usecase/pools/routable_result_pool.go(3 hunks)router/usecase/pools/routable_stableswap_pool.go(2 hunks)router/usecase/routertesting/quote.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (21)
ingest/types/candidate_route.go (1)
7-7: Well-structured addition of TokenInDenom field!The new field enhances the clarity of token flow direction and supports both ExactIn and ExactOut scenarios elegantly.
domain/routable_pool.go (1)
62-65: Clean interface extension for ExactOut operations!The new methods
CalculateTokenInByTokenOutandChargeTakerFeeExactOutcomplete the interface's support for both ExactIn and ExactOut operations, maintaining consistency with existing patterns.router/usecase/routertesting/quote.go (1)
44-48: Comprehensive test helper adaptation!The
newRoutablePoolmethod has been correctly updated to support both ExactIn and ExactOut scenarios, with proper parameter ordering.router/usecase/pools/routable_balancer_pool.go (1)
22-25: LGTM: Consistent struct tag formatting!The JSON struct tags now use backticks consistently, following Go best practices.
router/usecase/pools/pool_factory.go (2)
19-19: LGTM! The changes to support InGivenOut APIs are consistent.The addition of
tokenInDenomparameter and its propagation to all pool types is implemented correctly.Also applies to: 40-40, 60-60, 85-85, 91-91
96-96: LGTM! The changes to support InGivenOut APIs in CosmWasm pools are consistent.The addition of
tokenInDenomparameter and its propagation to CosmWasm pool types is implemented correctly.Also applies to: 119-119
router/usecase/pools/routable_cw_pool.go (1)
33-40: LGTM! The struct field tag changes improve readability.The change from double quotes to backticks for JSON tags is a good practice.
router/usecase/pools/routable_concentrated_pool.go (1)
28-32: LGTM! The struct field tag changes improve readability.The change from double quotes to backticks for JSON tags is a good practice.
router/usecase/pools/routable_cw_orderbook_pool.go (1)
26-32: LGTM! The struct field tag changes improve readability.The change from double quotes to backticks for JSON tags is a good practice.
domain/mocks/pool_mock.go (2)
22-22: LGTM! Function pointer for mocking token calculation.The function pointer allows flexible mocking of token in calculation behavior during tests.
37-37: LGTM! Mock variable for token input.The
mockedTokenInvariable complements the existingmockedTokenOutfor comprehensive mocking capabilities.router/usecase/pools/routable_cw_alloy_transmuter_pool.go (1)
23-29: LGTM! Improved JSON struct tags.Changed from double quotes to backticks for better readability and to allow special characters in tags.
pools/usecase/pools_usecase.go (2)
183-183: LGTM! Added token in denomination parameter.The change correctly propagates the token in denomination to the routable pool creation.
266-268: LGTM! Clear documentation for empty parameters.The comment clearly explains why empty strings are used for token denominations in spot price calculation.
router/usecase/pools/routable_stableswap_pool.go (3)
22-25: LGTM!The JSON struct tag formatting changes improve code readability.
38-46: LGTM! Implementation is correct and consistent.The implementation correctly uses
CalcInAmtGivenOutwith proper error handling and spread factor consideration.
63-68: LGTM! Fee calculation is correctly implemented.The implementation properly utilizes
poolmanager.CalcTakerFeeExactOutfor fee calculation.router/usecase/pools/routable_cw_transmuter_pool.go (2)
21-26: LGTM!The JSON struct tag formatting changes improve code readability.
118-122: LGTM! Fee calculation is correctly implemented.The implementation properly utilizes
poolmanager.CalcTakerFeeExactOutfor fee calculation.router/usecase/pools/routable_result_pool.go (1)
28-35: LGTM!The JSON struct tag formatting changes improve code readability.
domain/mocks/pools_usecase_mock.go (1)
114-114: LGTM! Parameter update is correct.The
NewRoutablePoolcall has been properly updated to include the requiredtokenInDenomparameter.
| @@ -4,6 +4,7 @@ package types | |||
| // candidate pool to be used for routing. | |||
| type CandidatePool struct { | |||
| ID uint64 | |||
| TokenInDenom string | |||
There was a problem hiding this comment.
Please add a note that only one must be set at a time
| TokenInDenom: tokenInDenom, | ||
| TokenOutDenom: tokenOutDenom, |
There was a problem hiding this comment.
This TokenInDenom and TokenOutDenom makes sense in the context of this PR but I am worried that if we were to look at it from a higher level, it would be confusing and unclear that only one of can be set at a time. Is there a way we could enhance the abstraction to reflect the restriction?
There was a problem hiding this comment.
(Since I don't have a specific recommendation and, if you've already spent meaningful time on thinking through this, please feel free not to block on this comment)
There was a problem hiding this comment.
Actually both will be set since when we are requesting a quote we specify denoms for both tokenIn and tokenOut, however depending on swap method we specify amount only for one of those.
|
Nice work! |
|
This pull request implements InGivenOut APIs for balancer, stableswap, transmuter pools, more specifically:
New APIs ( and some existing ) were covered with unit test while all other changes are a ripple effect caused by introducing these new APIs.
Summary by CodeRabbit
New Features
Improvements
Technical Updates