feat(rfq-guard): v2 guard logic [SLT-387] #3324
Conversation
dwasse
requested review from
trajan0x,
aureliusbtc and
ChiTimesChi
as code owners
WalkthroughThe pull request introduces significant changes to the RFQ (Request for Quote) functionality across several components. Key modifications include the restructuring of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/relayer-arb-call #3324 +/- ##
===============================================================
- Coverage 26.29482% 24.85424% -1.44059%
===============================================================
Files 212 207 -5
Lines 13554 13378 -176
Branches 160 82 -78
===============================================================
- Hits 3564 3325 -239
- Misses 9701 9765 +64
+ Partials 289 288 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Deploying sanguine-fe with
|
| Latest commit: |
e1b452c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5c4132f7.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-guard-v2.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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (32)
services/rfq/contracts/testcontracts/recipientmock/recipientmock.metadata.go (1)
18-25: 🛠️ Refactor suggestion
Enhance error handling and add data validation.
While panicking in init() is acceptable for critical startup requirements, consider:
- Adding more context to the error message
- Validating the unmarshalled data before proceeding
Here's a suggested improvement:
func init() { - // load contract metadata - var err error - err = json.Unmarshal(rawContracts, &Contracts) - if err != nil { - panic(err) - } + // Load and validate contract metadata + if err := json.Unmarshal(rawContracts, &Contracts); err != nil { + panic(fmt.Errorf("failed to unmarshal recipientmock contract metadata: %w", err)) + } + + // Validate contract data + if len(Contracts) == 0 { + panic("no contracts found in recipientmock metadata") + } + for name, contract := range Contracts { + if contract == nil { + panic(fmt.Sprintf("nil contract found in metadata: %s", name)) + } + } }📝 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.func init() { // Load and validate contract metadata if err := json.Unmarshal(rawContracts, &Contracts); err != nil { panic(fmt.Errorf("failed to unmarshal recipientmock contract metadata: %w", err)) } // Validate contract data if len(Contracts) == 0 { panic("no contracts found in recipientmock metadata") } for name, contract := range Contracts { if contract == nil { panic(fmt.Sprintf("nil contract found in metadata: %s", name)) } } }services/rfq/contracts/testcontracts/fastbridgemockv2/fastbridgemockv2.metadata.go (1)
18-25: 🛠️ Refactor suggestion
Consider improving error handling and adding data validation.
While panicking in init() is acceptable, consider these improvements:
- More descriptive error message
- Basic validation of unmarshalled data
Consider this enhanced implementation:
func init() { - // load contract metadata - var err error - err = json.Unmarshal(rawContracts, &Contracts) - if err != nil { - panic(err) - } + // Initialize contract metadata from embedded JSON + if err := json.Unmarshal(rawContracts, &Contracts); err != nil { + panic(fmt.Errorf("failed to unmarshal contract metadata: %w", err)) + } + + // Validate contract metadata + if len(Contracts) == 0 { + panic("no contracts found in metadata") + } + + for name, contract := range Contracts { + if contract == nil || len(contract.Info.AbiDefinition) == 0 { + panic(fmt.Errorf("invalid contract metadata for %s: missing ABI", name)) + } + }📝 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.func init() { // Initialize contract metadata from embedded JSON if err := json.Unmarshal(rawContracts, &Contracts); err != nil { panic(fmt.Errorf("failed to unmarshal contract metadata: %w", err)) } // Validate contract metadata if len(Contracts) == 0 { panic("no contracts found in metadata") } for name, contract := range Contracts { if contract == nil || len(contract.Info.AbiDefinition) == 0 { panic(fmt.Errorf("invalid contract metadata for %s: missing ABI", name)) } } }packages/contracts-rfq/package.json (1)
27-27: 💡 Codebase verification
Warning: Missing flatten.sh script in the expected location
The
build:goscript inpackage.jsonreferencesflatten.sh, but this script is missing in the root of thepackages/contracts-rfqdirectory. The repository contains other flatten scripts (e.g., inmake/scripts/flatten.shandpackages/contracts-core/script/sh/flatten.sh), but none at the expected location.
- The script should be copied or symlinked to
packages/contracts-rfq/flatten.sh- The test/mocks directory exists and contains Solidity files that need to be included in the flattening process
🔗 Analysis chain
LGTM! Verify flatten.sh script.
The addition of mock contracts to the build process is appropriate for testing the new FastBridge v2 functionality.
Let's verify the flatten.sh script exists and has appropriate permissions:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if flatten.sh exists and is executable if [ -x "./flatten.sh" ]; then echo "flatten.sh exists and is executable" else echo "Warning: flatten.sh might be missing or not executable" fi # Verify the mock contracts directory exists fd -t d "mocks" test/Length of output: 259
Script:
#!/bin/bash # Let's first check if the file exists, regardless of permissions ls -la flatten.sh 2>/dev/null || echo "flatten.sh not found" # Check the directory structure to understand where we are ls -la # Look for flatten.sh in the entire repository fd "flatten.sh" # Check if test directory exists and its structure ls -la test/ 2>/dev/null || echo "test directory not found" # Search for .sol files in the repository to verify the paths fd -e solLength of output: 19892
Script:
#!/bin/bash # Let's check the content of the flatten.sh in contracts-rfq package cat packages/contracts-rfq/flatten.sh 2>/dev/null || echo "File not found in expected location" # Let's also check if the test and mocks directories exist in contracts-rfq ls -la packages/contracts-rfq/test/ 2>/dev/null ls -la packages/contracts-rfq/test/mocks/ 2>/dev/null # Check for .sol files in test/mocks directory fd -e sol . packages/contracts-rfq/test/mocks/Length of output: 4665
services/rfq/contracts/testcontracts/fastbridgemockv2/helper.go (1)
23-33: 🛠️ Refactor suggestion
Add input validation for the address parameter.
Consider validating that the provided address is not zero to prevent initialization with an invalid contract address.
func NewFastBridgeMockRef(address common.Address, backend bind.ContractBackend) (*FastBridgeMockRef, error) { + if address == (common.Address{}) { + return nil, errors.New("invalid contract address: zero address") + } fastbridgemock, err := NewFastBridgeMock(address, backend)Committable suggestion was skipped due to low confidence.
services/rfq/contracts/fastbridgev2/status.go (1)
27-36:
⚠️ Potential issueAdd validation in init function.
The init function lacks validation and has an incomplete assertion comment. Consider adding proper validation to ensure the integrity of the bridge statuses.
func init() { + expectedCount := 5 // Number of defined statuses for i := 0; i < len(_BridgeStatus_index)-1; i++ { contractType := BridgeStatus(i) + // Validate that the status is within expected range + if contractType > REFUNDED { + panic("invalid bridge status encountered during initialization") + } allBridgeStatuses = append(allBridgeStatuses, contractType) - // assert type is correct } + if len(allBridgeStatuses) != expectedCount { + panic("unexpected number of bridge statuses") + } }Committable suggestion was skipped due to low confidence.
services/rfq/contracts/fastbridgev2/status_test.go (2)
37-37: 🛠️ Refactor suggestion
Consider making chainID configurable.
The chainID is hardcoded to 1 (mainnet). Consider making it configurable through a constant or test parameter to support testing against different networks.
+const defaultTestChainID = 1 + func (s *FastBridgeSuite) SetupTest() { s.TestSuite.SetupTest() - s.backend = simulated.NewSimulatedBackendWithChainID(s.GetTestContext(), s.T(), big.NewInt(1)) + s.backend = simulated.NewSimulatedBackendWithChainID(s.GetTestContext(), s.T(), big.NewInt(defaultTestChainID))Committable suggestion was skipped due to low confidence.
41-49: 🛠️ Refactor suggestion
Enhance test robustness and cleanup.
The test could benefit from some improvements:
- More descriptive error messages
- Proper cleanup of resources
- Consider adding test cases for invalid status values
Here's a suggested improvement:
func (s *FastBridgeSuite) TestStatusEnum() { + defer s.backend.Close() + _, fb := s.manager.GetMockFastBridge(s.GetTestContext(), s.backend) for _, status := range fastbridgev2.GetAllBridgeStatuses() { solstatus, err := fb.GetEnumKeyByValue(&bind.CallOpts{Context: s.GetTestContext()}, status.Int()) - s.Require().NoError(err, "error getting enum key by value") - s.Require().Equal(solstatus, status.String(), "status %s does not match. BridgeStatus enums out of sync.", status) + s.Require().NoError(err, "failed to get enum key for status value %d", status.Int()) + s.Require().Equal(solstatus, status.String(), + "status enum mismatch: got %s, want %s (value: %d)", + solstatus, status.String(), status.Int()) } + + // Test invalid status + invalidStatus := big.NewInt(999999) + _, err := fb.GetEnumKeyByValue(&bind.CallOpts{Context: s.GetTestContext()}, invalidStatus) + s.Require().Error(err, "expected error for invalid status value %d", invalidStatus) }📝 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.// TestStatusEnum makes sure. func (s *FastBridgeSuite) TestStatusEnum() { defer s.backend.Close() _, fb := s.manager.GetMockFastBridge(s.GetTestContext(), s.backend) for _, status := range fastbridgev2.GetAllBridgeStatuses() { solstatus, err := fb.GetEnumKeyByValue(&bind.CallOpts{Context: s.GetTestContext()}, status.Int()) s.Require().NoError(err, "failed to get enum key for status value %d", status.Int()) s.Require().Equal(solstatus, status.String(), "status enum mismatch: got %s, want %s (value: %d)", solstatus, status.String(), status.Int()) } // Test invalid status invalidStatus := big.NewInt(999999) _, err := fb.GetEnumKeyByValue(&bind.CallOpts{Context: s.GetTestContext()}, invalidStatus) s.Require().Error(err, "expected error for invalid status value %d", invalidStatus) }services/rfq/relayer/reldb/base/model_test.go (1)
26-40: 💡 Codebase verification
Update test coverage for V2 transaction fields across test files
Based on the verification results, I found that:
services/rfq/relayer/limiter/limiter_test.goneeds to be updated as it's not setting the new V2 fields at allservices/rfq/relayer/relapi/server_test.gois using zero values for the new fields, similar to the file under reviewservices/rfq/relayer/quoter/quoter_test.goneeds to be updated as it's not setting the new V2 fieldsThe original review comment's suggestion to improve test coverage is valid and should be extended to these additional files:
limiter_test.goandquoter_test.goshould be updated to include the new V2 fields- All test files should use meaningful non-zero values for better test coverage, especially for:
- ExclusivityEndTime (using future timestamps)
- OriginFeeAmount (using non-zero amounts)
- CallValue (using non-zero values)
- CallParams (using non-empty byte arrays)
🔗 Analysis chain
Enhance test coverage for new V2 fields
While the basic roundtrip test is implemented, consider improving the test coverage:
- The new V2 fields (ExclusivityEndTime, OriginFeeAmount, CallValue, CallParams) are currently initialized with zero values, which might not catch potential serialization issues.
- Add test cases with non-zero values, especially for CallParams which is a byte array.
Let's check if there are other test files that might need similar updates:
Here's a suggested improvement to make the test more robust:
Transaction: fastbridgev2.IFastBridgeV2BridgeTransactionV2{ OriginChainId: 1, DestChainId: 2, OriginSender: common.Address{}, DestRecipient: common.Address{}, OriginToken: common.Address{}, DestToken: common.Address{}, OriginAmount: big.NewInt(1000), DestAmount: big.NewInt(2000), Deadline: big.NewInt(time.Now().Unix()), Nonce: big.NewInt(1), - ExclusivityEndTime: big.NewInt(0), - OriginFeeAmount: big.NewInt(0), - CallValue: big.NewInt(0), - CallParams: []byte{}, + ExclusivityEndTime: big.NewInt(time.Now().Add(1 * time.Hour).Unix()), + OriginFeeAmount: big.NewInt(100), + CallValue: big.NewInt(50), + CallParams: []byte{0x1, 0x2, 0x3}, // Add some test data },
Let me check the usage of FastBridge transaction in these test files to verify if they need similar updates.
Let me fix the regex and try again to check the transaction usage in other test files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find other test files that might use the FastBridge transaction type rg -l "IFastBridge.*Transaction" --type go --glob "*_test.go"Length of output: 240
Script:
#!/bin/bash # Check how FastBridge transaction is used in other test files for file in services/rfq/relayer/limiter/limiter_test.go services/rfq/relayer/relapi/server_test.go services/rfq/relayer/quoter/quoter_test.go; do echo "=== $file ===" rg "IFastBridge.*Transaction.*{" -A 15 "$file" doneLength of output: 1379
Script:
#!/bin/bash # Check how FastBridge transaction is used in other test files for file in services/rfq/relayer/limiter/limiter_test.go services/rfq/relayer/relapi/server_test.go services/rfq/relayer/quoter/quoter_test.go; do echo "=== $file ===" rg "IFastBridgeV2BridgeTransactionV2" -A 15 "$file" doneLength of output: 4738
services/rfq/testutil/typecast.go (2)
34-38: 💡 Codebase verification
Potential inconsistency in FastBridge version usage
Based on the verification results:
- The relayer service tests correctly use
fastbridgemockv2- However, both
fastbridge/status_test.goandfastbridgev2/status_test.goappear to be using the sameGetMockFastBridgemethod but with different package imports (fastbridgevsfastbridgev2)This suggests a potential version mismatch that needs attention:
services/rfq/contracts/fastbridge/status_test.gomight need updating to use v1-specific mock bridgeservices/rfq/contracts/fastbridgev2/status_test.gois correctly using v2 imports🔗 Analysis chain
Verify test cases using GetMockFastBridge are updated for v2
The mock bridge changes correctly align with the v2 upgrade.
Let me gather more information to verify if the test cases are properly updated for v2.
Let me check one more thing to verify the imports and usage in the status test files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for GetMockFastBridge usage in test files rg -t go -l "GetMockFastBridge\(" | grep "_test.go" | xargs rg -A 2 "GetMockFastBridge\("Length of output: 1804
Script:
#!/bin/bash # Check imports in test files using GetMockFastBridge to verify they're using v2 rg -t go "GetMockFastBridge\(" | grep "_test.go" | cut -d: -f1 | xargs rg -l "fastbridgemock"Length of output: 263
Script:
#!/bin/bash # Check the actual mock bridge type being used in these test files rg -t go "fastbridgemock" services/rfq/contracts/fastbridge/status_test.go services/rfq/contracts/fastbridgev2/status_test.go services/rfq/relayer/service/suite_test.goLength of output: 697
Script:
#!/bin/bash # Check imports in status test files rg -t go "^import \(" -A 10 services/rfq/contracts/fastbridge/status_test.go services/rfq/contracts/fastbridgev2/status_test.goLength of output: 2051
20-23: 💡 Codebase verification
Issues found with FastBridge v2 migration
The verification reveals inconsistencies in the FastBridge version usage:
services/explorer/testutil/typecast.goandservices/explorer/testutil/typecast_test.goare still using v1 FastBridge imports and typesservices/rfq/e2e/rfq_test.gohas mixed imports of both v1 and v2 FastBridge packagesservices/rfq/relayer/relapi/suite_test.gois still using v1 FastBridge importWhile the
services/rfq/testutil/typecast.gocorrectly uses v2, the changes need to be propagated to all dependent services to maintain consistency.🔗 Analysis chain
Verify callers of GetFastBridge are updated for v2
The method changes look good, correctly updating to the v2 contract reference.
Let me analyze the findings and generate additional verification scripts to ensure all callers are properly updated.
The first command will help verify the import statements to ensure they're using the v2 package, and the second command will show how the returned handle is being used in the code. This will help us confirm if all callers are properly updated for v2.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for GetFastBridge calls to ensure they handle the new v2 return type rg -l "GetFastBridge\(" | xargs rg -A 2 "GetFastBridge\("Length of output: 3652
Script:
#!/bin/bash # Check the import statements in files using GetFastBridge to verify they're using v2 rg -l "GetFastBridge\(" | xargs rg "fastbridge.*\"$" # Check the type assertions or direct usage of the returned handle rg -l "GetFastBridge\(" | xargs rg -A 3 "GetFastBridge.*\)\s*$"Length of output: 4597
services/rfq/relayer/service/relayer.go (1)
102-102: 💡 Codebase verification
Based on the search results, I can now provide a final response as there's clear evidence of potential issues:
FastBridge V1 to V2 migration is incomplete
The codebase shows multiple components still using FastBridge V1 that need to be updated:
- Guard service components:
services/rfq/guard/service/handlers.goservices/rfq/guard/service/guard.go- Explorer service components:
services/explorer/api/server.goservices/explorer/contracts/fastbridge/helpers.go- Other RFQ components:
services/rfq/relayer/chain/chain.goservices/rfq/relayer/relapi/server.goWhile the relayer service has been updated to use FastBridgeV2, other dependent services are still using V1, which could lead to compatibility issues. This change requires a coordinated update across all services.
🔗 Analysis chain
Verify FastBridge V2 contract compatibility
The contract instantiation has been correctly updated to use FastBridgeV2Ref. However, let's verify the compatibility of the V2 contract with existing callers.
Let's check for any potential interface mismatches or unconverted V1 contract usage:
Let me gather more specific information about the V1 to V2 contract usage to better understand the impact.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential V1/V2 contract compatibility issues # Test 1: Search for any remaining V1 contract references echo "Checking for remaining V1 contract references..." rg -l "fastbridge\.FastBridge[^V]" # Test 2: Search for V1-specific method calls that might need updates echo "Checking for V1-specific method calls..." rg -l "NewFastBridgeRef" # Test 3: Check interface differences between V1 and V2 echo "Checking V1/V2 interface differences..." ast-grep --pattern 'contract FastBridge { $$$ }' echo "Checking V2 interface..." ast-grep --pattern 'contract FastBridgeV2 { $$$ }'Length of output: 1190
Script:
#!/bin/bash # Let's examine the actual usage patterns and changes # Check for specific V1 method calls that might need updates echo "Checking V1 method call patterns..." rg -A 2 "NewFastBridgeRef\(" # Look for any V2 migration patterns echo "Checking V2 migration patterns..." rg -A 2 "NewFastBridgeV2Ref\(" # Check the contract helper implementations echo "Checking contract helper implementations..." rg -A 5 "func NewFastBridge" services/rfq/contracts/fastbridge/ # Check for any potential interface mismatches echo "Checking interface usage patterns..." rg -A 3 "fastbridge\.FastBridge\." services/rfq/relayer/Length of output: 7622
services/rfq/contracts/fastbridgev2/events.go (3)
11-23: 🛠️ Refactor suggestion
Consider unexporting variables to prevent accidental mutation
There is a TODO comment suggesting that the variables should not be exported to avoid accidental mutation. Since these variables are intended to be constants after initialization and should not be modified externally, consider unexporting them by renaming them to start with a lowercase letter or defining them as constants if appropriate.
76-79:
⚠️ Potential issueAvoid returning the address of a loop variable
In Go, taking the address of a loop variable and returning it can lead to unexpected behavior because the loop variable
eventTypeis reused in each iteration. The returned pointer may point to the last value assigned in the loop.To fix this issue, create a new variable inside the loop to capture the current value of
eventTypeand return its address.Apply the following diff:
func eventTypeFromTopic(ogTopic common.Hash) *EventType { for eventType, topic := range topicMap() { if bytes.Equal(ogTopic.Bytes(), topic.Bytes()) { + e := eventType - return &eventType + return &e } } return nil }📝 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.for eventType, topic := range topicMap() { if bytes.Equal(ogTopic.Bytes(), topic.Bytes()) { e := eventType return &e }
37-58:
⚠️ Potential issueAdd missing validation for
BridgeDepositClaimedTopicWhile you have assigned
BridgeDepositClaimedTopicat line 37, its existence is not validated withparsedABI.EventByIDas done for the other topics. To ensure consistency and catch potential errors, add a validation step forBridgeDepositClaimedTopic.Apply the following diff to include the missing validation:
BridgeProofDisputedTopic = parsedABI.Events["BridgeProofDisputed"].ID + _, err = parsedABI.EventByID(BridgeDepositClaimedTopic) + if err != nil { + panic(err) + } _, err = parsedABI.EventByID(BridgeProofDisputedTopic)📝 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.BridgeDepositClaimedTopic = parsedABI.Events["BridgeDepositClaimed"].ID BridgeProofDisputedTopic = parsedABI.Events["BridgeProofDisputed"].ID _, err = parsedABI.EventByID(BridgeRequestedTopic) if err != nil { panic(err) } _, err = parsedABI.EventByID(BridgeRelayedTopic) if err != nil { panic(err) } _, err = parsedABI.EventByID(BridgeProofProvidedTopic) if err != nil { panic(err) } _, err = parsedABI.EventByID(BridgeDepositClaimedTopic) if err != nil { panic(err) } _, err = parsedABI.EventByID(BridgeProofDisputedTopic) if err != nil { panic(err) }services/rfq/contracts/fastbridgev2/parser.go (5)
47-97: 🛠️ Refactor suggestion
Consider refactoring 'ParseEvent' method to reduce complexity
The
ParseEventmethod has a high cyclomatic complexity, as indicated by the// nolint: cyclopdirective. To improve maintainability and readability, consider refactoring this method to reduce its complexity.One approach is to create separate helper functions for each event parsing case or use a mapping of event types to parsing functions.
51-51:
⚠️ Potential issueUndefined function 'topicMap()' used
The function
topicMap()is used but not defined within this file or imported from another package. This will result in a compilation error. Ensure thattopicMap()is defined or properly imported.
89-94:
⚠️ Potential issueInconsistent naming between event type and parsing method
The event type is named
BridgeDisputeEvent, but the corresponding parsing method isParseBridgeProofDisputed. To maintain consistency and avoid confusion, consider renaming either the event type or the parsing method to match each other.For example, you could rename the event type to
BridgeProofDisputedEventto align with the parsing method:-// BridgeDisputeEvent is the event type for the BridgeDispute event. -BridgeDisputeEvent +// BridgeProofDisputedEvent is the event type for the BridgeProofDisputed event. +BridgeProofDisputedEventCommittable suggestion was skipped due to low confidence.
48-48: 🛠️ Refactor suggestion
Use pointer receiver for 'parserImpl' methods
The
ParseEventmethod has a value receiver(p parserImpl). To follow best practices and ensure consistency, consider using a pointer receiver(p *parserImpl), especially sinceparserImplcontains a pointer field.Apply the following diff:
-func (p parserImpl) ParseEvent(log ethTypes.Log) (_ EventType, event interface{}, ok bool) { +func (p *parserImpl) ParseEvent(log ethTypes.Log) (_ EventType, event interface{}, ok bool) {📝 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.func (p *parserImpl) ParseEvent(log ethTypes.Log) (_ EventType, event interface{}, ok bool) {
28-28:
⚠️ Potential issueTypo in comment: 'contracat' should be 'contract'
There's a typographical error in the comment. The word "contracat" should be corrected to "contract".
Apply the following diff to fix the typo:
-// Parser parses events from the fastbridge contracat. +// Parser parses events from the fastbridge contract.📝 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.// Parser parses events from the fastbridge contract.services/rfq/guard/guardconfig/config.go (2)
88-103: 🛠️ Refactor suggestion
Reduce code duplication in GetRFQAddressV1 and GetRFQAddressV2
The methods
GetRFQAddressV1andGetRFQAddressV2share similar logic. Consider refactoring them into a single method to improve maintainability.Example refactored method:
func (c Config) GetRFQAddress(chainID int, version int) (string, error) { chainCfg, ok := c.Chains[chainID] if !ok { return "", fmt.Errorf("chain config not found for chain %d", chainID) } switch version { case 1: return chainCfg.RFQAddressV1, nil case 2: return chainCfg.RFQAddressV2, nil default: return "", fmt.Errorf("invalid RFQ version %d", version) } }You can then adjust your calls accordingly.
37-40: 🛠️ Refactor suggestion
Clarify documentation comments for RFQAddressV1 and RFQAddressV2
The comments for
RFQAddressV1andRFQAddressV2are identical and may cause confusion. Consider updating them to clearly specify which version each address corresponds to.Apply this diff to improve the comments:
- // RFQAddressV1 is the rfq bridge contract address. + // RFQAddressV1 is the RFQ bridge contract address for version 1. - // RFQAddressV2 is the rfq bridge contract address. + // RFQAddressV2 is the RFQ bridge contract address for version 2.📝 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.// RFQAddressV1 is the RFQ bridge contract address for version 1. RFQAddressV1 string `yaml:"rfq_address_v1"` // RFQAddressV2 is the RFQ bridge contract address for version 2. RFQAddressV2 string `yaml:"rfq_address_v2"`services/rfq/testutil/deployers.go (2)
138-140: 🛠️ Refactor suggestion
Inconsistent function naming in
fastbridgemockv2packageThe function
DeployFastBridgeMockin thefastbridgemockv2package does not include the 'V2' suffix in its name, whereas the corresponding function infastbridgev2is namedDeployFastBridgeV2. For consistency and to avoid confusion, consider renaming the function toDeployFastBridgeMockV2.
140-141: 🛠️ Refactor suggestion
Inconsistent reference function naming in
fastbridgemockv2packageSimilarly, the function
NewFastBridgeMockRefdoes not include the 'V2' suffix. To maintain consistency withfastbridgev2.NewFastBridgeV2Ref, consider renaming it toNewFastBridgeMockV2Ref.services/rfq/relayer/reldb/db.go (1)
84-87: 🛠️ Refactor suggestion
Consider unifying 'TransactionV1' and 'Transaction' fields
Adding both
TransactionV1andTransactionfields to theQuoteRequeststruct may increase complexity and potential for confusion. SinceSendChainGasis the only difference between v1 and v2, consider using a singleTransactionfield and handling version differences within the field or through interface abstraction.services/rfq/relayer/reldb/base/model.go (1)
241-242:
⚠️ Potential issuePossible loss of precision in integer division
Dividing
big.Intamounts to scale them can result in integer division, potentially causing precision loss if amounts are not exact multiples of the scaling factor.Consider using precise decimal arithmetic or alternative methods to ensure accurate amount representation without losing precision due to integer division.
services/rfq/guard/service/handlers.go (2)
244-253:
⚠️ Potential issueVerify the correctness of field assignments in
relayDetails.In both
isProveValidV1andisProveValidV2, therelayDetailsstruct is populated from event data. Please ensure that the event fields are correctly assigned to the correspondingrelayDetailsfields.For instance:
- The
Tofield inrelayDetailsis assignedevent.OriginToken, which represents a token address. Should this beevent.To, representing the recipient address?- Similarly, check that
OriginTokenandDestTokenare correctly assigned.Also applies to: 293-302
266-269:
⚠️ Potential issueUse the correct parser for FastBridgeV2 events.
In the
isProveValidV2function, the parser is initialized usingfastbridge.NewParser, which is intended for FastBridgeV1 events. To correctly parse FastBridgeV2 events, usefastbridgev2.NewParserinstead.Apply this diff to fix the parser initialization:
- parser, err := fastbridge.NewParser(common.HexToAddress(rfqAddr)) + parser, err := fastbridgev2.NewParser(common.HexToAddress(rfqAddr))📝 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.parser, err := fastbridgev2.NewParser(common.HexToAddress(rfqAddr)) if err != nil { return false, fmt.Errorf("could not get parser: %w", err) }services/rfq/guard/service/guard.go (3)
63-71: 🛠️ Refactor suggestion
Reduce code duplication in handler initialization
The initialization of
fastBridgeHandlerV1andfastBridgeHandlerV2is nearly identical. Refactoring this into a loop or a helper function can reduce duplication and improve maintainability.You could refactor the handler initialization as follows:
handlers := map[string]*fastBridgeHandler{} for _, version := range []string{"v1", "v2"} { handlers[version] = &fastBridgeHandler{ listeners: make(map[int]listener.ContractListener), contracts: make(map[int]*fastbridge.FastBridgeRef), } }Update the
Guardstruct and usage accordingly to utilize thehandlersmap.
321-349:
⚠️ Potential issueAvoid casting V2 events to V1 types
In
runChainIndexerV2, V2 events are being converted to V1 event structs before handling. This approach may lead to errors if the event structures diverge in the future. It's safer to handle V2 events directly or define a common interface for shared handling.Consider updating the event handlers to accept interfaces or create separate handling functions for V2 events to ensure type safety and future compatibility.
389-394: 🛠️ Refactor suggestion
Optimize
isV2Addressmethod by utilizing existing dataThe
isV2Addressmethod fetches the RFQ address from the config each time it's called. Since the address is already stored infastBridgeHandlerV2.contracts, consider checking against the stored contract address to avoid redundant configuration calls.For example:
func (g *Guard) isV2Address(chainID int, addr common.Address) bool { contract, ok := g.fastBridgeHandlerV2.contracts[chainID] if !ok { return false } return contract.Address() == addr }services/rfq/relayer/service/handlers.go (2)
85-85: 🛠️ Refactor suggestion
Address the TODO: Retrieve decimals from inventory and handle invalid tokens
The TODO comment at line 85 suggests improving the retrieval of
originDecimalsanddestDecimalsby pulling them from the inventory. Additionally, tokens that are not present in the inventory should be marked as invalid to prevent processing invalid requests.Would you like assistance in implementing this change, or should I open a GitHub issue to track this task?
103-104: 🛠️ Refactor suggestion
Clarify field naming by renaming 'Transaction' to 'TransactionV2'
In the
dbReqstruct, the fieldTransactionis assignedtxV2, whileTransactionV1is assignedtxV1. To improve code clarity and maintain consistency, consider renaming theTransactionfield toTransactionV2.Apply this diff to update the field name:
dbReq := reldb.QuoteRequest{ BlockNumber: req.Raw.BlockNumber, RawRequest: req.Request, OriginTokenDecimals: *originDecimals, DestTokenDecimals: *destDecimals, TransactionID: req.TransactionId, Sender: req.Sender, TransactionV1: txV1, - Transaction: txV2, + TransactionV2: txV2, Status: reldb.Seen, OriginTxHash: req.Raw.TxHash, }Ensure that all references to
Transactionare updated accordingly throughout the codebase.Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (4)
services/rfq/guard/service/guard.go (2)
Line range hint
220-236: Improve error message in group.Wait()The error message in group.Wait() is incomplete and doesn't include the error details.
Apply this fix:
err = group.Wait() if err != nil { - return fmt.Errorf("could not run chain indexers") + return fmt.Errorf("could not run chain indexers: %w", err) }
Line range hint
239-294: Consider reducing code duplication in indexer implementationsThe
runChainIndexerV1andrunChainIndexerV2functions share significant code structure. Consider extracting common functionality into a shared helper function to improve maintainability.Example refactor:
func (g Guard) runChainIndexerCommon( ctx context.Context, chainID int, listener listener.ContractListener, parser eventParser, handleEvent func(context.Context, interface{}) error, ) error { return listener.Listen(ctx, func(parentCtx context.Context, log types.Log) error { et, parsedEvent, ok := parser.ParseEvent(log) if !ok { if len(log.Topics) != 0 { logger.Warnf("unknown event %s", log.Topics[0]) } return nil } ctx, span := g.metrics.Tracer().Start(parentCtx, /* ... */) defer func() { metrics.EndSpanWithErr(span, err) }() return handleEvent(ctx, parsedEvent) }) }Also applies to: 295-362
services/rfq/guard/guardconfig/config.go (1)
76-76: Differentiate Error Messages for V1 and V2 RFQ AddressesIn the
Validatemethod, the error messages for invalid RFQ addresses are identical for both V1 and V2:"invalid rfq address: %s". This could lead to confusion when diagnosing issues. Consider specifying whether the invalid address isRFQAddressV1orRFQAddressV2for clearer debugging.Suggested Change:
// For addrV1 validation - return fmt.Errorf("invalid rfq address: %s", *addrV1) + return fmt.Errorf("invalid RFQAddressV1: %s", *addrV1) // For addrV2 validation - return fmt.Errorf("invalid rfq address: %s", addrV2) + return fmt.Errorf("invalid RFQAddressV2: %s", addrV2)Also applies to: 83-83
services/rfq/guard/service/handlers.go (1)
356-374: Complete the exhaustive checks inrelayMatchesBridgeRequestThe
relayMatchesBridgeRequestfunction includes a TODO comment questioning the exhaustiveness of the checks. To ensure the relay details accurately match the bridge request, review all relevant fields and confirm that no critical data points are omitted.Would you like assistance in identifying additional fields or conditions that should be included in the matching logic?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- services/rfq/guard/guardconfig/config.go (4 hunks)
- services/rfq/guard/service/guard.go (8 hunks)
- services/rfq/guard/service/handlers.go (7 hunks)
- services/rfq/relayer/relconfig/config.go (1 hunks)
🔇 Additional comments (5)
services/rfq/guard/service/guard.go (3)
20-20: LGTM: Clean separation of v1 and v2 contract handlingThe Guard struct's design cleanly separates v1 and v2 contract concerns using distinct maps, which will make version-specific operations clearer and easier to maintain.
Also applies to: 33-42
60-91: LGTM: Proper initialization of v1 contract handlersThe v1 contract initialization is well-structured with proper error handling and conditional setup based on address availability.
205-219: LGTM: Well-structured v1 indexer initializationThe v1 indexer initialization is properly guarded with null checks and has good error handling.
services/rfq/guard/guardconfig/config.go (1)
136-137:⚠️ Potential issueVerify Field Names in NewGuardConfigFromRelayer
In
NewGuardConfigFromRelayer, you're assigningchainCfg.RFQAddressV1toRFQAddressV1andchainCfg.RFQAddresstoRFQAddressV2. Please ensure thatchainCfg.RFQAddressV1exists and is correctly populated inrelconfig.ChainConfig. There may be a mismatch in field names or missing fields between the relayer and guard configurations.Verification Script:
This script searches the codebase for the definition of
RFQAddressV1within theChainConfigstruct of therelconfigpackage. Ensure thatRFQAddressV1is defined and properly used inrelconfig.ChainConfig.✅ Verification successful
Let me try a different approach to verify this by searching for the ChainConfig struct definition and checking its fields.
This will help us understand:
- The actual fields in ChainConfig struct
- All occurrences of RFQAddressV1 in the codebase to verify if there's a mismatch
Field names in NewGuardConfigFromRelayer are correct
Based on the search results, the field assignments in
NewGuardConfigFromRelayerare correct:
- In
services/rfq/relayer/relconfig/config.go,RFQAddressV1is defined as:RFQAddressV1 *string `yaml:"rfq_address_v1"`
- In
services/rfq/guard/guardconfig/config.go, the same field exists:RFQAddressV1 *string `yaml:"rfq_address_v1"`The assignment
RFQAddressV1: chainCfg.RFQAddressV1is correct as both structs have matching field names and types. Similarly,RFQAddressin the relayer config maps correctly toRFQAddressV2in the guard config.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `RFQAddressV1` exists in `relconfig.ChainConfig` # Search for the definition of RFQAddressV1 in relconfig package ast-grep --pattern $'type ChainConfig { $$$ RFQAddressV1 $_ $$$ }'Length of output: 79
Script:
#!/bin/bash # Search for ChainConfig struct definition rg "type ChainConfig struct" -A 20 # Also search for any usage of RFQAddressV1 rg "RFQAddressV1"Length of output: 17280
services/rfq/guard/service/handlers.go (1)
298-301: Verify consistent usage of FastBridgeV2 methods and typesEnsure that all FastBridgeV2 specific methods and types are consistently used throughout the code, particularly in the
isProveValidV2function and related logic.Run the following script to identify usages of FastBridgeV2 methods and confirm correct implementation:
This will help ensure that all V2 contract interactions are correctly handled.
✅ Verification successful
Let me analyze the code further to verify the consistent usage of FastBridgeV2 methods and types.
This will help us verify the consistent usage of V2-specific methods in the guard service, particularly around the code snippet in question.
Let me verify one more aspect to ensure complete analysis.
This will help us verify the implementation details of the V2-specific validation method.
Based on the analysis of the codebase through the executed scripts, I can now provide a final response:
FastBridgeV2 methods and types are consistently implemented
The code shows proper and consistent implementation of FastBridgeV2-specific functionality:
- Correct usage of
GetRFQAddressV2throughout the guard service- Proper V2 contract interactions in the relayer service
- Consistent type handling for
FastBridgeV2BridgeTransactionV2and related structures- Proper event handling for V2-specific events
The code in question correctly uses
GetRFQAddressV2which aligns with the rest of the V2 implementation patterns seen across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of FastBridgeV2 methods and types. # Search for references to FastBridgeV2 in the codebase. rg 'fastbridgev2\.' -A 3Length of output: 16350
Script:
#!/bin/bash # Search for all V2-specific method calls and types in the guard service rg -A 3 'GetRFQAddressV2|isProveValidV2|BridgeTransactionV2' services/rfq/guard/Length of output: 2304
Script:
#!/bin/bash # Search for the implementation of isProveValidV2 and related V2 methods ast-grep --pattern 'func $_ isProveValidV2($_) { $$$ }'Length of output: 59
| func (g *Guard) isV2Address(chainID int, addr common.Address) bool { | ||
| rfqAddr, err := g.cfg.GetRFQAddressV2(chainID) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return addr == common.HexToAddress(rfqAddr) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider caching the v2 address lookup
The isV2Address function performs a config lookup and address comparison on every call. Consider caching the v2 addresses in a map during initialization to improve performance.
Example implementation:
type Guard struct {
// ... existing fields ...
v2AddressCache map[int]common.Address
}
func (g *Guard) isV2Address(chainID int, addr common.Address) bool {
if cachedAddr, ok := g.v2AddressCache[chainID]; ok {
return addr == cachedAddr
}
return false
}
services/rfq/guard/service/guard.go
Outdated
| // setup v2 | ||
| rfqAddrV2, err := cfg.GetRFQAddressV2(chainID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get rfq address: %w", err) | ||
| } | ||
| chainClient, err := omniClient.GetChainClient(ctx, chainID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get chain client: %w", err) | ||
| } | ||
|
|
||
| contract, err := fastbridge.NewFastBridgeRef(common.HexToAddress(rfqAddr), chainClient) | ||
| contract, err := fastbridgev2.NewFastBridgeV2Ref(common.HexToAddress(rfqAddrV2), chainClient) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create fast bridge contract: %w", err) | ||
| } | ||
| startBlock, err := contract.DeployBlock(&bind.CallOpts{Context: ctx}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get deploy block: %w", err) | ||
| } | ||
| chainListener, err := listener.NewChainListener(chainClient, store, common.HexToAddress(rfqAddr), uint64(startBlock.Int64()), metricHandler, listener.WithName("guard")) | ||
| chainListener, err := listener.NewChainListener(chainClient, store, common.HexToAddress(rfqAddrV2), uint64(startBlock.Int64()), metricHandler, listener.WithName("guard")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get chain listener: %w", err) | ||
| } | ||
| chainListeners[chainID] = chainListener | ||
|
|
||
| // setup FastBridge contract on this chain | ||
| contracts[chainID], err = fastbridge.NewFastBridgeRef(common.HexToAddress(rfqAddr), chainClient) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create bridge contract: %w", err) | ||
| } | ||
| listenersV2[chainID] = chainListener | ||
| contractsV2[chainID] = contract |
There was a problem hiding this comment.
Add null check for v2 address
The v2 contract initialization assumes rfqAddrV2 is always non-empty. Consider adding a null check similar to v1 to handle cases where v2 contracts might not be deployed yet.
Apply this pattern:
rfqAddrV2, err := cfg.GetRFQAddressV2(chainID)
if err != nil {
return nil, fmt.Errorf("could not get rfq address: %w", err)
}
+if rfqAddrV2 == "" {
+ continue
+}
chainClient, err := omniClient.GetChainClient(ctx, chainID)📝 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.
| // setup v2 | |
| rfqAddrV2, err := cfg.GetRFQAddressV2(chainID) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get rfq address: %w", err) | |
| } | |
| chainClient, err := omniClient.GetChainClient(ctx, chainID) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get chain client: %w", err) | |
| } | |
| contract, err := fastbridge.NewFastBridgeRef(common.HexToAddress(rfqAddr), chainClient) | |
| contract, err := fastbridgev2.NewFastBridgeV2Ref(common.HexToAddress(rfqAddrV2), chainClient) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not create fast bridge contract: %w", err) | |
| } | |
| startBlock, err := contract.DeployBlock(&bind.CallOpts{Context: ctx}) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get deploy block: %w", err) | |
| } | |
| chainListener, err := listener.NewChainListener(chainClient, store, common.HexToAddress(rfqAddr), uint64(startBlock.Int64()), metricHandler, listener.WithName("guard")) | |
| chainListener, err := listener.NewChainListener(chainClient, store, common.HexToAddress(rfqAddrV2), uint64(startBlock.Int64()), metricHandler, listener.WithName("guard")) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get chain listener: %w", err) | |
| } | |
| chainListeners[chainID] = chainListener | |
| // setup FastBridge contract on this chain | |
| contracts[chainID], err = fastbridge.NewFastBridgeRef(common.HexToAddress(rfqAddr), chainClient) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not create bridge contract: %w", err) | |
| } | |
| listenersV2[chainID] = chainListener | |
| contractsV2[chainID] = contract | |
| // setup v2 | |
| rfqAddrV2, err := cfg.GetRFQAddressV2(chainID) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get rfq address: %w", err) | |
| } | |
| if rfqAddrV2 == "" { | |
| continue | |
| } | |
| chainClient, err := omniClient.GetChainClient(ctx, chainID) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get chain client: %w", err) | |
| } | |
| contract, err := fastbridgev2.NewFastBridgeV2Ref(common.HexToAddress(rfqAddrV2), chainClient) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not create fast bridge contract: %w", err) | |
| } | |
| startBlock, err := contract.DeployBlock(&bind.CallOpts{Context: ctx}) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get deploy block: %w", err) | |
| } | |
| chainListener, err := listener.NewChainListener(chainClient, store, common.HexToAddress(rfqAddrV2), uint64(startBlock.Int64()), metricHandler, listener.WithName("guard")) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get chain listener: %w", err) | |
| } | |
| listenersV2[chainID] = chainListener | |
| contractsV2[chainID] = contract |
| // RFQAddress is the rfq bridge contract address (v2). | ||
| RFQAddress string `yaml:"rfq_address"` | ||
| // RFQAddressV1 is the rfq bridge contract address (v1, optional). | ||
| RFQAddressV1 *string `yaml:"rfq_address_v1"` |
There was a problem hiding this comment.
Add validation logic for RFQ addresses
The addition of dual RFQ addresses (v1 and v2) requires additional validation to ensure proper configuration:
- When
RFQAddressV1is present, both addresses should be valid Ethereum addresses - Consider adding validation to ensure proper migration state (e.g., at least one address must be configured)
- Document the migration path for existing configurations
Consider adding validation in the Validate method:
func (c Config) Validate(ctx context.Context, omniclient omniClient.RPCClient) (err error) {
+ // Validate RFQ addresses
+ for chainID, chainCfg := range c.Chains {
+ if !common.IsHexAddress(chainCfg.RFQAddress) {
+ return fmt.Errorf("invalid RFQAddress (v2) for chain %d: %s", chainID, chainCfg.RFQAddress)
+ }
+ if chainCfg.RFQAddressV1 != nil {
+ if !common.IsHexAddress(*chainCfg.RFQAddressV1) {
+ return fmt.Errorf("invalid RFQAddressV1 for chain %d: %s", chainID, *chainCfg.RFQAddressV1)
+ }
+ }
+ }
// Existing validation logic...
maintenancePctSums := map[string]float64{}Consider adding documentation in the struct to clarify:
- The relationship between v1 and v2 addresses
- Migration requirements and timeline
- Configuration examples for different scenarios (v2 only, dual-version support)
Committable suggestion was skipped due to low confidence.
| RFQAddressV1 *string `yaml:"rfq_address_v1"` | ||
| // RFQAddressV2 is the rfq bridge contract address. | ||
| RFQAddressV2 string `yaml:"rfq_address_v2"` |
There was a problem hiding this comment.
Ensure Consistent Types for RFQAddressV1 and RFQAddressV2
RFQAddressV1 is declared as a pointer to string (*string), whereas RFQAddressV2 is declared as a string (string). This inconsistency may lead to confusion and potential errors in handling these fields. If both addresses are required configurations, consider using string for both. If RFQAddressV1 is optional and may be nil, ensure this distinction is clearly documented and consistently handled throughout the codebase.
Suggested Change:
Option 1: If both addresses are required:
- RFQAddressV1 *string `yaml:"rfq_address_v1"`
+ RFQAddressV1 string `yaml:"rfq_address_v1"`Option 2: If both addresses can be optional:
- RFQAddressV2 string `yaml:"rfq_address_v2"`
+ RFQAddressV2 *string `yaml:"rfq_address_v2"`Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
@dwasse I agree with the Rabbit that highlighting the fields as optional/required would be helpful here.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| // GetRFQAddressV1 returns the RFQ address for the given chain. | ||
| func (c Config) GetRFQAddressV1(chainID int) (*string, error) { | ||
| chainCfg, ok := c.Chains[chainID] | ||
| if !ok { | ||
| return nil, fmt.Errorf("chain config not found for chain %d", chainID) | ||
| } | ||
| return chainCfg.RFQAddressV1, nil | ||
| } | ||
|
|
||
| // GetRFQAddressV2 returns the RFQ address for the given chain. | ||
| func (c Config) GetRFQAddressV2(chainID int) (string, error) { | ||
| chainCfg, ok := c.Chains[chainID] | ||
| if !ok { | ||
| return "", fmt.Errorf("chain config not found for chain %d", chainID) | ||
| } | ||
| return chainCfg.RFQAddress, nil | ||
| return chainCfg.RFQAddressV2, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize Return Types of GetRFQAddress Methods
GetRFQAddressV1 returns a pointer to string (*string, error), while GetRFQAddressV2 returns a string (string, error). This inconsistency might cause confusion and complicate error handling. Consider standardizing the return types of both methods to improve consistency and reduce potential errors.
Suggested Change:
Option 1: Have both methods return (*string, error):
// For GetRFQAddressV2
-func (c Config) GetRFQAddressV2(chainID int) (string, error) {
+func (c Config) GetRFQAddressV2(chainID int) (*string, error) {
chainCfg, ok := c.Chains[chainID]
if !ok {
- return "", fmt.Errorf("chain config not found for chain %d", chainID)
+ return nil, fmt.Errorf("chain config not found for chain %d", chainID)
}
- return chainCfg.RFQAddressV2, nil
+ return &chainCfg.RFQAddressV2, nil
}Option 2: Have both methods return (string, error) and adjust handling of optional values accordingly.
Committable suggestion was skipped due to low confidence.
| func (g *Guard) disputeV1(ctx context.Context, proven *guarddb.PendingProven, bridgeRequest *guarddb.BridgeRequest) error { | ||
| contract, ok := g.contractsV1[int(bridgeRequest.Transaction.OriginChainId)] | ||
| if !ok { | ||
| return errors.New("could not get contract") | ||
| } | ||
| _, err := g.txSubmitter.SubmitTransaction(ctx, big.NewInt(int64(bridgeRequest.Transaction.OriginChainId)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||
| tx, err = contract.Dispute(transactor, proven.TransactionID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not dispute: %w", err) | ||
| } | ||
|
|
||
| return tx, nil | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("could not dispute: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (g *Guard) disputeV2(ctx context.Context, proven *guarddb.PendingProven, bridgeRequest *guarddb.BridgeRequest) error { | ||
| contract, ok := g.contractsV2[int(bridgeRequest.Transaction.OriginChainId)] | ||
| if !ok { | ||
| return errors.New("could not get contract") | ||
| } | ||
| _, err := g.txSubmitter.SubmitTransaction(ctx, big.NewInt(int64(bridgeRequest.Transaction.OriginChainId)), func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||
| tx, err = contract.Dispute(transactor, proven.TransactionID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not dispute: %w", err) | ||
| } | ||
|
|
||
| return tx, nil | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("could not dispute: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor disputeV1 and disputeV2 to eliminate code duplication
The disputeV1 and disputeV2 functions share similar logic, differing mainly in the contract version used. Refactoring these into a single function can reduce redundancy and improve maintainability.
Consider creating a unified dispute function:
func (g *Guard) dispute(ctx context.Context, proven *guarddb.PendingProven, bridgeRequest *guarddb.BridgeRequest, version int) error {
var contract YourContractInterface
switch version {
case 1:
contract, ok = g.contractsV1[int(bridgeRequest.Transaction.OriginChainId)]
case 2:
contract, ok = g.contractsV2[int(bridgeRequest.Transaction.OriginChainId)]
default:
return errors.New("invalid contract version")
}
if !ok {
return errors.New("could not get contract")
}
_, err := g.txSubmitter.SubmitTransaction(ctx, big.NewInt(int64(bridgeRequest.Transaction.OriginChainId)), func(transactor *bind.TransactOpts) (*types.Transaction, error) {
tx, err := contract.Dispute(transactor, proven.TransactionID)
if err != nil {
return nil, fmt.Errorf("could not dispute: %w", err)
}
return tx, nil
})
if err != nil {
return fmt.Errorf("could not submit dispute transaction: %w", err)
}
return nil
}Then, in your handleProveCalled function, you can call:
if g.isV2Address(int(bridgeRequest.Transaction.OriginChainId), proven.FastBridgeAddress) {
err = g.dispute(ctx, proven, bridgeRequest, 2)
} else {
err = g.dispute(ctx, proven, bridgeRequest, 1)
}| func (g *Guard) isProveValidV1(ctx context.Context, proven *guarddb.PendingProven, bridgeRequest *guarddb.BridgeRequest, receipt *types.Receipt) (bool, error) { | ||
| span := trace.SpanFromContext(ctx) | ||
|
|
||
| rfqAddr, err := g.cfg.GetRFQAddressV1(int(bridgeRequest.Transaction.DestChainId)) | ||
| if err != nil { | ||
| return false, fmt.Errorf("could not get rfq address v1: %w", err) | ||
| } | ||
| if rfqAddr == nil { | ||
| return false, errors.New("rfq address v1 is nil") | ||
| } | ||
| parser, err := fastbridge.NewParser(common.HexToAddress(*rfqAddr)) | ||
| if err != nil { | ||
| return false, fmt.Errorf("could not get rfq address: %w", err) | ||
| return false, fmt.Errorf("could not get parser: %w", err) | ||
| } | ||
|
|
||
| for _, log := range receipt.Logs { | ||
| _, parsedEvent, ok := parser.ParseEvent(*log) | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| if log.Address != common.HexToAddress(*rfqAddr) { | ||
| span.AddEvent(fmt.Sprintf("log address %s does not match rfq address %s", log.Address.Hex(), *rfqAddr)) | ||
| continue | ||
| } | ||
|
|
||
| event, ok := parsedEvent.(*fastbridge.FastBridgeBridgeRelayed) | ||
| if !ok { | ||
| span.AddEvent("event is not a BridgeRelayed event") | ||
| continue | ||
| } | ||
|
|
||
| if event.Relayer != proven.RelayerAddress { | ||
| span.AddEvent(fmt.Sprintf("relayer address %s does not match prover address %s", event.Relayer.Hex(), proven.RelayerAddress.Hex())) | ||
| continue | ||
| } | ||
|
|
||
| details := relayDetails{ | ||
| TransactionID: event.TransactionId, | ||
| OriginAmount: event.OriginAmount, | ||
| DestAmount: event.DestAmount, | ||
| OriginChainID: event.OriginChainId, | ||
| To: event.OriginToken, | ||
| DestToken: event.DestToken, | ||
| } | ||
|
|
||
| return relayMatchesBridgeRequest(details, bridgeRequest), nil | ||
| } | ||
|
|
||
| return false, nil | ||
| } |
There was a problem hiding this comment.
Verify field mappings in relayDetails assignments
In both isProveValidV1 and isProveValidV2, ensure that the fields from the events are correctly mapped to the relayDetails struct. Inconsistent field assignments may lead to incorrect validation outcomes.
Review the assignments, particularly:
- Confirm whether
Toshould map toevent.OriginTokenor another field. - Ensure
OriginTokenandDestTokenare assigned the correct values from the event.
For example, in isProveValidV2:
details := relayDetails{
TransactionID: event.TransactionId,
OriginAmount: event.OriginAmount,
DestAmount: event.DestAmount,
OriginChainID: event.OriginChainId,
To: event.OriginToken, // Verify if this is correct
DestToken: event.DestToken,
OriginToken: event.OriginToken, // Is this duplicated?
}Consider clarifying the purpose of each field and adjusting the assignments accordingly.
Also applies to: 295-343
| rfqAddr, err := g.cfg.GetRFQAddressV2(int(bridgeRequest.Transaction.DestChainId)) | ||
| if err != nil { | ||
| return false, fmt.Errorf("could not get rfq address v2: %w", err) | ||
| } |
There was a problem hiding this comment.
Use fastbridgev2.NewParser instead of fastbridge.NewParser
In the isProveValidV2 function, the parser is instantiated using fastbridge.NewParser, which is intended for the V1 contract. To correctly parse events from the V2 contract, it should use fastbridgev2.NewParser.
Apply this diff to fix the parser instantiation:
func (g *Guard) isProveValidV2(ctx context.Context, proven *guarddb.PendingProven, bridgeRequest *guarddb.BridgeRequest, receipt *types.Receipt) (bool, error) {
span := trace.SpanFromContext(ctx)
rfqAddr, err := g.cfg.GetRFQAddressV2(int(bridgeRequest.Transaction.DestChainId))
if err != nil {
return false, fmt.Errorf("could not get rfq address v2: %w", err)
}
- parser, err := fastbridge.NewParser(common.HexToAddress(rfqAddr))
+ parser, err := fastbridgev2.NewParser(common.HexToAddress(rfqAddr))
if err != nil {
return false, fmt.Errorf("could not get parser: %w", err)
}Committable suggestion was skipped due to low confidence.
trajan0x
changed the title
trajan0x
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
services/rfq/guard/service/handlers.go (1)
231-233: Enhance error messages with more context.The error messages could be more specific about what failed during validation.
Apply this diff to improve error messages:
- return false, fmt.Errorf("could not check prove validity v2: %w", err) + return false, fmt.Errorf("could not validate V2 proof for transaction %s: %w", hexutil.Encode(proven.TransactionID[:]), err) - return false, fmt.Errorf("could not check prove validity v1: %w", err) + return false, fmt.Errorf("could not validate V1 proof for transaction %s: %w", hexutil.Encode(proven.TransactionID[:]), err)Also applies to: 236-238
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- services/rfq/guard/guarddb/base/model.go (3 hunks)
- services/rfq/guard/service/guard.go (8 hunks)
- services/rfq/guard/service/handlers.go (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/rfq/guard/guarddb/base/model.go
🔇 Additional comments (4)
services/rfq/guard/service/guard.go (3)
Line range hint
205-228: LGTM! Well-structured chain indexer setupThe separation of v1 and v2 indexers with clear error messages and proper goroutine management is well implemented.
391-397: Previous review comment about caching is still valid
93-115:⚠️ Potential issueAdd null check for v2 address
The v2 contract initialization should handle cases where v2 contracts might not be deployed yet, similar to how v1 is handled.
Apply this diff:
// setup v2 rfqAddrV2, err := cfg.GetRFQAddressV2(chainID) if err != nil { return nil, fmt.Errorf("could not get rfq address: %w", err) } +if rfqAddrV2 == "" { + continue +} chainClient, err := omniClient.GetChainClient(ctx, chainID)Likely invalid or redundant comment.
services/rfq/guard/service/handlers.go (1)
356-357: Address TODO comment about validation completeness.The TODO comment suggests uncertainty about whether the validation checks are exhaustive. Let's verify the completeness of the validation.
Let's analyze the bridge request fields to ensure we're not missing any critical validations:
services/rfq/guard/service/guard.go
Outdated
| //nolint:cyclop,gosec | ||
| func (g Guard) runChainIndexerV2(ctx context.Context, chainID int) (err error) { | ||
| chainListener := g.listenersV2[chainID] | ||
|
|
||
| parser, err := fastbridgev2.NewParser(chainListener.Address()) | ||
| if err != nil { | ||
| return fmt.Errorf("could not parse: %w", err) | ||
| } | ||
|
|
||
| err = chainListener.Listen(ctx, func(parentCtx context.Context, log types.Log) (err error) { | ||
| et, parsedEvent, ok := parser.ParseEvent(log) | ||
| // handle unknown event | ||
| if !ok { | ||
| if len(log.Topics) != 0 { | ||
| logger.Warnf("unknown event %s", log.Topics[0]) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| ctx, span := g.metrics.Tracer().Start(parentCtx, fmt.Sprintf("handleLog-%s", et), trace.WithAttributes( | ||
| attribute.String(metrics.TxHash, log.TxHash.String()), | ||
| attribute.Int(metrics.Origin, chainID), | ||
| attribute.String(metrics.Contract, log.Address.String()), | ||
| attribute.String("block_hash", log.BlockHash.String()), | ||
| attribute.Int64("block_number", int64(log.BlockNumber)), | ||
| )) | ||
|
|
||
| defer func() { | ||
| metrics.EndSpanWithErr(span, err) | ||
| }() | ||
|
|
||
| switch event := parsedEvent.(type) { | ||
| case *fastbridgev2.FastBridgeV2BridgeRequested: | ||
| v1Event := &fastbridge.FastBridgeBridgeRequested{ | ||
| TransactionId: event.TransactionId, | ||
| Sender: event.Sender, | ||
| Request: event.Request, | ||
| DestChainId: event.DestChainId, | ||
| OriginToken: event.OriginToken, | ||
| DestToken: event.DestToken, | ||
| DestAmount: event.DestAmount, | ||
| SendChainGas: event.SendChainGas, | ||
| Raw: event.Raw, | ||
| } | ||
| err = g.handleBridgeRequestedLog(ctx, v1Event, chainID) | ||
| if err != nil { | ||
| return fmt.Errorf("could not handle request: %w", err) | ||
| } | ||
| // following events match ABIs exactly, so no need to differentiate | ||
| case *fastbridgev2.FastBridgeV2BridgeProofProvided: | ||
| v1Event := &fastbridge.FastBridgeBridgeProofProvided{ | ||
| TransactionId: event.TransactionId, | ||
| Relayer: event.Relayer, | ||
| TransactionHash: event.TransactionHash, | ||
| Raw: event.Raw, | ||
| } | ||
| err = g.handleProofProvidedLog(ctx, v1Event, chainID) | ||
| if err != nil { | ||
| return fmt.Errorf("could not handle request: %w", err) | ||
| } | ||
| case *fastbridgev2.FastBridgeV2BridgeProofDisputed: | ||
| v1Event := &fastbridge.FastBridgeBridgeProofDisputed{ | ||
| TransactionId: event.TransactionId, | ||
| Relayer: event.Relayer, | ||
| Raw: event.Raw, | ||
| } | ||
| err = g.handleProofDisputedLog(ctx, v1Event) | ||
| if err != nil { | ||
| return fmt.Errorf("could not handle request: %w", err) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("listener failed: %w", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting event conversion logic
The event conversion logic in runChainIndexerV2 could be moved to separate helper methods to improve maintainability and reduce the cyclomatic complexity.
Consider creating helper methods for event conversions:
func convertToV1BridgeRequestedEvent(v2Event *fastbridgev2.FastBridgeV2BridgeRequested) *fastbridge.FastBridgeBridgeRequested {
return &fastbridge.FastBridgeBridgeRequested{
TransactionId: v2Event.TransactionId,
Sender: v2Event.Sender,
Request: v2Event.Request,
DestChainId: v2Event.DestChainId,
OriginToken: v2Event.OriginToken,
DestToken: v2Event.DestToken,
DestAmount: v2Event.DestAmount,
SendChainGas: v2Event.SendChainGas,
Raw: v2Event.Raw,
}
}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
services/rfq/guard/service/handlers.go (2)
248-253: Consider improving error handling for RFQ address retrievalThe error messages could be more specific. When the RFQ address is nil, include the chain ID in the error message to aid debugging.
if rfqAddr == nil { - return false, errors.New("rfq address v1 is nil") + return false, fmt.Errorf("rfq address v1 is nil for chain ID %d", bridgeRequest.Transaction.DestChainId) }
357-380: Document validation checks and address TODO commentThe validation logic would benefit from:
- Comments explaining the purpose of each field comparison
- Resolution of the TODO comment about exhaustiveness
Consider:
- Adding field-level validation comments
- Documenting any additional checks that might be needed
- Removing the TODO once confirmed exhaustive
func relayMatchesBridgeRequest(details relayDetails, bridgeRequest *guarddb.BridgeRequest) bool { - // TODO: is this exhaustive? + // Validate transaction identity if details.TransactionID != bridgeRequest.TransactionID { return false } + // Validate amount consistency if details.OriginAmount.Cmp(bridgeRequest.Transaction.OriginAmount) != 0 { return false } if details.DestAmount.Cmp(bridgeRequest.Transaction.DestAmount) != 0 { return false } + // Validate chain and address consistency if details.OriginChainID != bridgeRequest.Transaction.OriginChainId { return false } if details.To != bridgeRequest.Transaction.DestRecipient { return false } + // Validate token addresses if details.OriginToken != bridgeRequest.Transaction.OriginToken { return false } if details.DestToken != bridgeRequest.Transaction.DestToken { return false } return true }
| RFQAddressV1 *string `yaml:"rfq_address_v1"` | ||
| // RFQAddressV2 is the rfq bridge contract address. | ||
| RFQAddressV2 string `yaml:"rfq_address_v2"` |
There was a problem hiding this comment.
@dwasse I agree with the Rabbit that highlighting the fields as optional/required would be helpful here.
* relabeling & using v2 events/parsing for both versions * lint * typo * addtl nil check on deref * prove multicalled relays [SLT-422] * runChainListener tweak. Parse proof relabel * removing fastbridgev1 BridgeRequested & refs * test probe - revert guard listener label * restore legacy listener "guard" label. Establish "guardV2" for new version * poke to rerun w/o explorer check * removing dev comments
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
services/rfq/guard/guardconfig/config.go (2)
37-39: Consider standardizing the naming convention for RFQ addressesWhile the optional vs. required nature is well documented, consider renaming
RFQAddresstoRFQAddressV2for consistency and clarity, making the versioning explicit throughout the codebase.// RFQAddressV1 is the legacy V1 rfq bridge contract address. OPTIONAL. Only populate if also guarding a deprecated V1 contract. RFQAddressV1 *string `yaml:"rfq_address_v1"` -// RFQAddress is the current/latest rfq bridge contract address. REQUIRED. -RFQAddress string `yaml:"rfq_address"` +// RFQAddressV2 is the current/latest rfq bridge contract address. REQUIRED. +RFQAddressV2 string `yaml:"rfq_address_v2"`
95-110: Standardize error messages in getter methodsThe error messages in GetRFQAddressV1 and GetRFQAddressV2 use slightly different formats. Consider standardizing them:
-return nil, fmt.Errorf("v1 chain config not found for chain %d", chainID) +return nil, fmt.Errorf("chain config not found for chain %d when getting RFQ V1 address", chainID) -return "", fmt.Errorf("v2 chain config not found for chain %d", chainID) +return "", fmt.Errorf("chain config not found for chain %d when getting RFQ V2 address", chainID)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 96-98: services/rfq/guard/guardconfig/config.go#L96-L98
Added lines #L96 - L98 were not covered by tests
[warning] 100-101: services/rfq/guard/guardconfig/config.go#L100-L101
Added lines #L100 - L101 were not covered by tests
[warning] 105-105: services/rfq/guard/guardconfig/config.go#L105
Added line #L105 was not covered by testsservices/rfq/guard/guarddb/base/model.go (2)
54-60: Add test coverage for FromPendingProven.The function changes look correct, but test coverage is missing for these modifications. Consider adding test cases that verify:
- Correct conversion of FastBridgeAddress to hex
- Proper handling of zero addresses
- Edge cases with invalid addresses
Would you like me to help generate comprehensive test cases for this function?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 54-60: services/rfq/guard/guarddb/base/model.go#L54-L60
Added lines #L54 - L60 were not covered by tests
77-83: Consider adding address validation.While the conversion logic is correct, consider adding validation for the FastBridgeAddress to ensure it's not empty or invalid before conversion.
func (p PendingProvenModel) ToPendingProven() (*guarddb.PendingProven, error) { + if len(p.FastBridgeAddress) == 0 { + return nil, fmt.Errorf("fast bridge address cannot be empty") + } + if !common.IsHexAddress(p.FastBridgeAddress) { + return nil, fmt.Errorf("invalid fast bridge address format: %s", p.FastBridgeAddress) + } txID, err := hexutil.Decode(p.TransactionID)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 77-83: services/rfq/guard/guarddb/base/model.go#L77-L83
Added lines #L77 - L83 were not covered by testsservices/rfq/guard/service/guard.go (1)
205-224: Enhance error handling in chain indexersThe error messages could be more descriptive to aid debugging:
- Include the RFQ address in error messages
- Consider wrapping errors with additional context
-return fmt.Errorf("could not runChainIndexer chain indexer for chain %d [v2]: %w", chainID, err) +return fmt.Errorf("could not run chain indexer for chain %d at address %s [v2]: %w", + chainID, g.listenersV2[chainID].Address().String(), err)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 205-212: services/rfq/guard/service/guard.go#L205-L212
Added lines #L205 - L212 were not covered by tests
[warning] 214-217: services/rfq/guard/service/guard.go#L214-L217
Added lines #L214 - L217 were not covered by tests
[warning] 224-224: services/rfq/guard/service/guard.go#L224
Added line #L224 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
services/rfq/guard/guardconfig/config.go(4 hunks)services/rfq/guard/guarddb/base/model.go(5 hunks)services/rfq/guard/guarddb/db.go(2 hunks)services/rfq/guard/service/guard.go(8 hunks)services/rfq/guard/service/handlers.go(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/rfq/guard/guarddb/db.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/rfq/guard/guardconfig/config.go
[warning] 71-71: services/rfq/guard/guardconfig/config.go#L71
Added line #L71 was not covered by tests
[warning] 75-75: services/rfq/guard/guardconfig/config.go#L75
Added line #L75 was not covered by tests
[warning] 77-79: services/rfq/guard/guardconfig/config.go#L77-L79
Added lines #L77 - L79 were not covered by tests
[warning] 81-82: services/rfq/guard/guardconfig/config.go#L81-L82
Added lines #L81 - L82 were not covered by tests
[warning] 96-98: services/rfq/guard/guardconfig/config.go#L96-L98
Added lines #L96 - L98 were not covered by tests
[warning] 100-101: services/rfq/guard/guardconfig/config.go#L100-L101
Added lines #L100 - L101 were not covered by tests
[warning] 105-105: services/rfq/guard/guardconfig/config.go#L105
Added line #L105 was not covered by tests
[warning] 136-136: services/rfq/guard/guardconfig/config.go#L136
Added line #L136 was not covered by tests
services/rfq/guard/guarddb/base/model.go
[warning] 54-60: services/rfq/guard/guarddb/base/model.go#L54-L60
Added lines #L54 - L60 were not covered by tests
[warning] 77-83: services/rfq/guard/guarddb/base/model.go#L77-L83
Added lines #L77 - L83 were not covered by tests
services/rfq/guard/service/guard.go
[warning] 60-64: services/rfq/guard/service/guard.go#L60-L64
Added lines #L60 - L64 were not covered by tests
[warning] 67-90: services/rfq/guard/service/guard.go#L67-L90
Added lines #L67 - L90 were not covered by tests
[warning] 94-94: services/rfq/guard/service/guard.go#L94
Added line #L94 was not covered by tests
[warning] 102-102: services/rfq/guard/service/guard.go#L102
Added line #L102 was not covered by tests
[warning] 114-115: services/rfq/guard/service/guard.go#L114-L115
Added lines #L114 - L115 were not covered by tests
[warning] 133-142: services/rfq/guard/service/guard.go#L133-L142
Added lines #L133 - L142 were not covered by tests
[warning] 205-212: services/rfq/guard/service/guard.go#L205-L212
Added lines #L205 - L212 were not covered by tests
[warning] 214-217: services/rfq/guard/service/guard.go#L214-L217
Added lines #L214 - L217 were not covered by tests
[warning] 224-224: services/rfq/guard/service/guard.go#L224
Added line #L224 was not covered by tests
[warning] 240-240: services/rfq/guard/service/guard.go#L240
Added line #L240 was not covered by tests
[warning] 268-268: services/rfq/guard/service/guard.go#L268
Added line #L268 was not covered by tests
[warning] 273-273: services/rfq/guard/service/guard.go#L273
Added line #L273 was not covered by tests
[warning] 278-278: services/rfq/guard/service/guard.go#L278
Added line #L278 was not covered by tests
[warning] 309-314: services/rfq/guard/service/guard.go#L309-L314
Added lines #L309 - L314 were not covered by tests
services/rfq/guard/service/handlers.go
[warning] 81-87: services/rfq/guard/service/handlers.go#L81-L87
Added lines #L81 - L87 were not covered by tests
[warning] 152-155: services/rfq/guard/service/handlers.go#L152-L155
Added lines #L152 - L155 were not covered by tests
[warning] 171-180: services/rfq/guard/service/handlers.go#L171-L180
Added lines #L171 - L180 were not covered by tests
[warning] 182-182: services/rfq/guard/service/handlers.go#L182
Added line #L182 was not covered by tests
[warning] 184-186: services/rfq/guard/service/handlers.go#L184-L186
Added lines #L184 - L186 were not covered by tests
[warning] 188-188: services/rfq/guard/service/handlers.go#L188
Added line #L188 was not covered by tests
🔇 Additional comments (9)
services/rfq/guard/guarddb/base/model.go (2)
171-171: Verify compatibility with existing code after v2 migration.
The change from fastbridge to fastbridgev2 contract interface is a breaking change. Please ensure:
- All callers are updated to use the v2 interface
- Migration path is available for existing data
- Backward compatibility is maintained if needed
#!/bin/bash
# Search for usages of the old fastbridge interface
rg "fastbridge\.IFastBridgeBridgeTransaction" --type go
# Search for direct usages of this model
rg "BridgeRequestModel" --type go -A 512-12: LGTM! Consider adding tests for the new field.
The addition of FastBridgeAddress field and its corresponding import are well-structured and properly documented. However, test coverage should be added for this new field.
Also applies to: 39-40
services/rfq/guard/service/guard.go (3)
20-20: LGTM: Clean separation of v1 and v2 components
The struct changes properly maintain backward compatibility while introducing v2 support through separate maps for contracts and listeners.
Also applies to: 33-42
Line range hint 239-287: LGTM: Clean event handling implementation
The implementation using the v2 parser aligns with the team's strategy for easier v1 deprecation, as discussed in previous reviews.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 268-268: services/rfq/guard/service/guard.go#L268
Added line #L268 was not covered by tests
[warning] 273-273: services/rfq/guard/service/guard.go#L273
Added line #L273 was not covered by tests
[warning] 278-278: services/rfq/guard/service/guard.go#L278
Added line #L278 was not covered by tests
309-315: Add test coverage for isV2Address helper
This critical helper function lacks test coverage. Consider adding tests for:
- True case: matching v2 address
- False case: non-matching address
- Error handling case
#!/bin/bash
# Check for existing tests of isV2Address
rg -l "Test.*isV2Address" -g "*_test.go"Would you like me to help create test cases for this helper function?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 309-314: services/rfq/guard/service/guard.go#L309-L314
Added lines #L309 - L314 were not covered by tests
services/rfq/guard/service/handlers.go (4)
17-17: LGTM! V2 contract integration looks correct
The changes properly integrate the FastBridgeV2 contract with appropriate type updates and initialization.
Also applies to: 25-68
171-209: Implement the suggested refactoring from previous review
The previous review comment about refactoring disputeV1 and disputeV2 to eliminate code duplication is still valid and should be implemented.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 171-180: services/rfq/guard/service/handlers.go#L171-L180
Added lines #L171 - L180 were not covered by tests
[warning] 182-182: services/rfq/guard/service/handlers.go#L182
Added line #L182 was not covered by tests
[warning] 184-186: services/rfq/guard/service/handlers.go#L184-L186
Added lines #L184 - L186 were not covered by tests
[warning] 188-188: services/rfq/guard/service/handlers.go#L188
Added line #L188 was not covered by tests
286-294: LGTM! Comprehensive relay details capture
The relayDetails struct properly captures all necessary fields for validation, maintaining consistency with the bridge request structure.
316-339: Verify completeness of relay matching validation
The TODO comment raises a valid concern. Consider additional validation checks such as:
- Gas price/limit validation
- Timestamp bounds
- Chain-specific parameters
#!/bin/bash
# Search for additional validation parameters in the codebase
rg -A 5 "validate|verify" --type go| RFQAddressV1: chainCfg.RFQAddressV1, | ||
| RFQAddress: chainCfg.RFQAddress, | ||
| Confirmations: chainCfg.FinalityConfirmations, | ||
| } |
There was a problem hiding this comment.
Add test coverage for guard config creation
The NewGuardConfigFromRelayer function's handling of the new RFQAddressV1 field is untested. Consider adding test cases to verify:
- Correct mapping of both V1 and V2 addresses
- Proper handling of nil V1 addresses
Would you like me to help create test cases for the config creation logic?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 136-136: services/rfq/guard/guardconfig/config.go#L136
Added line #L136 was not covered by tests
| addrV1, err := c.GetRFQAddressV1(chainID) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get rfq address: %w", err) | ||
| return fmt.Errorf("could not get v1 rfq address: %w", err) | ||
| } | ||
| if !common.IsHexAddress(addr) { | ||
| return fmt.Errorf("invalid rfq address: %s", addr) | ||
| if addrV1 != nil && !common.IsHexAddress(*addrV1) { | ||
| return fmt.Errorf("invalid rfq v1 address: %s", *addrV1) | ||
| } | ||
| addrV2, err := c.GetRFQAddressV2(chainID) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get v2 rfq address: %w", err) | ||
| } | ||
| if !common.IsHexAddress(addrV2) { | ||
| return fmt.Errorf("invalid rfq v2 address: %s", addrV2) |
There was a problem hiding this comment.
Add test coverage for address validation logic
The validation logic for both V1 and V2 addresses lacks test coverage. Consider adding test cases for:
- Valid V1 and V2 addresses
- Missing V2 address
- Invalid hex addresses
- Nil V1 address
Would you like me to help create comprehensive test cases for the validation logic?
Additionally, consider making error messages more descriptive:
-return fmt.Errorf("could not get v1 rfq address: %w", err)
+return fmt.Errorf("failed to retrieve RFQ V1 address for chain %d: %w", chainID, err)
-return fmt.Errorf("invalid rfq v1 address: %s", *addrV1)
+return fmt.Errorf("invalid RFQ V1 address format for chain %d: %s", chainID, *addrV1)
-return fmt.Errorf("could not get v2 rfq address: %w", err)
+return fmt.Errorf("failed to retrieve RFQ V2 address for chain %d: %w", chainID, err)
-return fmt.Errorf("invalid rfq v2 address: %s", addrV2)
+return fmt.Errorf("invalid RFQ V2 address format for chain %d: %s", chainID, addrV2)📝 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.
| addrV1, err := c.GetRFQAddressV1(chainID) | |
| if err != nil { | |
| return fmt.Errorf("could not get rfq address: %w", err) | |
| return fmt.Errorf("could not get v1 rfq address: %w", err) | |
| } | |
| if !common.IsHexAddress(addr) { | |
| return fmt.Errorf("invalid rfq address: %s", addr) | |
| if addrV1 != nil && !common.IsHexAddress(*addrV1) { | |
| return fmt.Errorf("invalid rfq v1 address: %s", *addrV1) | |
| } | |
| addrV2, err := c.GetRFQAddressV2(chainID) | |
| if err != nil { | |
| return fmt.Errorf("could not get v2 rfq address: %w", err) | |
| } | |
| if !common.IsHexAddress(addrV2) { | |
| return fmt.Errorf("invalid rfq v2 address: %s", addrV2) | |
| addrV1, err := c.GetRFQAddressV1(chainID) | |
| if err != nil { | |
| return fmt.Errorf("failed to retrieve RFQ V1 address for chain %d: %w", chainID, err) | |
| } | |
| if addrV1 != nil && !common.IsHexAddress(*addrV1) { | |
| return fmt.Errorf("invalid RFQ V1 address format for chain %d: %s", chainID, *addrV1) | |
| } | |
| addrV2, err := c.GetRFQAddressV2(chainID) | |
| if err != nil { | |
| return fmt.Errorf("failed to retrieve RFQ V2 address for chain %d: %w", chainID, err) | |
| } | |
| if !common.IsHexAddress(addrV2) { | |
| return fmt.Errorf("invalid RFQ V2 address format for chain %d: %s", chainID, addrV2) |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 71-71: services/rfq/guard/guardconfig/config.go#L71
Added line #L71 was not covered by tests
[warning] 75-75: services/rfq/guard/guardconfig/config.go#L75
Added line #L75 was not covered by tests
[warning] 77-79: services/rfq/guard/guardconfig/config.go#L77-L79
Added lines #L77 - L79 were not covered by tests
[warning] 81-82: services/rfq/guard/guardconfig/config.go#L81-L82
Added lines #L81 - L82 were not covered by tests
| contractsV1 := make(map[int]*fastbridge.FastBridgeRef) | ||
| contractsV2 := make(map[int]*fastbridgev2.FastBridgeV2Ref) | ||
| listenersV1 := make(map[int]listener.ContractListener) | ||
| listenersV2 := make(map[int]listener.ContractListener) | ||
|
|
||
| // setup chain listeners | ||
| contracts := make(map[int]*fastbridge.FastBridgeRef) | ||
| for chainID := range cfg.GetChains() { | ||
| rfqAddr, err := cfg.GetRFQAddress(chainID) | ||
| // setup v1 | ||
| rfqAddrV1, err := cfg.GetRFQAddressV1(chainID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get rfq address: %w", err) | ||
| } | ||
| if rfqAddrV1 != nil { | ||
| chainClient, err := omniClient.GetChainClient(ctx, chainID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get chain client: %w", err) | ||
| } | ||
| contract, err := fastbridge.NewFastBridgeRef(common.HexToAddress(*rfqAddrV1), chainClient) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create fast bridge contract: %w", err) | ||
| } | ||
| startBlock, err := contract.DeployBlock(&bind.CallOpts{Context: ctx}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get deploy block: %w", err) | ||
| } | ||
| chainListener, err := listener.NewChainListener(chainClient, store, common.HexToAddress(*rfqAddrV1), uint64(startBlock.Int64()), metricHandler, listener.WithName("guard")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get chain listener: %w", err) | ||
| } | ||
| listenersV1[chainID] = chainListener | ||
| contractsV1[chainID] = contract | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for v2 address and consider refactoring initialization
- The v2 initialization lacks a null check that exists for v1, which could cause issues if v2 contracts aren't deployed.
- There's significant duplication in the initialization logic between v1 and v2.
Apply this pattern for v2:
rfqAddrV2, err := cfg.GetRFQAddressV2(chainID)
if err != nil {
return nil, fmt.Errorf("could not get rfq address: %w", err)
}
+if rfqAddrV2 == "" {
+ continue
+}Consider extracting the common initialization logic into a helper function:
func initializeContract[T any](
ctx context.Context,
chainID int,
addr string,
createContract func(common.Address) (T, error),
listenerName string,
) (T, listener.ContractListener, error) {
// Common initialization logic here
}Also applies to: 93-115
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 60-64: services/rfq/guard/service/guard.go#L60-L64
Added lines #L60 - L64 were not covered by tests
[warning] 67-90: services/rfq/guard/service/guard.go#L67-L90
Added lines #L67 - L90 were not covered by tests
| contractsV1 := make(map[int]*fastbridge.FastBridgeRef) | ||
| contractsV2 := make(map[int]*fastbridgev2.FastBridgeV2Ref) | ||
| listenersV1 := make(map[int]listener.ContractListener) | ||
| listenersV2 := make(map[int]listener.ContractListener) | ||
|
|
||
| // setup chain listeners | ||
| contracts := make(map[int]*fastbridge.FastBridgeRef) | ||
| for chainID := range cfg.GetChains() { | ||
| rfqAddr, err := cfg.GetRFQAddress(chainID) | ||
| // setup v1 | ||
| rfqAddrV1, err := cfg.GetRFQAddressV1(chainID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get rfq address: %w", err) | ||
| } | ||
| if rfqAddrV1 != nil { | ||
| chainClient, err := omniClient.GetChainClient(ctx, chainID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get chain client: %w", err) | ||
| } | ||
| contract, err := fastbridge.NewFastBridgeRef(common.HexToAddress(*rfqAddrV1), chainClient) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create fast bridge contract: %w", err) | ||
| } | ||
| startBlock, err := contract.DeployBlock(&bind.CallOpts{Context: ctx}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get deploy block: %w", err) | ||
| } | ||
| chainListener, err := listener.NewChainListener(chainClient, store, common.HexToAddress(*rfqAddrV1), uint64(startBlock.Int64()), metricHandler, listener.WithName("guard")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get chain listener: %w", err) | ||
| } | ||
| listenersV1[chainID] = chainListener | ||
| contractsV1[chainID] = contract | ||
| } | ||
|
|
||
| // setup v2 | ||
| rfqAddrV2, err := cfg.GetRFQAddressV2(chainID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get rfq address: %w", err) | ||
| } | ||
| chainClient, err := omniClient.GetChainClient(ctx, chainID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get chain client: %w", err) | ||
| } | ||
|
|
||
| contract, err := fastbridge.NewFastBridgeRef(common.HexToAddress(rfqAddr), chainClient) | ||
| contract, err := fastbridgev2.NewFastBridgeV2Ref(common.HexToAddress(rfqAddrV2), chainClient) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create fast bridge contract: %w", err) | ||
| } | ||
| startBlock, err := contract.DeployBlock(&bind.CallOpts{Context: ctx}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get deploy block: %w", err) | ||
| } | ||
| chainListener, err := listener.NewChainListener(chainClient, store, common.HexToAddress(rfqAddr), uint64(startBlock.Int64()), metricHandler, listener.WithName("guard")) | ||
| chainListener, err := listener.NewChainListener(chainClient, store, common.HexToAddress(rfqAddrV2), uint64(startBlock.Int64()), metricHandler, listener.WithName("guardV2")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get chain listener: %w", err) | ||
| } | ||
| chainListeners[chainID] = chainListener | ||
|
|
||
| // setup FastBridge contract on this chain | ||
| contracts[chainID], err = fastbridge.NewFastBridgeRef(common.HexToAddress(rfqAddr), chainClient) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create bridge contract: %w", err) | ||
| } | ||
| listenersV2[chainID] = chainListener | ||
| contractsV2[chainID] = contract |
There was a problem hiding this comment.
💡 Codebase verification
Missing test coverage for guard service contract initialization confirmed
The search results confirm that while there are extensive tests in the RFQ service, there are no dedicated test files for the guard service contract initialization logic. The only references to guard are in e2e tests, which don't specifically cover the contract initialization scenarios mentioned in the review.
- No test file exists at
services/rfq/guard/service/guard_test.go - The initialization logic for both V1 and V2 contracts requires unit tests to cover:
- Contract creation with valid addresses
- Null/missing address handling for V1
- Error scenarios during contract and listener creation
- Deploy block retrieval validation
🔗 Analysis chain
Add test coverage for contract initialization
The new initialization logic for v1 and v2 contracts lacks test coverage. Consider adding tests for:
- Successful initialization of both versions
- Handling of missing/null addresses
- Error cases during contract creation
Would you like me to help create comprehensive test cases for the initialization logic?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check existing test coverage
rg -l "TestNewGuard|TestGuard.*Init" -g "*_test.go"
Length of output: 42
Script:
#!/bin/bash
# Search for any test files in the guard service directory
fd "_test.go" services/rfq/guard/
# Search for any existing test cases related to guard initialization
rg -A 5 "func Test.*Guard" services/rfq/guard/
# Check for mock implementations that might be used in tests
fd "mock" services/rfq/guard/
# Look for test utilities or helper functions
rg "func.*test" services/rfq/guard/
Length of output: 150
Script:
#!/bin/bash
# Try a broader search for test files in the entire rfq service
fd "_test.go" services/rfq/
# Look for any test files that might reference guard service
rg -l "guard" -g "*_test.go" services/rfq/
# Check if there are any test utilities in the parent directories
fd "testutil" services/rfq/
# Look for any existing contract-related tests
rg -l "contract.*test" -g "*_test.go" services/rfq/
Length of output: 1988
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 60-64: services/rfq/guard/service/guard.go#L60-L64
Added lines #L60 - L64 were not covered by tests
[warning] 67-90: services/rfq/guard/service/guard.go#L67-L90
Added lines #L67 - L90 were not covered by tests
[warning] 94-94: services/rfq/guard/service/guard.go#L94
Added line #L94 was not covered by tests
[warning] 102-102: services/rfq/guard/service/guard.go#L102
Added line #L102 was not covered by tests
[warning] 114-115: services/rfq/guard/service/guard.go#L114-L115
Added lines #L114 - L115 were not covered by tests
| Origin: uint32(chainID), | ||
| RelayerAddress: event.Relayer, | ||
| FastBridgeAddress: event.Raw.Address, | ||
| TransactionID: event.TransactionId, | ||
| TxHash: event.TransactionHash, | ||
| Status: guarddb.ProveCalled, | ||
| BlockNumber: event.Raw.BlockNumber, |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now provide a final response:
Add test coverage for handleProofProvidedLog and PendingProven initialization
The code lacks test coverage for critical functionality:
- No test file exists for
handleProofProvidedLogfunction in the guard service - No tests verify the initialization of
PendingProvenstruct fields - The function handles important V2 contract compatibility fields like
FastBridgeAddress
Recommended test cases:
- Test initialization of all PendingProven fields from event data
- Verify correct status transitions (ProveCalled)
- Test error handling scenarios
- Validate FastBridgeAddress and other V2-specific fields
🔗 Analysis chain
Add test coverage for PendingProven struct initialization
The new fields, particularly FastBridgeAddress, are crucial for V2 contract compatibility but lack test coverage.
Would you like me to help generate comprehensive test cases for these changes?
Let me search for test coverage of the PendingProven struct initialization.
Let me check the test coverage more thoroughly by looking at the test files in the guard service directory.
Let me try one more search with the corrected path pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check existing test coverage for PendingProven
rg -l "TestHandleProofProvidedLog|TestPendingProven" --type go
Length of output: 515830
Script:
#!/bin/bash
# Search for test files related to PendingProven struct
rg -l "PendingProven" --type go services/rfq/guard/
# Search for test files that test handleProofProvidedLog
rg -l "TestHandleProofProvidedLog|handleProofProvidedLog.*test" --type go services/rfq/guard/
Length of output: 471
Script:
#!/bin/bash
# Search for test files in guard service directory
fd -e go -e _test.go$ services/rfq/guard/service/
# Look for any test functions that might test handleProofProvidedLog
rg -A 5 "func.*Test.*Proof" services/rfq/guard/service/
Length of output: 525
Script:
#!/bin/bash
# Search for test files in guard service directory with corrected pattern
fd . services/rfq/guard/service/ -t f -e go | grep "_test.go"
# Look at the implementation of handleProofProvidedLog to understand test coverage
rg -A 10 "handleProofProvidedLog" services/rfq/guard/service/
Length of output: 1893
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 81-87: services/rfq/guard/service/handlers.go#L81-L87
Added lines #L81 - L87 were not covered by tests
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
These changes enhance the system's flexibility and reliability in managing bridge contracts and their associated processes.