Allow unlisted denoms in the /custom-direct-quote #540
Conversation
WalkthroughThe changes in this pull request introduce modifications to the handling of token denominations, specifically allowing unlisted denominations in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RouterHandler
participant TokensUsecase
participant TokensHandler
Client->>RouterHandler: Request Quote
RouterHandler->>TokensUsecase: Validate Chain Denoms
TokensUsecase-->>RouterHandler: Validation Result
RouterHandler-->>Client: Response with Quote
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
domain/mocks/tokens_usecase_mock.go (1)
Interface implementation is complete but has incorrect panic behavior
The mock implements all methods from the main TokensUsecase interface and its embedded interfaces (TokenMetadataHolder and TokensPoolLiquidityHandler). However, there are two issues in the implementation:
SetTokenRegistryLoader: Has an unnecessarypanic("unimplemented")after the function callClearPoolDenomMetadata: Has an unnecessarypanic("unimplemented")after the function call
These panics will trigger even when the corresponding mock functions are set, which is incorrect behavior for a mock implementation.
🔗 Analysis chain
Line range hint 1-183: Verify interface implementation completeness.
Let's verify that this mock fully implements the mvc.TokensUsecase interface with the new changes.
Let me gather more information to verify the interface implementation completeness.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the interface is fully implemented by comparing method signatures
# Get the interface methods
echo "Interface methods:"
ast-grep --pattern 'type TokensUsecase interface {
$$$
}'
# Get the mock implementation methods
echo -e "\nMock implementation methods:"
ast-grep --pattern 'func \(m \*TokensUsecaseMock\) $$$'
Length of output: 5897
Script:
#!/bin/bash
# Let's check the embedded interfaces and their methods
# First, check TokenMetadataHolder interface
echo "TokenMetadataHolder interface methods:"
ast-grep --pattern 'type TokenMetadataHolder interface {
$$$
}'
# Then, check TokensPoolLiquidityHandler interface
echo -e "\nTokensPoolLiquidityHandler interface methods:"
ast-grep --pattern 'type TokensPoolLiquidityHandler interface {
$$$
}'
# Finally, let's get a clearer view of mock methods
echo -e "\nMock implementation methods (with better formatting):"
rg "^func \(m \*TokensUsecaseMock\)" -A 1
Length of output: 5945
domain/mvc/tokens.go (2)
85-86: Fix the documentation for IsValidListedDenom.
The comment appears to be copied from IsValidChainDenom. It should specifically describe that this method checks if the denom is a listed/registered denom rather than just being a valid chain denom.
-// IsValidListedDenom checks if the chain denom is valid
+// IsValidListedDenom checks if the chain denom is listed/registered in the token registry145-145: Add documentation for the new parameter.
The function's documentation should be updated to describe the doesAllowUnlistedDenoms parameter, similar to how it's documented in ValidateChainDenomQueryParam.
+// ValidateChainDenomsQueryParam validates the chain denom query parameters.
+// If doesAllowUnlistedDenoms is true, it allows unlisted denoms.
+// If doesAllowUnlistedDenoms is false, it does not allow unlisted denoms.
+// Returns the validated chain denoms and an error if any.
func ValidateChainDenomsQueryParam(c echo.Context, tokensUsecase TokensUsecase, denoms []string, doesAllowUnlistedDenoms bool) ([]string, error) {tokens/delivery/http/tokens_delivery.go (1)
144-144: Document debug endpoint in OpenAPI annotations.
Since this is a debug endpoint that allows unlisted denoms, please update the OpenAPI annotations above the function to:
- Mark it as a debug endpoint
- Document that it allows unlisted denoms
- Add any relevant security considerations
Apply this diff to update the documentation:
// @Summary Pool Denom Metadata
-// @Description returns pool denom metadata. As of today, this metadata is represented by the local market cap of the token computed over all Osmosis pools.
+// @Description [DEBUG ENDPOINT] Returns pool denom metadata, including unlisted denoms. The metadata is represented by the local market cap of the token computed over all Osmosis pools.
// @Description For testnet, uses osmo-test-5 asset list. For mainnet, uses osmosis-1 asset list.
// @Description See `config.json` and `config-testnet.json` in root for details.
+// @Description Note: This endpoint allows unlisted denominations as it is intended for debugging purposes.CHANGELOG.md (1)
40-40: Enhance the changelog entry format and content.
The changelog entry should follow the recommended format and include more details. Consider updating it as follows:
-Allow unlisted denoms in the /custom-direct-quote endpoint.
+- (Features) #BE-607 Enhanced /custom-direct-quote endpoint to support unlisted chain denominations from pool details page.This revision:
- Adds the bullet point for consistency with other entries
- Categorizes it as a feature
- References the Linear issue
- Provides more context about the change
router/delivery/http/router_handler.go (1)
Line range hint 108-261: Consider adding tests for unlisted denom validation.
The implementation looks good, but consider adding the following test cases:
- Test that
/custom-direct-quotesuccessfully accepts unlisted denoms - Test that
/quoteand/routesproperly reject unlisted denoms - Test edge cases with mixed listed and unlisted denoms
Would you like me to help generate these test cases?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
domain/mvc/tokens_test.gois excluded by!**/*_test.gotokens/usecase/tokens_usecase_test.gois excluded by!**/*_test.go
📒 Files selected for processing (6)
- CHANGELOG.md (1 hunks)
- domain/mocks/tokens_usecase_mock.go (2 hunks)
- domain/mvc/tokens.go (4 hunks)
- router/delivery/http/router_handler.go (3 hunks)
- tokens/delivery/http/tokens_delivery.go (1 hunks)
- tokens/usecase/tokens_usecase.go (2 hunks)
🔇 Additional comments (8)
domain/mocks/tokens_usecase_mock.go (1)
34-34: LGTM: Function pointer follows mock pattern.
The new IsValidListedDenomFunc field follows the established pattern in this mock struct, maintaining consistency with other mock function pointers.
domain/mvc/tokens.go (1)
Line range hint 108-142: LGTM! Well-structured validation logic.
The updated function properly implements the unlisted denoms feature with clear error messages and good documentation. The validation logic correctly branches between listed and unlisted denom validation based on the new parameter.
tokens/delivery/http/tokens_delivery.go (1)
144-146: LGTM! Change aligns with PR objectives.
The addition of doesAllowUnlistedDenoms parameter correctly implements the requirement to allow unlisted denoms in this endpoint.
tokens/usecase/tokens_usecase.go (2)
409-410: LGTM: Simplified chain denom validation
The simplified implementation correctly checks for the existence of the chain denomination in the metadata map, aligning with the PR objective to allow unlisted denoms.
Line range hint 409-473: Verify the impact of validation changes
The separation of chain denom validation from listed denom validation is a significant change. Let's verify that existing code doesn't rely on the old behavior of IsValidChainDenom.
✅ Verification successful
Let me gather more context about the validation usage in the codebase.
Based on the analysis of the code and test files, I can now provide a final response:
Chain denom validation changes are safe
The separation of IsValidChainDenom from IsValidListedDenom is well-designed and does not introduce breaking changes. The evidence shows:
-
IsValidChainDenomis only used in two places:tokens/delivery/http/tokens_delivery.go: For basic denom validationpassthrough/usecase/passthrough_usecase.go: For filtering valid denoms in portfolio pricing
-
The test cases confirm that
IsValidChainDenomcorrectly:- Returns true for both listed and unlisted denoms that exist in the registry
- Returns false only for non-existent denoms
-
The new
IsValidListedDenomadds an additional layer of validation without affecting the existing chain denom validation logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to IsValidChainDenom to identify potential impacts
echo "Searching for IsValidChainDenom usage..."
rg -A 5 "IsValidChainDenom\(" --type go
# Search for validation-related tests that might need updates
echo "Searching for related test files..."
fd -e go -e test | xargs rg -l "TestIsValidChainDenom|TestValidateChainDenom"
Length of output: 3353
Script:
#!/bin/bash
# Check the implementation of IsValidListedDenom usage
echo "Searching for IsValidListedDenom usage..."
rg -A 5 "IsValidListedDenom\(" --type go
# Check the test cases for both validations
echo "Checking test implementations..."
cat tokens/usecase/tokens_usecase_test.go
Length of output: 33130
router/delivery/http/router_handler.go (3)
108-110: LGTM! Clear intent and proper implementation.
The comment clearly explains why unlisted denoms are not allowed in this production endpoint, and the flag is correctly set to false for the swap tool.
258-261: Verify the compatibility requirement with /quote endpoint.
While maintaining compatibility with the /quote endpoint is a reasonable approach, please verify:
- If this strict compatibility is actually required
- If there are any use cases where allowing unlisted denoms in the routes endpoint could be beneficial
Let's check for dependencies between these endpoints:
✅ Verification successful
Based on the search results, I can now generate the final response as I have sufficient information to verify the compatibility requirement between the /quote and /routes endpoints:
Maintaining compatibility with /quote endpoint is justified
The code change to disallow unlisted denoms in /routes endpoint is correct because:
- Both
/quoteand/routesendpoints share the same validation logic throughValidateChainDenomsQueryParamfunction - The
/quoteendpoint is a core functionality that's heavily used in tests, monitoring (Locust load tests), and client integrations - The
/routesendpoint serves as a supporting endpoint to/quote, providing route information that follows the same token validation rules - Changelog entries show that token validation was intentionally aligned across
/quote,/routes, and/pricesendpoints
Allowing unlisted denoms in /routes while disallowing them in /quote would create an inconsistent API behavior where users could discover routes for tokens they cannot actually trade.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to these endpoints in the codebase
echo "Checking for quote endpoint usage..."
rg -A 3 "/quote"
echo "Checking for routes endpoint usage..."
rg -A 3 "/routes"
# Look for any shared validation logic
echo "Checking for shared validation code..."
rg -A 3 "ValidateChainDenomsQueryParam"
Length of output: 23310
205-208: Verify security implications of allowing unlisted denoms.
While the implementation correctly allows unlisted denoms as per the PR objective, please ensure that:
- The pools page UI properly warns users about the risks of interacting with unlisted tokens
- There are sufficient validation checks downstream to prevent potential security issues with malicious tokens
Let's check for any existing security measures:
✅ Verification successful
Let me search for more specific validation patterns related to unlisted denoms and token validation in the context of this endpoint.
Let me check the implementation of ValidateChainDenomQueryParam and related token validation logic.
Based on the gathered evidence, I can now provide a final response:
Security measures for unlisted denoms are properly implemented
The codebase shows appropriate handling of unlisted denoms:
- Token validation is implemented through
ValidateChainDenomQueryParamwhich explicitly handles thedoesAllowUnlistedDenomsflag - The
IsUnlistedproperty is properly tracked in the token domain model - Test cases demonstrate validation for both listed and unlisted tokens
- The implementation follows a clear pattern where unlisted tokens are only allowed when explicitly enabled
The original review comment's concerns about security implications are addressed by the existing implementation, which provides proper validation and tracking of unlisted token status.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for token validation and security checks in the codebase
echo "Checking for token validation patterns..."
rg -A 3 "ValidateToken|TokenValidation|SecurityCheck"
echo "Checking for security-related comments..."
rg -A 3 "SECURITY:|WARNING:|RISK:"
Length of output: 1748
Script:
#!/bin/bash
# Search for ValidateChainDenomsQueryParam implementation and related validation
echo "Checking ValidateChainDenomsQueryParam implementation..."
rg -A 10 "func.*ValidateChainDenomsQueryParam"
echo "Checking for unlisted denoms validation..."
rg -A 5 "allowUnlistedDenoms|unlistedDenom"
echo "Checking for token validation in usecase..."
rg -A 5 "func.*ValidateToken|func.*validateToken"
Length of output: 2214
Script:
#!/bin/bash
# Check the implementation of ValidateChainDenomQueryParam
echo "Checking ValidateChainDenomQueryParam implementation..."
rg -A 15 "func.*ValidateChainDenomQueryParam[^s]"
# Check token validation in the tokens usecase
echo "Checking token validation in tokens usecase..."
ast-grep --pattern 'func ($RECEIVER *tokensUseCase) IsTokenValid($ARGS) $RET {
$$$
}'
# Check for IsUnlisted token property usage
echo "Checking IsUnlisted usage..."
rg -A 5 "IsUnlisted"
Length of output: 8184
| // IsValidListedDenom implements mvc.TokensUsecase. | ||
| func (m *TokensUsecaseMock) IsValidListedDenom(chainDenom string) bool { | ||
| if m.IsValidListedDenomFunc != nil { | ||
| return m.IsValidListedDenomFunc(chainDenom) | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Fix potential panic in mock implementation.
The implementation returns false as default behavior, which is good. However, there's an inconsistency with other mock methods in this file that use panic("unimplemented") as their default behavior (see SetTokenRegistryLoader and ClearPoolDenomMetadata).
Consider either:
- Adding the panic for consistency:
func (m *TokensUsecaseMock) IsValidListedDenom(chainDenom string) bool {
if m.IsValidListedDenomFunc != nil {
return m.IsValidListedDenomFunc(chainDenom)
}
- return false
+ panic("unimplemented")
}- Or update other methods to return zero values instead of panicking for consistency across the mock.
Committable suggestion was skipped due to low confidence.
| // We allow unlisted denoms since this is a debug endpoint. | ||
| doesAllowUnlistedDenoms := true | ||
| chainDenoms, err := mvc.ValidateChainDenomsQueryParam(c, a.TUsecase, denoms, doesAllowUnlistedDenoms) |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
Review of doesAllowUnlistedDenoms usage shows consistent implementation
The codebase shows consistent and well-documented usage of ValidateChainDenomsQueryParam across different endpoints:
- Debug endpoints explicitly allow unlisted denoms (
doesAllowUnlistedDenoms = true) - Production/swap endpoints restrict to listed denoms (
doesAllowUnlistedDenoms = false) - Each usage has clear comments explaining the reasoning
However, there's a lack of direct test coverage for ValidateChainDenomsQueryParam in domain/mvc/tokens_test.go, which should be addressed.
🔗 Analysis chain
Verify test coverage and usage consistency.
Let's verify the test coverage and usage consistency for this change.
Let me generate additional verification scripts to check for test coverage since the previous fd command had an error.
Let me check the test coverage in the mvc package since we found a test file there.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage and other usages of ValidateChainDenomsQueryParam
# Test 1: Check for test files covering this endpoint
echo "Checking for test files..."
fd -e go -e _test.go tokens/delivery/http
# Test 2: Find other usages of ValidateChainDenomsQueryParam
echo "Checking other usages of ValidateChainDenomsQueryParam..."
rg "ValidateChainDenomsQueryParam" -A 3 -B 3
Length of output: 3786
Script:
#!/bin/bash
# Check for test files and test coverage
echo "Checking for test files in tokens/delivery/http..."
fd -e go . --full-path "tokens/delivery/http" | grep "_test.go"
# Check implementation of ValidateChainDenomsQueryParam
echo "Checking implementation of ValidateChainDenomsQueryParam..."
ast-grep --pattern 'func ValidateChainDenomsQueryParam($$$) {
$$$
}'
# Check for test files covering mvc package
echo "Checking for test files in domain/mvc..."
fd -e go . --full-path "domain/mvc" | grep "_test.go"
Length of output: 543
Script:
#!/bin/bash
# Check test coverage for ValidateChainDenomsQueryParam
echo "Checking test coverage in tokens_test.go..."
rg "TestValidateChainDenomsQueryParam|ValidateChainDenomsQueryParam.*test" -A 10 domain/mvc/tokens_test.go
# Check the actual implementation
echo "Checking implementation details..."
rg "func ValidateChainDenomsQueryParam" -A 15 domain/mvc/tokens.go
Length of output: 888
| // IsValidListedDenom implements mvc.TokensUsecase. | ||
| func (t *tokensUseCase) IsValidListedDenom(denom string) bool { | ||
| metaData, ok := t.tokenMetadataByChainDenom.Load(denom) | ||
| if !ok { | ||
| return false | ||
| } | ||
|
|
||
| v, ok := metaData.(domain.Token) | ||
| if !ok { | ||
| return false | ||
| } | ||
|
|
||
| return !v.IsUnlisted | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the implementation
The implementation is functionally correct but could be more concise. Consider this refactoring:
func (t *tokensUseCase) IsValidListedDenom(denom string) bool {
- metaData, ok := t.tokenMetadataByChainDenom.Load(denom)
- if !ok {
- return false
- }
-
- v, ok := metaData.(domain.Token)
- if !ok {
- return false
- }
-
- return !v.IsUnlisted
+ if metaData, ok := t.tokenMetadataByChainDenom.Load(denom); ok {
+ if v, ok := metaData.(domain.Token); ok {
+ return !v.IsUnlisted
+ }
+ }
+ return false
}Additionally, consider adding debug logging for failure cases to aid in troubleshooting:
func (t *tokensUseCase) IsValidListedDenom(denom string) bool {
if metaData, ok := t.tokenMetadataByChainDenom.Load(denom); ok {
if v, ok := metaData.(domain.Token); ok {
return !v.IsUnlisted
}
+ t.logger.Debug("Invalid type assertion for token metadata", zap.String("denom", denom))
}
+ t.logger.Debug("Denom not found in metadata", zap.String("denom", denom))
return false
}Committable suggestion was skipped due to low confidence.
| @@ -105,7 +105,9 @@ func (a *RouterHandler) GetOptimalQuote(c echo.Context) (err error) { | |||
| tokenIn, tokenOutDenom = req.TokenOut, req.TokenInDenom | |||
| } | |||
|
|
|||
| chainDenoms, err := mvc.ValidateChainDenomsQueryParam(c, a.TUsecase, []string{tokenIn.Denom, tokenOutDenom}) | |||
| // We do not allow unlisted denoms since this is a production from the swap tool. | |||
| doesAllowUnlistedDenoms := false | |||
There was a problem hiding this comment.
Should we allow client to specify doesAllowUnlistedDenoms param? Would defining AllowUnlistedDenoms field in types.GetQuoteRequest and setting default ( during Unmarsal ) make more sense and would allow to keep handlers smaller?
There was a problem hiding this comment.
Hmm not sure - we don't have a product use case for this.
deividaspetraitis
assigned deividaspetraitis and p0mvn and unassigned deividaspetraitis
|
This needs test fixing |
Request from @JeremyParish69
https://linear.app/osmosis/issue/BE-607/sqs-must-support-preview-assets
We would like to allow all chain denoms from the pool details page.
This PR enables that by relaxing the validating for denoms in the
/custom-direct-quoteendpointTesting
Summary by CodeRabbit
New Features
/custom-direct-quoteendpoint now supports unlisted denominations.Bug Fixes
Documentation
Refactor
IsValidChainDenommethod for improved performance and clarity.