proposed mods for feat/guard-v2 [SLT-422] #3364
Conversation
parodime
requested review from
dwasse,
trajan0x and
aureliusbtc
as code owners
WalkthroughThe changes in this pull request involve significant updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
parodime
requested review from
ChiTimesChi
and removed request for
dwasse,
aureliusbtc and
trajan0x
Deploying sanguine-fe with
|
| Latest commit: |
62320d0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d81e211c.sanguine-fe.pages.dev |
| Branch Preview URL: | https://guard-v2-updts.sanguine-fe.pages.dev |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/guard-v2 #3364 +/- ##
=======================================================
+ Coverage 24.58774% 30.51556% +5.92782%
=======================================================
Files 207 456 +249
Lines 13523 30142 +16619
Branches 82 82
=======================================================
+ Hits 3325 9198 +5873
- Misses 9910 20087 +10177
- Partials 288 857 +569
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
services/rfq/guard/guardconfig/config.go (1)
37-40: LGTM! Consider enhancing documentation.The consolidation of RFQ addresses and clear documentation of V1's legacy status looks good.
Consider adding a comment about version compatibility:
// RFQAddress is the current/latest rfq bridge contract address. REQUIRED. +// This address is used for V2 and future version compatibility. RFQAddress string `yaml:"rfq_address"`services/rfq/guard/service/guard.go (1)
239-241: Minor improvements needed in formatting and error messages.
- Remove the unnecessary newline before the function declaration.
- Make error messages in event handlers more specific to their respective events.
Apply these changes:
- func (g Guard) runChainIndexer(ctx context.Context, chainID int, listeners map[int]listener.ContractListener) (err error) { +func (g Guard) runChainIndexer(ctx context.Context, chainID int, listeners map[int]listener.ContractListener) (err error) {Update error messages in handlers:
- return fmt.Errorf("could not handle request: %w", err) + return fmt.Errorf("could not handle bridge request event: %w", err) - return fmt.Errorf("could not handle request: %w", err) + return fmt.Errorf("could not handle proof provided event: %w", err) - return fmt.Errorf("could not handle request: %w", err) + return fmt.Errorf("could not handle proof disputed event: %w", err)Also applies to: 272-285
🧰 Tools
🪛 GitHub Check: Lint (services/rfq)
[failure] 239-239:
unnecessary leading newline (whitespace)services/rfq/guard/service/handlers.go (2)
Line range hint
254-273: Potential issue with parsing V1 events using V2 parser inisProveValidParseThe function
isProveValidParsecurrently usesfastbridgev2.NewParserirrespective of whetherrfqContractAddrcorresponds to a V1 or V2 contract. IfrfqContractAddris a V1 address, using the V2 parser may result in incorrect parsing or runtime errors. Consider adding logic to instantiate the appropriate parser based on the contract version.Apply this diff to instantiate the correct parser:
func (g *Guard) isProveValidParse(ctx context.Context, proven *guarddb.PendingProven, bridgeRequest *guarddb.BridgeRequest, receipt *types.Receipt, rfqContractAddr string) (bool, error) { span := trace.SpanFromContext(ctx) + var parser EventParser + if g.isV2Address(int(bridgeRequest.Transaction.OriginChainId), proven.FastBridgeAddress) { + parser, err := fastbridgev2.NewParser(common.HexToAddress(rfqContractAddr)) + if err != nil { + return false, fmt.Errorf("could not get V2 parser: %w", err) + } + } else { + parser, err := fastbridge.NewParser(common.HexToAddress(rfqContractAddr)) + if err != nil { + return false, fmt.Errorf("could not get V1 parser: %w", err) + } + }This ensures that the correct parser is used based on the contract version.
268-269: Optimize address conversion by cachingrfqContractAddrconversion outside the loopCurrently,
common.HexToAddress(rfqContractAddr)is called inside the loop for each log entry. To improve performance, consider convertingrfqContractAddrto anAddressonce before the loop and reusing it within the loop.Apply this diff:
func (g *Guard) isProveValidParse(ctx context.Context, proven *guarddb.PendingProven, bridgeRequest *guarddb.BridgeRequest, receipt *types.Receipt, rfqContractAddr string) (bool, error) { span := trace.SpanFromContext(ctx) + rfqAddress := common.HexToAddress(rfqContractAddr) for _, log := range receipt.Logs { _, parsedEvent, ok := parser.ParseEvent(*log) if !ok { continue } - if log.Address != common.HexToAddress(rfqContractAddr) { + if log.Address != rfqAddress { span.AddEvent(fmt.Sprintf("log address %s does not match rfq address %s", log.Address.Hex(), rfqContractAddr)) continue }This change reduces redundant computations inside the loop.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
services/rfq/guard/guardconfig/config.go(5 hunks)services/rfq/guard/service/guard.go(6 hunks)services/rfq/guard/service/handlers.go(6 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint (services/rfq)
services/rfq/guard/service/guard.go
[failure] 239-239:
unnecessary leading newline (whitespace)
🔇 Additional comments (8)
services/rfq/guard/guardconfig/config.go (2)
99-99: LGTM! Error messages are clear and consistent.
The updated error messages properly differentiate between V1 and V2 addresses, improving debuggability.
Also applies to: 108-108, 110-110
137-137: Verify relayer config structure compatibility.
The change looks correct, but we should verify that chainCfg.RFQAddress exists in the relayer config structure.
✅ Verification successful
RFQ address field compatibility verified
The RFQAddress field exists in the relayer config structure at services/rfq/relayer/relconfig/config.go with the correct YAML tag rfq_address, confirming the compatibility of the configuration structures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that relayer config has the RFQAddress field
# Search for the RFQAddress field definition in the relayer config
rg -t go "RFQAddress.*\`yaml:\"rfq_address\"\`" --glob "services/rfq/relayer/**"
Length of output: 159
services/rfq/guard/service/guard.go (3)
85-85: LGTM: Clear listener naming convention.
The renaming of listeners to "guardListenerLegacy" and "guardListener" provides better clarity in distinguishing between V1 and V2 implementations.
Also applies to: 110-110
Line range hint 213-226: LGTM: Effective consolidation of chain indexer logic.
The unified approach using a single runChainIndexer method with version-specific listener maps reduces code duplication while maintaining clear error reporting for both V1 and V2 implementations.
Line range hint 241-290: LGTM: Robust event handling with proper security measures.
The implementation includes:
- Proper handling of unknown events with warning logs
- Comprehensive event type checking
- Efficient tracing implementation with proper span management
- Clear separation of concerns in event handling
services/rfq/guard/service/handlers.go (3)
Line range hint 26-51: Update to FastBridgeV2 types in handleBridgeRequestedLog is correct
The function handleBridgeRequestedLog correctly updates the parameter to use *fastbridgev2.FastBridgeV2BridgeRequested. The usage of the new type's fields appears appropriate, and the function logic remains consistent.
Line range hint 71-96: Update to FastBridgeV2 types in handleProofProvidedLog is correct
The function handleProofProvidedLog correctly updates the parameter to use *fastbridgev2.FastBridgeV2BridgeProofProvided. The adjustments are appropriate, and the function continues to handle events as expected.
Line range hint 98-111: Update to FastBridgeV2 types in handleProofDisputedLog is correct
The function handleProofDisputedLog correctly updates the parameter to use *fastbridgev2.FastBridgeV2BridgeProofDisputed. The changes are consistent with the transition to the new contract types.
services/rfq/guard/service/guard.go
Outdated
| @@ -82,7 +82,7 @@ func NewGuard(ctx context.Context, metricHandler metrics.Handler, cfg guardconfi | |||
| 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")) | |||
| chainListener, err := listener.NewChainListener(chainClient, store, common.HexToAddress(*rfqAddrV1), uint64(startBlock.Int64()), metricHandler, listener.WithName("guardListenerLegacy")) | |||
services/rfq/guard/service/guard.go
Outdated
| @@ -107,7 +107,7 @@ func NewGuard(ctx context.Context, metricHandler metrics.Handler, cfg guardconfi | |||
| 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")) | |||
| chainListener, err := listener.NewChainListener(chainClient, store, common.HexToAddress(rfqAddrV2), uint64(startBlock.Int64()), metricHandler, listener.WithName("guardListener")) | |||
There was a problem hiding this comment.
Might want to do the versioned naming here (e.g. guardV2)
There was a problem hiding this comment.
my thought was this would eventually be "the" listener (ie: the only listener) and will be simpler to remove the "Legacy" one when we're done with it
There was a problem hiding this comment.
@parodime the chain listener is using its name to store its unique "latest handled block" in the DB:
sanguine/ethergo/listener/db/store.go
Lines 29 to 59 in 7c5c4f1
So for me it seems that we may want to preserve the old listener name when a new one is added, hence guard and guardV2 suggestions (something that preserves the existing name; and something that will help to preserve the name whenever there's a v3).
| } | ||
|
|
||
| func (g *Guard) isProveValidV2(ctx context.Context, proven *guarddb.PendingProven, bridgeRequest *guarddb.BridgeRequest, receipt *types.Receipt) (bool, error) { | ||
| func (g *Guard) isProveValidParse(ctx context.Context, proven *guarddb.PendingProven, bridgeRequest *guarddb.BridgeRequest, receipt *types.Receipt, rfqContractAddr string) (bool, error) { |
There was a problem hiding this comment.
Maybe rename this one? isProveValid and isProveValidParse feels a bit confusing for reviewing, not sure about maintaining.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
services/rfq/guard/service/guard.go (1)
239-240: Consider passing chainListener directly instead of the map.The
listenersmap parameter could be simplified by directly passing thechainListenersince we're already selecting it immediately in the method.-func (g Guard) runChainIndexer(ctx context.Context, chainID int, listeners map[int]listener.ContractListener) (err error) { - chainListener := listeners[chainID] +func (g Guard) runChainIndexer(ctx context.Context, chainID int, chainListener listener.ContractListener) (err error) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
services/rfq/guard/guardconfig/config.go(5 hunks)services/rfq/guard/service/guard.go(6 hunks)services/rfq/guard/service/handlers.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/rfq/guard/service/handlers.go
🔇 Additional comments (6)
services/rfq/guard/guardconfig/config.go (3)
73-83: Previous comment about incorrect error messages is still valid.
The error messages still need to be fixed as per the previous review comment.
99-99: LGTM! Error message clarification.
The error message now clearly indicates this is for the V1 chain config.
137-137: LGTM! Consistent field mapping.
The field mapping correctly reflects the updated ChainConfig structure.
services/rfq/guard/service/guard.go (3)
Line range hint 213-226: LGTM! Clean consolidation of chain indexer logic.
The consolidation of V1 and V2 indexer logic improves maintainability while maintaining clear error messages with version information.
271-284: Verify V1 event compatibility with V2 parser.
The implementation uses the V2 parser for all events. Please ensure that V1 events are properly handled and mapped to V2 event structures to prevent potential runtime issues.
85-85: Ensure consistent versioning in listener names and plan DB migration.
The listener naming changes raise two concerns:
- The V2 listener name lacks version suffix ("guardListener" vs "guardListenerLegacy"), which could cause confusion.
- Changing existing listener names might require DB migration for historical records.
Run this script to check for potential impacts:
Also applies to: 110-110
| return "", fmt.Errorf("v2 chain config not found for chain %d", chainID) | ||
| } | ||
| return chainCfg.RFQAddressV2, nil | ||
| return chainCfg.RFQAddress, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming GetRFQAddressV2.
Since this method now returns the main RFQAddress and not a V2-specific address, the method name is misleading.
Consider this change:
-func (c Config) GetRFQAddressV2(chainID int) (string, error) {
+func (c Config) GetRFQAddress(chainID int) (string, error) {Committable suggestion skipped: line range outside the PR's diff.
| // 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"` | ||
| // RFQAddressV2 is the rfq bridge contract address. | ||
| RFQAddressV2 string `yaml:"rfq_address_v2"` | ||
| // RFQAddress is the current/latest rfq bridge contract address. REQUIRED. | ||
| RFQAddress string `yaml:"rfq_address"` |
There was a problem hiding this comment.
💡 Codebase verification
Update required: RFQAddressV2 references need to be modified
The consolidation of RFQAddress fields requires corresponding updates to the codebase. Several files still contain references to RFQAddressV2:
services/rfq/guard/guardconfig/config.go: 3 references including the methodGetRFQAddressV2services/rfq/guard/service/handlers.go: 1 reference in bridge request handlingservices/rfq/guard/service/guard.go: 2 references in contract address retrieval
These references should be updated to use the new consolidated RFQAddress field to maintain consistency with the configuration changes.
🔗 Analysis chain
LGTM! Verify impact of RFQAddress consolidation.
The struct changes improve clarity by marking V1 as legacy/optional and consolidating the primary RFQAddress field. This aligns well with the V2 transition.
Let's verify no direct references to the old RFQAddressV2 field remain:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to RFQAddressV2
rg "RFQAddressV2" --type go
Length of output: 620
Script:
#!/bin/bash
# Let's check the changes in the PR to see if these references are being updated
git diff HEAD^ HEAD -- services/rfq/guard/guardconfig/config.go services/rfq/guard/service/handlers.go services/rfq/guard/service/guard.go
Length of output: 923
Script:
#!/bin/bash
# Let's check if there are any other files in the PR that might be updating these references
git diff --name-only HEAD^ HEAD | grep -i rfq
Length of output: 88
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
services/rfq/guard/service/guard.go (3)
85-85: Consider adding tests for the new V1 listener initializationTo ensure the listener is initialized correctly and handles events as expected, consider adding unit tests for the V1 listener setup.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 85-85: services/rfq/guard/service/guard.go#L85
Added line #L85 was not covered by tests
110-110: Consider adding tests for the new V2 listener initializationAdding unit tests for the V2 listener setup will help ensure that it initializes correctly and processes events as intended.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 110-110: services/rfq/guard/service/guard.go#L110
Added line #L110 was not covered by tests
269-269: Consider adding unit tests for new event handlersThe functions
handleBridgeRequestedLog,handleProofProvidedLog, andhandleProofDisputedLogare key to processing chain events. Adding unit tests for these functions would improve test coverage and help ensure their correctness.Also applies to: 274-274, 279-279
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 269-269: services/rfq/guard/service/guard.go#L269
Added line #L269 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
services/rfq/guard/service/guard.go(6 hunks)services/rfq/guard/service/handlers.go(7 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/rfq/guard/service/guard.go
[warning] 85-85: services/rfq/guard/service/guard.go#L85
Added line #L85 was not covered by tests
[warning] 110-110: services/rfq/guard/service/guard.go#L110
Added line #L110 was not covered by tests
[warning] 269-269: services/rfq/guard/service/guard.go#L269
Added line #L269 was not covered by tests
[warning] 274-274: services/rfq/guard/service/guard.go#L274
Added line #L274 was not covered by tests
[warning] 279-279: services/rfq/guard/service/guard.go#L279
Added line #L279 was not covered by tests
services/rfq/guard/service/handlers.go
[warning] 26-26: services/rfq/guard/service/handlers.go#L26
Added line #L26 was not covered by tests
[warning] 71-71: services/rfq/guard/service/handlers.go#L71
Added line #L71 was not covered by tests
[warning] 98-98: services/rfq/guard/service/handlers.go#L98
Added line #L98 was not covered by tests
[warning] 214-214: services/rfq/guard/service/handlers.go#L214
Added line #L214 was not covered by tests
[warning] 229-230: services/rfq/guard/service/handlers.go#L229-L230
Added lines #L229 - L230 were not covered by tests
[warning] 232-232: services/rfq/guard/service/handlers.go#L232
Added line #L232 was not covered by tests
[warning] 234-234: services/rfq/guard/service/handlers.go#L234
Added line #L234 was not covered by tests
[warning] 237-237: services/rfq/guard/service/handlers.go#L237
Added line #L237 was not covered by tests
[warning] 239-242: services/rfq/guard/service/handlers.go#L239-L242
Added lines #L239 - L242 were not covered by tests
[warning] 244-244: services/rfq/guard/service/handlers.go#L244
Added line #L244 was not covered by tests
[warning] 247-247: services/rfq/guard/service/handlers.go#L247
Added line #L247 was not covered by tests
[warning] 254-254: services/rfq/guard/service/handlers.go#L254
Added line #L254 was not covered by tests
[warning] 260-260: services/rfq/guard/service/handlers.go#L260
Added line #L260 was not covered by tests
[warning] 271-272: services/rfq/guard/service/handlers.go#L271-L272
Added lines #L271 - L272 were not covered by tests
🔇 Additional comments (3)
services/rfq/guard/service/handlers.go (2)
229-244: LGTM: RFQ address handling improvements
The code now properly handles potential nil pointer dereference for V1 addresses and maintains consistent error handling between V1 and V2 paths.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 229-230: services/rfq/guard/service/handlers.go#L229-L230
Added lines #L229 - L230 were not covered by tests
[warning] 232-232: services/rfq/guard/service/handlers.go#L232
Added line #L232 was not covered by tests
[warning] 234-234: services/rfq/guard/service/handlers.go#L234
Added line #L234 was not covered by tests
[warning] 237-237: services/rfq/guard/service/handlers.go#L237
Added line #L237 was not covered by tests
[warning] 239-242: services/rfq/guard/service/handlers.go#L239-L242
Added lines #L239 - L242 were not covered by tests
[warning] 244-244: services/rfq/guard/service/handlers.go#L244
Added line #L244 was not covered by tests
Line range hint 321-322: Address TODO comment regarding validation completeness
The TODO comment questions whether the validation checks are exhaustive. This uncertainty should be resolved before proceeding.
Let's analyze the validation completeness:
Would you like help in reviewing the BridgeTransaction struct to ensure all relevant fields are validated?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 26-26: services/rfq/guard/service/handlers.go#L26
Added line #L26 was not covered by tests
services/rfq/guard/service/guard.go (1)
239-239: Consolidated runChainIndexer function improves maintainability
Great job refactoring the chain indexer functions into a single method. This enhances code maintainability and reduces redundancy.
| var valid bool | ||
| valid, err = g.parseProvenTransaction(ctx, proven, bridgeRequest, receipt, rfqContractAddr) | ||
|
|
||
| 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 parser: %w", err) | ||
| return false, fmt.Errorf("could not parse proven transaction for validity: %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.To, | ||
| OriginToken: event.OriginToken, | ||
| DestToken: event.DestToken, | ||
| } | ||
|
|
||
| return relayMatchesBridgeRequest(details, bridgeRequest), nil | ||
| } | ||
|
|
||
| return false, nil | ||
| return valid, nil |
There was a problem hiding this comment.
Add test coverage for proof validation logic
The refactored proof validation logic is well-structured but lacks test coverage. This is critical functionality that should be thoroughly tested.
Would you like assistance in generating test cases for the proof validation scenarios?
Also applies to: 257-303
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 247-247: services/rfq/guard/service/handlers.go#L247
Added line #L247 was not covered by tests
[warning] 254-254: services/rfq/guard/service/handlers.go#L254
Added line #L254 was not covered by tests
| @@ -219,7 +219,7 @@ | |||
| } | |||
|
|
|||
| group.Go(func() error { | |||
| err := g.runChainIndexerV2(ctx, chainID) | |||
| err := g.runChainIndexer(ctx, chainID, g.listenersV2[chainID]) | |||
There was a problem hiding this comment.
Check for nil listener before starting V2 chain indexer
The code starts the V2 chain indexer without verifying if the listener exists. If g.listenersV2[chainID] is nil, this could result in a runtime error. Consider adding a check to ensure the listener is initialized before starting the indexer.
You can add a nil check before starting the chain indexer:
if g.listenersV2[chainID] != nil {
group.Go(func() error {
err := g.runChainIndexer(ctx, chainID, g.listenersV2[chainID])
if err != nil {
return fmt.Errorf("could not runChainIndexer chain indexer for chain %d [v2]: %w", chainID, err)
}
return nil
})
}| Raw: event.Raw, | ||
| } | ||
| err = g.handleProofDisputedLog(ctx, v1Event) | ||
| err = g.handleProofDisputedLog(ctx, event) |
There was a problem hiding this comment.
Inconsistent parameters in handleProofDisputedLog call
Unlike handleBridgeRequestedLog and handleProofProvidedLog, the handleProofDisputedLog function is called without the chainID parameter. If chainID is required in handleProofDisputedLog, consider adding it to maintain consistency and ensure correct processing.
Apply this diff to include chainID in the function call:
- err = g.handleProofDisputedLog(ctx, event)
+ err = g.handleProofDisputedLog(ctx, event, chainID)Also, update the handleProofDisputedLog function signature to accept chainID if necessary.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 279-279: services/rfq/guard/service/guard.go#L279
Added line #L279 was not covered by tests
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)
Missing test coverage for critical transaction parsing logic
The verification reveals a significant gap in test coverage:
- No test file exists for
handlers.goin the guard service directory- No direct test coverage found for
parseProvenTransactionfunction- While FastBridgeV2 contract has basic status tests, they don't cover event parsing
- E2E tests don't explicitly test the BridgeRelayed event parsing logic
Critical areas requiring test coverage:
- Event parsing validation
- Relayer address matching
- Bridge request matching logic
- Error handling scenarios
- Edge cases with invalid or malformed events
The presence of mock implementations (
fastbridgemockv2) indicates the infrastructure for testing exists, but it's not being utilized for this critical parsing logic.🔗 Analysis chain
Line range hint
256-302: Add test coverage and verify parsing logicThe transaction parsing logic is critical for validating bridge operations. Please ensure:
- Test coverage for different event scenarios
- Verification of parsing behavior with actual V2 events
Would you like assistance in generating comprehensive test cases for the parsing logic?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of V2 event test cases # Expected: Find test files containing V2 event test cases echo "Searching for V2 event test files..." fd -e go test handlers echo "Searching for V2 event test cases..." rg -A 5 "TestFastBridgeV2"Length of output: 313
Script:
#!/bin/bash # Let's try a more comprehensive search for test files and V2-related tests # Search for test files in the services/rfq directory echo "=== Searching for test files in rfq service ===" fd -e go test$ . services/rfq # Search for V2-related test cases echo -e "\n=== Searching for V2-related test cases ===" rg -l "fastbridgev2.*Test" services/rfq # Look for specific parseProvenTransaction test coverage echo -e "\n=== Searching for parseProvenTransaction tests ===" rg -A 5 "parseProvenTransaction.*Test" services/rfq # Check for any BridgeRelayed event test cases echo -e "\n=== Searching for BridgeRelayed event tests ===" rg -A 5 "BridgeRelayed.*Test|Test.*BridgeRelayed" services/rfqLength of output: 630
Script:
#!/bin/bash # Let's verify if the test file exists in the same directory as handlers.go # First, find the exact path of handlers.go echo "=== Finding handlers.go location ===" fd handlers.go services/rfq # Then check for test files in the same directory echo -e "\n=== Looking for test files in the same directory ===" fd -e go . "$(dirname $(fd handlers.go services/rfq))" # Search for any test functions in these files echo -e "\n=== Searching for test functions in handler tests ===" rg -B 1 "^func Test" "$(dirname $(fd handlers.go services/rfq))" # Look for any bridge-related tests with broader search echo -e "\n=== Searching for bridge-related tests ===" rg -l "bridge.*test|test.*bridge" services/rfq -iLength of output: 2440
Script:
#!/bin/bash # Let's examine the content of relevant test files # Check guard service test files echo "=== Checking guard service test files ===" fd -e go test$ . services/rfq/guard/service # Examine e2e tests that might cover this functionality echo -e "\n=== Examining e2e test content ===" rg -A 10 "BridgeRelayed|parseProvenTransaction" services/rfq/e2e/rfq_test.go # Check FastBridgeV2 related test content echo -e "\n=== Examining FastBridgeV2 test content ===" cat services/rfq/contracts/fastbridgev2/status_test.go # Look for mock implementations that might be used in tests echo -e "\n=== Checking mock implementations ===" ls -la services/rfq/contracts/testcontracts/fastbridgemockv2/Length of output: 2712
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
services/rfq/guard/guarddb/base/model.go(2 hunks)services/rfq/guard/guarddb/db.go(2 hunks)services/rfq/guard/service/handlers.go(8 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/rfq/guard/service/handlers.go
[warning] 25-25: services/rfq/guard/service/handlers.go#L25
Added line #L25 was not covered by tests
[warning] 70-70: services/rfq/guard/service/handlers.go#L70
Added line #L70 was not covered by tests
[warning] 97-97: services/rfq/guard/service/handlers.go#L97
Added line #L97 was not covered by tests
[warning] 213-213: services/rfq/guard/service/handlers.go#L213
Added line #L213 was not covered by tests
[warning] 228-229: services/rfq/guard/service/handlers.go#L228-L229
Added lines #L228 - L229 were not covered by tests
[warning] 231-231: services/rfq/guard/service/handlers.go#L231
Added line #L231 was not covered by tests
[warning] 233-233: services/rfq/guard/service/handlers.go#L233
Added line #L233 was not covered by tests
[warning] 236-236: services/rfq/guard/service/handlers.go#L236
Added line #L236 was not covered by tests
[warning] 238-241: services/rfq/guard/service/handlers.go#L238-L241
Added lines #L238 - L241 were not covered by tests
[warning] 243-243: services/rfq/guard/service/handlers.go#L243
Added line #L243 was not covered by tests
[warning] 246-246: services/rfq/guard/service/handlers.go#L246
Added line #L246 was not covered by tests
[warning] 253-253: services/rfq/guard/service/handlers.go#L253
Added line #L253 was not covered by tests
[warning] 259-259: services/rfq/guard/service/handlers.go#L259
Added line #L259 was not covered by tests
[warning] 270-271: services/rfq/guard/service/handlers.go#L270-L271
Added lines #L270 - L271 were not covered by tests
🔇 Additional comments (7)
services/rfq/guard/guarddb/db.go (2)
10-10: LGTM: Import update aligns with V2 migration.
The import path change from fastbridge to fastbridgev2 is consistent with the PR objectives to utilize FbV2 events.
56-56: Verify the impact of BridgeRequest.Transaction type change.
The field type change from fastbridge.IFastBridgeBridgeTransaction to fastbridgev2.IFastBridgeBridgeTransaction is a breaking change that could affect code reading from or writing to this struct.
Let's verify the usage of this type across the codebase:
✅ Verification successful
The type change has been properly propagated across the codebase
The search results confirm that:
- All references to the old
fastbridge.IFastBridgeBridgeTransactiontype have been removed - The new
fastbridgev2.IFastBridgeBridgeTransactiontype is consistently used in all relevant files:- services/rfq/guard/guarddb/db.go
- services/rfq/guard/guarddb/base/model.go
- services/rfq/guard/service/handlers.go
- services/rfq/relayer/reldb/db.go
- services/rfq/relayer/reldb/base/model.go
- services/rfq/relayer/service/handlers.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of BridgeRequest struct to ensure compatibility
# with the new Transaction field type
# Search for BridgeRequest struct usage
echo "=== Searching for BridgeRequest struct usage ==="
rg -A 3 "BridgeRequest\s*{|BridgeRequest\s*\)" --type go
# Search for any remaining references to the old type
echo -e "\n=== Checking for any remaining references to old type ==="
rg "fastbridge\.IFastBridgeBridgeTransaction" --type go
Length of output: 490
Script:
#!/bin/bash
# Let's fix the regex and try again with separate commands
# Search for BridgeRequest struct usage
echo "=== Searching for BridgeRequest struct usage ==="
rg "BridgeRequest\s*\{" --type go -A 3
rg "BridgeRequest\s*\(" --type go -A 3
# Search for any references to both old and new types
echo -e "\n=== Checking for references to both old and new types ==="
rg "fastbridge\.IFastBridgeBridgeTransaction" --type go
rg "fastbridgev2\.IFastBridgeBridgeTransaction" --type go
# Search for imports of both packages
echo -e "\n=== Checking imports of both packages ==="
rg '".*fastbridge"' --type go
rg '".*fastbridgev2"' --type go
Length of output: 17509
services/rfq/guard/guarddb/base/model.go (2)
12-12: LGTM: Import update aligns with V2 transition.
The import change from fastbridge to fastbridgev2 is consistent with the PR's objective of transitioning to V2 events.
Line range hint 171-184: Verify backward compatibility with existing transactions.
While the update to fastbridgev2.IFastBridgeBridgeTransaction aligns with the V2 transition, we should ensure this change maintains compatibility with existing transactions in the database.
Let's verify the transaction structure compatibility:
Consider adding a version field or migration strategy if the V2 transaction structure differs significantly from V1.
services/rfq/guard/service/handlers.go (3)
39-46: LGTM: Clean transition to V2 contract with proper error handling
The implementation includes proper retry mechanism for RPC calls and maintains clean error wrapping.
228-243: LGTM: Proper nil pointer protection added
The implementation now safely handles potential nil V1 addresses while maintaining clean V2 address handling.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 228-229: services/rfq/guard/service/handlers.go#L228-L229
Added lines #L228 - L229 were not covered by tests
[warning] 231-231: services/rfq/guard/service/handlers.go#L231
Added line #L231 was not covered by tests
[warning] 233-233: services/rfq/guard/service/handlers.go#L233
Added line #L233 was not covered by tests
[warning] 236-236: services/rfq/guard/service/handlers.go#L236
Added line #L236 was not covered by tests
[warning] 238-241: services/rfq/guard/service/handlers.go#L238-L241
Added lines #L238 - L241 were not covered by tests
[warning] 243-243: services/rfq/guard/service/handlers.go#L243
Added line #L243 was not covered by tests
246-253: LGTM: Improved code organization with clear naming
The separation of concerns between validation and parsing improves code clarity and maintainability.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 246-246: services/rfq/guard/service/handlers.go#L246
Added line #L246 was not covered by tests
[warning] 253-253: services/rfq/guard/service/handlers.go#L253
Added line #L253 was not covered by tests
| @@ -23,7 +22,7 @@ | |||
|
|
|||
| var maxRPCRetryTime = 15 * time.Second | |||
|
|
|||
| func (g *Guard) handleBridgeRequestedLog(parentCtx context.Context, req *fastbridge.FastBridgeBridgeRequested, chainID int) (err error) { | |||
| func (g *Guard) handleBridgeRequestedLog(parentCtx context.Context, req *fastbridgev2.FastBridgeV2BridgeRequested, chainID int) (err error) { | |||
There was a problem hiding this comment.
Add test coverage for V2 event handlers
The transition to FastBridgeV2 event types is a critical change that requires thorough testing to ensure correct event handling and maintain system reliability.
Would you like assistance in generating comprehensive test cases for these event handlers?
Also applies to: 70-70, 97-97
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 25-25: services/rfq/guard/service/handlers.go#L25
Added line #L25 was not covered by tests
parodime
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
services/rfq/guard/service/guard.go (2)
Line range hint
213-226: Add test coverage for chain indexer functionalityThe static analysis indicates that the chain indexer implementation lacks test coverage. Consider adding tests to verify:
- Successful initialization of both V1 and V2 indexers
- Error handling scenarios
- Proper event processing
Would you like help creating test cases for these scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 222-222: services/rfq/guard/service/guard.go#L222
Added line #L222 was not covered by tests
Line range hint
239-290: Consider refactoring complex event handling logicThe function is marked with
nolint:cyclopdue to its complexity. Consider breaking down the event handling into separate methods to improve maintainability and testability.func (g Guard) runChainIndexer(ctx context.Context, chainID int, chainListener listener.ContractListener) (err error) { // ... initialization code ... - err = chainListener.Listen(ctx, func(parentCtx context.Context, log types.Log) (err error) { + return chainListener.Listen(ctx, g.handleChainEvent(chainID)) + } + + func (g Guard) handleChainEvent(chainID int) func(context.Context, types.Log) error { + return func(parentCtx context.Context, log types.Log) (err error) { // ... event handling logic ... }) - if err != nil { - return fmt.Errorf("listener failed: %w", err) - } - return nil }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 269-269: services/rfq/guard/service/guard.go#L269
Added line #L269 was not covered by tests
[warning] 274-274: services/rfq/guard/service/guard.go#L274
Added line #L274 was not covered by tests
[warning] 279-279: services/rfq/guard/service/guard.go#L279
Added line #L279 was not covered by testsethergo/backends/anvil/anvil.go (2)
83-85: Good addition of architecture compatibility warning!The warning about non-linux/amd64 architecture compatibility is crucial for developers. However, consider making this more visible by:
- Adding it to the package documentation
- Including it in the function's godoc comment
Consider moving this warning to the function's documentation:
// NewAnvilBackend creates a test anvil backend. + // Note: This will silently fail if executing from a non-linux/amd64 architecture. + // You will see "connection" issues downstream as the container is not created. + // To solve - install qemu to enable docker emulation: apt-get install qemu qemu-user-static qemu-user binfmt-support // nolint: cyclop func NewAnvilBackend(ctx context.Context, t *testing.T, args *OptionBuilder) (*Backend, error) {
Line range hint
249-251: Improved error handling for context cancellation!Good improvement in handling context cancellation errors during container operations. However, consider extracting the error check into a helper function for reusability.
Consider creating a helper function:
+func isExpectedError(err error) bool { + return err == nil || errors.Is(err, context.Canceled) +} func setupOtterscan(...) (string, error) { ... - if err != nil && !errors.Is(err, context.Canceled) { + if err != nil && !isExpectedError(err) { return "", fmt.Errorf("failed to run otterscan container: %w", err) } ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
ethergo/backends/anvil/anvil.go(1 hunks)services/rfq/guard/service/guard.go(5 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/rfq/guard/service/guard.go
[warning] 213-213: services/rfq/guard/service/guard.go#L213
Added line #L213 was not covered by tests
[warning] 222-222: services/rfq/guard/service/guard.go#L222
Added line #L222 was not covered by tests
[warning] 239-239: services/rfq/guard/service/guard.go#L239
Added line #L239 was not covered by tests
[warning] 269-269: services/rfq/guard/service/guard.go#L269
Added line #L269 was not covered by tests
[warning] 274-274: services/rfq/guard/service/guard.go#L274
Added line #L274 was not covered by tests
[warning] 279-279: services/rfq/guard/service/guard.go#L279
Added line #L279 was not covered by tests
🔇 Additional comments (2)
services/rfq/guard/service/guard.go (2)
110-110: LGTM: Appropriate versioned naming for the chain listener
The change to use "guardV2" as the listener name is a good practice as it preserves the DB state for existing listeners while clearly differentiating between versions.
Line range hint 213-226: LGTM: Improved code maintainability through consolidation
The consolidation of chain indexer logic into a single method with direct chainListener parameter improves maintainability by reducing code duplication.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 222-222: services/rfq/guard/service/guard.go#L222
Added line #L222 was not covered by tests
| func (g Guard) runChainIndexerV2(ctx context.Context, chainID int) (err error) { | ||
| chainListener := g.listenersV2[chainID] | ||
|
|
||
| func (g Guard) runChainIndexer(ctx context.Context, chainID int, chainListener listener.ContractListener) (err error) { |
There was a problem hiding this comment.
Add defensive nil check for chainListener
The function should verify that chainListener is not nil before proceeding to prevent potential runtime panics.
func (g Guard) runChainIndexer(ctx context.Context, chainID int, chainListener listener.ContractListener) (err error) {
+ if chainListener == nil {
+ return fmt.Errorf("chain listener is nil for chain %d", chainID)
+ }
parser, err := fastbridgev2.NewParser(chainListener.Address())📝 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 (g Guard) runChainIndexer(ctx context.Context, chainID int, chainListener listener.ContractListener) (err error) { | |
| func (g Guard) runChainIndexer(ctx context.Context, chainID int, chainListener listener.ContractListener) (err error) { | |
| if chainListener == nil { | |
| return fmt.Errorf("chain listener is nil for chain %d", chainID) | |
| } | |
| parser, err := fastbridgev2.NewParser(chainListener.Address()) |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 239-239: services/rfq/guard/service/guard.go#L239
Added line #L239 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
services/rfq/guard/service/guard.go (2)
119-119: Remove redundant commentThe empty comment line doesn't provide any additional context or value.
-//
Line range hint
214-228: Improve error handling and test coverage
- The error messages for both V1 and V2 indexers are identical, making it difficult to distinguish between them in logs
- The new consolidated chain indexer implementation lacks test coverage
Consider:
- Adding version-specific context to error messages
- Adding unit tests for the new implementation
- return fmt.Errorf("could not runChainIndexer chain indexer for chain %d [v1]: %w", chainID, err) + return fmt.Errorf("could not run V1 chain indexer for chain %d: %w", chainID, err) - return fmt.Errorf("could not runChainIndexer chain indexer for chain %d [v2]: %w", chainID, err) + return fmt.Errorf("could not run V2 chain indexer for chain %d: %w", chainID, err)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 223-223: services/rfq/guard/service/guard.go#L223
Added line #L223 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
services/rfq/guard/service/guard.go(6 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/rfq/guard/service/guard.go
[warning] 110-110: services/rfq/guard/service/guard.go#L110
Added line #L110 was not covered by tests
[warning] 214-214: services/rfq/guard/service/guard.go#L214
Added line #L214 was not covered by tests
[warning] 223-223: services/rfq/guard/service/guard.go#L223
Added line #L223 was not covered by tests
[warning] 240-240: services/rfq/guard/service/guard.go#L240
Added line #L240 was not covered by tests
[warning] 270-270: services/rfq/guard/service/guard.go#L270
Added line #L270 was not covered by tests
[warning] 275-275: services/rfq/guard/service/guard.go#L275
Added line #L275 was not covered by tests
[warning] 280-280: services/rfq/guard/service/guard.go#L280
Added line #L280 was not covered by tests
🔇 Additional comments (1)
services/rfq/guard/service/guard.go (1)
110-110: Consider DB migration strategy for listener name change
Changing the listener name from "guard" to "guardV2" will create a new entry in the DB for storing the latest handled block. This means the V2 listener will start from the deployment block instead of continuing from the last processed block. Consider:
- Adding a DB migration script to handle the transition
- Documenting the migration steps in the deployment guide
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 110-110: services/rfq/guard/service/guard.go#L110
Added line #L110 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
services/rfq/guard/service/guard.go(5 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/rfq/guard/service/guard.go
[warning] 110-110: services/rfq/guard/service/guard.go#L110
Added line #L110 was not covered by tests
[warning] 213-213: services/rfq/guard/service/guard.go#L213
Added line #L213 was not covered by tests
[warning] 222-222: services/rfq/guard/service/guard.go#L222
Added line #L222 was not covered by tests
[warning] 239-239: services/rfq/guard/service/guard.go#L239
Added line #L239 was not covered by tests
[warning] 269-269: services/rfq/guard/service/guard.go#L269
Added line #L269 was not covered by tests
[warning] 274-274: services/rfq/guard/service/guard.go#L274
Added line #L274 was not covered by tests
[warning] 279-279: services/rfq/guard/service/guard.go#L279
Added line #L279 was not covered by tests
🔇 Additional comments (3)
services/rfq/guard/service/guard.go (3)
110-110: LGTM! Good choice on versioned listener naming.
The change to "guardV2" for the V2 listener name is a good practice as it:
- Preserves historical block data in the DB
- Maintains backward compatibility
- Provides a clear upgrade path for future versions
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 110-110: services/rfq/guard/service/guard.go#L110
Added line #L110 was not covered by tests
Line range hint 239-291: Improve error handling and add test coverage.
The consolidated chain indexer is a good improvement, but there are a few areas that need attention:
- Error messages should be more specific to help with debugging
- Test coverage is missing for this critical component
Apply this diff to improve error messages:
func (g Guard) runChainIndexer(ctx context.Context, chainID int, chainListener listener.ContractListener) (err error) {
+ if chainListener == nil {
+ return fmt.Errorf("chain listener is nil for chain %d", chainID)
+ }
parser, err := fastbridgev2.NewParser(chainListener.Address())
if err != nil {
- return fmt.Errorf("could not parse: %w", err)
+ return fmt.Errorf("could not create parser for chain %d at address %s: %w",
+ chainID, chainListener.Address().String(), err)
}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 269-269: services/rfq/guard/service/guard.go#L269
Added line #L269 was not covered by tests
[warning] 274-274: services/rfq/guard/service/guard.go#L274
Added line #L274 was not covered by tests
[warning] 279-279: services/rfq/guard/service/guard.go#L279
Added line #L279 was not covered by tests
Line range hint 213-226: Add defensive nil checks for chain listeners.
While consolidating the chain indexers improves code maintainability, we should add defensive checks to prevent potential runtime panics.
Apply this diff:
group.Go(func() error {
+ if g.listenersV1[chainID] == nil {
+ return fmt.Errorf("v1 listener is nil for chain %d", chainID)
+ }
err := g.runChainIndexer(ctx, chainID, g.listenersV1[chainID])
if err != nil {
return fmt.Errorf("could not runChainIndexer chain indexer for chain %d [v1]: %w", chainID, err)
}
return nil
})
group.Go(func() error {
+ if g.listenersV2[chainID] == nil {
+ return fmt.Errorf("v2 listener is nil for chain %d", chainID)
+ }
err := g.runChainIndexer(ctx, chainID, g.listenersV2[chainID])🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 222-222: services/rfq/guard/service/guard.go#L222
Added line #L222 was not covered by tests
| err = g.handleBridgeRequestedLog(ctx, event, 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) | ||
| err = g.handleProofProvidedLog(ctx, event, 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) | ||
| err = g.handleProofDisputedLog(ctx, event) | ||
| if err != nil { | ||
| return fmt.Errorf("could not handle request: %w", err) | ||
| } |
There was a problem hiding this comment.
Standardize event handler parameters and improve error messages.
The event handling code has two issues:
handleProofDisputedLogis missing thechainIDparameter unlike other handlers- Error messages are generic and don't differentiate between event types
Apply this diff:
case *fastbridgev2.FastBridgeV2BridgeRequested:
err = g.handleBridgeRequestedLog(ctx, event, chainID)
if err != nil {
- return fmt.Errorf("could not handle request: %w", err)
+ return fmt.Errorf("could not handle bridge request event for chain %d: %w", chainID, err)
}
case *fastbridgev2.FastBridgeV2BridgeProofProvided:
err = g.handleProofProvidedLog(ctx, event, chainID)
if err != nil {
- return fmt.Errorf("could not handle request: %w", err)
+ return fmt.Errorf("could not handle proof provided event for chain %d: %w", chainID, err)
}
case *fastbridgev2.FastBridgeV2BridgeProofDisputed:
- err = g.handleProofDisputedLog(ctx, event)
+ err = g.handleProofDisputedLog(ctx, event, chainID)
if err != nil {
- return fmt.Errorf("could not handle request: %w", err)
+ return fmt.Errorf("could not handle proof disputed event for chain %d: %w", chainID, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = g.handleBridgeRequestedLog(ctx, event, 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) | |
| err = g.handleProofProvidedLog(ctx, event, 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) | |
| err = g.handleProofDisputedLog(ctx, event) | |
| if err != nil { | |
| return fmt.Errorf("could not handle request: %w", err) | |
| } | |
| err = g.handleBridgeRequestedLog(ctx, event, chainID) | |
| if err != nil { | |
| return fmt.Errorf("could not handle bridge request event for chain %d: %w", chainID, err) | |
| } | |
| case *fastbridgev2.FastBridgeV2BridgeProofProvided: | |
| err = g.handleProofProvidedLog(ctx, event, chainID) | |
| if err != nil { | |
| return fmt.Errorf("could not handle proof provided event for chain %d: %w", chainID, err) | |
| } | |
| case *fastbridgev2.FastBridgeV2BridgeProofDisputed: | |
| err = g.handleProofDisputedLog(ctx, event, chainID) | |
| if err != nil { | |
| return fmt.Errorf("could not handle proof disputed event for chain %d: %w", chainID, err) | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 269-269: services/rfq/guard/service/guard.go#L269
Added line #L269 was not covered by tests
[warning] 274-274: services/rfq/guard/service/guard.go#L274
Added line #L274 was not covered by tests
[warning] 279-279: services/rfq/guard/service/guard.go#L279
Added line #L279 was not covered by tests
minor relabeling / highlighting
modifying handlers to use FbV2 events/parsing for both versions
Summary by CodeRabbit
New Features
RFQAddressin the configuration, replacing the deprecatedRFQAddressV2.Bug Fixes
Refactor
fastbridgev2package for better compatibility.Chores