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 3 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
10 changes: 8 additions & 2 deletions services/rfq/relayer/inventory/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,12 +466,18 @@ func getRebalance(span trace.Span, cfg relconfig.Config, tokens map[int]map[comm
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
2 changes: 2 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 @@ -102,6 +102,8 @@ type TokenConfig struct {
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
32 changes: 32 additions & 0 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,11 +547,43 @@
return quoteAmountScaled
}

var defaultMinRebalanceAmount = big.NewInt(1000)

// 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) GetMinRebalanceAmount(chainID int, addr common.Address) *big.Int {

Check failure on line 554 in services/rfq/relayer/relconfig/getters.go

View workflow job for this annotation

GitHub Actions / Lint (services/rfq) 

554-580 lines are duplicate of `relayer/relconfig/getters.go:586-612` (dupl)
chainCfg, ok := c.Chains[chainID]
if !ok {
return defaultMinRebalanceAmount
}

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#L554-L558 

Added lines #L554 - L558 were not covered by tests

var tokenCfg *TokenConfig
for _, cfg := range chainCfg.Tokens {
if common.HexToAddress(cfg.Address).Hex() == addr.Hex() {
cfgCopy := cfg
tokenCfg = &cfgCopy
break

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

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relconfig/getters.go#L560-L565 

Added lines #L560 - L565 were not covered by tests
}
}
if tokenCfg == nil {
return defaultMinRebalanceAmount
}
rebalanceAmountFlt, ok := new(big.Float).SetString(tokenCfg.MinRebalanceAmount)
if !ok || rebalanceAmountFlt == nil {
return defaultMinRebalanceAmount
}

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

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relconfig/getters.go#L568-L574 

Added lines #L568 - L574 were not covered by tests

// 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 579 in services/rfq/relayer/relconfig/getters.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/relconfig/getters.go#L577-L579 

Added lines #L577 - L579 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.
func (c Config) GetMaxRebalanceAmount(chainID int, addr common.Address) *big.Int {

Check failure on line 586 in services/rfq/relayer/relconfig/getters.go

View workflow job for this annotation

GitHub Actions / Lint (services/rfq) 

586-612 lines are duplicate of `relayer/relconfig/getters.go:554-580` (dupl)
chainCfg, ok := c.Chains[chainID]
if !ok {
return defaultMaxRebalanceAmount
Expand Down
Loading