Add MinRebalanceAmount to RFQ relayer #2263
Conversation
WalkthroughThe update enhances rebalance management for RFQ services by introducing a minimum rebalance amount. This change ensures rebalance operations are first limited by the minimum amount before being capped by the maximum limit. It provides better control over inventory rebalancing, improving efficiency and effectiveness. Changes
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 (
|
Deploying with
|
| Latest commit: |
43436e3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2426b6a1.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-min-rebalance-amount.sanguine-fe.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yaml
Files selected for processing (4)
- services/rfq/relayer/inventory/manager.go (1 hunks)
- services/rfq/relayer/inventory/manager_test.go (6 hunks)
- services/rfq/relayer/relconfig/config.go (1 hunks)
- services/rfq/relayer/relconfig/getters.go (1 hunks)
Check Runs (15)
Go Generate (Module Changes) (services/stiprelayer) completed (3)
Go Generate (Module Changes) (services/rfq) completed (1)
Lint (services/rfq) completed (5)
- services/rfq/relayer/relconfig/getters.go: 554-554: 554-580 lines are duplicate of
relayer/relconfig/getters.go:586-612(dupl) - services/rfq/relayer/relconfig/getters.go: 586-586: 586-612 lines are duplicate of
relayer/relconfig/getters.go:554-580(dupl)
Go Coverage (1.21.x, ubuntu-latest, services/stiprelayer) completed (7)
Go Coverage (1.21.x, ubuntu-latest, services/rfq) completed (5)
Get PR Metadata completed (2)
SonarCloud completed (2)
general-linters completed (1)
Change Detection completed (13)
label-statuses completed (3)
pr-labeler completed (2)
Build (1.21.x, ubuntu-latest) completed (3)
run-goreleaser (services/stiprelayer) completed (7)
run-goreleaser (services/rfq) completed (7)
build-goreleaser completed (2)
Additional comments: 3
services/rfq/relayer/inventory/manager_test.go (1)
- 98-104: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [91-125]
The addition of
minRebalanceAmountto thegetConfigfunction and its subsequent usage in configuringMinRebalanceAmountfor token configurations is correctly implemented. This change aligns with the PR's objective to introduce a minimum threshold for rebalance transactions. However, ensure that theminRebalanceAmountandmaxRebalanceAmountparameters are validated for proper formatting and logical values (e.g.,minRebalanceAmountshould not exceedmaxRebalanceAmount) before being used in configurations.services/rfq/relayer/relconfig/config.go (1)
- 105-106: Adding the
MinRebalanceAmountfield to theTokenConfigstruct is correctly implemented and aligns with the PR's objective. However, consider the implications of using astringtype for monetary values. While this allows for flexibility in specifying amounts, it also introduces the need for parsing and validation. Ensure that there are appropriate validations in place to parse these string values into numerical types safely and accurately, especially considering the potential for decimal values and the need to handle different token decimals.services/rfq/relayer/relconfig/getters.go (1)
- 550-580: The implementation of
GetMinRebalanceAmountcorrectly retrieves and scales the minimum rebalance amount based on token decimals, which is crucial for accurate calculations. However, the static analysis tool flagged a potential duplication issue with lines 554-580 being similar to lines 586-612. Upon inspection, it seems there might have been a misunderstanding or a copy-paste error, as the latter lines do not exist in the provided code snippet. This might be a false positive from the tool. Nonetheless, ensure that there is no unintended code duplication within the project that could lead to maintenance issues.
| // clip the rebalance amount by the configured min | ||
| minAmount := cfg.GetMinRebalanceAmount(maxTokenData.ChainID, maxTokenData.Addr) | ||
| if amount.Cmp(minAmount) < 0 { | ||
| amount = minAmount | ||
| } |
There was a problem hiding this comment.
The implementation correctly applies the MinRebalanceAmount before considering the maximum rebalance amount. This ensures that rebalances are not executed for amounts below the specified minimum threshold, aligning with the PR's objectives. However, it's important to ensure that cfg.GetMinRebalanceAmount and cfg.GetMaxRebalanceAmount handle potential errors gracefully and return sensible defaults in case of misconfigurations or missing values. Additionally, consider adding logging or metrics around rebalance amounts to facilitate monitoring and debugging of the rebalancing process.
| cfgWithMax := getConfig("10", "1000000000") | ||
| rebalance, err = inventory.GetRebalance(cfgWithMax, tokens, origin, usdcDataOrigin.Addr) | ||
| i.NoError(err) | ||
| expected = &inventory.RebalanceData{ | ||
| OriginMetadata: &usdcDataOrigin, | ||
| DestMetadata: &usdcDataDest, | ||
| Amount: big.NewInt(1e7), | ||
| } | ||
| i.Equal(expected, rebalance) |
There was a problem hiding this comment.
The test case setup using getConfig("10", "1000000000") to set a minimum rebalance amount demonstrates the intended functionality. However, it's important to include additional test cases that cover edge cases, such as when the minRebalanceAmount is higher than the available balance, to ensure robustness and handle potential errors gracefully.
Would you like me to help generate these additional test cases?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2263 +/- ##
===================================================
+ Coverage 47.84597% 47.87293% +0.02695%
===================================================
Files 360 360
Lines 26903 26915 +12
Branches 83 83
===================================================
+ Hits 12872 12885 +13
- Misses 12691 12692 +1
+ Partials 1340 1338 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- services/rfq/relayer/inventory/manager.go (1 hunks)
- services/rfq/relayer/inventory/manager_test.go (6 hunks)
Check Runs (13)
codecov/patch completed (4)
Go Generate (Module Changes) (services/stiprelayer) completed (6)
Go Generate (Module Changes) (services/rfq) completed (3)
Go Coverage (1.21.x, ubuntu-latest, services/stiprelayer) completed (4)
Go Coverage (1.21.x, ubuntu-latest, services/rfq) completed (4)
Lint (services/rfq) completed (5)
Build (1.21.x, ubuntu-latest) completed (2)
Get PR Metadata completed (2)
SonarCloud completed (2)
Change Detection completed (13)
general-linters completed (1)
label-statuses completed (3)
pr-labeler completed (2)
Files skipped from review as they are similar to previous changes (2)
- services/rfq/relayer/inventory/manager.go
- services/rfq/relayer/inventory/manager_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yaml
Files selected for processing (3)
- services/rfq/relayer/inventory/manager.go (2 hunks)
- services/rfq/relayer/relconfig/config.go (1 hunks)
- services/rfq/relayer/relconfig/getters.go (1 hunks)
Check Runs (14)
codecov/patch completed (3)
- services/rfq/relayer/relconfig/getters.go: 556-556: Added line #L556 was not covered by tests
- services/rfq/relayer/relconfig/getters.go: 561-563: Added lines #L561 - L563 were not covered by tests
- services/rfq/relayer/relconfig/getters.go: 567-569: Added lines #L567 - L569 were not covered by tests
Go Generate (Module Changes) (services/stiprelayer) completed (6)
Lint (services/rfq) completed (4)
- services/rfq/relayer/relconfig/config.go: 98-98:
defualtsis a misspelling ofdefaults(misspell)
Go Coverage (1.21.x, ubuntu-latest, services/stiprelayer) completed (4)
Go Coverage (1.21.x, ubuntu-latest, services/rfq) completed (4)
run-goreleaser (services/stiprelayer) completed (7)
run-goreleaser (services/rfq) completed (7)
Build (1.21.x, ubuntu-latest) completed (2)
Get PR Metadata completed (2)
Change Detection completed (13)
general-linters completed (1)
label-statuses completed (3)
pr-labeler completed (2)
build-goreleaser completed (2)
Additional comments: 2
services/rfq/relayer/relconfig/config.go (1)
- 106-107: The addition of
MinRebalanceAmountto theTokenConfigstruct aligns with the PR objectives to introduce a minimum threshold for rebalance transactions. However, it's important to ensure that the typestringis the most appropriate choice for representing amounts, considering the operations that will be performed on these values.Consider verifying if a numeric type would be more suitable for representing
MinRebalanceAmount, especially if arithmetic operations or comparisons are involved. Using a string requires parsing to a numeric type before any such operations, which could introduce overhead and potential for errors.services/rfq/relayer/relconfig/getters.go (1)
- 556-570: The function
GetMinRebalanceAmountcorrectly implements the logic to retrieve and scale the minimum rebalance amount by token decimals. However, there are a few points to consider:
- Ensure that the function is covered by unit tests to validate its behavior under various scenarios, including error handling and edge cases.
- Verify the handling of
MinRebalanceAmountas a string, especially the implications of parsing and scaling this value. Consider the potential for parsing errors and the impact on performance.
| // no need to rebalance since amount would not be positive | ||
| if amount.Cmp(big.NewInt(0)) <= 0 { | ||
| //nolint:nilnil | ||
| return nil, nil | ||
| } | ||
|
|
||
| // clip the rebalance amount by the configured min | ||
| minAmount := cfg.GetMinRebalanceAmount(maxTokenData.ChainID, maxTokenData.Addr) | ||
| if amount.Cmp(minAmount) < 0 { | ||
| amount = minAmount | ||
| } |
There was a problem hiding this comment.
The implementation of clipping the rebalance amount by the configured minimum before the maximum is correctly aligned with the PR objectives. However, it's crucial to ensure that the cfg.GetMinRebalanceAmount and cfg.GetMaxRebalanceAmount methods handle potential errors gracefully. If these methods can return errors, the current implementation might panic or behave unexpectedly when encountering misconfigurations or missing values. Consider adding error handling for these method calls to improve robustness.
Additionally, incorporating logging or metrics around rebalance amounts could facilitate monitoring and debugging of the rebalancing process. This would provide visibility into the operation of the rebalance logic, especially in production environments.
Would you like assistance in adding error handling and logging around these configurations?
| @@ -95,13 +95,16 @@ | |||
| // For now, specify the USD price of the token in the config. | |||
| PriceUSD float64 `yaml:"price_usd"` | |||
| // MinQuoteAmount is the minimum amount to quote for this token in human-readable units. | |||
| // for USDC-through-cctp pairs this defualts to $1,000. | |||
There was a problem hiding this comment.
There's a typographical error in the comment above MinQuoteAmount. It should be "defaults" instead of "defualts".
- // for USDC-through-cctp pairs this defualts to $1,000.
+ // for USDC-through-cctp pairs this defaults to $1,000.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.
| // for USDC-through-cctp pairs this defualts to $1,000. | |
| // for USDC-through-cctp pairs this defaults to $1,000. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- services/rfq/relayer/relconfig/config.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/relconfig/config.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- services/rfq/relayer/inventory/manager.go (2 hunks)
- services/rfq/relayer/inventory/manager_test.go (6 hunks)
Check Runs (1)
codecov/patch completed (4)
- services/rfq/relayer/inventory/manager.go: 373-373: Added line #L373 was not covered by tests
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/inventory/manager_test.go
Additional comments: 1
services/rfq/relayer/inventory/manager.go (1)
- 469-478: The implementation of clipping the rebalance amount by the configured minimum amount (lines #L476-L478) before clipping it by the maximum amount is a critical addition to the rebalancing logic. This change aligns with the PR's objective to introduce
MinRebalanceAmountfor improving the efficiency of rebalancing operations. The logic correctly checks if the calculated rebalance amount is less than the minimum required amount and, if so, decides against proceeding with the rebalance.This change effectively ensures that rebalancing operations are economically viable by preventing transactions below the specified minimum rebalance amount. It's a well-integrated addition that enhances the rebalancing strategy's effectiveness.
| @@ -370,7 +370,7 @@ | |||
| if err != nil { | |||
| return fmt.Errorf("could not get rebalance: %w", err) | |||
| } | |||
| if rebalance == nil { | |||
| if rebalance == nil || rebalance.Amount.Cmp(big.NewInt(0)) <= 0 { | |||
There was a problem hiding this comment.
The logic to check if the rebalance amount is positive and proceed with rebalancing is sound. However, as noted in previous comments and static analysis hints, this new logic introduced at line #L373 is not covered by unit tests. It's crucial to ensure that this new behavior, especially the handling of MinRebalanceAmount, is thoroughly tested to prevent potential issues in rebalancing operations.
Consider adding unit tests that specifically cover scenarios where the rebalance amount is clipped by the minimum amount before the maximum. This will help validate the correctness of the new logic and ensure the feature works as intended across various scenarios.
Description
Adds
MinRebalanceAmountwhich puts a floor on the rebalance size.Summary by CodeRabbit