feat: orderbook orders cache plugin #622
Conversation
deividaspetraitis
force-pushed
the
connor/orders-caching-plugin
branch
from
ab9a762 to
34a48f8
Compare
WalkthroughThis pull request introduces the Orderbook Orders Cache Plugin into the system, enhancing the sidecar query server to initialize and utilize this new plugin. It includes updates to configuration files, modifications to the PluginFactory logic, and the addition of methods for order storage and retrieval in both repository implementations and their mocks. The passthrough GRPC client is also updated to support raw order queries. Additionally, the Go version and various dependencies in the go.mod file have been updated. Changes
Sequence Diagram(s)sequenceDiagram
participant E as EndBlock Event
participant OC as ordersCache Plugin
participant PU as PoolsUsecase
participant P as PassthroughGRPCClient
participant OR as OrderbookRepository
E->>OC: Trigger ProcessEndBlock
OC->>PU: Retrieve Canonical Orderbook PoolIDs
PU-->>OC: Return orderbook list & metadata
loop For each orderbook
OC->>P: GetOrderbookOrdersRaw(poolID)
P-->>OC: Return raw order data
OC->>OR: StoreOrders(poolID, orders)
OR-->>OC: Acknowledge storage
end
sequenceDiagram
participant U as OrderbookUseCase
participant OR as OrderbookRepository
U->>OR: GetOrders(poolID, ownerAddress)
OR-->>U: Return orders or indicate absence
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
orderbook/usecase/orderbook_usecase.go (1)
277-280: Modified to use repository cache instead of direct client calls.This change aligns with the PR objective of caching orderbook orders. Now orders are fetched from the repository cache instead of making direct client calls to the Node for each request.
Consider adding a comment explaining the transition from direct client calls to repository cache, to make the architectural change more explicit for future maintainers.
orderbook/repository/orderbook_repository.go (2)
120-125: StoreOrders implementation is thread-safe but could be improved.The implementation correctly uses mutex locking to ensure thread safety. However, note that it completely replaces any existing orders for the given pool ID.
Consider adding a parameter or separate method that allows for appending to the existing orders rather than always replacing them, which could be useful in scenarios where orders are processed incrementally.
func (o *orderbookRepositoryImpl) StoreOrders(poolID uint64, orders []orderbookdomain.Order) error { o.ordersByPoolIDLock.Lock() o.ordersByPoolID[poolID] = orders o.ordersByPoolIDLock.Unlock() return nil } + +// AppendOrders appends the given orders to the existing orders for the pool ID +func (o *orderbookRepositoryImpl) AppendOrders(poolID uint64, newOrders []orderbookdomain.Order) error { + o.ordersByPoolIDLock.Lock() + o.ordersByPoolID[poolID] = append(o.ordersByPoolID[poolID], newOrders...) + o.ordersByPoolIDLock.Unlock() + return nil +}
127-143: GetOrders implementation is functional but could be optimized.The method correctly retrieves and filters orders by owner address in a thread-safe manner. However, it could be more memory-efficient for cases with many orders.
Consider pre-allocating the result slice with a smaller capacity or using a more efficient filtering approach:
func (o *orderbookRepositoryImpl) GetOrders(poolID uint64, ownerAddress string) ([]orderbookdomain.Order, bool) { o.ordersByPoolIDLock.RLock() orders, ok := o.ordersByPoolID[poolID] o.ordersByPoolIDLock.RUnlock() if !ok { return nil, false } - var result []orderbookdomain.Order + // Pre-allocate with smaller capacity to reduce memory usage when there are few matches + result := make([]orderbookdomain.Order, 0, min(len(orders), 10)) for _, order := range orders { if order.Owner == ownerAddress { result = append(result, order) } } return result, true } + +// Helper function to find minimum of two integers +func min(a, b int) int { + if a < b { + return a + } + return b +}domain/mocks/orderbook_repository_mock.go (2)
58-62: Helper method naming inconsistency.The naming pattern is inconsistent with other helpers in this file.
Consider renaming this method to
WithStoreOrdersFuncto maintain consistency with other helper method names likeWithGetOrdersFunc.-func (m *OrderbookRepositoryMock) WithStoreOrders(err error) { +func (m *OrderbookRepositoryMock) WithStoreOrdersFunc(err error) { m.StoreOrdersFunc = func(poolID uint64, orders []orderbookdomain.Order) error { return err } }
72-76: WithGetOrdersFunc could be more flexible.The helper only allows setting fixed return values, which might limit testing scenarios.
Consider enhancing this helper to allow for conditional logic based on the input parameters:
+// WithGetOrdersFuncCustom allows setting a custom implementation for GetOrdersFunc +func (m *OrderbookRepositoryMock) WithGetOrdersFuncCustom(fn func(poolID uint64, ownerAddress string) ([]orderbookdomain.Order, bool)) { + m.GetOrdersFunc = fn +} func (m *OrderbookRepositoryMock) WithGetOrdersFunc(orders []orderbookdomain.Order, ok bool) { m.GetOrdersFunc = func(poolID uint64, ownerAddress string) ([]orderbookdomain.Order, bool) { return orders, ok } }ingest/usecase/plugins/orderbook/orderscache/plugin.go (3)
21-24: Consider adding more descriptive struct comments.The struct fields could benefit from having comments that explain their purpose. The
atomicBoolfield in particular isn't clear in its intended use.type ordersCache struct { + // config contains all the dependencies and settings needed by the plugin config *Config + // atomicBool provides thread-safe state management for the plugin atomicBool atomic.Bool }
60-67: Consider handling pool metadata more efficiently.The code iterates through all orderbooks and checks if each pool ID exists in the metadata. For better performance with large collections, consider filtering the orderbooks first or using a more efficient data structure for lookups.
for _, orderbook := range orderbooks { - if _, ok := metadata.PoolIDs[orderbook.PoolID]; ok { + // Only process orderbooks that are in the metadata + if _, ok := metadata.PoolIDs[orderbook.PoolID]; ok { if err := o.fetchAndCacheOrdersForOrderbook(ctx, orderbook); err != nil { o.config.Logger.Error("failed to fetch orders for orderbook", zap.Error(err), zap.Uint64("orderbook_id", orderbook.PoolID)) return err } } }
72-89: Add error context and consider batch processing.When fetching and caching orders, consider adding more context to errors and potentially implementing batch processing for better performance with large datasets.
func (o *ordersCache) fetchAndCacheOrdersForOrderbook(ctx context.Context, orderbook domain.CanonicalOrderBooksResult) error { + ctx, span := tracer.Start(ctx, "ordersCache.fetchAndCacheOrdersForOrderbook") + defer span.End() + ordersBz, err := o.config.PassthroughGRPCClient.GetOrderbookOrdersRaw(ctx, orderbook.PoolID) if err != nil { + o.config.Logger.Debug("failed to get raw orderbook orders", zap.Error(err), zap.Uint64("orderbook_id", orderbook.PoolID)) return err } orders := make([]orderbookdomain.Order, 0, len(ordersBz)) for _, orderBz := range ordersBz { var order orderbookdomain.Order if err := json.Unmarshal(orderBz, &order); err != nil { + o.config.Logger.Debug("failed to unmarshal order", zap.Error(err), zap.Uint64("orderbook_id", orderbook.PoolID)) return err } orders = append(orders, order) } + o.config.Logger.Debug("storing orders for orderbook", zap.Uint64("orderbook_id", orderbook.PoolID), zap.Int("orders_count", len(orders))) return o.config.OrderbookRepository.StoreOrders(orderbook.PoolID, orders) }go.mod (1)
285-286: Document the purpose of the Osmosis v28 replacement.The replacement for
github.com/osmosis-labs/osmosis/v28points to a specific version with a timestamp. Consider adding a comment explaining what specific changes or fixes this version provides that are necessary for this PR.// This replacement can be removed once Osmosis v28.0.3 or higher is released // It is required to pull in test cases for BE-676 from backport branch: https://github.com/osmosis-labs/osmosis/pull/8962 +// The specific version v28.0.5-0.20250218091759-db70d39ef671 includes fixes for orderbook functionality needed by the orders cache plugin github.com/osmosis-labs/osmosis/v28 v28.0.2 => github.com/osmosis-labs/osmosis/v28 v28.0.5-0.20250218091759-db70d39ef671
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workingest/usecase/plugins/orderbook/orderscache/export_test.gois excluded by!**/*_test.goingest/usecase/plugins/orderbook/orderscache/orderscache_test.gois excluded by!**/*_test.goorderbook/usecase/orderbook_usecase_test.gois excluded by!**/*_test.go
📒 Files selected for processing (14)
app/sidecar_query_server.go(2 hunks)config.json(1 hunks)domain/config.go(2 hunks)domain/mocks/orderbook_repository_mock.go(2 hunks)domain/mocks/passthrough_grpc_client_mock.go(2 hunks)domain/orderbook/orderbook_repository.go(1 hunks)domain/orderbook/plugin/config.go(1 hunks)domain/passthrough/passthrough_grpc_client.go(5 hunks)go.mod(7 hunks)ingest/usecase/plugins/orderbook/orderscache/config.go(1 hunks)ingest/usecase/plugins/orderbook/orderscache/orderbook.go(1 hunks)ingest/usecase/plugins/orderbook/orderscache/plugin.go(1 hunks)orderbook/repository/orderbook_repository.go(3 hunks)orderbook/usecase/orderbook_usecase.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (25)
domain/orderbook/plugin/config.go (1)
4-8: LGTM! Added new constant for Orderbook Orders Cache Plugin.The addition of the new
OrderbookOrdersCachePluginconstant is clean and follows the existing pattern. The constant will be used to identify the plugin in the configuration and code.config.json (1)
9-24: Proper configuration setup for the new plugin.The new plugin has been correctly added to the configuration JSON. It follows the same structure as the existing plugins and is disabled by default, which is a good practice for new features.
domain/orderbook/orderbook_repository.go (1)
18-24:✅ Verification successful
Interface extension with clear method signatures.
The
OrderBookRepositoryinterface has been extended with two new methods that follow the existing pattern. The method signatures are clear and well-documented.Let's ensure these methods are properly implemented in the repository:
🏁 Script executed:
#!/bin/bash # Check if the repository implementation contains the new methods rg -A 10 "func \(.*\) StoreOrders\(poolID uint64, orders \[\].*Order\)" --type go rg -A 10 "func \(.*\) GetOrders\(poolID uint64, ownerAddress string\)" --type goLength of output: 3318
Interface methods implementation confirmed
The new methods have been correctly added following the existing pattern. Both the main repository implementation (
orderbook/repository/orderbook_repository.go) and the mock (domain/mocks/orderbook_repository_mock.go) provide implementations forStoreOrdersandGetOrders, as verified by the search results. No further modifications are needed.app/sidecar_query_server.go (2)
31-31: LGTM! Required import added.The import for the
orderbookorderscachepackage is correctly added.
298-301:❓ Verification inconclusive
Proper plugin initialization with required dependencies.
The initialization of the new plugin follows the same pattern as other plugins. The plugin is created when its configuration is detected, and it's initialized with all the necessary dependencies.
Let's check the plugin implementation to confirm it correctly uses all the dependencies:
🏁 Script executed:
#!/bin/bash # Check the plugin's New function signature rg -A 5 "func New\(.*\) .*Plugin" --type go -- ingest/usecase/plugins/orderbook/orderscacheLength of output: 93
I'll re-run a broader search for the plugin initialization functions to verify that the
Newfunction is implemented and uses all required dependencies. Let's run a new shell script that lists all Go files in theingest/usecase/plugins/orderbook/orderscachedirectory and searches within them for any occurrence of aNewfunction.
🏁 Script executed:
#!/bin/bash # List all Go files in the orderscache plugin directory fd -H -t f -e go ingest/usecase/plugins/orderbook/orderscache # For each found Go file, search for occurrences of "func New" for file in $(fd -H -t f -e go ingest/usecase/plugins/orderbook/orderscache); do echo "------ $file ------" rg -n "func New" "$file" doneLength of output: 1092
Verify Plugin Initialization Dependencies
The code at
app/sidecar_query_server.go(lines 298–301) correctly initializes the plugin using the provided dependencies. However, our automated search for the implementation of theNewfunction within the orderscache plugin did not return any output due to command syntax issues. Please verify manually that theNewfunction in the orderscache plugin properly accepts and utilizes the dependencies (orderBookRepository,poolsUseCase,logger, andpassthroughGRPCClient).
- Action Required:
- Confirm that the orderscache plugin’s
Newfunction exists in the correct location (likely under a corrected full-path search, e.g., usingfd --full-path 'ingest/usecase/plugins/orderbook/orderscache') and that it follows the dependency injection pattern as seen in other plugins.ingest/usecase/plugins/orderbook/orderscache/orderbook.go (1)
1-22: Well-structured utility function for filtering orderbooks.The function is concise and follows best practices with proper error handling. It efficiently filters orderbooks to include only those whose pool IDs exist in the metadata, supporting the PR's objective of implementing a caching plugin for orderbook orders.
domain/config.go (2)
158-161: Good implementation of plugin configuration.The new OrderbookOrdersCachePlugin is properly added to the DefaultConfig with default disabled state, which is a good practice for new features.
388-390: Proper plugin factory implementation.The PluginFactory function is correctly updated to handle the new OrderbookOrdersCachePlugin type, following the established pattern for other plugins.
ingest/usecase/plugins/orderbook/orderscache/config.go (1)
1-29: Well-structured configuration setup for the plugin.The Config struct properly defines all required dependencies for the orderbook orders cache plugin with a clear constructor function. This follows the standard pattern used throughout the codebase.
domain/mocks/passthrough_grpc_client_mock.go (3)
12-13: Good practice moving interface implementation assertion to the top.Moving the interface implementation assertion to the top of the file improves visibility and follows good coding standards.
21-21: Proper mock callback declaration.The MockGetOrderbookOrdersRawCb field is correctly added to support mocking the new GetOrderbookOrdersRaw method.
84-90: Well-implemented mock method.The GetOrderbookOrdersRaw implementation follows the established pattern for other mock methods in this file, with proper conditional behavior based on whether the callback is defined.
domain/passthrough/passthrough_grpc_client.go (4)
43-44: Interface extension looks good.The new
GetOrderbookOrdersRawmethod in thePassthroughGRPCClientinterface is well defined with clear parameters and return types.
62-63: Field addition is properly structured.Adding the
cosmwasmpoolClientfield to thepassthroughGRPCClientstruct is appropriate for supporting the new orderbook orders fetching functionality.
86-86: Client initialization is complete.The
cosmwasmpoolClientis properly initialized in the constructor, ensuring it's available when needed.
201-207: Implementation is clear and error handling is appropriate.The
GetOrderbookOrdersRawmethod correctly queries the cosmwasmpool client with a filter for orders and handles potential errors properly. The method provides a clean interface for fetching raw orderbook data.orderbook/usecase/orderbook_usecase.go (1)
272-272: Method signature updated for clarity.The context parameter is now marked as unused with an underscore, which accurately reflects that it's no longer being used in the method body.
orderbook/repository/orderbook_repository.go (2)
13-14: Adding thread-safe storage for orders by pool ID.The new fields properly support the orderbook orders caching functionality with appropriate synchronization via RWMutex.
23-24: Fields correctly initialized.The new maps and mutexes are properly initialized in the constructor.
domain/mocks/orderbook_repository_mock.go (2)
15-16: Function signatures added to mock.The new function signatures in the mock align with the repository interface changes.
65-70: Method implementation follows consistent pattern.The
StoreOrdersandGetOrdersmethods are implemented consistently with the existing pattern in the mock, checking for the function and panicking if not set.Also applies to: 79-84
ingest/usecase/plugins/orderbook/orderscache/plugin.go (2)
55-55:✅ Verification successful
Verify that the getOrderbooks function exists.
The
getOrderbooksfunction is called here but isn't defined in this file. Ensure it's properly defined elsewhere in the package.
🏁 Script executed:
#!/bin/bash # Check if getOrderbooks function is defined in the package rg "func getOrderbooks" --type go -g "ingest/usecase/plugins/orderbook/orderscache/*.go"Length of output: 279
getOrderbooks Function Verified
The
getOrderbooksfunction is defined in the package atingest/usecase/plugins/orderbook/orderscache/orderbook.go. No changes are required.
43-43:✅ Verification successful
Verify that the NewConfig function exists.
The
NewConfigfunction is called here but isn't defined in this file. Make sure it's properly defined in a separate file within the same package.
🏁 Script executed:
#!/bin/bash # Check if NewConfig function is defined in the package rg "func NewConfig" --type go -g "ingest/usecase/plugins/orderbook/orderscache/*.go"Length of output: 157
NewConfig function verification complete
- The
NewConfigfunction is properly defined iningest/usecase/plugins/orderbook/orderscache/config.goas confirmed by the search.- Since the function exists in the designated file within the same package, no modifications are required in the current file.
go.mod (2)
3-3:❓ Verification inconclusive
Verify Go version compatibility.
The project is upgrading from Go 1.22.7 to Go 1.22.11. Ensure that all team members and CI/CD pipelines are updated to use this version.
🏁 Script executed:
#!/bin/bash # Check Go version in CI/CD configuration rg "go-version|go1\." --type yamlLength of output: 35
Action Required: Confirm CI/CD Pipeline Go Version
The
go.modfile now specifies Go version 1.22.11. However, our automated search (usingrg "go-version|go1\." --type yaml) did not reveal any explicit Go version references in your CI/CD configuration files. This might mean that these configurations either do not set the Go version explicitly or use a different mechanism, so please manually verify that all CI/CD pipelines and development environments are updated to use Go 1.22.11.
- Review File:
go.mod- Verification Needed: Confirm that CI/CD configurations (and possibly Dockerfiles or other build scripts) reference or enforce Go 1.22.11.
27-27:❓ Verification inconclusive
Verify gRPC backward compatibility.
The upgrade from gRPC v1.69.4 to v1.70.0 may introduce changes. Ensure that this update doesn't break any existing functionality.
🌐 Web query:
What are the breaking changes between gRPC v1.69.4 and v1.70.0?💡 Result:
The transition from gRPC v1.69.4 to v1.70.0 introduces two key breaking changes:
1. Client Rejection of Invalid Retry Policies
- Behavior Change: Clients now strictly reject service configurations containing invalid
retryPolicysettings (gRFC A21/A6 compliance)[1][3].- Impact: Systems relying on malformed retry configurations (e.g., missing required fields like
maxAttemptsor invalid timeout formats) will fail to establish connections[3].- Rationale: Ensures cross-language compatibility by aligning with standardized error-handling behavior[3].
2. C++17 Requirement for gRPC Core
- Build Requirement: gRPC Core now mandates C++17 for compilation and usage[2][5].
- Migration Path:
- CMake: Add
CMAKE_CXX_STANDARD=17- Bazel: Include
--cxxopt='/std:c++17'and--host_cxxopt='/std:c++17'- Legacy Support: gRPC v1.69.x remains the final version supporting C++14 until December 2025[2].
Additional Notes
- The Go implementation (
grpc-go) warns that experimental credentials added in v1.70 will be removed in future releases[3].- Performance improvements in unary request handling may alter memory usage patterns but maintain backward compatibility[3].
For detailed migration guidance, consult the official gRPC versioning documentation[9].
Citations:
- 1: https://gitlab.com/gitlab-org/gitlab-zoekt-indexer/-/merge_requests/367
- 2: https://github.com/grpc/grpc/releases
- 3: https://github.com/grpc/grpc-go/releases
- 4: https://javadoc.io/doc/io.grpc/grpc-api/latest/index.html
- 5: https://groups.google.com/g/grpc-io
- 6: http://pecl.php.net/package-changelog.php?package=grpc
- 7: https://pkg.go.dev/google.golang.org/grpc
- 8: https://fossies.org/diffs/grpc/1.69.0_vs_1.70.0/
- 9: https://learn.microsoft.com/en-us/aspnet/core/grpc/versioning?view=aspnetcore-9.0
- 10: https://newreleases.io/project/github/Permify/permify/release/v1.2.8
Subject: Verify gRPC Compatibility in v1.70.0 Upgrade
Please ensure that upgrading to gRPC v1.70.0 doesn’t disrupt existing functionality. In particular:
- Invalid Retry Policies: The updated client now strictly rejects service configurations containing invalid
retryPolicysettings. Verify that all retry configurations in your service/client adhere to the required format (e.g., propermaxAttemptsand timeout values).- grpc-go Considerations: Although the C++17 requirement mainly affects gRPC Core in C++, note that grpc-go issues a warning about experimental credentials introduced in this release. Confirm that your code does not rely on these experimental features.
- Testing: Run your integration tests (especially those involving retry behavior and credentials) to ensure that the update does not introduce any regressions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ingest/usecase/plugins/orderbook/orderscache/plugin.go (2)
52-64: Consider gracefully handling errors for each orderbook.
Right now, if fetching fails for one orderbook, the function returns immediately, skipping remaining orderbooks. Explore a partial success approach (e.g., logging the error for the failing orderbook) while continuing to process others.
69-86: Provide a fallback for malformed JSON entries.
Currently, a single unmarshalling error leads to an overall failure. If partial data salvage is desired, you might skip invalid entries or add detailed logs instead of aborting the entire process.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ingest/usecase/plugins/orderbook/orderscache/plugin.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (2)
ingest/usecase/plugins/orderbook/orderscache/plugin.go (2)
18-19: Docstring aligns with actual functionality.
Good job updating the documentation to reflect the plugin’s real purpose of caching orderbook orders.
48-48: Block height argument remains unused.
Although theblockHeightparameter is passed in, it is never referenced within the method. If the block height is needed for logging, versioning, or other uses, consider incorporating it; otherwise, consider removing it.
|
This pull request implements orderbook orders cache plugin. Plugin would be ran at each block and will process the block extracting orderbook pools from it to be fetched from the Node and cached on SQS. Fetching now uses more effective gRPC endpoint to retrieve orders from the node and SQS
/active-ordersendpoint serves orders from cache instead of requesting Node on each endpoint hit.Summary by CodeRabbit
New Features
Improvements