Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MinRebalanceAmount to RFQ relayer #2263

Merged
merged 10 commits into from
Mar 13, 2024
Merged

Add MinRebalanceAmount to RFQ relayer #2263

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
11e1ac4
Feat: add MinRebalanceAmount with tests
dwasse Mar 13, 2024
7347ae6
[goreleaser]
dwasse Mar 13, 2024
e74b193
Fix: handle zero case
dwasse Mar 13, 2024
6f0e782
Feat: check for non positive amount outside of getRebalance()
dwasse Mar 13, 2024
3f55ebb
Cleanup: lint
dwasse Mar 13, 2024
c751c46
Cleanup: utilize getTokenConfig() helper
dwasse Mar 13, 2024
5e75220
[goreleaser]
dwasse Mar 13, 2024
a2423d9
update godoc [goreleaser]
trajan0x Mar 13, 2024
112197b
Cleanup: lint
dwasse Mar 13, 2024
43436e3
Fix: don't rebalance if desired amount less than min
dwasse Mar 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions services/rfq/relayer/inventory/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Check warning on line 373 in services/rfq/relayer/inventory/manager.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/inventory/manager.go#L373 

Added line #L373 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return nil
}
span.SetAttributes(
Expand Down Expand Up @@ -466,12 +466,18 @@
initialThresh, _ := new(big.Float).Mul(new(big.Float).SetInt(totalBalance), big.NewFloat(initialPct/100)).Int(nil)
amount := new(big.Int).Sub(maxTokenData.Balance, initialThresh)

// no need to rebalance since amount would be negative
if amount.Cmp(big.NewInt(0)) < 0 {
// 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
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


// clip the rebalance amount by the configured max
maxAmount := cfg.GetMaxRebalanceAmount(maxTokenData.ChainID, maxTokenData.Addr)
if amount.Cmp(maxAmount) > 0 {
Expand Down
27 changes: 24 additions & 3 deletions services/rfq/relayer/inventory/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func (i *InventoryTestSuite) TestGetRebalance() {
usdcDataDest.Addr: &usdcDataDest,
},
}
getConfig := func(maxRebalanceAmount string) relconfig.Config {
getConfig := func(minRebalanceAmount, maxRebalanceAmount string) relconfig.Config {
return relconfig.Config{
Chains: map[int]relconfig.ChainConfig{
origin: {
Expand All @@ -98,6 +98,7 @@ func (i *InventoryTestSuite) TestGetRebalance() {
Decimals: 6,
MaintenanceBalancePct: 20,
InitialBalancePct: 50,
MinRebalanceAmount: minRebalanceAmount,
MaxRebalanceAmount: maxRebalanceAmount,
},
},
Expand All @@ -109,6 +110,7 @@ func (i *InventoryTestSuite) TestGetRebalance() {
Decimals: 6,
MaintenanceBalancePct: 20,
InitialBalancePct: 50,
MinRebalanceAmount: minRebalanceAmount,
MaxRebalanceAmount: maxRebalanceAmount,
},
},
Expand All @@ -120,6 +122,7 @@ func (i *InventoryTestSuite) TestGetRebalance() {
Decimals: 6,
MaintenanceBalancePct: 0,
InitialBalancePct: 0,
MinRebalanceAmount: minRebalanceAmount,
MaxRebalanceAmount: maxRebalanceAmount,
},
},
Expand All @@ -129,13 +132,20 @@ func (i *InventoryTestSuite) TestGetRebalance() {
}

// 10 USDC on both chains; no rebalance needed
cfg := getConfig("")
cfg := getConfig("", "")
usdcDataOrigin.Balance = big.NewInt(1e7)
usdcDataDest.Balance = big.NewInt(1e7)
rebalance, err := inventory.GetRebalance(cfg, tokens, origin, usdcDataOrigin.Addr)
i.NoError(err)
i.Nil(rebalance)

// Set balances to zero
usdcDataOrigin.Balance = big.NewInt(0)
usdcDataDest.Balance = big.NewInt(0)
rebalance, err = inventory.GetRebalance(cfg, tokens, origin, usdcDataOrigin.Addr)
i.NoError(err)
i.Nil(rebalance)

// Set origin balance below maintenance threshold; need rebalance
usdcDataOrigin.Balance = big.NewInt(9e6)
usdcDataDest.Balance = big.NewInt(1e6)
Expand All @@ -148,8 +158,19 @@ func (i *InventoryTestSuite) TestGetRebalance() {
}
i.Equal(expected, rebalance)

// Set min rebalance amount
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


// Set max rebalance amount
cfgWithMax := getConfig("1.1")
cfgWithMax = getConfig("0", "1.1")
rebalance, err = inventory.GetRebalance(cfgWithMax, tokens, origin, usdcDataOrigin.Addr)
i.NoError(err)
expected = &inventory.RebalanceData{
Expand Down
3 changes: 3 additions & 0 deletions services/rfq/relayer/relconfig/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Check failure on line 98 in services/rfq/relayer/relconfig/config.go

View workflow job for this annotation

GitHub Actions / Lint (services/rfq) 

`defualts` is a misspelling of `defaults` (misspell)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// for USDC-through-cctp pairs this defualts to $1,000.
// for USDC-through-cctp pairs this defaults to $1,000.

MinQuoteAmount string `yaml:"min_quote_amount"`
// RebalanceMethod is the method to use for rebalancing.
RebalanceMethod string `yaml:"rebalance_method"`
// MaintenanceBalancePct is the percentage of the total balance under which a rebalance will be triggered.
MaintenanceBalancePct float64 `yaml:"maintenance_balance_pct"`
// InitialBalancePct is the percentage of the total balance to retain when triggering a rebalance.
InitialBalancePct float64 `yaml:"initial_balance_pct"`
// MinRebalanceAmount is the minimum amount to rebalance in human-readable units.
MinRebalanceAmount string `yaml:"min_rebalance_amount"`
// MaxRebalanceAmount is the maximum amount to rebalance in human-readable units.
MaxRebalanceAmount string `yaml:"max_rebalance_amount"`
}
Expand Down
40 changes: 26 additions & 14 deletions services/rfq/relayer/relconfig/getters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,25 +547,37 @@
return quoteAmountScaled
}

var defaultMaxRebalanceAmount = abi.MaxInt256
var defaultMinRebalanceAmount = big.NewInt(1000)

// GetMaxRebalanceAmount returns the max rebalance amount for the given chain and address.
// GetMinRebalanceAmount returns the min rebalance amount for the given chain and address.
// Note that this getter returns the value in native token decimals.
func (c Config) GetMaxRebalanceAmount(chainID int, addr common.Address) *big.Int {
chainCfg, ok := c.Chains[chainID]
if !ok {
//
//nolint:dupl
func (c Config) GetMinRebalanceAmount(chainID int, addr common.Address) *big.Int {
tokenCfg, _, err := c.getTokenConfigByAddr(chainID, addr.Hex())
if err != nil {

Check warning on line 558 in services/rfq/relayer/relconfig/getters.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relconfig/getters.go#L556-L558 

Added lines #L556 - L558 were not covered by tests
return defaultMaxRebalanceAmount
}

var tokenCfg *TokenConfig
for _, cfg := range chainCfg.Tokens {
if common.HexToAddress(cfg.Address).Hex() == addr.Hex() {
cfgCopy := cfg
tokenCfg = &cfgCopy
break
}
rebalanceAmountFlt, ok := new(big.Float).SetString(tokenCfg.MinRebalanceAmount)
if !ok || rebalanceAmountFlt == nil {
return defaultMinRebalanceAmount

Check warning on line 563 in services/rfq/relayer/relconfig/getters.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relconfig/getters.go#L561-L563 

Added lines #L561 - L563 were not covered by tests
}
if tokenCfg == nil {

// Scale by the token decimals.
denomDecimalsFactor := new(big.Int).Exp(big.NewInt(10), big.NewInt(int64(tokenCfg.Decimals)), nil)
minRebalanceAmountScaled, _ := new(big.Float).Mul(rebalanceAmountFlt, new(big.Float).SetInt(denomDecimalsFactor)).Int(nil)
return minRebalanceAmountScaled

Check warning on line 569 in services/rfq/relayer/relconfig/getters.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relconfig/getters.go#L567-L569 

Added lines #L567 - L569 were not covered by tests
}

var defaultMaxRebalanceAmount = abi.MaxInt256

// GetMaxRebalanceAmount returns the max rebalance amount for the given chain and address.
// Note that this getter returns the value in native token decimals.
//
//nolint:dupl
func (c Config) GetMaxRebalanceAmount(chainID int, addr common.Address) *big.Int {
tokenCfg, _, err := c.getTokenConfigByAddr(chainID, addr.Hex())
if err != nil {
return defaultMaxRebalanceAmount
}
rebalanceAmountFlt, ok := new(big.Float).SetString(tokenCfg.MaxRebalanceAmount)
Expand Down
Loading