feat(rfq-relayer): add multicall support for prove / claim [SLT-355] #3261
Conversation
WalkthroughThe changes involve renaming the package from Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
Deploying sanguine-fe with
|
| Latest commit: |
bdb2742
|
| Status: | 🚫 Build failed. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3261 +/- ##
============================================
Coverage ? 43.82556%
============================================
Files ? 74
Lines ? 2316
Branches ? 82
============================================
Hits ? 1015
Misses ? 1295
Partials ? 6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (10)
services/rfq/contracts/fastbridge/status.go (1)
Line range hint
29-42: Simplifyinitfunction and remove unnecessary commentsThe
initfunction and surrounding code can be improved for clarity and efficiency:
- The
initfunction can be simplified by using a slice literal.- There's an unnecessary commented line that can be removed.
Consider applying the following changes:
var allBridgeStatuses []BridgeStatus func init() { - for i := 0; i < len(_BridgeStatus_index)-1; i++ { - contractType := BridgeStatus(i) - allBridgeStatuses = append(allBridgeStatuses, contractType) - // assert type is correct - } + allBridgeStatuses = []BridgeStatus{NULL, REQUESTED, RelayerProved, RelayerClaimed, REFUNDED} } -// allBridgeStatuses is a list of all bridge statuses Since we use stringer and this is a testing library, instead -// // of manually copying all these out we pull the names out of stringer. In order to make sure stringer is updated, we panic on -// // any method called where the index is higher than the stringer array length. +// allBridgeStatuses is a list of all bridge statuses.This change simplifies the initialization logic and removes the unnecessary comment, making the code more concise and easier to maintain.
services/rfq/contracts/fastbridge/status_test.go (2)
4-6: Import statements updated correctly.The import for the
fastbridgemultipackage has been correctly updated. The re-addition ofmath/bigandtestingimports is also good.Consider grouping the standard library imports (
math/bigandtesting) together at the top of the import block for better organization:import ( "math/big" "testing" "github.com/ethereum/go-ethereum/accounts/abi/bind" // ... other imports ... )Also applies to: 12-12
Incomplete Package Renaming Detected
Several files still reference the old
fastbridgepackage. Please update all remaining imports and usages tofastbridgemultito ensure consistency and prevent potential issues.
services/rfq/testutil/contracttype.goservices/rfq/tools/rfqdecoder/main.goservices/rfq/relayer/reldb/db.go...(and other listed files)🔗 Analysis chain
Line range hint
44-48: Function call updated correctly.The
GetAllBridgeStatuses()function call has been properly updated to use thefastbridgemultipackage, which is consistent with the package renaming.To ensure that all references to the old package name have been updated, please run the following command:
This will help catch any instances where the package name might have been missed during the update.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old 'fastbridge' package rg --type go 'fastbridge[^m]'Length of output: 18209
services/rfq/testutil/typecast.go (1)
19-22: LGTM! Consider adding error handling.The
GetFastBridgefunction has been correctly updated to use the newfastbridgemultipackage. The changes are consistent with the import statement update and maintain the existing logic.Consider adding error handling to the function. Currently, any error from
manager.GetContractis silently ignored. You could update the function signature to return an error:func (d *DeployManager) GetFastBridge(ctx context.Context, backend backends.SimulatedTestBackend) (contract contracts.DeployedContract, handle *fastbridgemulti.FastBridgeRef, err error) { d.T().Helper() return manager.GetContract[*fastbridgemulti.FastBridgeRef](ctx, d.T(), d, backend, FastBridgeType) }This would allow callers to handle any potential errors that occur during contract retrieval.
services/rfq/relayer/relconfig/getters.go (1)
111-119: LGTM! Consider a minor optimization.The implementation of
AnyMulticallEnabledis correct and efficient. It properly iterates through all chain configurations and returns true if any chain has multicall enabled.For a slight performance improvement, consider using a separate boolean variable to store the result:
func (c Config) AnyMulticallEnabled() bool { + result := false for _, chainCfg := range c.Chains { if chainCfg.MulticallEnabled { - return true + result = true + break } } - return false + return result }This change allows for easier addition of any future logic after the loop if needed.
services/rfq/relayer/service/handlers.go (1)
627-630: Simplify code by removing unnecessary slice conversionIn
submitClaim, the use ofrequest.TransactionID[:]is unnecessary sincerequest.TransactionIDis already a byte slice. Removing the[:]makes the code cleaner and more idiomatic.Apply this change:
- callData, err := bridgeABI.Pack("claim", request.TransactionID[:], q.RelayerAddress) + callData, err := bridgeABI.Pack("claim", request.TransactionID, q.RelayerAddress)services/rfq/e2e/rfq_test.go (2)
128-129: Consider removing debug print statementThe
fmt.Printfstatement seems to be used for debugging purposes. If it's not necessary for the test output, consider removing it to clean up the code.
619-622: Address the TODO: Refactor Anvil client setupThe
TODOcomment suggests improving the Anvil client setup. Consider refactoring this code to enhance readability and prevent code duplication if similar setups are used elsewhere.Would you like assistance in refactoring this section of the code?
services/rfq/relayer/service/multicall.go (2)
20-23: Add Documentation for theMulticallDispatcherInterface MethodsAdding comments to the
MulticallDispatcherinterface methods improves code readability and provides clarity on their purpose and usage.Enhance the interface with method comments:
type MulticallDispatcher interface { + // Start initializes and runs the dispatcher. Start(ctx context.Context) error + // Dispatch sends the provided call data to the designated chain's queue. Dispatch(ctx context.Context, chainID int, callData []byte) error }
51-70: Handle Errors in Goroutines AppropriatelyIn the
Startmethod, if any of the goroutines return an error, it causes the entire function to return. This might lead to the dispatcher stopping if one chain encounters an issue. Consider logging the error and continuing to process other chains.Modify the goroutine error handling to prevent a single chain's failure from stopping the entire dispatcher:
func (m *multicallDispatcher) Start(ctx context.Context) error { g, gctx := errgroup.WithContext(ctx) for c := range m.cfg.Chains { chainID := c g.Go(func() error { err := m.runQueue(gctx, chainID) if err != nil { - return fmt.Errorf("could not run dispatch: %w", err) + // Log the error and continue + m.metricHandler.Logger().Warnf("could not run dispatch for chain %d: %v", chainID, err) } return nil }) } err := g.Wait() if err != nil { - return fmt.Errorf("could not start multicall dispatcher: %w", err) + // Log the error and proceed + m.metricHandler.Logger().Errorf("error in multicall dispatcher: %v", err) } return nil }Additionally, ensure that errors are properly logged and monitored for operational awareness.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (20)
- services/rfq/contracts/fastbridge/bridgestatus_string.go (1 hunks)
- services/rfq/contracts/fastbridge/events.go (1 hunks)
- services/rfq/contracts/fastbridge/eventtype_string.go (1 hunks)
- services/rfq/contracts/fastbridge/export_test.go (1 hunks)
- services/rfq/contracts/fastbridge/fastbridgemulti.metadata.go (2 hunks)
- services/rfq/contracts/fastbridge/generate.go (1 hunks)
- services/rfq/contracts/fastbridge/helper.go (1 hunks)
- services/rfq/contracts/fastbridge/parser.go (1 hunks)
- services/rfq/contracts/fastbridge/status.go (1 hunks)
- services/rfq/contracts/fastbridge/status_test.go (2 hunks)
- services/rfq/e2e/rfq_test.go (10 hunks)
- services/rfq/relayer/chain/chain.go (1 hunks)
- services/rfq/relayer/relconfig/config.go (1 hunks)
- services/rfq/relayer/relconfig/getters.go (1 hunks)
- services/rfq/relayer/service/handlers.go (2 hunks)
- services/rfq/relayer/service/multicall.go (1 hunks)
- services/rfq/relayer/service/relayer.go (3 hunks)
- services/rfq/relayer/service/statushandler.go (2 hunks)
- services/rfq/testutil/deployers.go (2 hunks)
- services/rfq/testutil/typecast.go (2 hunks)
✅ Files skipped from review due to trivial changes (4)
- services/rfq/contracts/fastbridge/events.go
- services/rfq/contracts/fastbridge/export_test.go
- services/rfq/contracts/fastbridge/generate.go
- services/rfq/contracts/fastbridge/helper.go
🧰 Additional context used
🪛 GitHub Check: Build (1.22.x)
services/rfq/relayer/service/multicall.go
[failure] 14-14:
no required module provides package github.com/synapsecns/sanguine/services/rfq/contracts/fastbridgemulti; to add it:
🔇 Additional comments (26)
services/rfq/contracts/fastbridge/fastbridgemulti.metadata.go (2)
2-2: Package name updated correctly.The package name has been changed from
fastbridgetofastbridgemulti, which is consistent with the file renaming.To ensure all references to this package have been updated throughout the codebase, run the following script:
#!/bin/bash # Description: Check for any remaining references to the old package name # Test: Search for import statements or usage of the old package name rg --type go 'fastbridge' --glob '!**/fastbridge/**'If the script returns any results, those files may need to be updated to use the new package name.
12-12: Embedded file reference updated correctly.The embedded file reference has been updated from
fastbridge.contractinfo.jsontofastbridgemulti.contractinfo.json, which is consistent with the package renaming.To ensure the new JSON file exists and contains the correct data, run the following script:
This script will verify the existence of the new JSON file, compare its content with the old file (if it still exists), and check if it contains valid JSON data.
✅ Verification successful
Embedded File Reference Update Verified
- The new JSON file
services/rfq/contracts/fastbridge/fastbridgemulti.contractinfo.jsonexists.- The file contains valid JSON.
- The old JSON file does not exist, consistent with the package renaming.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and content of the new JSON file # Test 1: Check if the file exists if [ -f "services/rfq/contracts/fastbridge/fastbridgemulti.contractinfo.json" ]; then echo "File exists" else echo "File does not exist" exit 1 fi # Test 2: Compare the content with the old file (if it still exists) if [ -f "services/rfq/contracts/fastbridge/fastbridge.contractinfo.json" ]; then diff "services/rfq/contracts/fastbridge/fastbridge.contractinfo.json" "services/rfq/contracts/fastbridge/fastbridgemulti.contractinfo.json" else echo "Old file does not exist for comparison" fi # Test 3: Check if the file is valid JSON jq . "services/rfq/contracts/fastbridge/fastbridgemulti.contractinfo.json" > /dev/null if [ $? -eq 0 ]; then echo "File contains valid JSON" else echo "File does not contain valid JSON" exit 1 fiLength of output: 428
services/rfq/contracts/fastbridge/bridgestatus_string.go (1)
3-3: Package rename looks good.The package name has been successfully updated from
fastbridgetofastbridgemulti, which is consistent with the changes described in the PR summary.Let's verify the impact of this package rename on other files in the codebase:
services/rfq/contracts/fastbridge/eventtype_string.go (1)
3-3: Package rename looks good.The package name has been successfully changed from
fastbridgetofastbridgemulti, which is consistent with the project's renaming strategy.services/rfq/contracts/fastbridge/status.go (1)
1-1: Approve package rename and verify consistencyThe package name change from
fastbridgetofastbridgemultihas been implemented correctly. This change suggests an expansion of functionality, possibly related to the addition of multicall support mentioned in the PR title.To ensure consistency across the module, please run the following script:
services/rfq/contracts/fastbridge/status_test.go (1)
1-1: Package name change looks good.The package name has been updated from
fastbridge_testtofastbridgemulti_test, which is consistent with the shift to a multi-bridge context mentioned in the PR objectives.services/rfq/contracts/fastbridge/parser.go (1)
Line range hint
3-105: The rest of the file remains unchanged.The existing structure, interfaces, and functionality of the file have been maintained. This includes:
- The
Parserinterface and its implementation- Event type definitions and constants
- The
NewParserfunction- The
ParseEventmethodThe consistency in the rest of the file is good, as it minimizes the impact of the package rename.
services/rfq/testutil/typecast.go (1)
9-9: LGTM! Verify usage of the new package.The import statement has been correctly updated to use the new
fastbridgemultipackage, which aligns with the PR objective of adding multicall support.Let's verify that the new package is used consistently throughout the file:
✅ Verification successful
Verified: No remaining references to 'fastbridge' found and
fastbridgemultiis used consistently.The import statement and all related usages have been correctly updated to the new
fastbridgemultipackage, aligning with the PR's objective of adding multicall support.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old 'fastbridge' package # Test: Search for any remaining 'fastbridge' references rg --type go 'fastbridge[^m]' services/rfq/testutil/typecast.go # Test: Confirm usage of 'fastbridgemulti' rg --type go 'fastbridgemulti' services/rfq/testutil/typecast.goLength of output: 480
services/rfq/relayer/chain/chain.go (1)
95-96: Approve: Logging enhancement improves observabilityThe addition of transaction hash logging is a valuable improvement to the
SubmitRelayfunction. It enhances the observability of the relay process without altering the core logic or performance of the function. The log statement is correctly placed after the successful creation of the transaction but before returning, ensuring that only successfully created transactions are logged.services/rfq/testutil/deployers.go (1)
116-118: LGTM! Verify interface compatibility.The
Deploymethod has been correctly updated to use the newfastbridgemultipackage. The changes are consistent with the import statement update and the PR objective.To ensure the new contract's interface is compatible with the old one, run the following command to compare the function signatures:
If there are any differences in the function signatures, please ensure that they are intentional and don't break existing functionality.
✅ Verification successful
Verified! The
Deploymethod properly integrates the newfastbridgemultipackage, and all related references have been correctly updated across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Compare function signatures of old and new FastBridge contracts echo "Old FastBridge functions:" ast-grep --lang go --pattern 'func ($v *FastBridge) $f($p) $r' services/rfq/contracts/fastbridge echo "New FastBridgeMulti functions:" ast-grep --lang go --pattern 'func ($v *FastBridge) $f($p) $r' services/rfq/contracts/fastbridgemultiLength of output: 420
Script:
#!/bin/bash # Search for directories named 'fastbridgemulti' within 'services/rfq/contracts' fd 'fastbridgemulti' services/rfq/contracts/ --type dLength of output: 53
Script:
#!/bin/bash # Search for any files or directories containing 'fastbridgemulti' within 'services/rfq/contracts/' fd 'fastbridgemulti' services/rfq/contracts/ --type f,dLength of output: 226
Script:
#!/bin/bash # Corrected search for files and directories named 'fastbridgemulti' within 'services/rfq/contracts/' fd 'fastbridgemulti' services/rfq/contracts/ --type file --type directory # Additionally, search for any references to 'fastbridgemulti' in the codebase rg 'fastbridgemulti' services/rfq/Length of output: 4259
services/rfq/relayer/service/statushandler.go (1)
112-115: LGTM: Proper initialization of the new multicallDispatcher field.The
multicallDispatcherfield is correctly initialized in therequestToHandlerfunction, consistent with the existing pattern for other fields.To ensure the
Relayerstruct has been updated with themulticallDispatcherfield, please run the following script:#!/bin/bash # Description: Verify the Relayer struct update # Test: Check for the addition of multicallDispatcher field in the Relayer struct rg --type go "type Relayer struct {" -A 30 ./services/rfq | rg "multicallDispatcher"services/rfq/relayer/relconfig/config.go (1)
115-116: LGTM! Consider verifying usage in dependent code.The addition of the
MulticallEnabledfield to theChainConfigstruct is well-implemented and documented. It follows the existing naming conventions and provides a clear way to enable multicall support on a per-chain basis.To ensure this change is properly integrated, please run the following script to check for any code that might need updating to handle this new field:
This script will help identify areas of the codebase that might need to be updated to handle the new
MulticallEnabledfield.✅ Verification successful
Verified the addition of
MulticallEnabled. No issues found with its integration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential places where ChainConfig is used and might need updating. # Search for ChainConfig usage echo "Searching for ChainConfig usage:" rg --type go "ChainConfig" ./services/rfq # Search for yaml unmarshaling of chain configs echo "Searching for yaml unmarshaling of chain configs:" rg --type go "yaml.Unmarshal.*ChainConfig" ./services/rfq # Search for direct access to chain config fields echo "Searching for direct access to chain config fields:" rg --type go "chainConfig\.[A-Za-z]+" ./services/rfqLength of output: 13217
services/rfq/relayer/service/relayer.go (4)
67-69: LGTM: Added 'multicallDispatcher' field to 'Relayer' structThe new field
multicallDispatcherhas been added to theRelayerstruct appropriately to support multicall functionality.
154-158: Conditional initialization of 'multicallDispatcher'The code correctly initializes
multicallDispatcheronly when multicall is enabled in the configuration viacfg.AnyMulticallEnabled().
161-178: Proper inclusion of 'multicallDispatcher' in 'Relayer' initializationIn the
NewRelayerfunction, themulticallDispatcherfield is correctly assigned to theRelayerstruct, ensuring it is available for use elsewhere in the service.
239-248: Starting 'multicallDispatcher' in the 'Start' methodThe
Startmethod now includes logic to start themulticallDispatcherin a separate goroutine when multicall is enabled. This integrates multicall functionality into the relayer's operational flow as intended.services/rfq/relayer/service/handlers.go (1)
439-449: Code changes look goodThe addition of the
submitProvecall and status update in thehandleRelayCompletedmethod is appropriate and aligns with the intended bridge process enhancements.services/rfq/e2e/rfq_test.go (9)
24-24: Import updated to 'fastbridgemulti'The import statement has been correctly updated to use the new package
fastbridgemulti.
171-171: UpdatedBridgemethod to usefastbridgemulti.IFastBridgeBridgeParamsThe call to
originFastBridge.Bridgehas been updated to usefastbridgemulti.IFastBridgeBridgeParams. Ensure that all parameters are correctly set according to the new interface.
325-325: UpdatedBridgemethod to usefastbridgemulti.IFastBridgeBridgeParams
435-435: UpdatedBridgemethod to usefastbridgemulti.IFastBridgeBridgeParams
451-451: Updated parser to usefastbridgemulti.NewParserThe parser initialization now correctly uses
fastbridgemulti.NewParserwithoriginFastBridge.Address().
461-461: Updated event parsing to usefastbridgemulti.FastBridgeBridgeRequestedThe event casting has been updated to reflect the new
fastbridgemultipackage.
557-559: Initialize parser withfastbridgemulti.NewParserThe parser is correctly initialized using the new
fastbridgemultipackage, and error handling is appropriately performed withi.NoError(err).
Line range hint
731-773: Address potential concurrency issues insendBridgeReqfunction
Line range hint
755-795: Fix variable capture issue in goroutines
| @@ -1,6 +1,6 @@ | |||
| // Code generated by "stringer -type=EventType"; DO NOT EDIT. | |||
|
|
|||
| package fastbridge | |||
| package fastbridgemulti | |||
There was a problem hiding this comment.
💡 Codebase verification
Issues Found: Residual References to fastbridge Detected
Several files still reference the old package name fastbridge. Please update all remaining import statements and usages to fastbridgemulti to ensure consistency and prevent potential issues.
- Example affected files:
services/explorer/types/utils.goservices/rfq/e2e/rfq_test.goservices/rfq/testutil/contracttype.go- (and others as listed in the shell script output)
🔗 Analysis chain
Verify impact of package rename.
As the package has been renamed, we need to ensure that all import statements referencing this package have been updated accordingly throughout the project.
Let's run a script to check for any remaining references to the old package name:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining references to the old package name 'fastbridge'
# Test: Search for 'fastbridge' in all Go files, excluding the current directory
# Expect: No results, indicating all references have been updated
rg --type go -g '!./services/rfq/contracts/fastbridge*' 'fastbridge'
Length of output: 25891
| @@ -1,7 +1,8 @@ | |||
| package fastbridge | |||
| package fastbridgemulti | |||
There was a problem hiding this comment.
💡 Codebase verification
Package renaming incomplete
The package name has been changed to fastbridgemulti, but there are still imports referencing fastbridge, and the directory services/rfq/contracts/fastbridge remains unchanged. Please update these imports and rename the directory to ensure consistency across the codebase.
- Update all remaining imports from
fastbridgetofastbridgemulti. - Rename the directory
services/rfq/contracts/fastbridgetoservices/rfq/contracts/fastbridgemulti.
🔗 Analysis chain
Package name has been updated.
The package name has been changed from fastbridge to fastbridgemulti. This change might require updates in other parts of the codebase that import this package.
Let's verify if all imports have been updated correctly:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining imports of the old package name
# Test 1: Search for imports of the old package name
echo "Searching for imports of the old package name 'fastbridge':"
rg --type go 'import.*".*fastbridge"'
# Test 2: Search for qualified imports using the old package name
echo "Searching for qualified imports using the old package name:"
rg --type go 'fastbridge\.'
# Test 3: Check if the directory name has been updated
echo "Checking if the directory name has been updated:"
if [ -d "services/rfq/contracts/fastbridge" ]; then
echo "Warning: Directory 'services/rfq/contracts/fastbridge' still exists"
else
echo "Directory name has been updated successfully"
fi
Length of output: 12044
| fmt.Printf("relayed with tx hash: %s\n", tx.Hash()) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the logging approach
While adding transaction hash logging is beneficial for tracking and debugging, there are a few improvements we could make:
-
Instead of using
fmt.Printf, consider using a proper logging framework (e.g.,log,logrus, orzap) for better control over log levels and output destinations in production environments. -
Include more context in the log message, such as the chain ID or relevant request details. This will make it easier to correlate logs with specific operations.
Here's a suggested improvement:
- fmt.Printf("relayed with tx hash: %s\n", tx.Hash())
+ log.WithFields(log.Fields{
+ "chainID": c.ChainID,
+ "txHash": tx.Hash().Hex(),
+ }).Info("Transaction relayed successfully")Make sure to import the chosen logging package at the top of the file.
Committable suggestion was skipped due to low confidence.
| @@ -14,7 +14,7 @@ import ( | |||
| "github.com/synapsecns/sanguine/ethergo/contracts" | |||
| "github.com/synapsecns/sanguine/ethergo/deployer" | |||
| "github.com/synapsecns/sanguine/ethergo/manager" | |||
| "github.com/synapsecns/sanguine/services/rfq/contracts/fastbridge" | |||
| "github.com/synapsecns/sanguine/services/rfq/contracts/fastbridgemulti" | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue: fastbridgemulti package does not exist
The import statement references fastbridgemulti, but the package was not found in services/rfq/contracts. Please ensure that the fastbridgemulti package is correctly added.
🔗 Analysis chain
LGTM! Verify the new package.
The import statement has been correctly updated to use the new fastbridgemulti package, which aligns with the PR objective of adding multicall support.
To ensure the new package exists and is correctly placed, run the following command:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of the fastbridgemulti package
fd -t d fastbridgemulti services/rfq/contracts
Length of output: 420
| // multicallDispatcher is the multicall dispatcher. | ||
| multicallDispatcher MulticallDispatcher |
There was a problem hiding this comment.
💡 Codebase verification
Missing Import for MulticallDispatcher
The multicallDispatcher field is of type MulticallDispatcher, but there is no import statement for MulticallDispatcher in statushandler.go. Please add the necessary import to ensure the type is recognized.
🔗 Analysis chain
LGTM: New field added for multicall support.
The addition of the multicallDispatcher field aligns with the PR objective of adding multicall support. This change enhances the QuoteRequestHandler struct with new functionality.
To ensure the MulticallDispatcher type is properly defined and imported, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the MulticallDispatcher type definition and import
# Test 1: Check for the import of MulticallDispatcher
rg --type go "import.*MulticallDispatcher" ./services/rfq
# Test 2: Check for the definition of MulticallDispatcher type
rg --type go "type MulticallDispatcher" ./services/rfq
Length of output: 437
Script:
#!/bin/bash
# Description: Corrected script to verify the MulticallDispatcher type definition and import
# Test 1: Correctly check for the import of MulticallDispatcher
rg --type go 'import\s+\(".*MulticallDispatcher.*"\)' ./services/rfq
# Test 2: Check for the definition of MulticallDispatcher type
rg --type go "type MulticallDispatcher" ./services/rfq
Length of output: 208
| "github.com/synapsecns/sanguine/core/metrics" | ||
| "github.com/synapsecns/sanguine/ethergo/submitter" | ||
| omniClient "github.com/synapsecns/sanguine/services/omnirpc/client" | ||
| "github.com/synapsecns/sanguine/services/rfq/contracts/fastbridgemulti" |
There was a problem hiding this comment.
Missing Module Dependency for fastbridgemulti
The package github.com/synapsecns/sanguine/services/rfq/contracts/fastbridgemulti is imported but not found in the module dependencies. This will cause a build failure.
To resolve this, ensure that the fastbridgemulti package is properly added to your go.mod file. Run the following command to add the missing module:
# Add the missing module dependency
go get github.com/synapsecns/sanguine/services/rfq/contracts/fastbridgemulti🧰 Tools
🪛 GitHub Check: Build (1.22.x)
[failure] 14-14:
no required module provides package github.com/synapsecns/sanguine/services/rfq/contracts/fastbridgemulti; to add it:
| } | ||
| contract, err := fastbridgemulti.NewFastBridgeRef(rfqAddr, chainClient) | ||
| if err != nil { | ||
| return fmt.Errorf("could not create fast bridge contract at address %s: %w", contract.Address(), err) |
There was a problem hiding this comment.
Avoid Using Uninitialized contract in Error Message
If an error occurs while creating the FastBridgeRef contract, the contract variable may be nil, leading to a panic when calling contract.Address() in the error message.
Modify the error message to use the rfqAddr instead:
contract, err := fastbridgemulti.NewFastBridgeRef(rfqAddr, chainClient)
if err != nil {
- return fmt.Errorf("could not create fast bridge contract at address %s: %w", contract.Address(), err)
+ return fmt.Errorf("could not create fast bridge contract at address %s: %w", rfqAddr.Hex(), err)
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return fmt.Errorf("could not create fast bridge contract at address %s: %w", contract.Address(), err) | |
| return fmt.Errorf("could not create fast bridge contract at address %s: %w", rfqAddr.Hex(), err) |
| if len(callQueue) >= maxBatchSize { | ||
| if err := m.multicall(ctx, contract, chainID, callQueue); err != nil { | ||
| return fmt.Errorf("could not multicall: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Call Queue Not Cleared After Multicall Execution
When the callQueue reaches the maxBatchSize, the queue is sent via multicall, but the callQueue is not cleared afterward. This could result in duplicate calls being sent in subsequent batches.
Clear the callQueue after a successful multicall:
if len(callQueue) >= maxBatchSize {
if err := m.multicall(ctx, contract, chainID, callQueue); err != nil {
return fmt.Errorf("could not multicall: %w", err)
}
+ // Empty the call queue
+ callQueue = [][]byte{}
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(callQueue) >= maxBatchSize { | |
| if err := m.multicall(ctx, contract, chainID, callQueue); err != nil { | |
| return fmt.Errorf("could not multicall: %w", err) | |
| } | |
| } | |
| if len(callQueue) >= maxBatchSize { | |
| if err := m.multicall(ctx, contract, chainID, callQueue); err != nil { | |
| return fmt.Errorf("could not multicall: %w", err) | |
| } | |
| // Empty the call queue | |
| callQueue = [][]byte{} | |
| } |
| _, err := m.submitter.SubmitTransaction(ctx, big.NewInt(int64(chainID)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||
| tx, err = contract.MulticallNoResults(transactor, callQueue, true) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not multicall: %w", err) | ||
| } | ||
| return tx, nil | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("could not submit transaction: %w", err) | ||
| } | ||
|
|
||
| // empty the call queue | ||
| callQueue = [][]byte{} | ||
| return nil | ||
| } |
There was a problem hiding this comment.
callQueue Clearance Inside multicall Function Has No Effect
Reassigning callQueue within the multicall function does not clear the slice in the runQueue function due to Go's pass-by-value for slices. This means the original callQueue remains unchanged.
Remove the reinitialization of callQueue inside the multicall function:
func (m *multicallDispatcher) multicall(ctx context.Context, contract *fastbridgemulti.FastBridgeRef, chainID int, callQueue [][]byte) error {
// Existing code...
if err != nil {
return fmt.Errorf("could not submit transaction: %w", err)
}
- // Empty the call queue (this has no effect here)
- callQueue = [][]byte{}
return nil
}Ensure that callQueue is cleared in the runQueue function after successful execution:
case <-ticker.C:
if len(callQueue) > 0 {
if err := m.multicall(ctx, contract, chainID, callQueue); err != nil {
return fmt.Errorf("could not multicall: %w", err)
}
+ // Empty the call queue
+ callQueue = [][]byte{}
}Committable suggestion was skipped due to low confidence.
| // TODO: configure | ||
| var dispatchInterval = 30 * time.Second | ||
| var maxBatchSize = 100 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make Dispatch Interval and Batch Size Configurable
The dispatchInterval and maxBatchSize are currently hardcoded constants. Making these values configurable would enhance the flexibility and adaptability of the dispatcher in different environments.
Consider adding these configurations to relconfig.Config and updating the code accordingly:
// In your configuration struct
type Config struct {
// Existing fields...
+ DispatchInterval time.Duration
+ MaxBatchSize int
}
// In the code
-var dispatchInterval = 30 * time.Second
-var maxBatchSize = 100
+dispatchInterval := m.cfg.DispatchInterval
+maxBatchSize := m.cfg.MaxBatchSizeCommittable suggestion was skipped due to low confidence.
trajan0x
changed the title
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Summary by CodeRabbit
New Features
fastbridgemulti, enhancing clarity and functionality.Bug Fixes
Documentation