feat: simulate swap as part of quotes #547
Conversation
WalkthroughThe pull request introduces several enhancements across multiple files, primarily focusing on the Changes
Possibly related PRs
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: 9
🧹 Outside diff range and nitpick comments (10)
domain/quote_simulator.go (3)
10-18: Documentation improvements needed.The interface documentation has some issues:
- There's a typo in "Retursn" in the error conditions section
- The contract could be more specific about error conditions and parameter constraints
Consider applying these improvements:
// QuoteSimulator simulates a quote and returns the gas adjusted amount and the fee coin. type QuoteSimulator interface { // SimulateQuote simulates a quote and returns the gas adjusted amount and the fee coin. // CONTRACT: // - Only direct (non-split) quotes are supported. - // Retursn error if: - // - Simulator address does not have enough funds to pay for the quote. + // Parameters: + // - slippageToleranceMultiplier: must be > 0 and <= 1 + // - simulatorAddress: must be a valid bech32 address + // Returns error if: + // - Simulator address does not have enough funds to pay for the quote + // - Quote is a split quote + // - Invalid parameters are provided SimulateQuote(ctx context.Context, quote Quote, slippageToleranceMultiplier osmomath.Dec, simulatorAddress string) (uint64, sdk.Coin, error) }
20-23: Consider adding field documentation and validation.While the struct is well-structured, it could benefit from field documentation and validation methods.
Consider these improvements:
type QuotePriceInfo struct { + // AdjustedGasUsed represents the gas amount adjusted after simulation AdjustedGasUsed uint64 `json:"adjusted_gas_used"` + // FeeCoin represents the fee amount in specific denomination FeeCoin sdk.Coin `json:"fee_coin"` } + +// Validate performs basic validation of QuotePriceInfo fields +func (q QuotePriceInfo) Validate() error { + if q.AdjustedGasUsed == 0 { + return fmt.Errorf("adjusted gas used cannot be zero") + } + if err := q.FeeCoin.Validate(); err != nil { + return fmt.Errorf("invalid fee coin: %w", err) + } + return nil +}
10-18: Consider interface versioning strategy.As this is a new interface that might evolve over time, consider adding a version suffix (e.g.,
QuoteSimulatorV1) or documenting the versioning strategy for future compatibility.router/types/get_quote_request.go (1)
76-105: Simplify function structure by eliminating nested else block.While the validation logic is correct, the function's readability can be improved by eliminating the nested else block.
Consider this refactored version:
func validateSimulationParams(swapMethod domain.TokenSwapMethod, simulatorAddress string, slippageToleranceStr string) (osmomath.Dec, error) { + if simulatorAddress == "" { + if slippageToleranceStr != "" { + return osmomath.Dec{}, fmt.Errorf("slippage tolerance is not supported without simulator address") + } + return osmomath.Dec{}, nil + } + + _, err := sdk.ValAddressFromBech32(simulatorAddress) + if err != nil { + return osmomath.Dec{}, fmt.Errorf("simulator address is not valid: %w", err) + } + + if swapMethod == domain.TokenSwapMethodExactIn { + return osmomath.Dec{}, fmt.Errorf("only 'out given in' swap method is supported for simulation") + } + + if slippageToleranceStr == "" { + return osmomath.Dec{}, fmt.Errorf("slippage tolerance is required for simulation") + } + + slippageTolerance, err := osmomath.NewDecFromStr(slippageToleranceStr) + if err != nil { + return osmomath.Dec{}, fmt.Errorf("slippage tolerance is not valid: %w", err) + } + + return slippageTolerance, nildomain/cosmos/tx/msg_simulator.go (1)
147-165: Consider optimizing the implementation and improving error handling.While the implementation is functional, there are several potential improvements:
- Consider combining the simulation and fee calculation into a single operation if the underlying APIs support it, to reduce network calls.
- The gas adjustment factor (1.05) is hardcoded in
SimulateMsgs. Consider making it configurable.- The error handling could be more specific by wrapping errors with context.
- Consider validating input parameters, especially for nil checks on dependencies.
Example improvement for error handling:
func (c *txGasCalulator) PriceMsgs(ctx context.Context, txfeesClient txfeestypes.QueryClient, encodingConfig cosmosclient.TxConfig, account *authtypes.BaseAccount, chainID string, msg ...sdk.Msg) (uint64, sdk.Coin, error) { + if txfeesClient == nil || encodingConfig == nil || account == nil { + return 0, sdk.Coin{}, fmt.Errorf("invalid dependencies: txfeesClient=%v, encodingConfig=%v, account=%v", txfeesClient, encodingConfig, account) + } + _, gasAdjusted, err := c.SimulateMsgs( encodingConfig, account, chainID, msg, ) if err != nil { - return 0, sdk.Coin{}, err + return 0, sdk.Coin{}, fmt.Errorf("failed to simulate messages: %w", err) } feecoin, err := CalculateFeeCoin(ctx, txfeesClient, gasAdjusted) if err != nil { - return 0, sdk.Coin{}, err + return 0, sdk.Coin{}, fmt.Errorf("failed to calculate fee coin: %w", err) } return gasAdjusted, feecoin, nil }domain/router.go (1)
75-76: Add documentation for the new interface method.While the method name is descriptive, please add a comment explaining its purpose, expected behavior, and relationship to other
Quoteinterface methods. This will help maintainers understand the design intent.router/delivery/http/router_handler.go (1)
148-165: Consider extracting simulation logic to a separate method.The simulation logic adds significant complexity to the
GetOptimalQuotemethod. Consider extracting it to improve readability and maintainability.Example refactor:
+func (a *RouterHandler) simulateQuoteIfApplicable(ctx context.Context, c echo.Context, quote domain.Quote, req types.GetQuoteRequest) error { + if !req.SingleRoute || req.SimulatorAddress == "" || req.SwapMethod() != domain.TokenSwapMethodExactIn { + return nil + } + + value := c.Get("simulatorAddress") + simulatorAddress, ok := value.(string) + if !ok { + return c.JSON(http.StatusBadRequest, domain.ResponseError{Message: "invalid simulator address"}) + } + + gasUsed, feeCoin, err := a.QuoteSimulator.SimulateQuote(ctx, quote, req.SlippageToleranceMultiplier, simulatorAddress) + if err != nil { + return c.JSON(domain.GetStatusCode(err), domain.ResponseError{Message: err.Error()}) + } + + quote.SetQuotePriceInfo(&domain.QuotePriceInfo{ + AdjustedGasUsed: gasUsed, + FeeCoin: feeCoin, + }) + + return nil +}Then in
GetOptimalQuote:- // Simulate quote if applicable. - // Note: only single routes (non-splits) are supported for simulation. - // Additionally, the functionality is triggerred by the user providing a simulator address. - // Only "out given in" swap method is supported for simulation. Thus, we also check for tokenOutDenom being set. - if req.SingleRoute && req.SimulatorAddress != "" && req.SwapMethod() == domain.TokenSwapMethodExactIn { - simulatorAddress := c.Get("simulatorAddress").(string) - - gasUsed, feeCoin, err := a.QuoteSimulator.SimulateQuote(ctx, quote, req.SlippageToleranceMultiplier, simulatorAddress) - if err != nil { - return c.JSON(domain.GetStatusCode(err), domain.ResponseError{Message: err.Error()}) - } - - // Set the quote price info. - quote.SetQuotePriceInfo(&domain.QuotePriceInfo{ - AdjustedGasUsed: gasUsed, - FeeCoin: feeCoin, - }) - } + if err := a.simulateQuoteIfApplicable(ctx, c, quote, req); err != nil { + return err + }🧰 Tools
🪛 GitHub Check: Run linter
[failure] 153-153:
type assertion must be checked (forcetypeassert)quotesimulator/quote_simulator.go (3)
43-44: Validate thatpoolsInRouteis not empty.The code assumes that
poolsInRoutecontains at least one pool, but there's no check to ensure this. If it's empty, the subsequent loop will be skipped, which might not be the intended behavior. Add a validation to handle this case.Apply this diff to add the validation:
poolsInRoute := route[0].GetPools() +if len(poolsInRoute) == 0 { + return 0, sdk.Coin{}, fmt.Errorf("route must contain at least one pool") +} poolManagerRoute := make([]poolmanagertypes.SwapAmountInRoute, len(poolsInRoute))
65-67: Wrap error when retrieving account information for better context.To aid in debugging and provide more informative error messages, consider wrapping the error with additional context when the account retrieval fails.
Apply this diff to wrap the error:
if err != nil { - return 0, sdk.Coin{}, err + return 0, sdk.Coin{}, fmt.Errorf("failed to get account for simulator address: %w", err) }
70-72: Wrap error when pricing messages for better context.Similarly, wrapping the error returned by
PriceMsgswith context will make it easier to trace issues during execution.Apply this diff to wrap the error:
if err != nil { - return 0, sdk.Coin{}, err + return 0, sdk.Coin{}, fmt.Errorf("failed to price messages: %w", err) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
domain/cosmos/tx/msg_simulator.go(3 hunks)domain/mocks/msg_simulator_mock.go(2 hunks)domain/quote_simulator.go(1 hunks)domain/router.go(1 hunks)quotesimulator/quote_simulator.go(1 hunks)router/delivery/http/router_handler.go(3 hunks)router/types/get_quote_request.go(3 hunks)router/usecase/quote_out_given_in.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Run linter
router/delivery/http/router_handler.go
[failure] 153-153:
type assertion must be checked (forcetypeassert)
router/usecase/quote_out_given_in.go
[failure] 46-46:
structtag: struct field priceInfo has json tag but is not exported (govet)
🔇 Additional comments (9)
domain/quote_simulator.go (1)
1-8: LGTM! Clean package structure and imports.
The package declaration and imports are well-organized and follow Go best practices.
domain/mocks/msg_simulator_mock.go (1)
34-41: LGTM: Function pointer declaration follows mock patterns.
The PriceMsgsFn function pointer is well-defined with appropriate parameter and return types for gas estimation and fee calculation.
router/types/get_quote_request.go (1)
15-23: LGTM! Well-structured struct definition.
The new fields SimulatorAddress and SlippageToleranceMultiplier are properly typed and follow Go naming conventions.
router/usecase/quote_out_given_in.go (1)
157-160: Verify Quote interface implementation.
Let's ensure the Quote interface is properly implemented and all related code is using the correct field name.
✅ Verification successful
Let me gather more information about the Quote interface definition and its implementations.
Quote interface implementation is correct and consistent
The verification confirms:
- The
Quoteinterface correctly definesSetQuotePriceInfomethod indomain/router.go - The
quoteExactAmountInstruct properly implements the interface with matching method signature - The field
priceInfois consistently used in the struct and its setter - The implementation is being used correctly in
router_handler.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Quote interface implementation and field usage
# Test 1: Check Quote interface definition
echo "Checking Quote interface definition..."
ast-grep --pattern 'type Quote interface {
$$$
SetQuotePriceInfo($_) $_
$$$
}'
# Test 2: Check usage of PriceInfo field
echo "Checking PriceInfo field usage..."
rg -A 2 'PriceInfo.*domain\.QuotePriceInfo'
# Test 3: Check for any remaining lowercase priceInfo usage
echo "Checking for lowercase priceInfo usage..."
rg -A 2 'priceInfo.*domain\.QuotePriceInfo'
Length of output: 1052
Script:
#!/bin/bash
# Check Quote interface definition and all its methods
echo "Checking Quote interface definition..."
ast-grep --pattern 'type Quote interface {
$$$
}'
# Check all implementations of SetQuotePriceInfo
echo "Checking SetQuotePriceInfo implementations..."
ast-grep --pattern 'func ($_ $_) SetQuotePriceInfo($_ *domain.QuotePriceInfo) {
$$$
}'
# Check if priceInfo field is consistently used across all quote types
echo "Checking priceInfo field declarations..."
ast-grep --pattern 'type $_ struct {
$$$
priceInfo *domain.QuotePriceInfo
$$$
}'
Length of output: 2858
domain/cosmos/tx/msg_simulator.go (3)
41-51: LGTM! Well-documented interface addition.
The new PriceMsgs method is a good addition that consolidates gas estimation and fee calculation into a single call, which aligns well with the quote simulation requirements.
93-99: LGTM! Good refactoring of gas and fee calculation.
The changes improve code organization by using the consolidated PriceMsgs method, maintaining proper error handling and setting gas limit before fee amount.
147-165: Verify integration with quote simulation system.
Since this code is part of the swap simulation feature, please ensure:
- The gas estimation accounts for all operations in the swap
- The fee calculation considers the correct denomination for the chain
- The error cases are properly handled by the quote simulation system
✅ Verification successful
Integration with quote simulation system verified successfully
The code is properly integrated with the quote simulation system:
-
Gas estimation is handled correctly:
PriceMsgsis used byquoteSimulator.SimulateQuoteto calculate gas and fees- It internally uses
SimulateMsgsto get accurate gas estimation for all operations
-
Fee calculation is properly implemented:
CalculateFeeCoinis called with the adjusted gas amount- The fee calculation uses the chain-specific
txfeesClient
-
Error handling is comprehensive:
- Errors from both simulation and fee calculation are properly propagated
- The quote simulator properly handles these errors in its implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for quote simulation code that uses this new PriceMsgs method
rg -A 5 "PriceMsgs.*QuoteSimulator"
# Look for any existing swap simulation tests
fd -e go -x rg -l "TestSwap.*Simulation"
Length of output: 80
Script:
#!/bin/bash
# Let's search for any usage of PriceMsgs method
rg -A 5 "PriceMsgs\("
# Look for quote simulation related code
rg -A 10 "QuoteSimulator"
# Search for swap simulation tests with a broader pattern
rg -l "Test.*Swap.*Simulation"
# Look for the interface definition of MsgSimulator
ast-grep --pattern 'type MsgSimulator interface {
$$$
}'
Length of output: 9506
router/delivery/http/router_handler.go (2)
22-25: LGTM! Clean struct field addition.
The new QuoteSimulator field is well-placed and properly typed.
39-44: LGTM! Constructor updated consistently.
The QuoteSimulator parameter is properly integrated into the constructor, maintaining the existing pattern.
p0mvn
force-pushed
the
roman/sim-quote
branch
from
10eca0f to
331f7e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
domain/mocks/quote_simulator_mock.go (2)
11-13: Add godoc comments for the struct and its field.While the implementation is correct, adding documentation would improve code maintainability and help other developers understand the purpose of this mock.
+// QuoteSimulatorMock provides a mock implementation of domain.QuoteSimulator interface for testing. type QuoteSimulatorMock struct { + // SimulateQuoteFn allows tests to define custom behavior for SimulateQuote method. SimulateQuoteFn func(ctx context.Context, quote domain.Quote, slippageToleranceMultiplier math.LegacyDec, simulatorAddress string) (uint64, types.Coin, error) }
15-21: Consider enhancing error handling and validation.While the implementation is functional, consider these improvements:
- Add parameter validation for non-nil context and valid slippage tolerance.
- Make the panic message more descriptive for debugging.
func (q *QuoteSimulatorMock) SimulateQuote(ctx context.Context, quote domain.Quote, slippageToleranceMultiplier math.LegacyDec, simulatorAddress string) (uint64, types.Coin, error) { + if ctx == nil { + panic("context cannot be nil") + } + if slippageToleranceMultiplier.IsNegative() { + panic("slippage tolerance multiplier cannot be negative") + } if q.SimulateQuoteFn != nil { return q.SimulateQuoteFn(ctx, quote, slippageToleranceMultiplier, simulatorAddress) } - panic("SimulateQuoteFn not implemented") + panic("QuoteSimulatorMock: SimulateQuoteFn not implemented") }router/types/get_quote_request.go (2)
65-71: Improve error handling for simulation parameters.Consider wrapping the error from
validateSimulationParamswith additional context to help with debugging:- slippageToleranceDec, err := validateSimulationParams(r.SwapMethod(), simulatorAddress, slippageToleranceStr) - if err != nil { - return err - } + slippageToleranceDec, err := validateSimulationParams(r.SwapMethod(), simulatorAddress, slippageToleranceStr) + if err != nil { + return fmt.Errorf("failed to validate simulation parameters: %w", err) + }
101-108: Simplify the else block logic.The else block can be simplified by returning early, which improves readability:
- } else { - if slippageToleranceStr != "" { - return osmomath.Dec{}, fmt.Errorf("slippage tolerance is not supported without simulator address") - } - } - - return osmomath.Dec{}, nil + } + + if slippageToleranceStr != "" { + return osmomath.Dec{}, fmt.Errorf("slippage tolerance is not supported without simulator address") + } + + return osmomath.Dec{}, nil
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
router/delivery/http/router_handler_test.gois excluded by!**/*_test.gorouter/types/export_test.gois excluded by!**/*_test.gorouter/types/get_quote_request_test.gois excluded by!**/*_test.go
📒 Files selected for processing (10)
app/sidecar_query_server.go(3 hunks)domain/cosmos/tx/msg_simulator.go(3 hunks)domain/mocks/msg_simulator_mock.go(2 hunks)domain/mocks/quote_simulator_mock.go(1 hunks)domain/quote_simulator.go(1 hunks)domain/router.go(1 hunks)quotesimulator/quote_simulator.go(1 hunks)router/delivery/http/router_handler.go(3 hunks)router/types/get_quote_request.go(3 hunks)router/usecase/quote_out_given_in.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- domain/cosmos/tx/msg_simulator.go
- domain/quote_simulator.go
- domain/router.go
- quotesimulator/quote_simulator.go
- router/delivery/http/router_handler.go
- router/usecase/quote_out_given_in.go
🔇 Additional comments (8)
domain/mocks/quote_simulator_mock.go (3)
1-9: LGTM! Package and imports are well-organized.
The package declaration and imports follow Go best practices and contain all necessary dependencies.
23-23: LGTM! Proper interface compliance check.
The compile-time interface compliance check is correctly implemented using Go's type assertion pattern.
11-23: Verify interface completeness.
Let's ensure this mock implements all methods of the QuoteSimulator interface correctly.
✅ Verification successful
Based on the results, I can now generate the final response since I have found both the interface definition and its usage in the codebase:
Interface implementation is correct and complete
The mock correctly implements the only method required by the QuoteSimulator interface:
SimulateQuote(ctx context.Context, quote Quote, slippageToleranceMultiplier osmomath.Dec, simulatorAddress string) (uint64, sdk.Coin, error)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that QuoteSimulator interface is fully implemented
# Expected: Find the interface definition and compare with mock implementation
# Search for QuoteSimulator interface definition
echo "Searching for QuoteSimulator interface definition..."
ast-grep --pattern 'type QuoteSimulator interface {
$$$
}'
# Search for any other methods that might be required
echo "Searching for any method calls on QuoteSimulator..."
rg -A 2 'QuoteSimulator\.'
Length of output: 1701
domain/mocks/msg_simulator_mock.go (2)
33-41: LGTM: Function pointer field is well-defined.
The PriceMsgsFn field follows the established mock pattern and has appropriate parameter types for gas and fee calculation.
72-82: Type inconsistency issue was previously identified.
The message parameter type inconsistency between the method signature and the function pointer was already noted in a previous review.
Implementation pattern is correct.
The implementation follows the standard mock pattern with appropriate nil checks and panic behavior.
router/types/get_quote_request.go (1)
15-23: LGTM: Struct changes are well-designed.
The new fields SimulatorAddress and SlippageToleranceMultiplier are appropriately typed and follow Go naming conventions.
app/sidecar_query_server.go (2)
23-32: LGTM: Import statements are well-organized.
The new imports are properly organized and necessary for implementing the quote simulation feature.
218-228: Verify quote simulator integration with router handler.
The quote simulator is properly initialized with all required dependencies and correctly integrated with the router handler. However, let's verify the integration points.
Run the following script to verify the integration:
✅ Verification successful
Quote simulator integration is properly implemented and verified
The verification confirms that:
- QuoteSimulator is correctly integrated in the RouterHandler with proper initialization and dependency injection
- The interface is well-defined with a clear contract for SimulateQuote method
- The quote simulation is properly used in GetOptimalQuote endpoint with appropriate error handling
- The integration is covered by unit tests that verify the simulation functionality
- The simulation is only triggered under specific conditions:
- Single route (non-split) quotes
- Exact-in swap method
- When simulator address is provided
- With proper slippage tolerance handling
The implementation matches the intended design and is working as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify quote simulator integration with router handler
# Test 1: Check if RouterHandler correctly uses the quote simulator
ast-grep --pattern 'func NewRouterHandler($_, $_, $_, quoteSimulator $_)'
# Test 2: Check if quote simulator is used in route handling
rg -A 5 'quoteSimulator\.'
Length of output: 104
Script:
#!/bin/bash
# Let's try a more specific search for the router handler implementation
# Search for NewRouterHandler function definition
ast-grep --pattern 'func NewRouterHandler($$$)'
# Search for files containing RouterHandler
fd -e go RouterHandler --exec cat {}
# Search for quote simulator usage patterns
rg -l "quotesimulator\."
# Search for any files containing both router and quote simulator references
rg -l "RouterHandler|quotesimulator"
Length of output: 1874
Script:
#!/bin/bash
# Let's check how QuoteSimulator is used in the router handler
rg -A 5 "QuoteSimulator\." router/delivery/http/router_handler.go
# Check the interface definition to verify required methods
rg "type QuoteSimulator interface" -A 10
# Check the actual usage in GetOptimalQuote and other relevant methods
ast-grep --pattern 'func ($_ *RouterHandler) GetOptimalQuote($$$)'
# Check test coverage
cat router/delivery/http/router_handler_test.go
Length of output: 22222
p0mvn
force-pushed
the
roman/sim-quote
branch
from
331f7e3 to
7c42583
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
domain/mocks/quote_mock.go (1)
12-15: Add documentation for the mock struct and its fields.Consider adding documentation to explain the purpose of the mock and how to use its configurable functions:
+// MockQuote implements domain.Quote interface for testing purposes. type MockQuote struct { + // GetAmountInFunc allows customizing the GetAmountIn behavior GetAmountInFunc func() types.Coin + // GetRouteFunc allows customizing the GetRoute behavior GetRouteFunc func() []domain.SplitRoute }domain/mocks/route_mock.go (1)
88-102: LGTM with minor formatting suggestion.The implementations of
GetAmountInandGetAmountOutcorrectly follow the established mock pattern. However, there's inconsistent spacing around the panic statements.Consider standardizing the spacing around panic statements:
func (r *RouteMock) GetAmountIn() math.Int { if r.GetAmountInFunc != nil { return r.GetAmountInFunc() } - panic("unimplemented") } func (r *RouteMock) GetAmountOut() math.Int { if r.GetAmountOutFunc != nil { return r.GetAmountOutFunc() } - panic("unimplemented") }router/delivery/http/router_handler.go (1)
154-157: Consider adding metrics for simulation failures.The error handling for simulation failures could benefit from metrics to track failure rates and types.
Consider adding metrics:
+const ( + metricQuoteSimulationFailure = "quote_simulation_failure" +) if err != nil { + // Record simulation failure metric + metrics.IncrCounter(metricQuoteSimulationFailure, 1) return c.JSON(domain.GetStatusCode(err), domain.ResponseError{Message: err.Error()}) }quotesimulator/quote_simulator.go (2)
27-35: Add documentation comment forNewQuoteSimulator.Consider adding a GoDoc comment for the
NewQuoteSimulatorfunction to enhance code readability and provide clarity on its purpose and usage.
37-80: Add method documentation forSimulateQuote.Including a detailed GoDoc comment for
SimulateQuotewill improve maintainability and help other developers understand its functionality, parameters, and return values.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (5)
domain/cosmos/tx/msg_simulator_test.gois excluded by!**/*_test.goquotesimulator/quote_simulator_test.gois excluded by!**/*_test.gorouter/delivery/http/router_handler_test.gois excluded by!**/*_test.gorouter/types/export_test.gois excluded by!**/*_test.gorouter/types/get_quote_request_test.gois excluded by!**/*_test.go
📒 Files selected for processing (13)
app/sidecar_query_server.go(3 hunks)domain/cosmos/tx/msg_simulator.go(3 hunks)domain/mocks/msg_simulator_mock.go(2 hunks)domain/mocks/quote_mock.go(1 hunks)domain/mocks/quote_simulator_mock.go(1 hunks)domain/mocks/route_mock.go(2 hunks)domain/quote_simulator.go(1 hunks)domain/router.go(1 hunks)quotesimulator/quote_simulator.go(1 hunks)router/delivery/http/router_handler.go(3 hunks)router/types/get_quote_request.go(3 hunks)router/usecase/quote_out_given_in.go(2 hunks)router/usecase/routertesting/parsing/quote_amount_in_response_simulated.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- router/usecase/routertesting/parsing/quote_amount_in_response_simulated.json
🚧 Files skipped from review as they are similar to previous changes (5)
- domain/mocks/quote_simulator_mock.go
- domain/quote_simulator.go
- domain/router.go
- router/types/get_quote_request.go
- router/usecase/quote_out_given_in.go
🔇 Additional comments (17)
domain/mocks/quote_mock.go (3)
1-10: LGTM! Package declaration and imports are clean and well-organized.
70-70: LGTM! Interface compliance check is properly implemented.
1-70: Verify interface method coverage.
Let's ensure that all methods from the domain.Quote interface are implemented in this mock.
✅ Verification successful
Let me try a different search pattern to find the implemented methods since the previous one didn't yield results.
Let me try one final verification for the interface compliance check with a corrected regex.
Let me try one last time with a simpler pattern to find the interface compliance check.
Based on the results gathered, I can now provide the final response:
Mock implementation correctly covers all interface methods
Comparing the interface definition with the mock implementation:
- All 9 methods from the
Quoteinterface are implemented:GetAmountIn,GetAmountOut,GetRoute,GetEffectiveFee,GetPriceImpact,GetInBaseOutQuoteSpotPrice,PrepareResult,SetQuotePriceInfo, andString - The mock includes function fields for customizing behavior (
GetAmountInFunc,GetRouteFunc) - Unimplemented methods properly panic with "unimplemented" message
- Interface compliance is enforced with the type assertion
var _ domain.Quote = &MockQuote{}
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all domain.Quote interface methods are implemented in MockQuote
# Search for the Quote interface definition
echo "Quote interface definition:"
ast-grep --pattern 'type Quote interface {
$$$
}'
# Search for implemented methods in MockQuote
echo -e "\nImplemented methods in MockQuote:"
ast-grep --pattern 'func \(m \*MockQuote\) $_\('
Length of output: 1456
Script:
#!/bin/bash
# Description: Verify that all domain.Quote interface methods are implemented in MockQuote
# Search for the Quote interface definition (already found above)
echo "Implemented methods in MockQuote (using ripgrep):"
rg "func \(m \*MockQuote\)" -A 1
# Also verify the interface compliance check
echo -e "\nInterface compliance check:"
rg "var _.+ domain\.Quote.+= &MockQuote{}"
Length of output: 1731
Script:
#!/bin/bash
# Search for the interface compliance check with fixed regex
echo "Interface compliance check:"
rg "var _.* domain\.Quote.* = &MockQuote{}"
Length of output: 268
Script:
#!/bin/bash
# Search for the interface compliance check with simpler pattern
echo "Interface compliance check:"
rg "var _ domain.Quote = &MockQuote{}"
Length of output: 253
domain/mocks/msg_simulator_mock.go (2)
34-41: LGTM! Function field declaration is well-structured.
The PriceMsgsFn field declaration is correct with appropriate parameter types and return values.
72-82: Type inconsistency issue still present.
The previously identified issue regarding message parameter type inconsistency remains valid. The method should use sdk.Msg instead of the generic interface constraint to maintain consistency with other methods in the mock.
domain/mocks/route_mock.go (2)
20-22: LGTM: New function pointers follow established patterns.
The new function pointer fields GetAmountInFunc and GetAmountOutFunc maintain consistency with the existing mock implementation pattern and use appropriate types for amount calculations.
103-105: Verify SplitRoute interface implementation completeness.
The mock now implements both domain.Route and domain.SplitRoute interfaces. Let's ensure all required methods are implemented.
✅ Verification successful
Let me gather more information about the implemented methods in RouteMock specifically.
Interface implementations are complete and correct
The RouteMock correctly implements all required methods:
- All 7 methods from the
Routeinterface:ContainsGeneralizedCosmWasmPool,GetPools,CalculateTokenOutByTokenIn,GetTokenOutDenom,GetTokenInDenom,PrepareResultPools, andString - Both methods from the
SplitRouteinterface:GetAmountInandGetAmountOut(note thatSplitRouteembedsRoute, so it inherits allRoutemethods)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all required methods of SplitRoute interface are implemented
# Test: Search for the SplitRoute interface definition
echo "Searching for SplitRoute interface definition..."
ast-grep --pattern 'type SplitRoute interface {
$$$
}'
# Test: Compare with implemented methods
echo "Listing all implemented methods in RouteMock..."
ast-grep --pattern 'func (r *RouteMock) $_($_) $_ {
$$$
}'
Length of output: 527
Script:
#!/bin/bash
# Search for the Route interface definition first
echo "Searching for Route interface definition..."
ast-grep --pattern 'type Route interface {
$$$
}'
# Search for all method implementations in RouteMock with ripgrep for better context
echo "Searching for RouteMock method implementations..."
rg "func \(r \*RouteMock\)" -A 1
Length of output: 3936
app/sidecar_query_server.go (2)
219-227: Add error handling for quote simulator initialization.
The past review comment about adding error handling for quote simulator initialization is still valid. This ensures proper error propagation during server startup.
228-228: Verify router handler implementation.
The router handler is now initialized with the quote simulator. Please ensure that the RouterHandler implementation properly utilizes this parameter for quote simulations.
router/delivery/http/router_handler.go (4)
22-25: LGTM: Clean struct field addition.
The QuoteSimulator field is properly integrated into the RouterHandler struct, maintaining a logical field ordering.
39-44: LGTM: Clean constructor update.
The constructor properly initializes the new QuoteSimulator field while maintaining the existing initialization pattern.
152-152:
Fix unsafe variable assignment.
Direct assignment of req.SimulatorAddress could be unsafe if the field is not properly validated.
This issue was previously flagged in past reviews. The same fix applies here:
-simulatorAddress := req.SimulatorAddress
+// Validate simulator address if provided
+if req.SimulatorAddress != "" {
+ if err := domain.ValidateAddress(req.SimulatorAddress); err != nil {
+ return c.JSON(http.StatusBadRequest, domain.ResponseError{Message: "invalid simulator address"})
+ }
+}
+simulatorAddress := req.SimulatorAddress159-163: Consider validating simulation results.
The quote price info is set without validating the simulation results.
Let's verify if there are any validation requirements for gasUsed and feeCoin:
quotesimulator/quote_simulator.go (1)
1-82: Overall implementation is solid.
The code is well-structured and follows best practices.
domain/cosmos/tx/msg_simulator.go (3)
41-51: Addition of PriceMsgs method to MsgSimulator interface is well-defined
The new method PriceMsgs enhances the MsgSimulator interface by consolidating gas estimation and fee calculation, which improves functionality and code clarity.
93-93: Refactored BuildTx method improves efficiency
By using PriceMsgs to retrieve both the adjusted gas and fee coin, the BuildTx method is streamlined, enhancing code maintainability and reducing redundancy.
Also applies to: 98-99
147-166: Implementation of PriceMsgs method correctly reuses existing logic
The PriceMsgs function correctly simulates messages to calculate adjusted gas and fee coin, utilizing SimulateMsgs and CalculateFeeCoin with proper error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (11)
tests/quote_response.py (2)
44-46: Add type hints and validation for price_info.Consider adding type hints and validation to ensure
price_infohas the expected structure. This will help catch issues earlier and improve code maintainability.- def __init__(self, amount_in, amount_out, route, effective_fee, price_impact, in_base_out_quote_spot_price, price_info): + def __init__(self, amount_in, amount_out, route, effective_fee, price_impact, in_base_out_quote_spot_price, price_info: Optional[Dict[str, Any]] = None): self.amount_in = Coin(**amount_in) self.amount_out = int(amount_out) self.route = [Route(**r) for r in route] self.effective_fee = Decimal(effective_fee) self.price_impact = Decimal(price_impact) self.in_base_out_quote_spot_price = Decimal(in_base_out_quote_spot_price) if price_info: + # Add validation for required fields + if not isinstance(price_info, dict): + raise ValueError("price_info must be a dictionary") self.price_info = price_info
Line range hint
1-1: Improve decimal module import.The current import style could lead to namespace pollution. Consider using a more specific import.
-from decimal import * +from decimal import Decimal🧰 Tools
🪛 Ruff
40-40:
Decimalmay be undefined, or defined from star imports(F405)
41-41:
Decimalmay be undefined, or defined from star imports(F405)
42-42:
Decimalmay be undefined, or defined from star imports(F405)
tests/sqs_service.py (3)
98-98: Update docstring to document new parameters.The method signature has been updated with simulation-related parameters, but the docstring doesn't describe them. Consider updating it to include:
simulator_address: Description of its purpose and formatsimulation_slippage_tolerance: Description of its purpose and expected value range
111-112: Consider conditional parameter inclusion.Empty simulation parameters are currently always included in the request URL. Consider adding them only when they have non-empty values to keep the URLs clean and consistent with the API's behavior.
params = { "tokenIn": denom_in, "tokenOutDenom": denom_out, "humanDenoms": human_denoms, - "singleRoute": singleRoute, - "simulatorAddress": simulator_address, - "simulationSlippageTolerance": simulation_slippage_tolerance, + "singleRoute": singleRoute } + if simulator_address: + params["simulatorAddress"] = simulator_address + if simulation_slippage_tolerance: + params["simulationSlippageTolerance"] = simulation_slippage_tolerance
Test coverage needed for simulation parameters
The verification shows that while the
get_exact_amount_in_quotemethod andrun_quote_testhelper have been updated to support simulation parameters, there are no actual test cases exercising these new parameters. The codebase needs:
- Test cases that verify behavior with
simulator_addressparameter- Test cases that verify behavior with
simulation_slippage_toleranceparameter- Test cases that verify the interaction between both parameters
🔗 Analysis chain
Line range hint
98-112: Verify test coverage for simulation parameters.The changes add simulation-related parameters to the quote API. Let's verify that these parameters are properly tested.
Let me check the test coverage in
tests/quote.pyto verify if the simulation parameters are properly tested.
Let me check the test implementation to see how these parameters are being used in the actual test cases.
Let me check if there are any actual test cases that use these simulation parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files using get_exact_amount_in_quote with simulation parameters echo "Searching for test files using simulation parameters..." rg -l "get_exact_amount_in_quote.*simulator_address.*simulation_slippage_tolerance" tests/ # Search for test files using get_exact_amount_in_quote that might need updates echo "Searching for other test files that might need updates..." rg -l "get_exact_amount_in_quote" tests/ | grep -v "sqs_service.py"Length of output: 455
Script:
#!/bin/bash # Check the test coverage for simulation parameters in quote.py echo "Checking test coverage in quote.py..." rg -A 10 "get_exact_amount_in_quote" tests/quote.pyLength of output: 834
Script:
#!/bin/bash # Search for test cases using these parameters echo "Searching for test cases using simulation parameters..." rg -B 5 -A 15 "simulator_address.*simulation_slippage_tolerance" tests/quote.pyLength of output: 1941
Script:
#!/bin/bash # Search for test cases that call run_quote_test with simulation parameters echo "Searching for test cases using run_quote_test..." rg -B 5 "run_quote_test.*simulator.*slippage" tests/quote.py # Also search for test function definitions that might test simulation echo "Searching for test functions related to simulation..." rg "def test.*simul" tests/quote.pyLength of output: 732
tests/test_router_quote_out_given_in.py (3)
291-308: Enhance test coverage for simulation slippage tolerance.While the test provides basic validation, consider enhancing it with:
- Multiple test cases using
@pytest.mark.parametrizeto test different slippage tolerance values- Validation of the actual slippage behavior in the simulation
- Error cases (e.g., negative slippage, zero slippage)
- Documentation of why specific values were chosen (1000000uosmo, 1.2 slippage)
Example enhancement:
@pytest.mark.parametrize("slippage_tolerance,expected_result", [ (1.2, True), # Normal case (0, False), # Zero slippage (-0.1, False), # Negative slippage (100, True), # High slippage ]) def test_simulation_slippage_tolerance(self, environment_url, slippage_tolerance, expected_result): """ Test simulation slippage tolerance with various scenarios: - Normal case: 1.2% slippage tolerance - Edge case: 0% slippage tolerance - Invalid case: Negative slippage tolerance - Extreme case: Very high slippage tolerance Amount chosen (1000000uosmo) represents a significant trade to ensure observable price impact. """ token_in_coin = "1000000uosmo" # ~$1000 USD at typical OSMO prices denom_out = "uion" fillbot_address = "osmo10s3vlv40h64qs2p98yal9w0tpm4r30uyg6ceux" quote = ExactAmountInQuote.run_quote_test( environment_url, token_in_coin, denom_out, False, False, EXPECTED_LATENCY_UPPER_BOUND_MS, fillbot_address, slippage_tolerance ) assert quote is not None assert quote.price_info is not None # Add assertions to validate slippage behavior assert (quote.price_impact <= slippage_tolerance) == expected_result🧰 Tools
🪛 Ruff
304-304:
ExactAmountInQuotemay be undefined, or defined from star imports(F405)
304-304: Consider explicit imports instead of star imports.The static analysis tool indicates that
ExactAmountInQuotemay be imported via star import. For better code maintainability and clarity, consider making the import explicit.- from quote import * + from quote import ExactAmountInQuote🧰 Tools
🪛 Ruff
304-304:
ExactAmountInQuotemay be undefined, or defined from star imports(F405)
292-294: Align docstring with existing test patterns.Consider enhancing the docstring to match the detail level of other test methods in the file. Include:
- Test scenarios and edge cases covered
- Explanation of chosen test values
- Expected behavior and validation criteria
Example:
""" This test validates that the simulation slippage tolerance is working as expected. Test scenarios: - Validates quote simulation with specified slippage tolerance - Ensures price info is correctly populated - Verifies simulation with fillbot address Parameters: - token_in_coin: 1000000uosmo (~$1000 USD) chosen to ensure observable price impact - slippage_tolerance: 1.2% represents a typical trading scenario - fillbot_address: Known address with sufficient balance for simulation Expected behavior: - Quote simulation succeeds with given slippage tolerance - Price info reflects the simulation results - Transaction would be executable within slippage bounds """tests/quote.py (3)
120-120: Consider explicit import of QuoteExactAmountInResponse.While the method signature changes look good, the return type
QuoteExactAmountInResponseis imported via star import. Consider making this import explicit for better code maintainability.-from quote_response import * +from quote_response import QuoteExactAmountInResponse🧰 Tools
🪛 Ruff
120-120:
QuoteExactAmountInResponsemay be undefined, or defined from star imports(F405)
133-133: Replace lambda with def function for better readability.The lambda function should be rewritten as a regular function definition for improved code clarity and maintainability.
- service_call = lambda: conftest.SERVICE_MAP[environment_url].get_exact_amount_in_quote(token_in, token_out, human_denoms, single_route, simulator_address, simulation_slippage_tolerance) + def service_call(): + return conftest.SERVICE_MAP[environment_url].get_exact_amount_in_quote( + token_in, + token_out, + human_denoms, + single_route, + simulator_address, + simulation_slippage_tolerance + )🧰 Tools
🪛 Ruff
133-133: Do not assign a
lambdaexpression, use adefRewrite
service_callas adef(E731)
Test coverage for simulation parameters needs improvement
The current test coverage for simulation parameters is insufficient:
- Only one happy path test case exists with a fixed slippage tolerance of 1.2
- No validation of simulator_address format or edge cases
- No error scenarios tested (empty address, invalid slippage values)
- No assertions specific to simulation behavior
Key areas needing test coverage:
- Test with invalid simulator addresses
- Test with boundary values for slippage tolerance (0, negative, very large values)
- Test with empty/invalid slippage tolerance values
- Verify simulation-specific response fields and behavior
🔗 Analysis chain
Line range hint
120-133: Add test cases for simulation parameters.The new parameters
simulator_addressandsimulation_slippage_toleranceshould have dedicated test cases to verify:
- Valid values processing
- Edge cases (empty strings, invalid values)
- Error scenarios
Would you like me to help generate comprehensive test cases for these parameters?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any test cases for the new parameters rg -l "simulation_slippage_tolerance.*=.*['\"].*['\"]" tests/Length of output: 103
Script:
#!/bin/bash # Check the actual test cases in the files rg -B 5 -A 5 "simulation_slippage_tolerance.*=.*['\"].*['\"]" tests/Length of output: 1518
Script:
#!/bin/bash # Look for test functions that might be testing these parameters rg -B 5 -A 5 "def test.*simul" tests/Length of output: 990
Script:
#!/bin/bash # Check if there are any assertions related to these parameters rg "assert.*simul" tests/Length of output: 27
Script:
#!/bin/bash # Let's see the complete implementation of the simulation slippage tolerance test rg -B 10 -A 20 "def test_simulation_slippage_tolerance" tests/Length of output: 2621
🧰 Tools
🪛 Ruff
120-120:
QuoteExactAmountInResponsemay be undefined, or defined from star imports(F405)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
tests/quote.py(2 hunks)tests/quote_response.py(1 hunks)tests/sqs_service.py(2 hunks)tests/test_router_quote_out_given_in.py(1 hunks)
🧰 Additional context used
🪛 Ruff
tests/quote.py
120-120: QuoteExactAmountInResponse may be undefined, or defined from star imports
(F405)
133-133: Do not assign a lambda expression, use a def
Rewrite service_call as a def
(E731)
tests/quote_response.py
40-40: Decimal may be undefined, or defined from star imports
(F405)
41-41: Decimal may be undefined, or defined from star imports
(F405)
42-42: Decimal may be undefined, or defined from star imports
(F405)
tests/test_router_quote_out_given_in.py
304-304: ExactAmountInQuote may be undefined, or defined from star imports
(F405)
🔇 Additional comments (1)
tests/quote_response.py (1)
36-36: Verify backward compatibility of constructor changes.
The addition of the price_info parameter without a default value could break existing test cases.
✅ Verification successful
No compatibility concerns with constructor changes
The code inspection shows that all instances of QuoteExactAmountInResponse are instantiated using **response or **res pattern, which unpacks the API response directly. This pattern automatically handles new fields in the response without requiring code changes, making it backward compatible. The API will provide the price_info field, and the unpacking will handle it seamlessly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing instantiations of QuoteExactAmountInResponse to verify they've been updated
rg "QuoteExactAmountInResponse\(" --type python
Length of output: 80
Script:
#!/bin/bash
# Search for existing instantiations without file type flag since rg doesn't recognize python
rg "QuoteExactAmountInResponse\(" -A 2
Length of output: 735
Script:
#!/bin/bash
# Check the response dictionary construction to see if price_info is included
rg -B 5 "QuoteExactAmountInResponse\(\*\*"
Length of output: 2290
Script:
#!/bin/bash
# Check the response JSON structure from the API to verify price_info field
rg -B 10 '"price_info".*:' tests/
Length of output: 35
| type QuoteSimulator interface { | ||
| // SimulateQuote simulates a quote and returns the gas adjusted amount and the fee coin. | ||
| // CONTRACT: | ||
| // - Only direct (non-split) quotes are supported. |
There was a problem hiding this comment.
Why only non-split routes are supported? Nit: typo in Retursn
There was a problem hiding this comment.
Splits require a separate message and increase the complexity drastically.
My ultimate goal is to integrate SQS into Polaris at the lowest cost, and then we can iterate on improvements, including adding splits
| slippageBound := tokenOutAmt.ToLegacyDec().Mul(slippageToleranceMultiplier).TruncateInt() | ||
|
|
||
| // Create the swap message | ||
| swapMsg := &poolmanagertypes.MsgSwapExactAmountIn{ |
There was a problem hiding this comment.
Can it be that result can vary depending of swap method ( poolmanagertypes.MsgSwapExactAmountIn vs poolmanagertypes.MsgSwapExactAmountOut ) for some cases such as imbalanced pools, etc?
There was a problem hiding this comment.
"in given out" is not supported in simulation for the same reason I mentioned here:
#547 (comment)
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (10)
router/types/get_quote_request.go (1)
73-109: Consider structural improvements for better maintainability.A few suggestions to enhance the code:
- Define constants for magic values
- Simplify the else block by early return
- Add more specific error messages
Consider these improvements:
+const ( + // MaxSlippageTolerance represents the maximum allowed slippage (50%) + MaxSlippageTolerance = "0.5" +) func validateSimulationParams(swapMethod domain.TokenSwapMethod, simulatorAddress string, slippageToleranceStr string) (osmomath.Dec, error) { + // Return early if no simulation is requested + if simulatorAddress == "" { + if slippageToleranceStr != "" { + return osmomath.Dec{}, fmt.Errorf("slippage tolerance cannot be set without simulator address") + } + return osmomath.Dec{}, nil + } + // Rest of the function... - } else { - if slippageToleranceStr != "" { - return osmomath.Dec{}, fmt.Errorf("slippage tolerance is not supported without simulator address") - } - } - - return osmomath.Dec{}, nil }domain/passthrough/passthrough_grpc_client.go (1)
Line range hint
204-231: Consider architectural improvements for better maintainability.
The
paginateRequesthelper could be moved to a shared utility package as it's a generic pagination implementation that could be reused across other services.Consider adding context timeout management to prevent long-running queries from blocking resources.
Example implementation for context timeout:
func paginateRequest(ctx context.Context, fetchCoinsFn func(context.Context, *query.PageRequest) (*query.PageResponse, sdk.Coins, error)) (sdk.Coins, error) { // Add timeout to context ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() // ... rest of the implementation }tests/test_router_quote_out_given_in.py (1)
Line range hint
4-13: Consider replacing star imports with explicit imports.While star imports work in test files, explicit imports would make the code more maintainable by clearly showing which symbols are used from each module.
-from sqs_service import * -from quote import * -from quote_response import * -from rand_util import * -from e2e_math import * -from decimal import * -from constants import * -from chain_service import * -from util import * -from route import * +from sqs_service import SERVICE_MAP +from quote import ExactAmountInQuote, ExactAmountOutQuote, Quote +from quote_response import QuoteExactAmountInResponse +from rand_util import id_from_coin, id_from_swap_pair +from e2e_math import relative_error +from decimal import Decimal +from constants import USDC, UOSMO, UOSMO_USDC_POOL_ID +from chain_service import Coin +from util import skip_imbalanced_pool_test_if_imbalanced +from route import Route🧰 Tools
🪛 Ruff
286-286:
relative_errormay be undefined, or defined from star imports(F405)
307-307:
ExactAmountInQuotemay be undefined, or defined from star imports(F405)
docs/swagger.yaml (2)
145-146: Enhance themath.Intdefinition with detailed schema properties.The current definition only specifies it as an object type without any properties. Consider adding detailed schema properties to better document the structure and constraints of
math.Int.math.Int: type: object + properties: + value: + type: string + description: String representation of the integer value + description: Represents an arbitrary-precision integer
410-419: Consider using number type for slippageToleranceMultiplier.The
slippageToleranceMultiplierparameter represents a numerical value but is currently defined as a string type. For better type safety and validation, consider changing it to a number type with appropriate constraints.- description: Slippage tolerance multiplier for the simulation. If simulatorAddress is provided, this must be provided. in: query name: slippageToleranceMultiplier - type: string + type: number + format: double + minimum: 0 + maximum: 1tests/quote.py (1)
120-120: LGTM! Consider explicit imports.The method signature changes appropriately support simulation parameters. However, consider replacing star imports with explicit imports to improve code maintainability and avoid potential naming conflicts.
Replace:
from quote_response import *with:
from quote_response import QuoteExactAmountInResponse🧰 Tools
🪛 Ruff
120-120:
QuoteExactAmountInResponsemay be undefined, or defined from star imports(F405)
docs/swagger.json (1)
520-520: Consider enhancing the math.Int definition.While the basic object definition is sufficient, consider adding a description field to clarify that this represents a big integer type used for token amounts.
Apply this enhancement:
"math.Int": { - "type": "object" + "type": "object", + "description": "Represents a big integer type used for token amounts" },Also applies to: 669-671
quotesimulator/quote_simulator.go (3)
60-65: Abstract swap message creation to support different swap types.Currently, the swap message is hardcoded to
MsgSwapExactAmountIn. Abstracting the message creation would allow support forMsgSwapExactAmountOutand enhance the flexibility of the simulator.Consider refactoring the swap message creation like this:
-var swapMsg := &poolmanagertypes.MsgSwapExactAmountIn{ - Sender: simulatorAddress, - Routes: poolManagerRoute, - TokenIn: quote.GetAmountIn(), - TokenOutMinAmount: slippageBound, -} +var swapMsg sdk.Msg +if quote.IsExactIn() { + swapMsg = &poolmanagertypes.MsgSwapExactAmountIn{ + Sender: simulatorAddress, + Routes: poolManagerRoute, + TokenIn: quote.GetAmountIn(), + TokenOutMinAmount: slippageBound, + } +} else { + swapMsg = &poolmanagertypes.MsgSwapExactAmountOut{ + Sender: simulatorAddress, + Routes: poolManagerRoute, + TokenOut: quote.GetAmountOut(), + TokenInMaxAmount: slippageBound, + } +}
38-80: Enhance error messages for better troubleshooting.Some error messages could be more descriptive to aid in debugging, especially in a multi-component system where errors may propagate.
Consider updating error messages like so:
if len(route) != 1 { - return 0, sdk.Coin{}, fmt.Errorf("route length must be 1, got %d", len(route)) + return 0, sdk.Coin{}, fmt.Errorf("invalid route length in quote simulation: expected 1, got %d", len(route)) } ... if err != nil { - return 0, sdk.Coin{}, err + return 0, sdk.Coin{}, fmt.Errorf("failed to get account for simulator address %s: %w", simulatorAddress, err) }
1-82: Add unit tests forquoteSimulatormethods to ensure reliability.Implementing unit tests for
SimulateQuotewill help catch potential issues early and ensure that future changes do not break existing functionality.Would you like assistance in creating a suite of unit tests for this implementation?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
quotesimulator/quote_simulator_test.gois excluded by!**/*_test.go
📒 Files selected for processing (11)
docs/docs.go(3 hunks)docs/swagger.json(3 hunks)docs/swagger.yaml(3 hunks)domain/mocks/passthrough_grpc_client_mock.go(1 hunks)domain/mocks/quote_mock.go(1 hunks)domain/passthrough/passthrough_grpc_client.go(5 hunks)quotesimulator/quote_simulator.go(1 hunks)router/delivery/http/router_handler.go(5 hunks)router/types/get_quote_request.go(3 hunks)tests/quote.py(2 hunks)tests/test_router_quote_out_given_in.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- domain/mocks/quote_mock.go
- router/delivery/http/router_handler.go
🧰 Additional context used
🪛 Ruff
tests/quote.py
120-120: QuoteExactAmountInResponse may be undefined, or defined from star imports
(F405)
133-133: Do not assign a lambda expression, use a def
Rewrite service_call as a def
(E731)
138-138: QuoteExactAmountInResponse may be undefined, or defined from star imports
(F405)
tests/test_router_quote_out_given_in.py
307-307: ExactAmountInQuote may be undefined, or defined from star imports
(F405)
🔇 Additional comments (13)
domain/mocks/passthrough_grpc_client_mock.go (1)
23-23: LGTM: Good interface-based design!
The change from concrete type *grpc.ClientConn to interface type grpc.ClientConnInterface is a good practice as it:
- Makes the mock more flexible and loosely coupled
- Follows interface-based design principles
- Aligns with the actual implementation
router/types/get_quote_request.go (2)
15-23: LGTM! Well-structured struct definition.
The new fields are appropriately typed and follow Go naming conventions.
59-69: LGTM! Clean parameter handling.
The implementation properly retrieves and validates simulation parameters before setting them to the struct fields.
domain/passthrough/passthrough_grpc_client.go (4)
42-42: LGTM! Good interface design.
The change to grpc.ClientConnInterface improves abstraction and testability by depending on interfaces rather than concrete implementations.
59-59: LGTM! Consistent interface usage.
The struct field type change maintains consistency with the interface changes.
198-200: LGTM! Clean implementation.
The getter implementation is straightforward and correctly returns the interface.
14-14: Verify observability coverage with polarisgrpc.
The switch from OpenTelemetry to polarisgrpc might affect observability. Please ensure that polarisgrpc provides equivalent instrumentation capabilities for monitoring and tracing.
✅ Verification successful
Let me check the actual implementation to verify if the instrumentation is properly handled.
OpenTelemetry instrumentation is properly maintained
The polarisgrpc package maintains OpenTelemetry instrumentation through otelgrpc.NewClientHandler(). The implementation in delivery/grpc/grpc.go shows that the gRPC client is configured with the OpenTelemetry stats handler, ensuring observability is not compromised.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if polarisgrpc package includes instrumentation
rg -A 5 "instrumentation|telemetry|metrics|tracing" "delivery/grpc"
Length of output: 721
Script:
#!/bin/bash
# Check the NewClient implementation in polarisgrpc
ast-grep --pattern 'func NewClient($$$) $_ {
$$$
}' delivery/grpc/grpc.go
Length of output: 1153
tests/test_router_quote_out_given_in.py (1)
301-304: Verify fillbot configuration and simulation parameters.
The test assumes the fillbot address has sufficient OSMO tokens. Let's verify this assumption and check how simulation parameters are handled.
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information about the fillbot address usage and simulation parameters.
Fillbot address and simulation parameters are properly configured for testing
The fillbot address is only used in test scenarios and is properly passed through the stack:
- The address is only referenced in
tests/test_router_quote_out_given_in.py - The simulation parameters are correctly propagated through the service layer in
tests/sqs_service.py - The slippage tolerance value of 0.8 is used specifically for testing purposes and doesn't conflict with any other configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify fillbot configuration and simulation parameter handling
# Test 1: Check if fillbot address is configured in environment files
echo "Checking fillbot configuration..."
rg -l "osmo10s3vlv40h64qs2p98yal9w0tpm4r30uyg6ceux"
# Test 2: Check how simulation parameters are handled in the codebase
echo "Checking simulation parameter handling..."
rg -A 5 "simulation_slippage_tolerance"
# Test 3: Check for any hardcoded slippage values that might conflict
echo "Checking for hardcoded slippage values..."
rg -A 2 "slippage.*=.*0\.8"
Length of output: 3685
tests/quote.py (2)
133-133: LGTM! Service call correctly handles simulation parameters.
The lambda function appropriately forwards the simulation parameters to the quote service.
🧰 Tools
🪛 Ruff
133-133: Do not assign a lambda expression, use a def
Rewrite service_call as a def
(E731)
138-144: LGTM! Clean handling of optional price info.
The response handling properly manages the optional price info field, maintaining backward compatibility while supporting the new simulation features.
🧰 Tools
🪛 Ruff
138-138: QuoteExactAmountInResponse may be undefined, or defined from star imports
(F405)
docs/swagger.json (1)
293-304: LGTM! Well-documented query parameters for quote simulation.
The new parameters are clearly documented with appropriate descriptions and dependency rules.
docs/docs.go (2)
302-313: LGTM! Well-documented query parameters for quote simulation.
The new query parameters are clearly documented with their dependencies and requirements properly specified.
529-529: LGTM! Appropriate schema update for Cosmos SDK's math.Int type.
The schema reference has been correctly updated to use math.Int, and the new definition has been added with the appropriate type.
Also applies to: 678-680
| func (p *PassthroughGRPCClientMock) GetChainGRPCClient() grpc.ClientConnInterface { | ||
| panic("unimplemented") | ||
| } |
There was a problem hiding this comment.
Implement the mock method instead of panicking.
The current implementation panics with "unimplemented", which could cause tests to fail unexpectedly. Consider implementing a proper mock that returns a mock gRPC connection.
Here's a suggested implementation:
func (p *PassthroughGRPCClientMock) GetChainGRPCClient() grpc.ClientConnInterface {
- panic("unimplemented")
+ // Return a no-op implementation that satisfies the interface
+ return &grpc.ClientConn{}
}Alternatively, you could add a callback field to allow tests to provide their own implementation:
type PassthroughGRPCClientMock struct {
+ MockGetChainGRPCClientCb func() grpc.ClientConnInterface
MockAllBalancesCb func(ctx context.Context, address string) (sdk.Coins, error)
// ... other fields ...
}
func (p *PassthroughGRPCClientMock) GetChainGRPCClient() grpc.ClientConnInterface {
- panic("unimplemented")
+ if p.MockGetChainGRPCClientCb != nil {
+ return p.MockGetChainGRPCClientCb()
+ }
+ return &grpc.ClientConn{}
}Committable suggestion skipped: line range outside the PR's diff.
| if slippageTolerance.LTE(osmomath.ZeroDec()) { | ||
| return osmomath.Dec{}, fmt.Errorf("slippage tolerance must be greater than 0") | ||
| } |
There was a problem hiding this comment.
Add upper bound for slippage tolerance.
Currently, the code only checks if slippage tolerance is greater than 0, which could allow extremely high values that might be dangerous in production. Consider adding an upper bound check.
Apply this diff:
- if slippageTolerance.LTE(osmomath.ZeroDec()) {
- return osmomath.Dec{}, fmt.Errorf("slippage tolerance must be greater than 0")
+ // Common practice is to limit slippage to 50% max
+ maxSlippage := osmomath.NewDec(50).Quo(osmomath.NewDec(100))
+ if slippageTolerance.LTE(osmomath.ZeroDec()) || slippageTolerance.GT(maxSlippage) {
+ return osmomath.Dec{}, fmt.Errorf("slippage tolerance must be between 0 and 0.5 (50%%)")📝 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.
| if slippageTolerance.LTE(osmomath.ZeroDec()) { | |
| return osmomath.Dec{}, fmt.Errorf("slippage tolerance must be greater than 0") | |
| } | |
| // Common practice is to limit slippage to 50% max | |
| maxSlippage := osmomath.NewDec(50).Quo(osmomath.NewDec(100)) | |
| if slippageTolerance.LTE(osmomath.ZeroDec()) || slippageTolerance.GT(maxSlippage) { | |
| return osmomath.Dec{}, fmt.Errorf("slippage tolerance must be between 0 and 0.5 (50%%)") | |
| } |
| grpc.WithTransportCredentials(insecure.NewCredentials()), | ||
| grpc.WithStatsHandler(otelgrpc.NewClientHandler()), | ||
| ) | ||
| grpcClient, err := polarisgrpc.NewClient(grpcURI) |
There was a problem hiding this comment.
💡 Codebase verification
Security concern: Insecure gRPC connections detected
The verification reveals that the current implementation uses insecure credentials (insecure.NewCredentials()) without proper TLS configuration. This is consistent across multiple gRPC client implementations in the codebase. While connection timeouts are handled in various parts of the system, the simplified client creation with polarisgrpc.NewClient needs to be updated to:
- Replace insecure credentials with proper TLS configuration
- Explicitly configure connection timeouts
- Consider adding retry policies for improved reliability
Locations requiring attention:
delivery/grpc/grpc.go: Currently usinginsecure.NewCredentials()app/sidecar_query_server.go: Also using insecure transportpools/usecase/pools_usecase.go: Same security concern
🔗 Analysis chain
Verify security and reliability features of polarisgrpc.
The simplified client creation removes explicit transport credentials. Please ensure that polarisgrpc.NewClient properly handles:
- Secure connections (TLS)
- Connection timeouts
- Retry policies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check polarisgrpc client configuration
rg -A 10 "NewClient.*func" "delivery/grpc"
# Check for security configurations
rg "TLS|Certificate|Credential" "delivery/grpc"
# Check for timeout and retry configurations
rg "timeout|retry|backoff" "delivery/grpc"
Length of output: 756
Script:
#!/bin/bash
# Let's check the actual implementation of NewClient in polarisgrpc
fd "polaris.*grpc" --type f --exec cat {}
# Also check for any configuration or options being set
rg -A 5 "WithTransportCredentials|WithTimeout|WithRetry"
Length of output: 2390
| def test_simulation_slippage_tolerance(self, environment_url): | ||
| """ | ||
| This test validates that the simulation slippage tolerance is working as expected. | ||
| """ | ||
| token_in_coin = "1000000uosmo" | ||
| denom_out = "uion" | ||
|
|
||
| expected_status_code = 200 | ||
|
|
||
| # Fillbot address and slippage tolerance | ||
| # We choose fillbot address because we expect it to have at least one OSMO. | ||
| fillbot_address = "osmo10s3vlv40h64qs2p98yal9w0tpm4r30uyg6ceux" | ||
| # Note: relaxed | ||
| simulation_slippage_tolerance = 0.8 | ||
|
|
||
| # Run the quote test | ||
| quote = ExactAmountInQuote.run_quote_test(environment_url, token_in_coin, denom_out, False, True, EXPECTED_LATENCY_UPPER_BOUND_MS, expected_status_code, fillbot_address, simulation_slippage_tolerance) | ||
|
|
||
| # Validate that the price info is set to something | ||
| assert quote is not None | ||
| assert quote.price_info is not None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage and document high slippage tolerance.
The test has several areas for improvement:
- The slippage tolerance of 80% is unusually high and should be documented with a clear explanation of why such a relaxed value is used for testing.
- The test lacks comprehensive assertions - it only checks if the quote and price_info are not None.
- There's inconsistent indentation at line 306.
Consider enhancing the test with these changes:
def test_simulation_slippage_tolerance(self, environment_url):
"""
This test validates that the simulation slippage tolerance is working as expected.
+
+ Note: The high slippage tolerance of 80% is used only for testing purposes
+ to ensure the simulation works even in extreme conditions. In production,
+ much lower values should be used.
"""
token_in_coin = "1000000uosmo"
denom_out = "uion"
expected_status_code = 200
# Fillbot address and slippage tolerance
# We choose fillbot address because we expect it to have at least one OSMO.
fillbot_address = "osmo10s3vlv40h64qs2p98yal9w0tpm4r30uyg6ceux"
- # Note: relaxed
simulation_slippage_tolerance = 0.8
- # Run the quote test
+ # Run the quote test
quote = ExactAmountInQuote.run_quote_test(
environment_url,
token_in_coin,
denom_out,
False,
True,
EXPECTED_LATENCY_UPPER_BOUND_MS,
expected_status_code,
fillbot_address,
simulation_slippage_tolerance
)
# Validate that the price info is set to something
assert quote is not None
assert quote.price_info is not None
+ # Additional assertions to validate simulation behavior
+ assert quote.amount_in.denom == "uosmo"
+ assert quote.amount_in.amount == 1000000
+ assert quote.amount_out > 0
+ assert "simulation_slippage_tolerance" in quote.price_info
+ assert float(quote.price_info["simulation_slippage_tolerance"]) == simulation_slippage_tolerance📝 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.
| def test_simulation_slippage_tolerance(self, environment_url): | |
| """ | |
| This test validates that the simulation slippage tolerance is working as expected. | |
| """ | |
| token_in_coin = "1000000uosmo" | |
| denom_out = "uion" | |
| expected_status_code = 200 | |
| # Fillbot address and slippage tolerance | |
| # We choose fillbot address because we expect it to have at least one OSMO. | |
| fillbot_address = "osmo10s3vlv40h64qs2p98yal9w0tpm4r30uyg6ceux" | |
| # Note: relaxed | |
| simulation_slippage_tolerance = 0.8 | |
| # Run the quote test | |
| quote = ExactAmountInQuote.run_quote_test(environment_url, token_in_coin, denom_out, False, True, EXPECTED_LATENCY_UPPER_BOUND_MS, expected_status_code, fillbot_address, simulation_slippage_tolerance) | |
| # Validate that the price info is set to something | |
| assert quote is not None | |
| assert quote.price_info is not None | |
| def test_simulation_slippage_tolerance(self, environment_url): | |
| """ | |
| This test validates that the simulation slippage tolerance is working as expected. | |
| Note: The high slippage tolerance of 80% is used only for testing purposes | |
| to ensure the simulation works even in extreme conditions. In production, | |
| much lower values should be used. | |
| """ | |
| token_in_coin = "1000000uosmo" | |
| denom_out = "uion" | |
| expected_status_code = 200 | |
| # Fillbot address and slippage tolerance | |
| # We choose fillbot address because we expect it to have at least one OSMO. | |
| fillbot_address = "osmo10s3vlv40h64qs2p98yal9w0tpm4r30uyg6ceux" | |
| simulation_slippage_tolerance = 0.8 | |
| # Run the quote test | |
| quote = ExactAmountInQuote.run_quote_test( | |
| environment_url, | |
| token_in_coin, | |
| denom_out, | |
| False, | |
| True, | |
| EXPECTED_LATENCY_UPPER_BOUND_MS, | |
| expected_status_code, | |
| fillbot_address, | |
| simulation_slippage_tolerance | |
| ) | |
| # Validate that the price info is set to something | |
| assert quote is not None | |
| assert quote.price_info is not None | |
| # Additional assertions to validate simulation behavior | |
| assert quote.amount_in.denom == "uosmo" | |
| assert quote.amount_in.amount == 1000000 | |
| assert quote.amount_out > 0 | |
| assert "simulation_slippage_tolerance" in quote.price_info | |
| assert float(quote.price_info["simulation_slippage_tolerance"]) == simulation_slippage_tolerance |
🧰 Tools
🪛 Ruff
307-307: ExactAmountInQuote may be undefined, or defined from star imports
(F405)
| } | ||
|
|
||
| // SimulateQuote implements domain.QuoteSimulator | ||
| func (q *quoteSimulator) SimulateQuote(ctx context.Context, quote domain.Quote, slippageToleranceMultiplier osmomath.Dec, simulatorAddress string) (uint64, sdk.Coin, error) { |
There was a problem hiding this comment.
Add validation for simulatorAddress to ensure it's not empty or invalid.
The simulatorAddress is used to retrieve the account and set the sender in the swap message. If it's empty or invalid, it could lead to errors when querying the account or during message execution.
Apply this diff to add validation for simulatorAddress:
func (q *quoteSimulator) SimulateQuote(ctx context.Context, quote domain.Quote, slippageToleranceMultiplier osmomath.Dec, simulatorAddress string) (uint64, sdk.Coin, error) {
+ if simulatorAddress == "" {
+ return 0, sdk.Coin{}, fmt.Errorf("simulator address must not be empty")
+ }| poolsInRoute := route[0].GetPools() | ||
|
|
There was a problem hiding this comment.
Handle the case where poolsInRoute is empty.
If poolsInRoute is empty, the subsequent loop will not execute, and the poolManagerRoute will remain empty, potentially leading to an invalid swap message.
Apply this diff to check for an empty poolsInRoute:
poolsInRoute := route[0].GetPools()
+if len(poolsInRoute) == 0 {
+ return 0, sdk.Coin{}, fmt.Errorf("no pools found in the route")
+}📝 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.
| poolsInRoute := route[0].GetPools() | |
| poolsInRoute := route[0].GetPools() | |
| if len(poolsInRoute) == 0 { | |
| return 0, sdk.Coin{}, fmt.Errorf("no pools found in the route") | |
| } |
| // Slippage bound from the token in and provided slippage tolerance multiplier | ||
| tokenOutAmt := quote.GetAmountOut() | ||
| slippageBound := tokenOutAmt.ToLegacyDec().Mul(slippageToleranceMultiplier).TruncateInt() | ||
|
|
There was a problem hiding this comment.
Validate that slippageBound is positive to prevent invalid swap messages.
If slippageBound is zero or negative, it could result in an invalid TokenOutMinAmount in the swap message, causing the simulation to fail or behave unexpectedly.
Apply this diff to ensure slippageBound is positive:
slippageBound := tokenOutAmt.ToLegacyDec().Mul(slippageToleranceMultiplier).TruncateInt()
+if !slippageBound.IsPositive() {
+ return 0, sdk.Coin{}, fmt.Errorf("slippage bound must be positive")
+}Committable suggestion skipped: line range outside the PR's diff.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
tests/quote_response.py (2)
36-44: Add type hints and documentation for price_info parameter.While the implementation is correct, consider adding type hints and docstring to clarify:
- The expected type/structure of
price_info- The purpose and usage of this parameter
- Example values or format
Example improvement:
def __init__(self, amount_in, amount_out, route, effective_fee, price_impact, - in_base_out_quote_spot_price, price_info=None): + in_base_out_quote_spot_price, price_info: Optional[Dict[str, Any]] = None): + """Initialize QuoteExactAmountInResponse. + + Args: + price_info: Optional dictionary containing simulation price data + from quote estimation. + """🧰 Tools
🪛 Ruff
40-40:
Decimalmay be undefined, or defined from star imports(F405)
41-41:
Decimalmay be undefined, or defined from star imports(F405)
42-42:
Decimalmay be undefined, or defined from star imports(F405)
Line range hint
1-1: Replace wildcard import with explicit import.The current wildcard import
from decimal import *triggers static analysis warnings and can lead to namespace pollution. Consider explicitly importing theDecimalclass:-from decimal import * +from decimal import Decimal🧰 Tools
🪛 Ruff
40-40:
Decimalmay be undefined, or defined from star imports(F405)
41-41:
Decimalmay be undefined, or defined from star imports(F405)
42-42:
Decimalmay be undefined, or defined from star imports(F405)
docs/swagger.yaml (1)
410-419: Enhance simulation parameters with validation and examples.The new parameters would benefit from additional OpenAPI specifications for better documentation and validation.
- description: Address of the simulator to simulate the quote. If provided, the quote will be simulated. in: query name: simulatorAddress type: string + example: "osmo1cyyzpxplxdzkeea7kwsydadg87357qnahakaks" + pattern: "^osmo1[a-zA-Z0-9]{38}$" - description: Slippage tolerance multiplier for the simulation. If simulatorAddress is provided, this must be provided. in: query name: simulationSlippageTolerance type: string + example: "0.01" + pattern: "^0\\.\\d+$" + minimum: "0" + maximum: "1"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
docs/docs.go(3 hunks)docs/swagger.json(3 hunks)docs/swagger.yaml(3 hunks)ingest/usecase/plugins/orderbook/fillbot/context/block/block_context.go(1 hunks)router/delivery/http/router_handler.go(5 hunks)tests/quote_response.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/docs.go
- docs/swagger.json
- router/delivery/http/router_handler.go
🧰 Additional context used
🪛 Ruff
tests/quote_response.py
40-40: Decimal may be undefined, or defined from star imports
(F405)
41-41: Decimal may be undefined, or defined from star imports
(F405)
42-42: Decimal may be undefined, or defined from star imports
(F405)
🔇 Additional comments (1)
ingest/usecase/plugins/orderbook/fillbot/context/block/block_context.go (1)
61-61: Great improvement using interface instead of concrete type!
Changing from *grpc.ClientConn to grpc.ClientConnInterface is a good practice as it:
- Follows interface-based design principles
- Improves testability by allowing mock implementations
- Maintains backward compatibility since
*grpc.ClientConnimplements the interface
| @@ -58,7 +58,7 @@ type BlockGasPrice struct { | |||
| var _ BlockCtxI = &blockContext{} | |||
|
|
|||
| // New creates a new block context. | |||
| func New(ctx context.Context, chainGRPCClient *grpc.ClientConn, uniqueDenoms []string, orderBookDenomPrices domain.PricesResult, userBalances sdk.Coins, defaultQuoteDenom string, blockHeight uint64) (*blockContext, error) { | |||
| func New(ctx context.Context, chainGRPCClient grpc.ClientConnInterface, uniqueDenoms []string, orderBookDenomPrices domain.PricesResult, userBalances sdk.Coins, defaultQuoteDenom string, blockHeight uint64) (*blockContext, error) { | |||
There was a problem hiding this comment.
💡 Codebase verification
Remove unused parameter chainGRPCClient
The chainGRPCClient parameter is not used within the New function implementation or stored in the blockContext struct. It appears only in the function signature and is not referenced anywhere else in the file. This parameter can be safely removed.
🔗 Analysis chain
Verify if the chainGRPCClient parameter is needed.
The chainGRPCClient parameter isn't used within the function body. If it's not used elsewhere, consider removing it to maintain clean code.
Let's check if this parameter is used in other methods or stored for later use:
Let me gather more information to verify if the parameter is actually used within the block_context implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for chainGRPCClient usage in the codebase
rg -l 'chainGRPCClient'
# Search for any gRPC client field in the blockContext struct
ast-grep --pattern 'type blockContext struct {
$$$
$gRPC $Type
$$$
}'
Length of output: 219
Script:
#!/bin/bash
# Check the implementation of blockContext to see if chainGRPCClient is stored or used
rg -A 10 "func New\(" ingest/usecase/plugins/orderbook/fillbot/context/block/block_context.go
# Look for any method that might use chainGRPCClient in the same file
rg "chainGRPCClient" ingest/usecase/plugins/orderbook/fillbot/context/block/block_context.go
Length of output: 844
| math.Int: | ||
| type: object |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the math.Int type definition with proper schema.
The current definition as an empty object type lacks proper schema definition for validation and documentation purposes. Consider adding proper type constraints and description.
math.Int:
- type: object
+ type: object
+ description: Represents a arbitrary-precision integer using math/big package
+ properties:
+ i:
+ type: string
+ description: String representation of the arbitrary-precision integer
+ required:
+ - i📝 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.
| math.Int: | |
| type: object | |
| math.Int: | |
| type: object | |
| description: Represents a arbitrary-precision integer using math/big package | |
| properties: | |
| i: | |
| type: string | |
| description: String representation of the arbitrary-precision integer | |
| required: | |
| - i |
* feat: simulate swap as part of quotes * try adding e2e test * updates * lint * fix test * swagger * attempt fix grpc wiring * fix e2e sim test * updates (cherry picked from commit f2931d8) # Conflicts: # domain/cosmos/tx/msg_simulator.go # domain/cosmos/tx/msg_simulator_test.go # domain/mocks/msg_simulator_mock.go # domain/passthrough/passthrough_grpc_client.go
* feat: simulate swap as part of quotes * try adding e2e test * updates * lint * fix test * swagger * attempt fix grpc wiring * fix e2e sim test * updates (cherry picked from commit f2931d8) # Conflicts: # domain/cosmos/tx/msg_simulator.go # domain/cosmos/tx/msg_simulator_test.go # domain/mocks/msg_simulator_mock.go # domain/passthrough/passthrough_grpc_client.go Co-authored-by: Roman <[email protected]>
* feat: simulate swap as part of quotes * try adding e2e test * updates * lint * fix test * swagger * attempt fix grpc wiring * fix e2e sim test * updates
https://osmosis-network.slack.com/archives/C079LNKRUCB/p1730651766930679
Implementing
MsgSwapExactAmountInquote estimation logic directly into SQS.This will allow to wire SQS integration into Polaris. Ref: https://github.com/polaris-portal/polaris/pull/1011/files
Currently, in Polaris, as we get a quote, we also want to know the pricing for it.
To avoid round trips, we can have an optional query parameter to request SQS to run simulation logic immediately after estimating a quote.
Support
simulationAddressandsimulationSlippageTolerancequery parametersTesting
Summary by CodeRabbit
Release Notes
New Features
PriceMsgsmethod for improved gas estimation and fee calculation in message simulations.QuoteSimulatorinterface and aSimulateQuotemethod for simulating quotes in token swaps.GetQuoteRequestto handle simulation parameters, includingSimulatorAddressandSlippageToleranceMultiplier.QuoteSimulatorfor testing purposes.Improvements
RouterHandlerto utilize the new quote simulation functionality.SetQuotePriceInfomethod to manage quote price data effectively.Bug Fixes
Summary by CodeRabbit
Release Notes
New Features
PriceMsgsmethod for improved gas estimation and fee calculation in message simulation.QuoteSimulatorinterface andSimulateQuotemethod for simulating quotes with slippage tolerance.GetOptimalQuotemethod to support quote simulation based on a specified address and slippage tolerance.GetQuoteRequestto manage simulation parameters.price_infoparameter toQuoteExactAmountInResponsefor additional pricing details.Bug Fixes
Documentation
Tests