BE-594/CreateFormattedLimitOrder | OrderbookUsecase CreateFormattedLimitOrder #531
Conversation
…mitOrder method Refactor OrderbookUsecase to expose CreateFormattedLimitOrder as public method. That allows to reuse existing code for dealing with limit orders such as for example computing fill percentage of the orders.
| @@ -686,28 +686,6 @@ func (s *OrderbookUsecaseTestSuite) TestProcessOrderBookActiveOrders() { | |||
| expectedOrders: nil, | |||
| expectedIsBestEffort: false, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
Those test cases were moved to TestCreateFormattedLimitOrder.
WalkthroughThe changes in this pull request involve the addition of a new method, Changes
Possibly related PRs
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: 1
🧹 Outside diff range and nitpick comments (2)
domain/mvc/orderbook.go (1)
26-27: LGTM: New method added correctly with a minor suggestion.The
CreateFormattedLimitOrdermethod has been successfully added to theOrderBookUsecaseinterface. The method signature aligns with the PR objectives and seems appropriate for creating a formatted limit order.Consider enhancing the method comment to provide more context:
// CreateFormattedLimitOrder creates a formatted limit order from the given orderbook and order. // This method is used for tasks such as computing the fill percentage of limit orders.This additional information helps developers understand the method's use case more clearly.
domain/mocks/orderbook_usecase_mock.go (1)
50-54: LGTM: WithCreateFormattedLimitOrder method is well-implemented.The
WithCreateFormattedLimitOrdermethod is correctly implemented and follows the existing pattern for setting up mock functions. This will be useful for configuring test scenarios.Consider adding a comment to explain the purpose of this method, similar to other methods in the mock. For example:
// WithCreateFormattedLimitOrder sets up the mock to return the specified order and error when CreateFormattedLimitOrder is called. func (m *OrderbookUsecaseMock) WithCreateFormattedLimitOrder(order orderbookdomain.LimitOrder, err error) { // ... (existing implementation) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
orderbook/usecase/export_test.gois excluded by!**/*_test.goorderbook/usecase/orderbook_usecase_test.gois excluded by!**/*_test.go
📒 Files selected for processing (3)
- domain/mocks/orderbook_usecase_mock.go (3 hunks)
- domain/mvc/orderbook.go (2 hunks)
- orderbook/usecase/orderbook_usecase.go (4 hunks)
🧰 Additional context used
🔇 Additional comments (11)
domain/mvc/orderbook.go (1)
6-6: LGTM: Import statement added correctly.The new import for the
domainpackage is necessary and correctly placed. It aligns with the existing import style and supports the new method's parameter type.domain/mocks/orderbook_usecase_mock.go (4)
6-8: LGTM: Import addition is correct and necessary.The new import for the
domainpackage is correctly added and is required for the newCreateFormattedLimitOrdermethod. This change aligns with the existing import style and structure.
16-20: LGTM: OrderbookUsecaseMock struct updated correctly.The addition of the
CreateFormattedLimitOrderFuncfield to theOrderbookUsecaseMockstruct is correct and follows the existing pattern for mock functions. This new field will allow for flexible mocking of theCreateFormattedLimitOrdermethod in tests.
56-61: LGTM: CreateFormattedLimitOrder method is correctly implemented.The
CreateFormattedLimitOrdermethod is well-implemented and follows the existing pattern for mock methods in this struct. It correctly calls theCreateFormattedLimitOrderFuncif set and panics with "unimplemented" if not set, which is consistent with other methods in this mock.
Line range hint
1-61: Overall, the changes to the OrderbookUsecaseMock are well-implemented and align with the PR objectives.The additions to this mock file are consistent with the existing code structure and patterns. The new
CreateFormattedLimitOrderfunctionality has been correctly added, including the necessary struct field, setup method, and implementation method. These changes will facilitate effective testing of the new public method in theOrderbookUsecaseinterface.To further improve the file:
- Consider adding comments to new methods for better documentation.
- Ensure that the corresponding interface in the
mvcpackage has been updated to include theCreateFormattedLimitOrdermethod.Let's verify that the
OrderBookUsecaseinterface in themvcpackage has been updated:orderbook/usecase/orderbook_usecase.go (6)
265-265: Consistent Parameter Naming Improves ReadabilityRenaming the parameter from
orderBooktoorderbookaligns with Go naming conventions and improves code readability.
266-266: Good Addition of Orderbook ValidationAdding the
orderbook.Validate()check ensures that the orderbook data is valid before processing, enhancing robustness.
270-273: Proper Usage oforderbook.ContractAddressUpdating the method to use
orderbook.ContractAddressensures that the correct contract address is used when fetching active orders and handling errors.
293-294: Correct Refactoring of Method CallPassing
orderbookdirectly toCreateFormattedLimitOrdersimplifies the method call and leverages the updated method signature effectively.
316-317: MakingCreateFormattedLimitOrderPublic Enhances ReusabilityChanging
createFormattedLimitOrdertoCreateFormattedLimitOrderallows external packages to use this method, promoting code reuse for limit order formatting.
495-495: Consistent Use oforderbook.ContractAddressAssigning
OrderbookAddress: orderbook.ContractAddressensures consistency across the code and accurate tracking of the orderbook address in the limit order.
| quoteToken, err := o.tokensUsecease.GetMetadataByChainDenom(orderbook.Quote) | ||
| if err != nil { |
There was a problem hiding this comment.
Typo in Variable Name tokensUsecease
The field tokensUsecease appears to be a misspelling of tokensUsecase. This typo could lead to confusion and potential errors.
Apply this diff to correct the typo:
- quoteToken, err := o.tokensUsecease.GetMetadataByChainDenom(orderbook.Quote)
+ quoteToken, err := o.tokensUsecase.GetMetadataByChainDenom(orderbook.Quote)
- baseToken, err := o.tokensUsecease.GetMetadataByChainDenom(orderbook.Base)
+ baseToken, err := o.tokensUsecase.GetMetadataByChainDenom(orderbook.Base)Also applies to: 331-332
|
Note for reviewer: no idea why build fails on the runner, it builds locally fine, this issue is already logged as task in our backlog. |
This PR refactors
OrderbookUsecaseto exposeCreateFormattedLimitOrderas public method. That allows to reuse existing code for dealing with limit orders such as for example computing fill percentage of the orders.CreateFormattedLimitOrderis exactly for that purpose used in the claimbot here.Summary by CodeRabbit
New Features
Bug Fixes
Refactor