RFQ Relayer: add scroll rebalancing #2863
Conversation
|
Warning Rate limit exceeded@dwasse has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 48 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. WalkthroughThe recent updates enhance rebalancing mechanics across blockchain networks by introducing a versatile management system that supports multiple rebalance methods and improves error handling. The addition of 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 Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2863 +/- ##
===================================================
- Coverage 25.76370% 25.37995% -0.38375%
===================================================
Files 772 791 +19
Lines 55683 56718 +1035
Branches 82 82
===================================================
+ Hits 14346 14395 +49
- Misses 39845 40837 +992
+ Partials 1492 1486 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![greptile-apps[bot]](https://github.com/avatars/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
- Added
scroll-contractssubmodule for scroll rebalancing functionality (services/cctp-relayer/external/scroll-contracts) - Introduced metadata handling for
l1gatewayandl2gatewaycontracts (services/rfq/contracts/l1gateway/l1gateway.metadata.go,services/rfq/contracts/l2gateway/l2gateway.metadata.go) - Initialized event topics for
L1GatewayRouterandL2GatewayRoutercontracts (services/rfq/contracts/l1gateway/topics.go,services/rfq/contracts/l2gateway/topics.go) - Added support for Scroll rebalance method in inventory manager (
services/rfq/relayer/inventory/manager.go,services/rfq/relayer/inventory/scroll.go) - Updated configuration to include L1 and L2 Gateway addresses (
services/rfq/relayer/relconfig/config.go,services/rfq/relayer/relconfig/enum.go,services/rfq/relayer/relconfig/getters.go)
10 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings
| var err error | ||
| err = json.Unmarshal(rawContracts, &Contracts) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
🪶 style: Consider handling the error more gracefully instead of using panic.
| var err error | ||
| err = json.Unmarshal(rawContracts, &Contracts) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
🪶 style: Consider handling the error more gracefully instead of using panic.
|
|
||
| parsedL2GatewayRouter, err := abi.JSON(strings.NewReader(L2GatewayRouterMetaData.ABI)) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
🪶 style: Consider handling the error more gracefully instead of using panic.
| if c.boundL1Gateway == nil { | ||
| return fmt.Errorf("l1 gateway contract not set") | ||
| } | ||
| if c.boundL2Gateway == nil { | ||
| return fmt.Errorf("l2 gateway contract not set") | ||
| } |
There was a problem hiding this comment.
🧠 logic: Ensure both L1 and L2 gateway contracts are set.
| } | ||
|
|
||
| func (c *rebalanceManagerScroll) listenL2Gateway(ctx context.Context, ethClient client.EVM) (err error) { | ||
| addr, err := c.cfg.GetL2GatewayAddress(c.l1ChainID) |
There was a problem hiding this comment.
📚 spelling: Potential typo: should this be c.l2ChainID instead of c.l1ChainID?
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (4)
services/rfq/contracts/l1gateway/l1gateway.metadata.go (1)
18-25: Consider adding logging for better debugging.The init function correctly unmarshals the embedded JSON data. However, adding logging can help with debugging issues.
func init() { // load contract metadata var err error err = json.Unmarshal(rawContracts, &Contracts) if err != nil { panic(err) } // Log successful unmarshalling fmt.Println("Contract metadata successfully loaded.") }services/rfq/contracts/l2gateway/l2gateway.metadata.go (1)
18-25: Consider adding logging for better debugging.The init function correctly unmarshals the embedded JSON data. However, adding logging can help with debugging issues.
func init() { // load contract metadata var err error err = json.Unmarshal(rawContracts, &Contracts) if err != nil { panic(err) } // Log successful unmarshalling fmt.Println("Contract metadata successfully loaded.") }services/rfq/contracts/l1gateway/topics.go (1)
10-35: Consider adding logging for better debugging.The init function correctly initializes topic IDs for deposit and withdrawal events. However, adding logging can help with debugging issues.
func init() { var err error parsedL1GatewayRouter, err := abi.JSON(strings.NewReader(L1GatewayRouterMetaData.ABI)) if err != nil { panic(err) } DepositETHTopic = parsedL1GatewayRouter.Events["DepositETH"].ID DepositERC20Topic = parsedL1GatewayRouter.Events["DepositERC20"].ID FinalizeWithdrawETHTopic = parsedL1GatewayRouter.Events["FinalizeWithdrawETH"].ID FinalizeWithdrawERC20Topic = parsedL1GatewayRouter.Events["FinalizeWithdrawERC20"].ID if DepositETHTopic == (common.Hash{}) { panic("topic is nil") } if DepositERC20Topic == (common.Hash{}) { panic("topic is nil") } if FinalizeWithdrawETHTopic == (common.Hash{}) { panic("topic is nil") } if FinalizeWithdrawERC20Topic == (common.Hash{}) { panic("topic is nil") } // Log successful initialization fmt.Println("L1 Gateway topics successfully initialized.") }services/rfq/contracts/l2gateway/topics.go (1)
1-1: Add a package comment.The package declaration should be accompanied by a comment explaining the purpose of the package.
// Package l2gateway handles the initialization and management of event topics for the L2 gateway. package l2gatewayTools
GitHub Check: Lint (services/rfq)
[failure] 1-1:
package-comments: should have a package comment (revive)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (11)
- .gitmodules (1 hunks)
- services/cctp-relayer/external/scroll-contracts (1 hunks)
- services/rfq/contracts/l1gateway/l1gateway.metadata.go (1 hunks)
- services/rfq/contracts/l1gateway/topics.go (1 hunks)
- services/rfq/contracts/l2gateway/l2gateway.metadata.go (1 hunks)
- services/rfq/contracts/l2gateway/topics.go (1 hunks)
- services/rfq/relayer/inventory/manager.go (1 hunks)
- services/rfq/relayer/inventory/scroll.go (1 hunks)
- services/rfq/relayer/relconfig/config.go (1 hunks)
- services/rfq/relayer/relconfig/enum.go (3 hunks)
- services/rfq/relayer/relconfig/getters.go (1 hunks)
Files skipped from review due to trivial changes (2)
- .gitmodules
- services/cctp-relayer/external/scroll-contracts
Additional context used
GitHub Check: Lint (services/rfq)
services/rfq/contracts/l1gateway/topics.go
[failure] 1-1:
package-comments: should have a package comment (revive)services/rfq/contracts/l2gateway/topics.go
[failure] 1-1:
package-comments: should have a package comment (revive)services/rfq/relayer/inventory/scroll.go
[failure] 120-120:
if isEthereumChain(chainID)has complex nested blocks (complexity: 7) (nestif)
[failure] 113-113:
calculated cyclomatic complexity for function initContracts is 13, max is 10 (cyclop)
Additional comments not posted (18)
services/rfq/contracts/l1gateway/l1gateway.metadata.go (2)
1-8: Imports look good.The import statements are necessary and correctly included.
10-16: Variable declarations look good.The variable declarations for
rawContractsandContractsare correct and follow best practices.services/rfq/contracts/l2gateway/l2gateway.metadata.go (2)
1-8: Imports look good.The import statements are necessary and correctly included.
10-16: Variable declarations look good.The variable declarations for
rawContractsandContractsare correct and follow best practices.services/rfq/relayer/relconfig/enum.go (3)
15-16: Constant addition looks good.The new rebalance method
RebalanceMethodScrollis correctly added.
26-27: Function modification looks good.The
RebalanceMethodFromStringfunction correctly handles the new rebalance methodscroll.
43-44: Method modification looks good.The
Stringmethod correctly handles the new rebalance methodscroll.services/rfq/contracts/l1gateway/topics.go (2)
1-8: Imports look good.The import statements are necessary and correctly included.
Tools
GitHub Check: Lint (services/rfq)
[failure] 1-1:
package-comments: should have a package comment (revive)
37-47: Variable declarations look good.The variable declarations for topic IDs are correct and follow best practices.
services/rfq/relayer/relconfig/config.go (1)
72-75: LGTM!The added fields
L1GatewayAddressandL2GatewayAddressin theChainConfigstruct are consistent and correctly formatted.services/rfq/relayer/inventory/scroll.go (5)
1-48: LGTM!The package declaration, imports, and
rebalanceManagerScrollstruct definition are correct and well-structured.
50-59: LGTM!The
newRebalanceManagerScrollfunction correctly initializes therebalanceManagerScrollinstance.
61-76: LGTM!The constants and helper functions for identifying chain types are well-defined and improve code readability.
78-111: LGTM! Verify initialization process.The
Startmethod is well-structured. Ensure the initialization process is thoroughly tested.
163-470: LGTM!The remaining methods for event listeners and transaction submissions are well-structured and handle errors appropriately.
services/rfq/relayer/relconfig/getters.go (2)
134-146: LGTM!The
GetL1GatewayAddressmethod is well-implemented and follows the pattern of other getter methods.
148-160: LGTM!The
GetL2GatewayAddressmethod is well-implemented and follows the pattern of other getter methods.services/rfq/relayer/inventory/manager.go (1)
192-193: Verify the new rebalance method addition.The new rebalance method
relconfig.RebalanceMethodScrollhas been added correctly. Ensure thatnewRebalanceManagerScrollis implemented and tested thoroughly.
Deploying sanguine-fe with
|
| Latest commit: |
950ad1c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8515dbba.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-scroll-rebalance.sanguine-fe.pages.dev |
There was a problem hiding this comment.
PR Summary
(updates since last review)
Based on the provided information and the previous review, here's a summary of the recent changes in this pull request:
The PR continues to enhance the RFQ relayer system, focusing on Scroll rebalancing support and improving overall robustness. Key updates include:
- Implemented Scroll rebalancing in the RFQ relayer system, adding a new rebalance method specifically for Scroll.
- Enhanced the
ApproveAllTokensfunction to include approval logic for L1GatewayRouter and L2GatewayRouter contracts, supporting Scroll's cross-chain functionality. - Improved error handling and configuration management, particularly in retrieving addresses for various contracts (RFQ, SynapseCCTP, TokenMessenger, L1Gateway, L2Gateway).
- Updated the rebalance interval process with better context management and error handling, improving the system's reliability during rebalancing operations.
- Increased flexibility in token configurations by allowing multiple rebalance methods to be specified for each token.
These changes build upon the previous improvements to error handling and configuration management for Circle's CCTP integration, further enhancing the system's capabilities and robustness in handling cross-chain operations.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/relayer/inventory/manager.go (16 hunks)
Additional comments not posted (3)
services/rfq/relayer/inventory/manager.go (3)
Line range hint
178-191:
LGTM! But verify the new rebalance method.The changes to retrieve all rebalance methods and the addition of the new rebalance method
relconfig.RebalanceMethodScrolllook good.However, ensure that the new rebalance method
newRebalanceManagerScrollis correctly implemented and integrated.Verification successful
Implementation of
newRebalanceManagerScrollis correct.The function
newRebalanceManagerScrollhas been implemented correctly inservices/rfq/relayer/inventory/scroll.go. It initializes therebalanceManagerScrollstruct with the required parameters and additional configurations.
services/rfq/relayer/inventory/scroll.go: Implementation ofnewRebalanceManagerScroll.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of newRebalanceManagerScroll. # Test: Search for the implementation of newRebalanceManagerScroll. Expect: The function should be implemented correctly. rg --type go 'func newRebalanceManagerScroll'Length of output: 322
Script:
#!/bin/bash # Extract the implementation of the function `newRebalanceManagerScroll` from `services/rfq/relayer/inventory/scroll.go` rg --type go --context 20 'func newRebalanceManagerScroll' services/rfq/relayer/inventory/scroll.goLength of output: 1513
339-367: LGTM! But verify the new contract addresses.The changes to include approval logic for
L1GatewayRouterandL2GatewayRoutercontracts look good.However, ensure that the addresses for
L1GatewayRouterandL2GatewayRouterare correctly retrieved and used.Verification successful
Verified: The retrieval of L1GatewayRouter and L2GatewayRouter addresses is correctly implemented.
The functions
GetL1GatewayAddressandGetL2GatewayAddressare properly defined and used in the codebase to retrieve the respective addresses.
services/rfq/relayer/relconfig/getters.go: FunctionsGetL1GatewayAddressandGetL2GatewayAddressare implemented here.services/rfq/relayer/inventory/manager.go: These functions are used to retrieve the addresses.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the retrieval of L1GatewayRouter and L2GatewayRouter addresses. # Test: Search for the address retrieval functions. Expect: The functions should correctly retrieve the addresses. rg --type go 'func GetL1GatewayAddress' rg --type go 'func GetL2GatewayAddress'Length of output: 83
Script:
#!/bin/bash # Description: Verify the retrieval of L1GatewayRouter and L2GatewayRouter addresses. # Test: Search for the address retrieval methods using broader patterns. rg --type go 'L1GatewayAddress' rg --type go 'L2GatewayAddress'Length of output: 2528
Line range hint
468-513:
LGTM! But verify the usage of the list-based approach.The changes to use a list of rebalance methods and update the error message look good.
However, ensure that the list-based approach for rebalance methods is correctly used throughout the codebase.
Verification successful
To ensure the list-based approach for rebalance methods is correctly used throughout the codebase, we need to examine the context around each usage of
GetRebalanceMethods.
LGTM! The list-based approach for rebalance methods is correctly used throughout the codebase.
The changes to use a list of rebalance methods have been consistently applied and the logic correctly handles the list of methods.
services/rfq/relayer/relconfig/getters.goservices/rfq/relayer/inventory/manager.goservices/rfq/relayer/inventory/rebalance.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the list-based approach for rebalance methods. # Test: Search for the usage of GetRebalanceMethods. Expect: The function should be used consistently. rg --type go 'GetRebalanceMethods'Length of output: 788
Script:
#!/bin/bash # Description: Examine the context around each usage of GetRebalanceMethods to verify the correct handling of the list-based approach. # Extract context around GetRebalanceMethods in services/rfq/relayer/relconfig/getters.go rg --type go --context 10 'GetRebalanceMethods' services/rfq/relayer/relconfig/getters.go # Extract context around GetRebalanceMethods in services/rfq/relayer/inventory/manager.go rg --type go --context 10 'GetRebalanceMethods' services/rfq/relayer/inventory/manager.go # Extract context around GetRebalanceMethods in services/rfq/relayer/inventory/rebalance.go rg --type go --context 10 'GetRebalanceMethods' services/rfq/relayer/inventory/rebalance.goLength of output: 4090
There was a problem hiding this comment.
PR Summary
(updates since last review)
The PR introduces support for new rebalance methods, including the Scroll method, and updates the inventory manager to handle these new methods.
- Enhanced
services/rfq/relayer/inventory/circle.goto support new configurations and improve error handling. - Updated
services/rfq/relayer/inventory/manager.goto include Scroll rebalancing, dynamic management of multiple rebalance strategies, and approval logic forL1GatewayRouterandL2GatewayRoutercontracts. - Added
services/rfq/relayer/inventory/scroll.goto introduce Scroll rebalancing, including contract initialization, listener setup, and periodic claiming of L2 to L1 transactions.
3 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings
| return fmt.Errorf("l1 gateway contract not set") | ||
| } | ||
| if c.boundL1ScrollMessenger == nil { | ||
| return fmt.Errorf("l1 scroll messenger not set") | ||
| } |
There was a problem hiding this comment.
logic: Ensure both L1 and L2 gateway contracts are set to avoid runtime errors.
| // set API URL | ||
| baseURL := mainnetScrollAPIURL | ||
| if isTestnetChain(c.l1ChainID) { | ||
| baseURL = testnetScrollAPIURL |
There was a problem hiding this comment.
syntax: Potential typo: should this be c.l2ChainID instead of c.l1ChainID?
| func newRebalanceManagerScroll(cfg relconfig.Config, handler metrics.Handler, chainClient submitter.ClientFetcher, txSubmitter submitter.TransactionSubmitter, relayerAddress common.Address, db reldb.Service) *rebalanceManagerScroll { | ||
| claimCache := ttlcache.New[uint64, bool]( | ||
| ttlcache.WithTTL[uint64, bool](claimCacheTTL), | ||
| ttlcache.WithDisableTouchOnHit[uint64, bool](), | ||
| ) |
There was a problem hiding this comment.
logic: Consider defining claimCacheTTL as a constant outside the function to avoid re-creating it on each call.
| func (c *rebalanceManagerScroll) Start(ctx context.Context) (err error) { | ||
| err = c.initContracts(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("could not initialize contracts: %w", err) |
There was a problem hiding this comment.
style: Ensure initContracts handles all possible error scenarios to avoid partial initialization.
| return nil | ||
| } | ||
|
|
||
| func (c *rebalanceManagerScroll) initL1Contracts(parentCtx context.Context, chainID int) (err error) { |
There was a problem hiding this comment.
style: Consider adding retries for network-related errors when fetching chain clients to improve robustness.
| return nil | ||
| } | ||
|
|
||
| func (c *rebalanceManagerScroll) initL2Contracts(parentCtx context.Context, chainID int) (err error) { |
There was a problem hiding this comment.
style: Check if chainClient.GetClient handles network failures gracefully to avoid runtime errors.
| const scrollGasLimit = 200_000 | ||
|
|
||
| func (c *rebalanceManagerScroll) initiateL1ToL2(parentCtx context.Context, rebalance *RebalanceData) (err error) { | ||
| scrollMsgFee, err := c.cfg.GetScrollMessageFee(c.l1ChainID) |
There was a problem hiding this comment.
style: Ensure that the transaction submission logic handles all edge cases, such as network failures and gas estimation errors.
| defer resp.Body.Close() | ||
|
|
There was a problem hiding this comment.
style: Ensure the response body is closed even if an error occurs to avoid potential resource leaks.
| c.claimCache.Set(uint64(nonce.Int64()), true, 0) | ||
| return tx, nil | ||
| }) |
There was a problem hiding this comment.
style: Check if apiURL is correctly set before making HTTP requests to avoid nil pointer dereference.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- services/rfq/relayer/inventory/circle.go (6 hunks)
- services/rfq/relayer/inventory/manager.go (18 hunks)
- services/rfq/relayer/inventory/scroll.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- services/rfq/relayer/inventory/circle.go
- services/rfq/relayer/inventory/scroll.go
Additional comments not posted (6)
services/rfq/relayer/inventory/manager.go (6)
178-178: EnsureGetAllRebalanceMethodsis correctly implemented.The function now uses
cfg.GetAllRebalanceMethods(). Verify that this method is correctly implemented and returns the expected results.
190-191: New rebalance method added.The new rebalance method
relconfig.RebalanceMethodScrollhas been added. Ensure thatnewRebalanceManagerScrollis correctly implemented and integrates well with the existing rebalance managers.
339-353: Verify new approval logic forL1GatewayRouter.Ensure that the new approval logic for
L1GatewayRouteris correctly implemented and that the contract interactions and error handling are appropriate.
355-369: Verify new approval logic forL2GatewayRouter.Ensure that the new approval logic for
L2GatewayRouteris correctly implemented and that the contract interactions and error handling are appropriate.
468-472: Ensure correct handling of multiple rebalance methods.The function now retrieves multiple rebalance methods. Verify that this change correctly handles multiple methods and integrates well with the existing logic.
513-513: Update error message to reflect new rebalance method handling.The error message has been updated to reflect the new list-based approach for rebalance methods. Ensure that this change is consistent with the rest of the code.
There was a problem hiding this comment.
PR Summary
(updates since last review)
The PR introduces support for new rebalance methods, including the Scroll method, and updates the inventory manager to handle these new methods.
- Enhanced
services/rfq/relayer/inventory/rebalance.goto support Scroll rebalancing and improve error handling. - Updated
services/rfq/relayer/inventory/export_test.goto include tests for new rebalance methods. - Modified
services/rfq/relayer/inventory/synapse.goto handle Scroll rebalancing, including contract initialization and listener setup. - Added approval logic for
L1GatewayRouterandL2GatewayRoutercontracts inservices/rfq/relayer/inventory/manager.go.
7 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
The PR introduces support for new rebalance methods, including the Scroll method, and updates the inventory manager to handle these new methods.
- Enhanced
services/rfq/relayer/inventory/rebalance.goto support Scroll rebalancing and improve error handling. - Updated
services/rfq/relayer/inventory/export_test.goto include tests for new rebalance methods. - Modified
services/rfq/relayer/inventory/synapse.goto handle Scroll rebalancing, including contract initialization and listener setup. - Added approval logic for
L1GatewayRouterandL2GatewayRoutercontracts inservices/rfq/relayer/inventory/manager.go.
1 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings
| @@ -33,11 +33,11 @@ type RebalanceManager interface { | |||
| //nolint:cyclop,nilnil | |||
| func getRebalance(span trace.Span, cfg relconfig.Config, tokens map[int]map[common.Address]*TokenMetadata, chainID int, token common.Address) (rebalance *RebalanceData, err error) { | |||
| // get rebalance method | |||
| method, err := cfg.GetRebalanceMethod(chainID, token.Hex()) | |||
| methods, err := cfg.GetRebalanceMethods(chainID, token.Hex()) | |||
There was a problem hiding this comment.
logic: Consider caching rebalance methods to avoid repeated configuration lookups.
| @@ -51,7 +51,7 @@ func getRebalance(span trace.Span, cfg relconfig.Config, tokens map[int]map[comm | |||
| } | |||
|
|
|||
| // evaluate the origin and dest of the rebalance based on min/max token balances | |||
| originTokenData, destTokenData := getRebalanceMetadatas(cfg, tokens, rebalanceTokenData.Name, method) | |||
| originTokenData, destTokenData, method := getRebalanceMetadatas(cfg, tokens, rebalanceTokenData.Name, methods) | |||
| if originTokenData == nil { | |||
There was a problem hiding this comment.
style: Ensure originTokenData is always initialized to avoid potential nil pointer dereference.
| @@ -64,6 +64,12 @@ func getRebalance(span trace.Span, cfg relconfig.Config, tokens map[int]map[comm | |||
| } | |||
| return nil, nil | |||
| } | |||
| if method == relconfig.RebalanceMethodNone { | |||
There was a problem hiding this comment.
style: Handle cases where method is not recognized to avoid unexpected behavior.
| destTokenData = tokenData | ||
| // | ||
| //nolint:nestif,cyclop,gocognit | ||
| func getRebalanceMetadatas(cfg relconfig.Config, tokens map[int]map[common.Address]*TokenMetadata, tokenName string, methods []relconfig.RebalanceMethod) (originTokenData, destTokenData *TokenMetadata, method relconfig.RebalanceMethod) { |
There was a problem hiding this comment.
logic: Refactor nested loops to improve readability and maintainability.
| return originTokenData, destTokenData, method | ||
| } | ||
|
|
||
| func isTokenCompatible(tokenData *TokenMetadata, method relconfig.RebalanceMethod, cfg relconfig.Config) bool { |
There was a problem hiding this comment.
logic: Validate token compatibility more efficiently to reduce redundant checks.
| // | ||
| //nolint:nestif,cyclop,gocognit | ||
| func getRebalanceMetadatas(cfg relconfig.Config, tokens map[int]map[common.Address]*TokenMetadata, tokenName string, methods []relconfig.RebalanceMethod) (originTokenData, destTokenData *TokenMetadata, method relconfig.RebalanceMethod) { | ||
| candidates := map[relconfig.RebalanceMethod][2]TokenMetadata{} |
There was a problem hiding this comment.
logic: Consider initializing the candidates map with a predefined capacity to optimize memory allocation.
| // select candidates with largest delta between origin and dest balance | ||
| maxDelta := big.NewInt(0) | ||
| for m, c := range candidates { | ||
| delta := new(big.Int).Sub(c[0].Balance, c[1].Balance) |
There was a problem hiding this comment.
style: Ensure that the delta calculation handles potential integer overflow issues.
| } | ||
|
|
||
| // select candidates with largest delta between origin and dest balance | ||
| maxDelta := big.NewInt(0) |
There was a problem hiding this comment.
style: Consider adding a check to ensure that originTokenData and destTokenData are not nil before using them.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/relayer/inventory/rebalance.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/inventory/rebalance.go
There was a problem hiding this comment.
PR Summary
(updates since last review)
The PR introduces support for new rebalance methods, including the Scroll method, and updates the inventory manager to handle these new methods.
- Modified
services/rfq/relayer/quoter/quoter_test.goto addOriginFastBridgeAddressandDestFastBridgeAddressto expected quotes, aligning tests with the updated quote structure. - Updated
services/rfq/relayer/quoter/suite_test.goto include new RFQ addresses for origin, destination, and destinationEth chains, ensuring tests reflect the updated configuration.
No critical issues or potential pitfalls were identified in these changes.
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
The PR introduces support for new rebalance methods, including the Scroll method, and updates the inventory manager to handle these new methods.
- Updated
services/rfq/relayer/relconfig/config_test.goto add and modify test cases for the new Scroll rebalance method. - Removed the test for
GetRFQAddressinservices/rfq/relayer/relconfig/config_test.go, which might need further investigation to ensure it is intentional. - Enhanced error handling and messaging for rebalance management.
- Improved flexibility in token configurations by allowing multiple rebalance methods.
- Added a new utility package for reusable functions in the RFQ service.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
Summary by CodeRabbit
New Features
Scrollmethod for enhanced cross-chain interactions.L1GatewayRouterandL2GatewayRoutercontracts.Enhancements