BE-586 | Claimbot #524
BE-586 | Claimbot #524
Conversation
ingest/usecase/plugins/orderbookclaimer/ordebook_filler_ingest_plugin.go
Outdated
Show resolved
Hide resolved
ingest/usecase/plugins/orderbookclaimer/ordebook_filler_ingest_plugin.go
Outdated
Show resolved
Hide resolved
ingest/usecase/plugins/orderbookclaimer/ordebook_filler_ingest_plugin.go
Outdated
Show resolved
Hide resolved
ingest/usecase/plugins/orderbookclaimer/ordebook_filler_ingest_plugin.go
Outdated
Show resolved
Hide resolved
ingest/usecase/plugins/orderbookclaimer/ordebook_filler_ingest_plugin.go
Outdated
Show resolved
Hide resolved
ingest/usecase/plugins/orderbookclaimer/ordebook_filler_ingest_plugin.go
Outdated
Show resolved
Hide resolved
Cleans up OrderBookClient by reusing slices.Split instead of duplicating splitting slices into chunks logic in some of the methods.
Fixes errors running fillbot via docker-compose
| orderbooks, err := getOrderbooks(o.config.PoolsUseCase, blockHeight, metadata) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Instead of having this as a function and propagating pools use case as an arg, why not make this be a method on the pools use case?
There was a problem hiding this comment.
My attempt to address this comment within reasonable timeframe did not produced satifying result. The problem I faced was testing. If we would end up moving this func as a method to usecase end result might look like following:
func (p *poolsUseCase) GetBlockCanonicalOrderbookPools(metadata domain.BlockPoolMetadata) ([]domain.CanonicalOrderBooksResult, error) {
orderbooks, err := p.GetAllCanonicalOrderbookPoolIDs()
if err != nil {
return nil, err
}
var result []domain.CanonicalOrderBooksResult
for _, orderbook := range orderbooks {
if _, ok := metadata.PoolIDs[orderbook.PoolID]; ok {
result = append(result, orderbook)
}
}
return result, nil
}
But now, we need to either mock p.GetAllCanonicalOrderbookPoolIDs ( which is not easy task ) or setup Poolsusecase in a proper way so expected pool IDs are returned in order to cover all scenarios as defined in the tests. However, that would require a lot of boilerplate as for example here and we still would not be able to test error case. I think this case is a good example when current design starts limitting us and presents a good opportunity rethink. What do you think?
There was a problem hiding this comment.
The linked example looks much cleaner IMO, and this is how I would expect the API to look like if minimizing code smells
I don't understand what the problem with mocking out poolsUseCase was. (the link did not point me to anything specific btw)
Happy to not block this PR on this but a task would be greatly appreciated
| OrderBookClient orderbookgrpcclientdomain.OrderBookClient | ||
| AccountQueryClient authtypes.QueryClient | ||
| TxfeesClient txfeestypes.QueryClient |
There was a problem hiding this comment.
Consider abstracting all of the chain clients behind a struct
| } | ||
|
|
||
| // processOrderbooksAndGetClaimableOrders processes a list of orderbooks and returns claimable orders for each. | ||
| func processOrderbooksAndGetClaimableOrders( |
There was a problem hiding this comment.
I'm seeing only this function being covered by tests but there are many downstream function of varying branching complexity.
Could we spend some time covering those with tests?
I think it would pay off in the long-term as it would make any possible refactors drastically easier under simpler test coverage
There was a problem hiding this comment.
Added a several tests in the use case: https://github.com/osmosis-labs/sqs/pull/524/files#diff-5fa55bd6410efa835405589edaffbb41ba95f810344564647d2d711e8bc15b67
| // Wait for block inclusion with buffer to avoid sequence mismatch | ||
| time.Sleep(blockInclusionWaitTime) |
There was a problem hiding this comment.
Consider querying the sequence number at the beginning of the block and incrementing it in-memory. This should be fine since we have a lock on block processing. That is, if block X is being processed, block X+1 processing cannot start.
This would drastically speed up processing all orders within a block
There was a problem hiding this comment.
Addressed that here 👍
deividaspetraitis
force-pushed
the
BE-586
branch
from
b959eba to
d298179
Compare
![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 (2)
ingest/usecase/plugins/orderbook/claimbot/order.go (2)
201-206: Consider changing log level from Info to Warn when unable to create limit order
When failing to create a formatted limit order, logging at the Info level might not capture sufficient attention. Changing the log level to Warn will highlight potential issues that may need investigation.
Apply this diff to change the log level:
- logger.Info(
+ logger.Warn(
"unable to create orderbook limit order; marking as not claimable",
zap.String("orderbook", orderbook.ContractAddress),
zap.Int64("order", order.OrderId),
zap.Error(err),
)125-128: Enhance error handling when fetching orders by tick
Currently, if GetOrdersByTick returns an error, it is propagated up. Provide additional context to the error to aid in debugging.
Apply this diff to wrap the error with more information:
func getClaimableOrdersForTick(
ctx context.Context,
fillThreshold osmomath.Dec,
orderbook domain.CanonicalOrderBooksResult,
tick orderbookdomain.OrderbookTick,
orderBookClient orderbookgrpcclientdomain.OrderBookClient,
orderbookusecase mvc.OrderBookUsecase,
logger log.Logger,
) (orderbookdomain.Orders, error) {
orders, err := orderBookClient.GetOrdersByTick(ctx, orderbook.ContractAddress, tick.Tick.TickId)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("unable to get orders by tick for orderbook %s, tick %d: %w", orderbook.ContractAddress, tick.Tick.TickId, err)
}
if len(orders) == 0 {
return nil, nil
}📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
ingest/usecase/plugins/orderbook/claimbot/export_test.gois excluded by!**/*_test.goingest/usecase/plugins/orderbook/claimbot/order_test.gois excluded by!**/*_test.goingest/usecase/plugins/orderbook/claimbot/orderbook_test.gois excluded by!**/*_test.go
📒 Files selected for processing (5)
- ingest/usecase/plugins/orderbook/claimbot/order.go (1 hunks)
- ingest/usecase/plugins/orderbook/claimbot/orderbook.go (1 hunks)
- ingest/usecase/plugins/orderbook/claimbot/plugin.go (1 hunks)
- ingest/usecase/plugins/orderbook/claimbot/tx.go (1 hunks)
- ingest/usecase/plugins/orderbook/fillbot/create_copy_config.sh (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ingest/usecase/plugins/orderbook/claimbot/orderbook.go
- ingest/usecase/plugins/orderbook/claimbot/tx.go
- ingest/usecase/plugins/orderbook/fillbot/create_copy_config.sh
🧰 Additional context used
🪛 GitHub Check: Run linter
ingest/usecase/plugins/orderbook/claimbot/plugin.go
[failure] 136-136:
(*claimbot).processOrderbookOrders - result 0 (error) is always nil (unparam)
🔇 Additional comments (3)
ingest/usecase/plugins/orderbook/claimbot/plugin.go (2)
72-72: Correct the span name in tracer.Start()
The span name in tracer.Start() seems incorrect for the claimbot plugin. It should accurately reflect the context, such as "claimbot.ProcessEndBlock".
158-165: Adjust logging level for failed transactions
The log statement for failed transactions is using the Info level. Consider using Warn or Error level to better reflect the severity of the failure.
ingest/usecase/plugins/orderbook/claimbot/order.go (1)
92-94: Handle potential race conditions when accessing shared resources
The GetAllTicks function might access shared resources. Ensure that appropriate synchronization mechanisms are in place to prevent race conditions when orderbookRepository is accessed concurrently.
Run the following script to check for concurrent access patterns:
✅ Verification successful
Let me gather more context about the implementation of orderbookRepository to understand its thread-safety mechanisms.
Let me check the implementation of the GetAllTicks method to verify how the synchronization is handled.
Let me check one final piece - the GetAllTicks implementation since it wasn't fully shown in the previous output.
Race conditions are properly handled in the orderbook repository
The implementation of orderbookRepositoryImpl uses appropriate synchronization mechanisms:
- Uses
sync.RWMutex(tickMapByPoolIDLock) for protecting access to the pool map - Uses
sync.Mapfor thread-safe access to individual tick maps - Implements proper read/write locking patterns in both GetAllTicks and StoreTicks methods
The review comment's concern about race conditions has been addressed in the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for race conditions in orderbookRepository usage
# Search for all accesses to orderbookRepository methods
rg --type go 'orderbookRepository\.\w+\('
# Expected: Verify that access to shared resources is properly synchronized
Length of output: 467
Script:
#!/bin/bash
# Check the implementation of orderbookRepository
ast-grep --pattern 'type orderbookRepository struct {
$$$
}'
# Check for mutex usage in the repository
rg --type go -B 2 -A 5 'sync\.(\w+)?Mutex'
# Look for interface definition
ast-grep --pattern 'type OrderbookRepository interface {
$$$
}'
Length of output: 7909
Script:
#!/bin/bash
# Check the implementation of GetAllTicks
ast-grep --pattern 'func ($VAR *orderbookRepositoryImpl) GetAllTicks($$$) {
$$$
}'
# Also check StoreTicks implementation for completeness
ast-grep --pattern 'func ($VAR *orderbookRepositoryImpl) StoreTicks($$$) {
$$$
}'
Length of output: 1433
Script:
#!/bin/bash
# Search for GetAllTicks implementation with more context
rg --type go -B 2 -A 10 'func \(.*\) GetAllTicks\('
Length of output: 4024
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (13)
domain/mvc/orderbook.go (1)
30-33: Enhance method documentation with specific criteria and examples.While the current documentation provides a good overview, it would be more helpful to include:
- The specific criteria for an order to be considered "claimable"
- The expected format of the
fillThresholdparameter (e.g., 0.98 for 98%)- Error conditions that callers should handle
Consider expanding the documentation like this:
// GetClaimableOrdersForOrderbook retrieves all claimable orders for a given orderbook. // It fetches all ticks for the orderbook, processes each tick to find claimable orders, // and returns a combined list of all claimable orders across all ticks. +// +// An order is considered claimable when its fill percentage meets or exceeds the fillThreshold +// (e.g., 0.98 for 98% filled). The orderbook must meet canonical criteria as defined by SQS. +// +// Returns: +// - Orders: Collection of claimable orders +// - Error: If context is canceled, if orderbook validation fails, or if tick processing failsdomain/orderbook/orderbook_tick.go (1)
57-59: Follow Go documentation conventions.The function comment should start with the function name and follow Go's documentation conventions.
-// isTickFullyFilled checks if a tick is fully filled by comparing its cumulative total value -// to its effective total amount swapped. +// IsTickFullyFilled checks if a tick is fully filled by comparing its cumulative total value +// to its effective total amount swapped. It returns true if the tick is fully filled, +// and false otherwise. Returns an error if the decimal conversion fails.domain/mocks/orderbook_usecase_mock.go (1)
66-77: Add documentation to the new methods.The implementation looks good and follows the established patterns. Consider adding documentation comments to describe the purpose and parameters of these methods.
Add documentation like this:
+// GetClaimableOrdersForOrderbook returns orders that are filled above the specified threshold for a given orderbook. +// If the mock function is not set, it panics. func (m *OrderbookUsecaseMock) GetClaimableOrdersForOrderbook(ctx context.Context, fillThreshold osmomath.Dec, orderbook domain.CanonicalOrderBooksResult) (orderbookdomain.Orders, error) { +// WithGetClaimableOrdersForOrderbook sets up the mock to return specified orders and error when GetClaimableOrdersForOrderbook is called. func (m *OrderbookUsecaseMock) WithGetClaimableOrdersForOrderbook(orders orderbookdomain.Orders, err error) {ingest/usecase/plugins/orderbook/claimbot/plugin.go (2)
66-67: Remove duplicate commentThe comment "ProcessEndBlock implements domain.EndBlockProcessPlugin" appears twice.
// ProcessEndBlock implements domain.EndBlockProcessPlugin. // This method is called at the end of each block to process and claim eligible orderbook orders. -// ProcessEndBlock implements domain.EndBlockProcessPlugin. func (o *claimbot) ProcessEndBlock(ctx context.Context, blockHeight uint64, metadata domain.BlockPoolMetadata) error {
91-94: Enhance error context for account retrievalThe error returned from account retrieval lacks context that would be helpful for debugging.
account, err := o.config.AccountQueryClient.GetAccount(ctx, o.config.Keyring.GetAddress().String()) if err != nil { - return err + return fmt.Errorf("failed to get account for address %s: %w", o.config.Keyring.GetAddress().String(), err) }ingest/usecase/plugins/orderbook/claimbot/order.go (3)
21-21: Typo in comment: 'in' should be 'is'There's a minor typo in the comment. The word "in" should be "is" to correctly convey the intended meaning.
-// Under the hood processing of each orderbook in done concurrently to speed up the process. +// Under the hood processing of each orderbook is done concurrently to speed up the process.
37-37: Preallocate the results slice for efficiencySince the number of orderbooks is known, you can preallocate the
resultsslice to improve performance by reducing memory allocations.-var results []processedOrderbook +results := make([]processedOrderbook, 0, len(orderbooks))
50-50: Clarify the return type in the function commentThe comment refers to "an order struct," but the function returns a
processedOrderbookstruct. Updating the comment improves clarity.-// processOrderbook processes a single orderbook and returns an order struct containing the processed orderbook and its claimable orders. +// processOrderbook processes a single orderbook and returns a processedOrderbook struct containing the processed orderbook and its claimable orders.ingest/usecase/plugins/orderbook/claimbot/tx.go (2)
21-23: Consider PassingencodingConfigas a Parameter for Enhanced TestabilityDefining
encodingConfigas a package-level variable may affect testability and could lead to issues with package initialization order. Consider passingencodingConfigas a parameter to functions that require it or encapsulating it within a struct to improve modularity and reduce potential side effects.
27-58: Enhance Observability with LoggingConsider adding logging statements at key points within the
sendBatchClaimTxfunction, such as before sending the transaction or upon encountering errors. This can improve observability, making it easier to trace the execution flow and debug issues in production environments.orderbook/usecase/orderbook_usecase.go (3)
515-521: Consider aggregating errors during tick processingIn the
GetClaimableOrdersForOrderbookfunction, when an error occurs while processing a tick, the error is logged, and the loop continues to the next tick. Depending on the importance of processing every tick successfully, you might consider aggregating these errors to provide a comprehensive error report after the loop. This approach can help identify all problematic ticks in a single run.
537-540: Ensure consistent error handling in getClaimableOrdersForTickIn
getClaimableOrdersForTick, ifGetOrdersByTickreturns an error, the function returns the error immediately, causing the calling function to log the error and continue. For consistency with how errors are handled inGetClaimableOrdersForOrderbook, consider logging the error withingetClaimableOrdersForTickand returning an empty slice of orders instead. This way, the processing of other ticks continues smoothly without propagating the error up the call stack unnecessarily.Apply this diff to adjust the error handling:
func (o *OrderbookUseCaseImpl) getClaimableOrdersForTick( ctx context.Context, fillThreshold osmomath.Dec, orderbook domain.CanonicalOrderBooksResult, tick orderbookdomain.OrderbookTick, ) (orderbookdomain.Orders, error) { orders, err := o.orderBookClient.GetOrdersByTick(ctx, orderbook.ContractAddress, tick.Tick.TickId) if err != nil { - return nil, err + o.logger.Error( + "error fetching orders by tick", + zap.String("orderbook", orderbook.ContractAddress), + zap.Int64("tick", tick.Tick.TickId), + zap.Error(err), + ) + return nil, nil } // Rest of the function... }
597-603: Use appropriate log level for errors in isOrderClaimableIn the
isOrderClaimablefunction, when an error occurs during the creation of a formatted limit order, the error is logged at theInfolevel. Since this is an unexpected error that affects the determination of an order's claimability, it would be more appropriate to log it at theErrorlevel to highlight the severity of the issue.Apply this diff to change the log level:
func (o *OrderbookUseCaseImpl) isOrderClaimable( orderbook domain.CanonicalOrderBooksResult, order orderbookdomain.Order, fillThreshold osmomath.Dec, ) bool { result, err := o.CreateFormattedLimitOrder(orderbook, order) if err != nil { - o.logger.Info( + o.logger.Error( "unable to create orderbook limit order; marking as not claimable", zap.String("orderbook", orderbook.ContractAddress), zap.Int64("order", order.OrderId), zap.Error(err), ) return false } return result.IsClaimable(fillThreshold) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
ingest/usecase/plugins/orderbook/claimbot/export_test.gois excluded by!**/*_test.goingest/usecase/plugins/orderbook/claimbot/order_test.gois excluded by!**/*_test.goingest/usecase/plugins/orderbook/claimbot/tx_test.gois excluded by!**/*_test.go
📒 Files selected for processing (9)
app/sidecar_query_server.go(3 hunks)domain/mocks/orderbook_usecase_mock.go(2 hunks)domain/mvc/orderbook.go(2 hunks)domain/orderbook/orderbook_tick.go(1 hunks)ingest/usecase/plugins/orderbook/claimbot/config.go(1 hunks)ingest/usecase/plugins/orderbook/claimbot/order.go(1 hunks)ingest/usecase/plugins/orderbook/claimbot/plugin.go(1 hunks)ingest/usecase/plugins/orderbook/claimbot/tx.go(1 hunks)orderbook/usecase/orderbook_usecase.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ingest/usecase/plugins/orderbook/claimbot/config.go
🧰 Additional context used
🪛 GitHub Check: Run linter
ingest/usecase/plugins/orderbook/claimbot/plugin.go
[failure] 135-135:
(*claimbot).processOrderbookOrders - result 0 (error) is always nil (unparam)
🔇 Additional comments (11)
domain/mvc/orderbook.go (2)
6-9: LGTM! Import changes are appropriate.
The addition of osmomath package is necessary for the new fillThreshold parameter type.
Line range hint 13-33: Consider standardizing error handling patterns across interface methods.
The interface currently mixes different error handling patterns:
- Some methods return
(result, bool)for existence checks - Others return
(result, bool, error) - The new method returns
(result, error)
This inconsistency could make error handling more complex for clients. Consider standardizing to always return an error that can indicate "not found" cases, following Go's idiomatic error handling.
Let's check if this pattern exists elsewhere in the codebase:
domain/orderbook/orderbook_tick.go (1)
59-71: Verify the decimal comparison logic across the codebase.
Let's verify how this method is used in relation to the 98% fill threshold mentioned in the PR objectives.
domain/mocks/orderbook_usecase_mock.go (2)
10-11: LGTM: Import changes are appropriate.
The addition of the osmomath package is necessary for the new functionality.
18-23: LGTM: Struct field additions maintain consistency.
The new field follows the established pattern and the function signature aligns with the PR objectives for claiming filled orders.
Let's verify the interface compliance:
✅ Verification successful
✅ Mock struct fields correctly match the interface definition
The mock's function fields exactly match the method signatures defined in the OrderBookUsecase interface, including the newly added GetClaimableOrdersForOrderbook method. The parameters and return types are consistent between the interface and mock implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the mock implements all methods of the OrderBookUsecase interface
# and that the interface includes the GetClaimableOrdersForOrderbook method
# Search for the interface definition
ast-grep --pattern 'type OrderBookUsecase interface {
$$$
GetClaimableOrdersForOrderbook($$$) $$$
$$$
}'
Length of output: 2205
ingest/usecase/plugins/orderbook/claimbot/plugin.go (1)
45-63: LGTM: Constructor implementation is clean and well-structured
The constructor properly initializes all required fields and handles errors appropriately.
app/sidecar_query_server.go (1)
25-25: LGTM: Import statement follows conventions
The new import for the orderbook claimbot is correctly placed and follows the project's import grouping conventions.
ingest/usecase/plugins/orderbook/claimbot/tx.go (4)
27-58: Well-Implemented Transaction Function with Proper Error Handling
The sendBatchClaimTx function is well-structured and demonstrates appropriate error handling at each stage of the transaction preparation and sending process. The use of context, clear error messages, and concise code enhances maintainability and readability.
60-69: Clear Struct Definitions Enhance Readability
The introduction of the batchClaim and batchClaimOrders structs provides a clear and explicit representation of the batch claim message structure. This refactoring improves code readability and aligns with best practices for defining data models.
72-89: Efficient Batch Claim Message Preparation
The prepareBatchClaimMsg function efficiently constructs the JSON-encoded batch claim message from the provided orders. The function includes proper error handling for the marshaling process, ensuring robustness.
92-99: Correct Construction of Execute Contract Message
The buildExecuteContractMsg function correctly builds the MsgExecuteContract message with the appropriate sender, contract address, and message bytes. Setting Funds to sdk.NewCoins() indicates that no additional funds are being sent with the transaction, which is appropriate in this context.
| @@ -119,7 +119,7 @@ func SimulateMsgs( | |||
| txFactory = txFactory.WithAccountNumber(account.AccountNumber) | |||
| txFactory = txFactory.WithSequence(account.Sequence) | |||
| txFactory = txFactory.WithChainID(chainID) | |||
| txFactory = txFactory.WithGasAdjustment(1.05) | |||
| txFactory = txFactory.WithGasAdjustment(1.15) | |||
There was a problem hiding this comment.
Shall we define a const for this so that it is not like being "buried" somewhere in the code?
|
|
||
| var ( | ||
| tracer = otel.Tracer(tracerName) | ||
| fillThreshold = osmomath.MustNewDecFromStr("0.98") |
There was a problem hiding this comment.
Dito. Consider having a const representing 0.98
|
SonarCloud is complaining regarding |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (6)
domain/orderbook/order.go (1)
123-129: Enhance error field documentationThe structure looks good and aligns well with the PR objectives. Consider enhancing the documentation to clarify error handling behavior:
// ClaimableOrderbook represents a list of claimable orders for an orderbook. -// If an error occurs processing the orders, it is stored in the error field. +// If an error occurs while processing the orderbook or its orders, it is stored +// in the Error field. Individual order processing errors are captured in the +// respective ClaimableOrder's Error field. type ClaimableOrderbook struct {ingest/usecase/plugins/orderbook/claimbot/order.go (1)
21-21: Fix typo in function commentThere's a typo in the comment: "processing of each orderbook in done concurrently" should be "processing of each orderbook is done concurrently".
orderbook/usecase/orderbook_usecase.go (4)
508-508: Consider returning a specific error type instead of a generic oneReturning a specific error type can improve error handling and make it easier to distinguish different error cases.
575-581: Potential nil error handling in appending claimable ordersWhen appending to
result, theErrorfield is set toerr, which might benil. Ensure that the error handling logic correctly represents any errors encountered during processing.
532-535: Add context to error when fetching orders by tickIncluding additional context in the error message can help with debugging by indicating which tick and orderbook encountered the issue.
Apply this diff to enhance the error message:
if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get orders for tick %d in orderbook %s: %w", tick.Tick.TickId, orderbook.ContractAddress, err) }
563-566: Enhance error message when checking if tick is fully filledProviding more context in the error message can aid in identifying the specific tick causing the issue.
Apply this diff to improve the error handling:
if err != nil { - return nil, err + return nil, fmt.Errorf("failed to determine if tick %d is fully filled: %w", tickValues.TickID, err) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
ingest/usecase/plugins/orderbook/claimbot/order_test.gois excluded by!**/*_test.goorderbook/usecase/orderbook_usecase_test.gois excluded by!**/*_test.go
📒 Files selected for processing (7)
domain/mocks/orderbook_usecase_mock.go(2 hunks)domain/mvc/orderbook.go(2 hunks)domain/orderbook/order.go(1 hunks)ingest/usecase/plugins/orderbook/claimbot/order.go(1 hunks)ingest/usecase/plugins/orderbook/claimbot/plugin.go(1 hunks)orderbook/usecase/orderbook_usecase.go(1 hunks)orderbook/usecase/orderbooktesting/suite.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- domain/mocks/orderbook_usecase_mock.go
- domain/mvc/orderbook.go
- ingest/usecase/plugins/orderbook/claimbot/plugin.go
🔇 Additional comments (3)
orderbook/usecase/orderbooktesting/suite.go (2)
11-11: LGTM: Import is required for cosmwasmpool integration.
The new import is necessary for the cosmwasmpool.OrderbookTick type used in the changes below.
145-147: LGTM: Proper initialization of the Tick field.
The initialization of the Tick field with empty TickLiquidity is correct and type-safe.
orderbook/usecase/orderbook_usecase.go (1)
588-598: Avoid unnecessary creation of formatted limit orders for non-claimable orders
In isOrderClaimable, CreateFormattedLimitOrder is called even if the order is not claimable, which might be inefficient. Consider refactoring to determine claimability without needing to format the limit order unless necessary.
[performance]
Apply this diff to optimize:
func (o *OrderbookUseCaseImpl) isOrderClaimable(
orderbook domain.CanonicalOrderBooksResult,
order orderbookdomain.Order,
fillThreshold osmomath.Dec,
) (bool, error) {
- result, err := o.CreateFormattedLimitOrder(orderbook, order)
+ percentFilled, err := o.calculateOrderPercentFilled(orderbook, order)
if err != nil {
return false, err
}
- return result.IsClaimable(fillThreshold), nil
+ return percentFilled.GTE(fillThreshold), nil
}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
orderbook/usecase/orderbooktesting/suite.go (1)
147-148: Consider adding a comment explaining the purpose of the Tick field.The initialization looks correct, but adding a brief comment would help other developers understand why this field is necessary for testing.
+ // Tick represents the CosmWasm pool tick data required for testing tick.Tick = &cosmwasmpool.OrderbookTick{ TickLiquidity: cosmwasmpool.OrderbookTickLiquidity{}, }orderbook/usecase/orderbook_usecase.go (1)
505-522: Consider architectural improvements for scalabilityThe current implementation processes one tick at a time sequentially. For future scalability, consider:
- Implementing batch processing of ticks.
- Adding configurable concurrency limits.
- Supporting pagination for large orderbooks.
This aligns with the mentioned future enhancement of processing multiple order books in a single transaction.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
orderbook/usecase/orderbook_usecase.go(1 hunks)orderbook/usecase/orderbooktesting/suite.go(2 hunks)
🔇 Additional comments (3)
orderbook/usecase/orderbooktesting/suite.go (3)
11-11: LGTM!
The new import is correctly placed and necessary for the cosmwasmpool types.
118-118: Skipping comment as past review is still valid.
A previous review already suggested making CumulativeTotalValue configurable.
125-145: LGTM! Well-structured switch statement.
The refactored code:
- Improves readability with a clear switch structure
- Properly handles all directions including the new "all" case
- Includes appropriate error handling for invalid directions
|
* 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 (cherry picked from commit 2a46eeb)
* BE-586 | Claimbot (#524) --------- Co-authored-by: Deividas Petraitis <[email protected]>
* 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
This PR introduces The Orderbook Claimbot plugin. Orderbook Claimbot plugin is a plugin that claims filled order book orders.
It scans all active orders for each order book determining which orders have been filled and need to be claimed. At the moment order is said to be claimable if it is filled 98 percent or more. In order for an order book to be processed to claim its active orders it must be canonical as per SQS definition.
At this moment there is a few drawbacks of current implementation that can be improved in future, such as for example claiming orders from multiple orderbooks within single transaction, or using test keyring alternative.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores