From 61ee610c04231c5a656aae72362b17c5cd7b3f09 Mon Sep 17 00:00:00 2001 From: Roman Date: Tue, 29 Oct 2024 16:41:27 -0600 Subject: [PATCH] Allow unlisted denoms in the /custom-direct-quote --- CHANGELOG.md | 4 + domain/mocks/tokens_usecase_mock.go | 9 ++ domain/mvc/tokens.go | 22 +++- domain/mvc/tokens_test.go | 162 ++++++++++++++++++++++++ router/delivery/http/router_handler.go | 14 +- tokens/delivery/http/tokens_delivery.go | 4 +- tokens/usecase/tokens_usecase.go | 29 +++-- tokens/usecase/tokens_usecase_test.go | 49 ++++++- 8 files changed, 271 insertions(+), 22 deletions(-) create mode 100644 domain/mvc/tokens_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 844ceb7e1..b2ac84823 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,10 @@ Ref: https://keepachangelog.com/en/1.0.0/ # Changelog +## Unreleased + +- Allow unlisted denoms in the /custom-direct-quote endpoint. + ## v26.1.0 e42b32bc SQS-412 | Active Orders Query: SSE (#518) diff --git a/domain/mocks/tokens_usecase_mock.go b/domain/mocks/tokens_usecase_mock.go index ce2c4c92e..462042529 100644 --- a/domain/mocks/tokens_usecase_mock.go +++ b/domain/mocks/tokens_usecase_mock.go @@ -31,6 +31,7 @@ type TokensUsecaseMock struct { UpdateAssetsAtHeightIntervalSyncFunc func(height uint64) error SetTokenRegistryLoaderFunc func(loader domain.TokenRegistryLoader) ClearPoolDenomMetadataFunc func() + IsValidListedDenomFunc func(chainDenom string) bool } var _ mvc.TokensUsecase = &TokensUsecaseMock{} @@ -172,3 +173,11 @@ func (m *TokensUsecaseMock) ClearPoolDenomMetadata() { } panic("unimplemented") } + +// IsValidListedDenom implements mvc.TokensUsecase. +func (m *TokensUsecaseMock) IsValidListedDenom(chainDenom string) bool { + if m.IsValidListedDenomFunc != nil { + return m.IsValidListedDenomFunc(chainDenom) + } + return false +} diff --git a/domain/mvc/tokens.go b/domain/mvc/tokens.go index a611dbd4a..9b63481b6 100644 --- a/domain/mvc/tokens.go +++ b/domain/mvc/tokens.go @@ -79,8 +79,12 @@ type TokensUsecase interface { // RegisterPricingStrategy registers a pricing strategy for a given pricing source. RegisterPricingStrategy(source domain.PricingSourceType, strategy domain.PricingSource) + // IsValidChainDenom checks if the chain denom is valid IsValidChainDenom(chainDenom string) bool + // IsValidListedDenom checks if the chain denom is valid + IsValidListedDenom(chainDenom string) bool + // IsValidPricingSource checks if the pricing source is a valid one IsValidPricingSource(pricingSource int) bool @@ -101,8 +105,10 @@ type TokensUsecase interface { // ValidateChainDenomQueryParam validates the chain denom query parameter. // If isHumanDenoms is true, it converts the human denom to chain denom. // If isHumanDenoms is false, it validates the chain denom. +// If doesAllowUnlistedDenoms is true, it allows unlisted denoms. +// If doesAllowUnlistedDenoms is false, it does not allow unlisted denoms. // Returns the chain denom and an error if any. -func ValidateChainDenomQueryParam(tokensUsecase TokensUsecase, denom string, isHumanDenoms bool) (string, error) { +func ValidateChainDenomQueryParam(tokensUsecase TokensUsecase, denom string, isHumanDenoms bool, doesAllowUnlistedDenoms bool) (string, error) { // Note that sdk.Coins initialization // auto-converts base denom from human // to IBC notation. @@ -120,8 +126,14 @@ func ValidateChainDenomQueryParam(tokensUsecase TokensUsecase, denom string, isH return tokensUsecase.GetChainDenom(denom) } } else { - if !tokensUsecase.IsValidChainDenom(denom) { - return "", fmt.Errorf("denom is not a valid chain denom (%s)", denom) + if !doesAllowUnlistedDenoms { + if !tokensUsecase.IsValidListedDenom(denom) { + return "", fmt.Errorf("denom is not a valid listed chain denom (%s)", denom) + } + } else { + if !tokensUsecase.IsValidChainDenom(denom) { + return "", fmt.Errorf("denom is not a valid chain denom (%s)", denom) + } } } @@ -130,7 +142,7 @@ func ValidateChainDenomQueryParam(tokensUsecase TokensUsecase, denom string, isH } // ValidateChainDenomsQueryParam validates the chain denom query parameters. -func ValidateChainDenomsQueryParam(c echo.Context, tokensUsecase TokensUsecase, denoms []string) ([]string, error) { +func ValidateChainDenomsQueryParam(c echo.Context, tokensUsecase TokensUsecase, denoms []string, doesAllowUnlistedDenoms bool) ([]string, error) { isHumanDenoms, err := domain.GetIsHumanDenomsQueryParam(c) if err != nil { return nil, err @@ -138,7 +150,7 @@ func ValidateChainDenomsQueryParam(c echo.Context, tokensUsecase TokensUsecase, chainDenoms := make([]string, len(denoms)) for i, denom := range denoms { - chainDenom, err := ValidateChainDenomQueryParam(tokensUsecase, denom, isHumanDenoms) + chainDenom, err := ValidateChainDenomQueryParam(tokensUsecase, denom, isHumanDenoms, doesAllowUnlistedDenoms) if err != nil { return nil, err } diff --git a/domain/mvc/tokens_test.go b/domain/mvc/tokens_test.go new file mode 100644 index 000000000..697c85ba3 --- /dev/null +++ b/domain/mvc/tokens_test.go @@ -0,0 +1,162 @@ +package mvc_test + +import ( + "errors" + "fmt" + "testing" + + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/osmosis-labs/sqs/domain/mocks" + "github.com/osmosis-labs/sqs/domain/mvc" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + denom = "uatom" + ibcDenom = "ibc/" + denom +) + +func TestValidateChainDenomQueryParam(t *testing.T) { + + baseDenom, err := sdk.GetBaseDenom() + require.NoError(t, err) + + defaultError := errors.New("default error") + + // Test cases + tests := []struct { + name string + denom string + isHumanDenoms bool + doesAllowUnlistedDenoms bool + mock *mocks.TokensUsecaseMock + expectedDenom string + expectedError string + baseError bool // true if we expect base denom error + }{ + { + name: "Error validating base denom", + denom: "not" + baseDenom, + isHumanDenoms: true, + doesAllowUnlistedDenoms: true, + mock: &mocks.TokensUsecaseMock{ + GetChainDenomFunc: func(humanDenom string) (string, error) { + return "", defaultError + }, + }, + expectedDenom: "", + expectedError: defaultError.Error(), + }, + { + name: "Human denom conversion success", + denom: denom, + isHumanDenoms: true, + doesAllowUnlistedDenoms: true, + mock: &mocks.TokensUsecaseMock{ + GetChainDenomFunc: func(humanDenom string) (string, error) { + return "ibc/" + humanDenom, nil + }, + }, + expectedDenom: ibcDenom, + expectedError: "", + }, + { + name: "Human denom conversion success", + denom: denom, + isHumanDenoms: true, + doesAllowUnlistedDenoms: true, + mock: &mocks.TokensUsecaseMock{ + GetChainDenomFunc: func(humanDenom string) (string, error) { + return ibcDenom, nil + }, + }, + expectedDenom: ibcDenom, + expectedError: "", + }, + { + name: "Human denom conversion error", + denom: "invalid", + isHumanDenoms: true, + doesAllowUnlistedDenoms: true, + mock: &mocks.TokensUsecaseMock{ + GetChainDenomFunc: func(humanDenom string) (string, error) { + return "", fmt.Errorf("invalid denom") + }, + }, + expectedDenom: "", + expectedError: "invalid denom", + }, + { + name: "Valid unlisted chain denom", + denom: ibcDenom, + isHumanDenoms: false, + doesAllowUnlistedDenoms: true, + mock: &mocks.TokensUsecaseMock{ + IsValidChainDenomFunc: func(chainDenom string) bool { + return true + }, + }, + expectedDenom: ibcDenom, + expectedError: "", + }, + { + name: "Invalid chain denom with unlisted allowed", + denom: "invalid", + isHumanDenoms: false, + doesAllowUnlistedDenoms: true, + mock: &mocks.TokensUsecaseMock{ + IsValidChainDenomFunc: func(chainDenom string) bool { + return false + }, + }, + expectedDenom: "", + expectedError: "denom is not a valid chain denom (invalid)", + }, + { + name: "Unlisted chain denom when not allowed", + denom: ibcDenom, + isHumanDenoms: false, + doesAllowUnlistedDenoms: false, + mock: &mocks.TokensUsecaseMock{ + IsValidListedDenomFunc: func(denom string) bool { + return false + }, + }, + expectedDenom: "", + expectedError: "denom is not a valid listed chain denom (" + ibcDenom + ")", + }, + { + name: "Valid listed chain denom", + denom: ibcDenom, + isHumanDenoms: false, + doesAllowUnlistedDenoms: false, + mock: &mocks.TokensUsecaseMock{ + IsValidListedDenomFunc: func(denom string) bool { + return true + }, + }, + expectedDenom: ibcDenom, + expectedError: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // System under test + result, err := mvc.ValidateChainDenomQueryParam(tt.mock, tt.denom, tt.isHumanDenoms, tt.doesAllowUnlistedDenoms) + + // Assert results + if tt.baseError { + assert.Empty(t, result) + assert.NoError(t, err) + } else if tt.expectedError != "" { + assert.Empty(t, result) + assert.EqualError(t, err, tt.expectedError) + } else { + assert.Equal(t, tt.expectedDenom, result) + assert.NoError(t, err) + } + }) + } +} diff --git a/router/delivery/http/router_handler.go b/router/delivery/http/router_handler.go index df9928951..914a1751f 100644 --- a/router/delivery/http/router_handler.go +++ b/router/delivery/http/router_handler.go @@ -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 + chainDenoms, err := mvc.ValidateChainDenomsQueryParam(c, a.TUsecase, []string{tokenIn.Denom, tokenOutDenom}, doesAllowUnlistedDenoms) if err != nil { return c.JSON(http.StatusBadRequest, domain.ResponseError{Message: err.Error()}) } @@ -200,7 +202,10 @@ func (a *RouterHandler) GetDirectCustomQuote(c echo.Context) (err error) { } // Apply human denoms conversion if required. - chainDenoms, err := mvc.ValidateChainDenomsQueryParam(c, a.TUsecase, append([]string{tokenIn.Denom}, tokenOutDenom...)) + // We allow unlisted denoms since this is a production endpoint from the pools page + // where we want to support all chain denoms. + doesAllowUnlistedDenoms := true + chainDenoms, err := mvc.ValidateChainDenomsQueryParam(c, a.TUsecase, append([]string{tokenIn.Denom}, tokenOutDenom...), doesAllowUnlistedDenoms) if err != nil { return c.JSON(http.StatusBadRequest, domain.ResponseError{Message: err.Error()}) } @@ -250,7 +255,10 @@ func (a *RouterHandler) GetCandidateRoutes(c echo.Context) error { return c.JSON(domain.GetStatusCode(err), domain.ResponseError{Message: err.Error()}) } - chainDenoms, err := mvc.ValidateChainDenomsQueryParam(c, a.TUsecase, []string{tokenIn, tokenOutDenom}) + // We do not allow unlisted denoms to maintain + // compatibility with the /quote endpoint. + doesAllowUnlistedDenoms := false + chainDenoms, err := mvc.ValidateChainDenomsQueryParam(c, a.TUsecase, []string{tokenIn, tokenOutDenom}, doesAllowUnlistedDenoms) if err != nil { return c.JSON(domain.GetStatusCode(err), domain.ResponseError{Message: err.Error()}) } diff --git a/tokens/delivery/http/tokens_delivery.go b/tokens/delivery/http/tokens_delivery.go index 92e98ab1e..92df2aced 100644 --- a/tokens/delivery/http/tokens_delivery.go +++ b/tokens/delivery/http/tokens_delivery.go @@ -141,7 +141,9 @@ func (a *TokensHandler) GetPoolDenomMetadata(c echo.Context) (err error) { denoms := strings.Split(denomsStr, ",") // Validate denom parameters and convert to chain denoms if necessary. - chainDenoms, err := mvc.ValidateChainDenomsQueryParam(c, a.TUsecase, denoms) + // We allow unlisted denoms since this is a debug endpoint. + doesAllowUnlistedDenoms := true + chainDenoms, err := mvc.ValidateChainDenomsQueryParam(c, a.TUsecase, denoms, doesAllowUnlistedDenoms) if err != nil { return c.JSON(http.StatusBadRequest, domain.ResponseError{Message: err.Error()}) } diff --git a/tokens/usecase/tokens_usecase.go b/tokens/usecase/tokens_usecase.go index fa75cb93d..cea14b541 100644 --- a/tokens/usecase/tokens_usecase.go +++ b/tokens/usecase/tokens_usecase.go @@ -406,18 +406,8 @@ func (t *tokensUseCase) RegisterPricingStrategy(source domain.PricingSourceType, // IsValidChainDenom implements mvc.TokensUsecase. func (t *tokensUseCase) IsValidChainDenom(chainDenom string) bool { - metaData, ok := t.tokenMetadataByChainDenom.Load(chainDenom) - if !ok { - return false - } - - v, ok := metaData.(domain.Token) - if !ok { - return false - } - - // is valid only if token is found and is not unlisted - return !v.IsUnlisted + _, ok := t.tokenMetadataByChainDenom.Load(chainDenom) + return ok } // GetMinPoolLiquidityCap implements mvc.TokensUsecase. @@ -466,3 +456,18 @@ func (t *tokensUseCase) GetCoingeckoIdByChainDenom(chainDenom string) (string, e return v, nil } + +// 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 +} diff --git a/tokens/usecase/tokens_usecase_test.go b/tokens/usecase/tokens_usecase_test.go index 357dd2c40..c2c2a1e6e 100644 --- a/tokens/usecase/tokens_usecase_test.go +++ b/tokens/usecase/tokens_usecase_test.go @@ -562,7 +562,7 @@ func (s *TokensUseCaseTestSuite) TestGetMinPoolLiquidityCap() { } // Test to validate valid chain denom works as expected. -func (s *TokensUseCaseTestSuite) TestIsValidChainDenom() { +func (s *TokensUseCaseTestSuite) TestIsValidListedDenom() { testcases := []struct { name string chainDenom string @@ -595,6 +595,53 @@ func (s *TokensUseCaseTestSuite) TestIsValidChainDenom() { }, } + for _, tt := range testcases { + s.Run(tt.name, func() { + usecase := tokensusecase.NewTokensUsecase(nil, 0, nil) + for k, v := range tt.tokens { + usecase.SetTokenMetadataByChainDenom(k, v) + } + + result := usecase.IsValidListedDenom(tt.chainDenom) + s.Require().Equal(tt.expectedResult, result) + }) + } +} + +// Test to validate valid chain denom works as expected. +func (s *TokensUseCaseTestSuite) TestIsValidChainDenom() { + testcases := []struct { + name string + chainDenom string + tokens map[string]any // domain.Token + expectedResult bool + }{ + { + name: "Valid chain denom", + chainDenom: "validDenom", + tokens: map[string]any{"validDenom": domain.Token{IsUnlisted: false}}, + expectedResult: true, + }, + { + name: "Invalid chain denom - not found", + chainDenom: "invalidDenom", + tokens: map[string]any{"validDenom": domain.Token{IsUnlisted: false}}, + expectedResult: false, + }, + { + name: "unlisted chain denom - still valid", + chainDenom: "unlistedDenom", + tokens: map[string]any{"unlistedDenom": domain.Token{IsUnlisted: true}}, + expectedResult: true, + }, + { + name: "Invalid type - not a domain.Token (no type type chain denom)", + chainDenom: "invalidtype", + tokens: map[string]any{"invalidtype": 1}, + expectedResult: true, + }, + } + for _, tt := range testcases { s.Run(tt.name, func() { usecase := tokensusecase.NewTokensUsecase(nil, 0, nil)