feat(poller): if indexer is unresponsive, get orders from the node #19
Conversation
|
Warning Rate limit exceeded@zale144 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 0 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughRecent updates introduce enhancements across several Go files to improve error handling, logging, and data structures for handling orders. Key changes include adding a condition to check the length of the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
order_poller.go (1)
Line range hint
150-150: FunctiongetDemandOrdersFromIndexeris slightly too long. Consider refactoring to improve readability and maintainability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- order_eventer.go (1 hunks)
- order_poller.go (6 hunks)
Files skipped from review due to trivial changes (1)
- order_eventer.go
Additional context used
golangci-lint
order_poller.go
150-150: Function 'getDemandOrdersFromIndexer' is too long (63 > 60) (funlen)
Additional comments not posted (3)
order_poller.go (3)
17-18: No issues with the added imports. They are necessary for the new functionalities introduced in this pull request.
97-100: Robust error handling by falling back to node data when indexer fails. This enhances system resilience.
52-52: Reduced timeout may affect behavior under slow network conditions. Ensure to test this change thoroughly.
order_poller.go
Outdated
| func (p *orderPoller) getDemandOrdersFromNode(ctx context.Context) ([]Order, error) { | ||
| res, err := p.getDemandOrdersByStatus(ctx, commontypes.Status_PENDING.String()) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get demand orders: %w", err) | ||
| } | ||
|
|
||
| orders := make([]Order, 0, len(res)) | ||
|
|
||
| for _, order := range res { | ||
| if len(order.Price) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| denom := order.Price[0].Denom | ||
| if !p.tracker.canFulfillOrder(order.Id, denom) { | ||
| continue | ||
| } | ||
|
|
||
| newOrder := Order{ | ||
| EibcOrderId: order.Id, | ||
| Denom: denom, | ||
| Amount: order.Price.String(), | ||
| } | ||
| orders = append(orders, newOrder) | ||
| } | ||
|
|
||
| return orders, nil | ||
| } |
There was a problem hiding this comment.
Ensure that error messages are informative and consistent. Consider adding more detailed logging for debugging purposes.
+ p.logger.Debug("Querying demand orders by status", zap.String("status", status))Also applies to: 245-254
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (6)
order_fulfiller.go (1)
Line range hint
100-100: Consider refactoringprocessBatchto reduce its complexity and length, possibly by extracting some logic into separate functions.- func (ol *orderFulfiller) processBatch(ctx context.Context, batch []*demandOrder) error { + // Example refactoring by extracting a method + func (ol *orderFulfiller) processSingleOrder(ctx context.Context, order *demandOrder) error { + // processing logic for a single order + } + + func (ol *orderFulfiller) processBatch(ctx context.Context, batch []*demandOrder) error { + for _, order := range batch { + if err := ol.processSingleOrder(ctx, order); err != nil { + return err + } + } + return nil + }account.go (1)
Line range hint
108-108: Consider pre-allocating theaccountsslice to optimize memory usage, especially if the number of accounts is known or can be estimated.- var accounts []account + accounts := make([]account, 0, estimatedCount) // Replace `estimatedCount` with the actual estimated number of accountsorder_poller.go (2)
Line range hint
154-154: The functiongetDemandOrdersFromIndexeris too long. Consider breaking it down into smaller, more manageable functions.
[!TIP]
Codebase VerificationThe search across all identified test files revealed only one test function,
TestBotStore, instore/store_test.go. This indicates that there are no direct or indirect tests for the time parsing logic in the current test suite.
- Ensure that the time parsing logic in
order_poller.gohandles all expected formats by adding relevant test cases.Analysis chain
Line range hint
172-199: Good implementation of time parsing and filtering logic. However, ensure the time parsing logic handles all expected formats.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the time parsing logic handles all expected formats. # Test: Search for different time formats in the test cases. rg --type go "time format" "test_*"Length of output: 84
Script:
#!/bin/bash # Step 1: Locate all Go test files in the repository. fd --type f --extension go --exec echo {} > test_files.txt # Step 2: Search for time parsing test cases within the identified test files. rg "time format" $(cat test_files.txt)Length of output: 440
Script:
#!/bin/bash # Search for test functions that might indirectly test the time parsing logic. rg "func Test" ./store/store_test.goLength of output: 72
Script:
#!/bin/bash # Search for test functions across all identified test files. rg "func Test" $(cat test_files.txt)Length of output: 353
order_client.go (2)
Line range hint
29-29: The functionnewOrderClientis too long. Consider refactoring to improve readability and maintainability.
Line range hint
248-248: The functionrefundFromExtraBotsToWhaleis too long. Breaking it down into smaller functions would enhance readability and maintainability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- account.go (3 hunks)
- order_client.go (2 hunks)
- order_eventer.go (2 hunks)
- order_fulfiller.go (2 hunks)
- order_poller.go (7 hunks)
- types.go (1 hunks)
Additional context used
golangci-lint
order_fulfiller.go
100-100: Function 'processBatch' is too long (78 > 60) (funlen)
account.go
108-108: Consider pre-allocating
accounts(prealloc)order_poller.go
154-154: Function 'getDemandOrdersFromIndexer' is too long (64 > 60) (funlen)
order_client.go
29-29: Function 'newOrderClient' has too many statements (46 > 40) (funlen)
248-248: Function 'refundFromExtraBotsToWhale' is too long (85 > 60) (funlen)
Additional comments not posted (12)
types.go (1)
16-16: Addition offromfield indemandOrderstruct aligns with the PR's objective to enhance order source tracking.Verification successful
The
fromfield is used consistently across multiple files for logging and error handling, indicating it is integrated correctly for tracking the source of orders.
order_poller.go: The field is used in error messages and assignment.order_fulfiller.go: The field is used in debug and info logs.order_eventer.go: The field is used in an error message.order_client.go: The field is not directly used but related comments are present.config.goandcmd.go: The field is referenced in comments and log messages.order_tracker.go: The field is mentioned in a comment.This confirms that the addition of the
fromfield in thedemandOrderstruct is consistent and aligns with the project's needs.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `from` field across the codebase. # Test: Search for the field usage. Expect: Consistent use in related modules. rg --type go $'from'Length of output: 1255
order_eventer.go (2)
53-55: Checking ifnewOrdersis empty before processing is a good practice to avoid unnecessary operations.
116-116: Properly setting thefromfield indemandOrderduring parsing aligns with the new data structure requirements.Verification successful
The
fromfield is set to "events" inorder_eventer.goand appears in multiple other files. To ensure correct handling and alignment with the new data structure requirements, we need to verify its context and usage in these files.
The
fromfield is being set and used correctly across the relevant files, providing context about the source of the orders. The field is set to "events" inorder_eventer.go, "indexer" and "node" inorder_poller.go, and is used in logging inorder_fulfiller.go.
order_eventer.go: Thefromfield is set to "events" when creating ademandOrder.order_poller.go: Thefromfield is set to "indexer" or "node" based on the source of the order.order_fulfiller.go: Thefromfield is used in log messages to indicate the source of the orders being processed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure that the `from` field is being set and used correctly in event parsing and other relevant parts of the application. # Test: Search for the `from` field setting and usage. Expect: Proper handling and usage. rg --type go $'from'Length of output: 1255
Script:
#!/bin/bash # Description: Gather context around the `from` field in relevant files to ensure proper handling and usage. # Extract 5 lines before and after the `from` field in `order_eventer.go` to understand its context. rg --type go -A 5 -B 5 'from: *"events"' order_eventer.go # Extract 5 lines before and after the `from` field in `order_poller.go` to understand its context. rg --type go -A 5 -B 5 'from:' order_poller.go # Extract 5 lines before and after the `from` field in `order_fulfiller.go` to understand its context. rg --type go -A 5 -B 5 'from:' order_fulfiller.goLength of output: 1090
order_fulfiller.go (1)
138-138: Enhanced logging with additional context (fromfield) helps in better monitoring and debugging of order processing.Also applies to: 170-170
account.go (1)
Line range hint
154-198: Simplification of theensureBalancesmethod by removing the error tuple helps streamline the code and reduce error handling complexity. Ensure that all potential errors are still appropriately handled or logged.order_poller.go (6)
17-18: Imports forcommontypesandeibctypeshave been correctly added as per the summary.
52-52: The HTTP client timeout setup is a good practice for network calls to avoid indefinitely hanging requests.
67-67: The addition of thefromfield in theOrderstruct aligns with the changes described in the summary and is consistent across related files.
120-120: Thefromfield is correctly set based on the data source, ensuring traceability of order origins.
207-207: Setting thefromfield to "indexer" provides clear traceability of the data source.
221-249: The methodgetDemandOrdersFromNodeis implemented to handle fetching orders directly from the node when necessary, aligning with the PR's objectives.order_client.go (1)
19-22: The restructuring of theorderClientstruct is well done, removing unnecessary fields and simplifying the design.
| totalAmount, err := sdk.ParseCoinNormalized(d.Amount) | ||
| if err != nil { | ||
| p.logger.Error("failed to parse amount", zap.Error(err)) | ||
| continue | ||
| amountStr := fmt.Sprintf("%s%s", d.Amount, d.Denom) | ||
| totalAmount, err = sdk.ParseCoinNormalized(amountStr) | ||
| if err != nil { | ||
| p.logger.Error("failed to parse amount", zap.Error(err), zap.String("amount", amountStr)) | ||
| continue | ||
| } |
There was a problem hiding this comment.
The error handling for parsing the coin amount has been enhanced, but consider refactoring to avoid code duplication.
- totalAmount, err := sdk.ParseCoinNormalized(d.Amount)
- if err != nil {
- amountStr := fmt.Sprintf("%s%s", d.Amount, d.Denom)
- totalAmount, err = sdk.ParseCoinNormalized(amountStr)
- if err != nil {
- p.logger.Error("failed to parse amount", zap.Error(err), zap.String("amount", amountStr))
- continue
- }
- }
+ if totalAmount, err := parseAmount(d.Amount, d.Denom); err != nil {
+ p.logger.Error("failed to parse amount", zap.Error(err), zap.String("amount", amountStr))
+ continue
+ }Add a helper function parseAmount to handle the parsing logic.
Committable suggestion was skipped due to low confidence.
| p.logger.Error("failed to get demand orders from indexer. fallback to getting demand orders from the node", zap.Error(err)) | ||
| orders, err = p.getDemandOrdersFromNode(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get demand orders: %w", err) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The fallback mechanism for getting demand orders from the node when the indexer fails is not covered by existing tests. Please add unit tests to ensure this fallback logic is thoroughly tested.
Analysis chain
Enhanced error handling with fallback logic to get orders from the node when the indexer fails is a robust addition. However, ensure that this fallback mechanism is tested thoroughly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the fallback mechanism by simulating indexer failure.
# Test: Check the unit test coverage for the fallback logic.
rg --type go "Test_pollPendingDemandOrders"
Length of output: 43
Script:
#!/bin/bash
# Description: Verify the fallback mechanism by finding tests related to polling demand orders.
# Search for test functions related to polling demand orders.
rg --type go "func Test_"
# Search for any references to the getDemandOrdersFromNode function in test files.
rg --type go "getDemandOrdersFromNode"
Length of output: 231
Script:
#!/bin/bash
# Description: Verify the fallback mechanism by finding test files related to order_poller.
# Search for test files containing references to order_poller.
rg --type go "order_poller"
Length of output: 27
order_poller.go
Outdated
| func (p *orderPoller) getDemandOrdersByStatus(ctx context.Context, status string) ([]*eibctypes.DemandOrder, error) { | ||
| queryClient := eibctypes.NewQueryClient(p.client.Context()) | ||
| resp, err := queryClient.DemandOrdersByStatus(ctx, &eibctypes.QueryDemandOrdersByStatusRequest{ | ||
| Status: status, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get demand orders: %w", err) | ||
| } | ||
| return resp.DemandOrders, nil | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
There are no unit tests for the getDemandOrdersByStatus function or any other Go test files in the repository. This indicates that the function is not covered by tests.
- Ensure that unit tests are added for
getDemandOrdersByStatusto cover various edge cases and error handling.
Analysis chain
Proper implementation of getDemandOrdersByStatus. Ensure that the error handling is robust and that unit tests cover various edge cases.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `getDemandOrdersByStatus` has comprehensive unit tests.
# Test: Search for unit tests covering this function.
rg --type go "Test_getDemandOrdersByStatus"
Length of output: 43
Script:
#!/bin/bash
# Description: Verify that `getDemandOrdersByStatus` has comprehensive unit tests.
# Search for references to `getDemandOrdersByStatus` in Go test files
rg --type go "getDemandOrdersByStatus" --tests
Length of output: 478
Script:
#!/bin/bash
# Description: Verify that `getDemandOrdersByStatus` has comprehensive unit tests.
# Search for references to `getDemandOrdersByStatus` in Go test files
rg --type go "getDemandOrdersByStatus"
Length of output: 269
Script:
#!/bin/bash
# Description: Verify that `getDemandOrdersByStatus` has comprehensive unit tests.
# Search for test files related to order_poller.go
rg --type go "order_poller"
Length of output: 27
Script:
#!/bin/bash
# Description: List all Go test files in the repository to check for any that might be relevant to order_poller.go.
# List all Go test files
fd --type f --extension go --exec rg --files-with-matches "_test\.go"
Length of output: 71
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
order_fulfiller.go (1)
Line range hint
100-100: TheprocessBatchfunction is too long, which may hinder readability and maintainability.Consider refactoring this function to break down its responsibilities or extracting parts of the logic into separate methods.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- order_fulfiller.go (3 hunks)
Additional context used
golangci-lint
order_fulfiller.go
100-100: Function 'processBatch' is too long (78 > 60) (funlen)
Additional comments not posted (3)
order_fulfiller.go (3)
138-138: Added context to logging statements is beneficial for tracing and debugging.
140-140: The methodensureBalancesnow returns only theensuredDenomswithout an error, simplifying error handling in this context.
170-170: Enhanced logging with thefromfield provides better traceability of order processing sources, useful for debugging and operational monitoring.Also applies to: 176-176
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- cmd.go (3 hunks)
- order_client.go (3 hunks)
- order_poller.go (8 hunks)
Files not reviewed due to errors (1)
- order_poller.go (no review received)
Additional context used
golangci-lint
order_poller.go
155-155: Function 'getDemandOrdersFromIndexer' is too long (64 > 60) (funlen)
order_client.go
29-29: Function 'newOrderClient' has too many statements (46 > 40) (funlen)
248-248: Function 'refundFromExtraBotsToWhale' is too long (85 > 60) (funlen)
Additional comments not posted (2)
order_client.go (1)
19-22: The reorganization of theorderClientstruct is clear and improves readability. Ensure that all references and usages of this struct are updated accordingly.cmd.go (1)
93-195: Added a new commandfulfill-from-fileto handle demand orders from a file. This is a significant feature addition. Ensure that all necessary configurations and error handling are robustly implemented.
order_client.go
Outdated
| /*if err := oc.orderEventer.start(ctx); err != nil { | ||
| return fmt.Errorf("failed to subscribe to demand orders: %w", err) | ||
| } | ||
| }*/ |
There was a problem hiding this comment.
The commented-out code for starting the order eventer should either be removed or properly handled if it's intended to be used in the future.
- /*if err := oc.orderEventer.start(ctx); err != nil {
- return fmt.Errorf("failed to subscribe to demand orders: %w", err)
- }*/
+ // TODO: Decide whether to start order eventer based on configuration or remove this code.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.
| /*if err := oc.orderEventer.start(ctx); err != nil { | |
| return fmt.Errorf("failed to subscribe to demand orders: %w", err) | |
| } | |
| }*/ | |
| // TODO: Decide whether to start order eventer based on configuration or remove this code. |
Summary by CodeRabbit
New Features
fulfill-from-fileto fulfill demand orders from a file.Improvements
Refactor
orderClientstruct.