BE-636 | Add pagination + sorting + query filtering support for /pools endpoint #553
Conversation
deividaspetraitis
force-pushed
the
BE-636
branch
from
3862c80 to
1f8b7e9
Compare
adding backport mergify action and created A:backport/v27.x label
* BE-586 | Claimbot prototype Init * BE-586 | WIP * BE-586 | WIP * BE-595 | Clean up * BE-586 | Add docs * BE-586 | Clean up * BE-586 | Add docs * BE-596 | Tests init * BE-586 | Add tests for slices, orderbook packages * BE-586 | claimbot/tx tests * BE-586 | claimbot/order.go tests * BE-586 | Requested changes * BE-586 | Process block orderbooks, tests * BE-586 | Requested changes * BE-586 | Config update * BE-586 | OrderBookClient use slices.Split for pagination Cleans up OrderBookClient by reusing slices.Split instead of duplicating splitting slices into chunks logic in some of the methods. * BE-586 | Clean up * BE-586 | Fix fillbot docker-compose Fixes errors running fillbot via docker-compose * BE-586 | Docs, docker compose fixes * BE-586 | Run fillbot via docker-compose * BE-586 | Run claimbot via docker-compose, clean up * BE-586 | Cleanup * BE-586 | Named logger * BE-586 | Requested changes * BE-586 | Logging failing tx * BE-586 | Increase gas adjustment * BE-586 | Error logging fix * BE-586 | Trace name update * BE-586 | Requested changes #1 * BE-586 | Requested changes #2 * BE-586 | Sequence number update * BE-586 | added tests * BE-586 | Suggested improvements
* refactor: gas estimation APIs * changelog * lint
* 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
…#550) * refactor: return base fee in /quote regardless of simulation success. * separate base fee from simulation. Pre-compute during ingest * updates * updates * Update router/repository/memory_router_repository.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix * separate interface * rename --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Updates CHANGELOG.md to reflect changes included for v26.2.0
deividaspetraitis
changed the title
WalkthroughThe pull request introduces significant changes across multiple files, primarily focusing on the enhancement of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 4
🧹 Outside diff range and nitpick comments (3)
domain/mocks/pool_handler_mock.go (1)
Line range hint
31-44: Ensure pools are returned when no filters are appliedIn the
GetPoolsmethod, when no pool IDs are specified and no filters are provided, pools are only added if they meet theMinLiquidityCapcondition. Ifoptions.Filterisnil, no pools are added, which may unintentionally exclude all pools.Apply this diff to ensure all pools are returned when no filters are applied:
} else { for _, pool := range p.Pools { - if f := options.Filter; f != nil { + if options.Filter != nil && options.Filter.MinLiquidityCap > 0 { if pool.GetLiquidityCap().Uint64() > options.Filter.MinLiquidityCap { result = append(result, pool) } } else { + result = append(result, pool) } } }This change ensures that all pools are returned when no filters are specified.
domain/pools.go (1)
131-132: Fix duplicate commentThe comment for WithFilter is incorrectly copied from WithPagination.
-// WithPagination configures the pools options with the pagination request. +// WithFilter configures the pools options with the filter request.app/sidecar_query_server.go (1)
150-157: LGTM: Well-structured dependency injection for enhanced pool operations.The addition of
tokensUseCaseas a dependency topoolsUseCaseis a good architectural decision that enables the new filtering and sorting capabilities for pools while maintaining proper initialization order.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (5)
pools/usecase/export_test.gois excluded by!**/*_test.gopools/usecase/pools_usecase_test.gois excluded by!**/*_test.gorouter/usecase/optimized_routes_test.gois excluded by!**/*_test.gorouter/usecase/router_usecase_test.gois excluded by!**/*_test.gotokens/usecase/pricing/worker/pool_liquidity_pricer_worket_test.gois excluded by!**/*_test.go
📒 Files selected for processing (11)
app/sidecar_query_server.go(1 hunks)domain/mocks/pool_handler_mock.go(2 hunks)domain/mocks/pools_usecase_mock.go(2 hunks)domain/mvc/pools.go(1 hunks)domain/pools.go(2 hunks)pools/delivery/http/pools_handler.go(4 hunks)pools/usecase/pools_usecase.go(8 hunks)router/delivery/http/http.go(0 hunks)router/usecase/routertesting/suite.go(1 hunks)router/usecase/worker/candidate_route_search_data_worker.go(1 hunks)tokens/usecase/pricing/worker/pool_liquidity_pricer_worker.go(1 hunks)
💤 Files with no reviewable changes (1)
- router/delivery/http/http.go
🔇 Additional comments (18)
pools/delivery/http/pools_handler.go (6)
14-15: New imports added are appropriate
The added imports for v1beta1 and api are necessary for the new functionalities introduced in the file.
49-54: Introduce GetPoolsResponse struct for response formatting
The new GetPoolsResponse struct properly encapsulates the Data and Meta fields, allowing for structured and paginated responses to the client.
94-96: Properly set filters, pagination, and sorting options
Filters, pagination, and sorting options are correctly extracted from the request and passed to the use case, enhancing the flexibility of the GetPools endpoint.
100-100: Update method to handle pagination total count
The GetPools use case now returns pools and total, correctly accommodating pagination by providing the total pool count.
108-108: Construct response using convertPoolsToResponse
The response is correctly constructed using the updated convertPoolsToResponse function, ensuring that both data and pagination metadata are included.
213-221: Update convertPoolsToResponse to include pagination metadata
The convertPoolsToResponse function now returns a GetPoolsResponse with both Data and Meta fields, utilizing v1beta1.NewPaginationResponse to include pagination information.
pools/usecase/pools_usecase.go (4)
46-48: Introduce TokenMetadataHolder interface
The new TokenMetadataHolder interface provides a method for retrieving token metadata, which enhances the ability to filter and search pools based on token information.
57-59: Add tokenMetadataHolder to poolsUseCase struct
Including tokenMetadataHolder in the poolsUseCase struct allows the use case to access token metadata necessary for enhanced filtering and sorting.
80-87: Update NewPoolsUsecase constructor to accept tokenMetadataHolder
The constructor is updated to accept tokenMetadataHolder, ensuring that the poolsUseCase initializes with all required dependencies.
114-116: Initialize tokenMetadataHolder in poolsUseCase
The tokenMetadataHolder field is properly assigned in the poolsUseCase initialization, maintaining consistency with the updated constructor.
domain/mvc/pools.go (1)
49-51: Update GetPools method signature to include total pool count
The GetPools method in the PoolHandler interface now returns an additional uint64 representing the total number of pools, which supports pagination features.
router/usecase/worker/candidate_route_search_data_worker.go (1)
89-89: Update GetPools call to accommodate new method signature
The GetPools method now returns an additional value for the total pool count. In this context, it's correctly ignored since the total count isn't needed.
domain/pools.go (4)
91-93: LGTM! Well-structured options pattern
The consolidation of options into Filter, Pagination, and Sort fields using pointers provides a clean and extensible way to handle optional parameters.
99-107: LGTM! Robust nil-safety pattern
The helper function ensures safe initialization of Filter before configuration, preventing potential nil pointer dereferences.
111-129: LGTM! Consistent use of WithNonNilFilter pattern
The filter-related functions consistently use WithNonNilFilter, providing a clean and safe way to configure filters.
131-149: LGTM! Clean option setters
The WithFilter, WithPagination, and WithSort functions provide a clean way to set their respective options.
domain/mocks/pools_usecase_mock.go (1)
23-23: LGTM! Consistent mock updates
The mock has been correctly updated to match the new interface signature, returning the additional count parameter.
Also applies to: 82-82
tokens/usecase/pricing/worker/pool_liquidity_pricer_worker.go (1)
210-210: LGTM! Appropriate handling of new return value
The function correctly handles the new GetPools signature, appropriately discarding the unused count parameter.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
domain/pools.go (1)
131-132: Fix function name to match its comment.The function comment indicates "WithPagination" but the function is named "WithFilter".
Apply this diff to fix the comment:
-// WithPagination configures the pools options with the pagination request. +// WithFilter configures the pools options with the filter request.pools/usecase/pools_usecase.go (1)
408-414: Consider logging metadata retrieval errors.The function silently continues when token metadata retrieval fails, which might hide issues. Consider logging these errors for better debugging.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumpools/usecase/pools_usecase_test.gois excluded by!**/*_test.go
📒 Files selected for processing (4)
domain/pools.go(2 hunks)go.mod(4 hunks)pools/usecase/pools_usecase.go(8 hunks)router/usecase/routertesting/suite.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- router/usecase/routertesting/suite.go
🧰 Additional context used
📓 Learnings (1)
pools/usecase/pools_usecase.go (1)
Learnt from: deividaspetraitis
PR: osmosis-labs/sqs#553
File: pools/usecase/pools_usecase.go:323-368
Timestamp: 2024-12-02T11:30:29.982Z
Learning: In the Golang file `pools/usecase/pools_usecase.go`, the methods `GetFeesData()` and `GetAPRData()` return non-nil values (not pointers), so nil checks are unnecessary when accessing their fields in functions like `getPoolsSortFuncs`.
🔇 Additional comments (9)
go.mod (1)
8-8: Dependencies look appropriate for pagination implementation
The added and updated dependencies align well with the PR objectives:
cosmossdk.io/x/txfor transaction handlinggithub.com/osmosis-labs/sqs/sqsdomainstable version updategithub.com/golang/protobuffor API definitionsgoogle.golang.org/genproto/googleapis/apifor API support
Let's verify these dependencies are being used appropriately:
Also applies to: 16-16, 140-140, 242-242
domain/pools.go (3)
91-93: LGTM! Well-structured options pattern.
The struct effectively separates filtering, pagination, and sorting concerns using pointers to distinguish between unset and zero values.
99-107: LGTM! Well-implemented initialization helper.
The function properly ensures Filter initialization and follows the functional options pattern.
111-129: LGTM! Consistent filter option implementations.
The filter option functions consistently use WithNonNilFilter and follow the same pattern.
pools/usecase/pools_usecase.go (5)
323-367: LGTM! Well-organized sorting functions.
The sorting functions are properly implemented for various criteria. As confirmed by the learnings, GetFeesData() and GetAPRData() return non-nil values, so no nil checks are needed.
459-530: LGTM! Well-structured implementation with proper separation of concerns.
The function effectively handles filtering, sorting, and pagination while maintaining good performance characteristics:
- Early return for empty pool ID filter
- Setting APR and fees data before incentive filtering
- Proper use of transformer pattern
810-816: LGTM! Proper validation and error handling.
The function properly validates the APR prefetcher and handles errors appropriately.
493-496: Address the TODO comment about pool denoms ordering.
The comment raises an important question about base/quote ordering that needs to be resolved.
Let's check the pool denoms implementation:
385-391: Review the type filter implementation.
The current implementation casts the pool type to uint64, but pool types might be represented differently. Consider using the string representation as suggested in the past review comments.
Let's verify the pool type implementation:
✅ Verification successful
Based on the search results, I can now provide a definitive response about the pool type implementation:
Type casting in pool filter is correct
The implementation that casts pool type to uint64 is correct. The search results show that:
- Pool types are defined as an enum in
poolmanagertypes.PoolType - All pool implementations (Balancer, Concentrated, CosmWasm, etc.) implement the
GetType()method that returnspoolmanagertypes.PoolType - The type casting in the filter is necessary to match the API request filter which accepts uint64 array
The current implementation correctly handles the type conversion between the internal pool type enum and the API layer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check pool type implementation
# Look for pool type definitions and usage
ast-grep --pattern 'type PoolType'
Length of output: 36
Script:
#!/bin/bash
# Let's try different approaches to find pool type definitions and usage
# Search for type-related definitions in the codebase
rg -A 5 "type.*Pool"
# Search for GetType method implementations
ast-grep --pattern 'GetType() $_'
# Search for specific type conversions or comparisons
rg "pool.*Type|Type.*pool" -i
Length of output: 65781
| @@ -151,7 +151,7 @@ require ( | |||
| github.com/gorilla/mux v1.8.1 // indirect | |||
| github.com/gorilla/websocket v1.5.3 // indirect | |||
| github.com/grpc-ecosystem/go-grpc-middleware v1.4.0 // indirect | |||
| github.com/grpc-ecosystem/grpc-gateway v1.16.0 // indirect | |||
| github.com/grpc-ecosystem/grpc-gateway v1.16.0 | |||
There was a problem hiding this comment.
Consider upgrading grpc-gateway to v2
The current version github.com/grpc-ecosystem/grpc-gateway v1.16.0 is quite old. Version 2.x.x offers significant improvements in terms of performance, security, and features that could benefit the pagination implementation.
Consider upgrading to the latest v2 version:
-github.com/grpc-ecosystem/grpc-gateway v1.16.0
+github.com/grpc-ecosystem/grpc-gateway/v2 v2.19.0📝 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.
| github.com/grpc-ecosystem/grpc-gateway v1.16.0 | |
| github.com/grpc-ecosystem/grpc-gateway/v2 v2.19.0 |
This pull request indroduces changes to
/poolspage. Until now, FE would fetch all available pools and will do relevant manipulations to display this data correctly to end user. This approach resulted in high load times, slowed down rendering of pools page and negatively impacted our user experience.Introduces changes moves pagination + sorting + query filtering and manipulations and mutations logic of /pools page to SQS to keep FE fast and lightweight.
Linear Task
BE-636
Summary by CodeRabbit
Release Notes
New Features
GetPoolsmethod now returns both pool data and total count for better pagination support.GetPoolsrequests, including pagination metadata.Bug Fixes
Documentation
Chores