feat(rfq-relayer): wait for finality before proving #3062
Conversation
|
Warning Rate limit exceeded@dwasse has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 3 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce new configuration fields, Changes
Possibly related issues
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
|
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- services/rfq/relayer/relconfig/config.go (1 hunks)
- services/rfq/relayer/relconfig/getters.go (1 hunks)
- services/rfq/relayer/service/handlers.go (1 hunks)
- services/rfq/relayer/service/statushandler.go (3 hunks)
Additional comments not posted (4)
services/rfq/relayer/service/statushandler.go (2)
58-59: LGTM!The addition of the
cfgfield to theQuoteRequestHandlerstruct is a good practice for managing configuration settings within the handler.The code changes are approved.
94-94: LGTM!The initialization of the
cfgfield in therequestToHandlerfunction ensures that the handler has access to the relayer configuration settings.The code changes are approved.
services/rfq/relayer/relconfig/config.go (1)
77-78: LGTM!The addition of the
ProveConfirmationsfield to theChainConfigstruct enhances the configuration capabilities by allowing for more granular control over the confirmation process.The code changes are approved.
services/rfq/relayer/service/handlers.go (1)
381-401: LGTM!The modifications to the
handleRelayCompletedmethod enhance its functionality by ensuring that sufficient confirmations have been received before proceeding with the proof submission. This change improves the reliability of the proof submission process.The code changes are approved.
| func (c Config) GetProveConfirmations(chainID int) (value uint64, err error) { | ||
| rawValue, err := c.getChainConfigValue(chainID, "ProveConfirmations") | ||
| if err != nil { | ||
| return value, err | ||
| } | ||
|
|
||
| value, ok := rawValue.(uint64) | ||
| if !ok { | ||
| return value, fmt.Errorf("failed to cast ProveConfirmations to int") | ||
| } | ||
| return value, nil | ||
| } |
There was a problem hiding this comment.
Add unit tests for the new function.
The function should include unit tests to ensure its correctness.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Correct the error message in the type assertion failure.
The error message should mention uint64 instead of int.
- return value, fmt.Errorf("failed to cast ProveConfirmations to int")
+ return value, fmt.Errorf("failed to cast ProveConfirmations to uint64")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 (c Config) GetProveConfirmations(chainID int) (value uint64, err error) { | |
| rawValue, err := c.getChainConfigValue(chainID, "ProveConfirmations") | |
| if err != nil { | |
| return value, err | |
| } | |
| value, ok := rawValue.(uint64) | |
| if !ok { | |
| return value, fmt.Errorf("failed to cast ProveConfirmations to int") | |
| } | |
| return value, nil | |
| } | |
| func (c Config) GetProveConfirmations(chainID int) (value uint64, err error) { | |
| rawValue, err := c.getChainConfigValue(chainID, "ProveConfirmations") | |
| if err != nil { | |
| return value, err | |
| } | |
| value, ok := rawValue.(uint64) | |
| if !ok { | |
| return value, fmt.Errorf("failed to cast ProveConfirmations to uint64") | |
| } | |
| return value, nil | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3062 +/- ##
===================================================
+ Coverage 23.06899% 30.14768% +7.07869%
===================================================
Files 193 439 +246
Lines 11639 28304 +16665
Branches 82 82
===================================================
+ Hits 2685 8533 +5848
- Misses 8712 18980 +10268
- Partials 242 791 +549
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: |
db7f9fd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e2c074f1.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-prove-finality.sanguine-fe.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- services/rfq/guard/guardconfig/config.go (1 hunks)
- services/rfq/guard/service/handlers.go (2 hunks)
- services/rfq/relayer/relconfig/config.go (1 hunks)
- services/rfq/relayer/relconfig/getters.go (1 hunks)
- services/rfq/relayer/service/handlers.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/relconfig/config.go
Additional context used
GitHub Check: Lint (services/rfq)
services/rfq/guard/service/handlers.go
[failure] 277-277:
SA5011: possible nil pointer dereference (staticcheck)
[failure] 286-286:
SA5011(related information): this check suggests that the pointer can be nil (staticcheck)
[failure] 282-282:
SA5011: possible nil pointer dereference (staticcheck)
GitHub Check: codecov/patch
services/rfq/relayer/service/handlers.go
[warning] 381-387: services/rfq/relayer/service/handlers.go#L381-L387
Added lines #L381 - L387 were not covered by tests
[warning] 389-400: services/rfq/relayer/service/handlers.go#L389-L400
Added lines #L389 - L400 were not covered by testsservices/rfq/relayer/relconfig/getters.go
[warning] 221-223: services/rfq/relayer/relconfig/getters.go#L221-L223
Added lines #L221 - L223 were not covered by tests
[warning] 225-226: services/rfq/relayer/relconfig/getters.go#L225-L226
Added lines #L225 - L226 were not covered by tests
[warning] 228-229: services/rfq/relayer/relconfig/getters.go#L228-L229
Added lines #L228 - L229 were not covered by tests
Additional comments not posted (2)
services/rfq/guard/guardconfig/config.go (1)
119-119: LGTM! But verify the function usage in the codebase.The change aligns the confirmation mechanism with finality rather than a simple confirmation count. Ensure that all function calls to
NewGuardConfigFromRelayerare updated accordingly.The code changes are approved.
Run the following script to verify the function usage:
Verification successful
Function usage is consistent with the new logic.
The function
NewGuardConfigFromRelayeris used correctly with the updatedFinalityConfirmationslogic in the following files:
services/rfq/relayer/service/relayer.goservices/rfq/e2e/setup_test.goNo issues were found with the function usage.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `NewGuardConfigFromRelayer` match the new logic. # Test: Search for the function usage. Expect: Only occurrences of the new logic. rg --type go -A 5 $'NewGuardConfigFromRelayer'Length of output: 1557
services/rfq/guard/service/handlers.go (1)
118-126: LGTM! But verify the function usage in the codebase.The change enhances the robustness of the transaction handling by ensuring that only finalized transactions are processed further. Ensure that all function calls to
handleProveCalledare updated accordingly.The code changes are approved.
Run the following script to verify the function usage:
Verification successful
Function usage verified successfully.
The function
handleProveCalledis used in a manner consistent with the new logic introduced. The single call to this function inservices/rfq/guard/service/guard.gohandles errors appropriately, ensuring robustness in transaction handling. No further updates are needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `handleProveCalled` match the new logic. # Test: Search for the function usage. Expect: Only occurrences of the new logic. rg --type go -A 5 $'handleProveCalled'Length of output: 999
| // isFinalized checks if a transaction is finalized versus the configured confirmations threshold. | ||
| func (g *Guard) isFinalized(ctx context.Context, txHash common.Hash, chainID int) (bool, error) { | ||
| span := trace.SpanFromContext(ctx) | ||
|
|
||
| client, err := g.client.GetChainClient(ctx, chainID) | ||
| if err != nil { | ||
| return false, fmt.Errorf("could not get chain client: %w", err) | ||
| } | ||
| receipt, err := client.TransactionReceipt(ctx, txHash) | ||
| if err != nil { | ||
| return false, fmt.Errorf("could not get receipt: %w", err) | ||
| } | ||
| currentBlockNumber, err := client.BlockNumber(ctx) | ||
| if err != nil { | ||
| return false, fmt.Errorf("could not get block number: %w", err) | ||
| } | ||
|
|
||
| chainCfg, ok := g.cfg.Chains[chainID] | ||
| if !ok { | ||
| return false, fmt.Errorf("could not get chain config for chain %d", chainID) | ||
| } | ||
| threshBlockNumber := uint64(receipt.BlockNumber.Int64()) + chainCfg.Confirmations | ||
| span.SetAttributes( | ||
| attribute.String("tx_hash", txHash.Hex()), | ||
| attribute.Int("chain_id", chainID), | ||
| attribute.Int64("current_block_number", int64(currentBlockNumber)), | ||
| attribute.Int64("receipt_block_number", receipt.BlockNumber.Int64()), | ||
| attribute.Int("confirmations", int(chainCfg.Confirmations)), | ||
| ) | ||
|
|
||
| return receipt != nil && currentBlockNumber >= threshBlockNumber, nil | ||
| } |
There was a problem hiding this comment.
Fix potential nil pointer dereference.
The function isFinalized has a potential issue with nil pointer dereference. Ensure that the receipt is checked for nil before accessing its fields.
Apply this diff to fix the potential nil pointer dereference:
func (g *Guard) isFinalized(ctx context.Context, txHash common.Hash, chainID int) (bool, error) {
span := trace.SpanFromContext(ctx)
client, err := g.client.GetChainClient(ctx, chainID)
if err != nil {
return false, fmt.Errorf("could not get chain client: %w", err)
}
receipt, err := client.TransactionReceipt(ctx, txHash)
if err != nil {
return false, fmt.Errorf("could not get receipt: %w", err)
}
+ if receipt == nil {
+ return false, fmt.Errorf("receipt is nil")
+ }
currentBlockNumber, err := client.BlockNumber(ctx)
if err != nil {
return false, fmt.Errorf("could not get block number: %w", err)
}
chainCfg, ok := g.cfg.Chains[chainID]
if !ok {
return false, fmt.Errorf("could not get chain config for chain %d", chainID)
}
threshBlockNumber := uint64(receipt.BlockNumber.Int64()) + chainCfg.Confirmations
span.SetAttributes(
attribute.String("tx_hash", txHash.Hex()),
attribute.Int("chain_id", chainID),
attribute.Int64("current_block_number", int64(currentBlockNumber)),
attribute.Int64("receipt_block_number", receipt.BlockNumber.Int64()),
attribute.Int("confirmations", int(chainCfg.Confirmations)),
)
return receipt != nil && currentBlockNumber >= threshBlockNumber, 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.
| // isFinalized checks if a transaction is finalized versus the configured confirmations threshold. | |
| func (g *Guard) isFinalized(ctx context.Context, txHash common.Hash, chainID int) (bool, error) { | |
| span := trace.SpanFromContext(ctx) | |
| client, err := g.client.GetChainClient(ctx, chainID) | |
| if err != nil { | |
| return false, fmt.Errorf("could not get chain client: %w", err) | |
| } | |
| receipt, err := client.TransactionReceipt(ctx, txHash) | |
| if err != nil { | |
| return false, fmt.Errorf("could not get receipt: %w", err) | |
| } | |
| currentBlockNumber, err := client.BlockNumber(ctx) | |
| if err != nil { | |
| return false, fmt.Errorf("could not get block number: %w", err) | |
| } | |
| chainCfg, ok := g.cfg.Chains[chainID] | |
| if !ok { | |
| return false, fmt.Errorf("could not get chain config for chain %d", chainID) | |
| } | |
| threshBlockNumber := uint64(receipt.BlockNumber.Int64()) + chainCfg.Confirmations | |
| span.SetAttributes( | |
| attribute.String("tx_hash", txHash.Hex()), | |
| attribute.Int("chain_id", chainID), | |
| attribute.Int64("current_block_number", int64(currentBlockNumber)), | |
| attribute.Int64("receipt_block_number", receipt.BlockNumber.Int64()), | |
| attribute.Int("confirmations", int(chainCfg.Confirmations)), | |
| ) | |
| return receipt != nil && currentBlockNumber >= threshBlockNumber, nil | |
| } | |
| func (g *Guard) isFinalized(ctx context.Context, txHash common.Hash, chainID int) (bool, error) { | |
| span := trace.SpanFromContext(ctx) | |
| client, err := g.client.GetChainClient(ctx, chainID) | |
| if err != nil { | |
| return false, fmt.Errorf("could not get chain client: %w", err) | |
| } | |
| receipt, err := client.TransactionReceipt(ctx, txHash) | |
| if err != nil { | |
| return false, fmt.Errorf("could not get receipt: %w", err) | |
| } | |
| if receipt == nil { | |
| return false, fmt.Errorf("receipt is nil") | |
| } | |
| currentBlockNumber, err := client.BlockNumber(ctx) | |
| if err != nil { | |
| return false, fmt.Errorf("could not get block number: %w", err) | |
| } | |
| chainCfg, ok := g.cfg.Chains[chainID] | |
| if !ok { | |
| return false, fmt.Errorf("could not get chain config for chain %d", chainID) | |
| } | |
| threshBlockNumber := uint64(receipt.BlockNumber.Int64()) + chainCfg.Confirmations | |
| span.SetAttributes( | |
| attribute.String("tx_hash", txHash.Hex()), | |
| attribute.Int("chain_id", chainID), | |
| attribute.Int64("current_block_number", int64(currentBlockNumber)), | |
| attribute.Int64("receipt_block_number", receipt.BlockNumber.Int64()), | |
| attribute.Int("confirmations", int(chainCfg.Confirmations)), | |
| ) | |
| return receipt != nil && currentBlockNumber >= threshBlockNumber, nil | |
| } |
Tools
GitHub Check: Lint (services/rfq)
[failure] 277-277:
SA5011: possible nil pointer dereference (staticcheck)
[failure] 286-286:
SA5011(related information): this check suggests that the pointer can be nil (staticcheck)
[failure] 282-282:
SA5011: possible nil pointer dereference (staticcheck)
| func (q *QuoteRequestHandler) handleRelayCompleted(ctx context.Context, span trace.Span, request reldb.QuoteRequest) (err error) { | ||
| // first, ensure that enough confirmations have passed before proving | ||
| receipt, err := q.Dest.Client.TransactionReceipt(ctx, request.DestTxHash) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get transaction receipt: %w", err) | ||
| } | ||
| currentBlockNumber := q.Origin.LatestBlock() | ||
| proveConfirmations, err := q.cfg.GetFinalityConfirmations(int(q.Dest.ChainID)) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get prove confirmations: %w", err) | ||
| } | ||
| span.SetAttributes( | ||
| attribute.Int("current_block_number", int(currentBlockNumber)), | ||
| attribute.Int("dest_block_number", int(receipt.BlockNumber.Int64())), | ||
| attribute.Int("prove_confirmations", int(proveConfirmations)), | ||
| ) | ||
| if currentBlockNumber < request.BlockNumber+proveConfirmations { | ||
| span.AddEvent("not enough confirmations") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Reminder: Add tests.
The added lines are not covered by tests. Ensure that the new logic is adequately tested.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 381-387: services/rfq/relayer/service/handlers.go#L381-L387
Added lines #L381 - L387 were not covered by tests
[warning] 389-400: services/rfq/relayer/service/handlers.go#L389-L400
Added lines #L389 - L400 were not covered by tests
| func (c Config) GetFinalityConfirmations(chainID int) (value uint64, err error) { | ||
| rawValue, err := c.getChainConfigValue(chainID, "FinalityConfirmations") | ||
| if err != nil { | ||
| return value, err | ||
| } | ||
|
|
||
| value, ok := rawValue.(uint64) | ||
| if !ok { | ||
| return value, fmt.Errorf("failed to cast FinalityConfirmations to int") | ||
| } | ||
| return value, nil | ||
| } |
There was a problem hiding this comment.
Add unit tests for the new function.
The function should include unit tests to ensure its correctness.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 221-223: services/rfq/relayer/relconfig/getters.go#L221-L223
Added lines #L221 - L223 were not covered by tests
[warning] 225-226: services/rfq/relayer/relconfig/getters.go#L225-L226
Added lines #L225 - L226 were not covered by tests
[warning] 228-229: services/rfq/relayer/relconfig/getters.go#L228-L229
Added lines #L228 - L229 were not covered by tests
Correct the error message in the type assertion failure.
The error message should mention uint64 instead of int.
- return value, fmt.Errorf("failed to cast FinalityConfirmations to int")
+ return value, fmt.Errorf("failed to cast FinalityConfirmations to uint64")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 (c Config) GetFinalityConfirmations(chainID int) (value uint64, err error) { | |
| rawValue, err := c.getChainConfigValue(chainID, "FinalityConfirmations") | |
| if err != nil { | |
| return value, err | |
| } | |
| value, ok := rawValue.(uint64) | |
| if !ok { | |
| return value, fmt.Errorf("failed to cast FinalityConfirmations to int") | |
| } | |
| return value, nil | |
| } | |
| func (c Config) GetFinalityConfirmations(chainID int) (value uint64, err error) { | |
| rawValue, err := c.getChainConfigValue(chainID, "FinalityConfirmations") | |
| if err != nil { | |
| return value, err | |
| } | |
| value, ok := rawValue.(uint64) | |
| if !ok { | |
| return value, fmt.Errorf("failed to cast FinalityConfirmations to uint64") | |
| } | |
| return value, nil | |
| } |
Tools
GitHub Check: codecov/patch
[warning] 221-223: services/rfq/relayer/relconfig/getters.go#L221-L223
Added lines #L221 - L223 were not covered by tests
[warning] 225-226: services/rfq/relayer/relconfig/getters.go#L225-L226
Added lines #L225 - L226 were not covered by tests
[warning] 228-229: services/rfq/relayer/relconfig/getters.go#L228-L229
Added lines #L228 - L229 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- services/rfq/guard/guarddb/base/model.go (3 hunks)
- services/rfq/guard/guarddb/db.go (1 hunks)
- services/rfq/guard/service/handlers.go (3 hunks)
Additional comments not posted (7)
services/rfq/guard/guarddb/db.go (1)
67-67: LGTM!The addition of the
BlockNumberfield enhances the struct by allowing it to store the block number associated with a pending transaction.The code changes are approved.
services/rfq/guard/guarddb/base/model.go (3)
45-46: LGTM!The addition of the
BlockNumberfield enhances the struct by allowing it to store the block number associated with an event.The code changes are approved.
57-57: LGTM!The function correctly populates the new
BlockNumberfield from theprovenparameter.The code changes are approved.
79-79: LGTM!The function correctly includes the
BlockNumberwhen converting thePendingProvenModelback to aguarddb.PendingProvenobject.The code changes are approved.
services/rfq/guard/service/handlers.go (3)
85-85: LGTM!The function correctly stores the
BlockNumberfrom the event, enhancing the tracking of transaction states.The code changes are approved.
119-128: LGTM!The function correctly includes a verification step to check if a transaction is finalized before proceeding, ensuring that only finalized transactions are processed further.
The code changes are approved.
257-284: LGTM!The function correctly encapsulates the logic for checking if a transaction is finalized versus the configured confirmations threshold.
The code changes are approved.
| return nil | ||
| } | ||
|
|
||
| // relay has been finalized, it's time to go back to the origin chain and try to prove |
There was a problem hiding this comment.
@dwasse we need to do a check similar to Guard's isProveValid here to handle the case where our relay tx was reorged.
In the event where we submitted a tx, witnessed a BridgeRelayed event, and later on this tx was reorged and ended up being reverted, we need a way for the Relayer to mark the tx as RelayRaceLost here (I'm not sure if we can safely assume that we will index the correct event with another relayer before that).
The current code will still get a valid receipt for DestTxHash, it's just the receipt will not have a corresponding BridgeRelayed event there.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/relayer/service/handlers.go (2 hunks)
Additional comments not posted (8)
services/rfq/relayer/service/handlers.go (8)
381-381: Ensure sufficient confirmations before proving.The function now includes logic to wait for sufficient confirmations before proving the transaction.
The code changes are approved.
382-385: Retrieve relay block number.The function correctly retrieves the relay block number using
getRelayBlockNumber.The code changes are approved.
387-400: Retrieve current block number and prove confirmations.The function correctly retrieves the current block number and prove confirmations and performs the necessary check.
The code changes are approved.
Line range hint
402-419: Submit proof transaction.The function correctly submits the proof transaction if the necessary conditions are met.
The code changes are approved.
422-422: New function: getRelayBlockNumber.The function retrieves the relay block number from the transaction receipt.
The code changes are approved.
423-427: Fetch transaction receipt.The function correctly fetches the transaction receipt for the destination transaction hash.
The code changes are approved.
428-445: Parse transaction receipt logs.The function correctly parses the transaction receipt logs to find the
Relayedevent.The code changes are approved.
448-449: Handle missingRelayedevent.The function correctly handles the case where the
Relayedevent is not found in the transaction receipt logs.The code changes are approved.
| func (q *QuoteRequestHandler) handleRelayCompleted(ctx context.Context, span trace.Span, request reldb.QuoteRequest) (err error) { | ||
| relayBlockNumber, err := q.getRelayBlockNumber(ctx, request) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get relay block number: %w", err) | ||
| } | ||
| currentBlockNumber := q.Origin.LatestBlock() | ||
| proveConfirmations, err := q.cfg.GetFinalityConfirmations(int(q.Dest.ChainID)) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get prove confirmations: %w", err) | ||
| } | ||
|
|
||
| span.SetAttributes( | ||
| attribute.Int("current_block_number", int(currentBlockNumber)), | ||
| attribute.Int("relay_block_number", int(relayBlockNumber)), | ||
| attribute.Int("prove_confirmations", int(proveConfirmations)), | ||
| ) | ||
| if currentBlockNumber < relayBlockNumber+proveConfirmations { | ||
| span.AddEvent("not enough confirmations") | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
LGTM! Ensure tests cover the new logic.
The changes improve the reliability of the proof submission process by ensuring that it only occurs when the blockchain state is sufficiently advanced. Ensure that the new logic is adequately tested.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
| // getRelayBlockNumber fetches the block number of the relay transaction for a given quote request. | ||
| func (q *QuoteRequestHandler) getRelayBlockNumber(ctx context.Context, request reldb.QuoteRequest) (blockNumber uint64, err error) { | ||
| // fetch the transaction receipt for corresponding tx hash | ||
| receipt, err := q.Dest.Client.TransactionReceipt(ctx, request.DestTxHash) | ||
| if err != nil { | ||
| return blockNumber, fmt.Errorf("could not get transaction receipt: %w", err) | ||
| } | ||
| parser, err := fastbridge.NewParser(q.Dest.Bridge.Address()) | ||
| if err != nil { | ||
| return blockNumber, fmt.Errorf("could not create parser: %w", err) | ||
| } | ||
|
|
||
| // check that a Relayed event was emitted | ||
| for _, log := range receipt.Logs { | ||
| if log == nil { | ||
| continue | ||
| } | ||
| _, parsedEvent, ok := parser.ParseEvent(*log) | ||
| if !ok { | ||
| continue | ||
| } | ||
| _, ok = parsedEvent.(*fastbridge.FastBridgeBridgeRelayed) | ||
| if ok { | ||
| return receipt.BlockNumber.Uint64(), nil | ||
| } | ||
| } | ||
|
|
||
| return blockNumber, fmt.Errorf("relayed event not found for dest tx hash: %s", request.DestTxHash.Hex()) | ||
| } |
There was a problem hiding this comment.
LGTM! Consider adding error handling for potential reorgs.
The function encapsulates the logic for fetching the block number of the relay transaction, improving code modularity and readability. Consider adding error handling for potential reorgs to ensure robustness.
Add error handling for potential reorgs.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- services/rfq/relayer/reldb/base/quote.go (1 hunks)
- services/rfq/relayer/service/handlers.go (2 hunks)
Additional context used
GitHub Check: codecov/patch
services/rfq/relayer/service/handlers.go
[warning] 381-381: services/rfq/relayer/service/handlers.go#L381
Added line #L381 was not covered by tests
[warning] 383-384: services/rfq/relayer/service/handlers.go#L383-L384
Added lines #L383 - L384 were not covered by tests
[warning] 391-392: services/rfq/relayer/service/handlers.go#L391-L392
Added lines #L391 - L392 were not covered by tests
[warning] 400-409: services/rfq/relayer/service/handlers.go#L400-L409
Added lines #L400 - L409 were not covered by tests
[warning] 430-432: services/rfq/relayer/service/handlers.go#L430-L432
Added lines #L430 - L432 were not covered by tests
[warning] 441-441: services/rfq/relayer/service/handlers.go#L441
Added line #L441 was not covered by tests
[warning] 443-445: services/rfq/relayer/service/handlers.go#L443-L445
Added lines #L443 - L445 were not covered by tests
[warning] 449-458: services/rfq/relayer/service/handlers.go#L449-L458
Added lines #L449 - L458 were not covered by tests
[warning] 461-463: services/rfq/relayer/service/handlers.go#L461-L463
Added lines #L461 - L463 were not covered by tests
[warning] 465-466: services/rfq/relayer/service/handlers.go#L465-L466
Added lines #L465 - L466 were not covered by tests
GitHub Check: Lint (services/rfq)
services/rfq/relayer/service/handlers.go
[failure] 453-453:
Duplicate words (get) found (dupword)
Additional comments not posted (1)
services/rfq/relayer/reldb/base/quote.go (1)
136-136: Approve addition of new state transition.The inclusion of
RelayRaceLostas a valid state transition inisValidStateTransitionis logical and enhances the system's ability to handle different outcomes of the relay process.However, verify that this new state is integrated correctly across all relevant parts of the system.
Run the following script to verify the handling of the
RelayRaceLoststate:Verification successful
RelayRaceLost state is integrated correctly across the system.
The
RelayRaceLoststate is handled in various parts of the codebase, including updates to quote request statuses and specific handling logic in service handlers and chain indexers. This confirms that the state is integrated as intended.
- Files Handling RelayRaceLost:
services/rfq/relayer/service/handlers.goservices/rfq/relayer/reldb/db.goservices/rfq/relayer/reldb/base/quote.goservices/rfq/relayer/reldb/quoterequeststatus_string.goservices/rfq/relayer/service/chainindexer.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `RelayRaceLost` state is handled correctly in all relevant parts of the system. # Test: Search for handling of `RelayRaceLost`. Expect: Handling logic in relevant parts of the system. rg --type python -A 5 $'RelayRaceLost'Length of output: 68
Script:
#!/bin/bash # Description: Verify that the `RelayRaceLost` state is handled correctly in all relevant parts of the system. # Test: Search for handling of `RelayRaceLost`. Expect: Handling logic in relevant parts of the system. rg -A 5 'RelayRaceLost'Length of output: 4541
| func (q *QuoteRequestHandler) handleRelayCompleted(ctx context.Context, span trace.Span, request reldb.QuoteRequest) (err error) { | ||
| // fetch the block of the relay transaction to confirm that it has been finalized | ||
| relayBlockNumber, err := q.getRelayBlockNumber(ctx, request) | ||
| if err != nil { | ||
| // relay tx must have gotten reorged; mark as RelayRaceLost | ||
| span.SetAttributes(attribute.String("receipt_error", err.Error())) | ||
| err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.RelayRaceLost, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("could not update request status: %w", err) | ||
| } | ||
| return fmt.Errorf("could not get relay block number: %w", err) | ||
| } | ||
|
|
||
| currentBlockNumber := q.Origin.LatestBlock() | ||
| proveConfirmations, err := q.cfg.GetFinalityConfirmations(int(q.Dest.ChainID)) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get prove confirmations: %w", err) | ||
| } | ||
|
|
||
| span.SetAttributes( | ||
| attribute.Int("current_block_number", int(currentBlockNumber)), | ||
| attribute.Int("relay_block_number", int(relayBlockNumber)), | ||
| attribute.Int("prove_confirmations", int(proveConfirmations)), | ||
| ) | ||
| if currentBlockNumber < relayBlockNumber+proveConfirmations { | ||
| span.AddEvent("not enough confirmations") | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Ensure sufficient test coverage for new logic in handleRelayCompleted.
The changes in handleRelayCompleted introduce significant logic related to transaction finality checks. However, static analysis indicates that these lines are not covered by tests. Ensuring that new logic is adequately tested is crucial for maintaining the reliability of the system, especially given the critical nature of blockchain transaction handling.
Please add unit tests to cover the new logic introduced in this method. If you need assistance with this, I can help generate the unit testing code or open a GitHub issue to track this task.
Tools
GitHub Check: codecov/patch
[warning] 381-381: services/rfq/relayer/service/handlers.go#L381
Added line #L381 was not covered by tests
[warning] 383-384: services/rfq/relayer/service/handlers.go#L383-L384
Added lines #L383 - L384 were not covered by tests
[warning] 391-392: services/rfq/relayer/service/handlers.go#L391-L392
Added lines #L391 - L392 were not covered by tests
[warning] 400-409: services/rfq/relayer/service/handlers.go#L400-L409
Added lines #L400 - L409 were not covered by tests
| // getRelayBlockNumber fetches the block number of the relay transaction for a given quote request. | ||
| func (q *QuoteRequestHandler) getRelayBlockNumber(ctx context.Context, request reldb.QuoteRequest) (blockNumber uint64, err error) { | ||
| // fetch the transaction receipt for corresponding tx hash | ||
| var receipt *types.Receipt | ||
| err = retry.WithBackoff(ctx, func(context.Context) error { | ||
| receipt, err = q.Dest.Client.TransactionReceipt(ctx, request.DestTxHash) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get transaction receipt: %w", err) | ||
| } | ||
| return nil | ||
| }, retry.WithMaxTotalTime(15*time.Second)) | ||
| if err != nil { | ||
| return blockNumber, fmt.Errorf("could not get get receipt: %w", err) | ||
| } | ||
| parser, err := fastbridge.NewParser(q.Dest.Bridge.Address()) | ||
| if err != nil { | ||
| return blockNumber, fmt.Errorf("could not create parser: %w", err) | ||
| } | ||
|
|
||
| // check that a Relayed event was emitted | ||
| for _, log := range receipt.Logs { | ||
| if log == nil { | ||
| continue | ||
| } | ||
| _, parsedEvent, ok := parser.ParseEvent(*log) | ||
| if !ok { | ||
| continue | ||
| } | ||
| _, ok = parsedEvent.(*fastbridge.FastBridgeBridgeRelayed) | ||
| if ok { | ||
| return receipt.BlockNumber.Uint64(), nil | ||
| } | ||
| } | ||
|
|
||
| return blockNumber, fmt.Errorf("relayed event not found for dest tx hash: %s", request.DestTxHash.Hex()) | ||
| } |
There was a problem hiding this comment.
Refactor getRelayBlockNumber to handle errors more gracefully and ensure event parsing is robust.
The method getRelayBlockNumber fetches the block number of a relay transaction, which is crucial for ensuring that the transaction has been finalized. However, there are several areas where the error handling and event parsing could be improved:
- Error Handling: The method retries fetching the transaction receipt but does not handle potential continuous failures beyond the retry limit. This could lead to unhandled errors in critical transaction handling logic.
- Event Parsing: The loop checking for a
Relayedevent does not account for possible anomalies like missing or malformed logs.
Consider implementing more comprehensive error handling strategies and robust log parsing to ensure the reliability of this method. Here's a proposed refactor for better error handling and event parsing:
func (q *QuoteRequestHandler) getRelayBlockNumber(ctx context.Context, request reldb.QuoteRequest) (blockNumber uint64, err error) {
var receipt *types.Receipt
err = retry.WithBackoff(ctx, func(context.Context) error {
receipt, err = q.Dest.Client.TransactionReceipt(ctx, request.DestTxHash)
if err != nil {
return fmt.Errorf("could not get transaction receipt: %w", err)
}
return nil
}, retry.WithMaxTotalTime(15*time.Second))
if err != nil {
return blockNumber, fmt.Errorf("could not get receipt after retries: %w", err)
}
parser, err := fastbridge.NewParser(q.Dest.Bridge.Address())
if err != nil {
return blockNumber, fmt.Errorf("could not create parser: %w", err)
}
found := false
for _, log := range receipt.Logs {
if log == nil {
continue
}
_, parsedEvent, ok := parser.ParseEvent(*log)
if ok {
if _, ok := parsedEvent.(*fastbridge.FastBridgeBridgeRelayed); ok {
found = true
blockNumber = receipt.BlockNumber.Uint64()
break
}
}
}
if !found {
return blockNumber, fmt.Errorf("relayed event not found for dest tx hash: %s", request.DestTxHash.Hex())
}
return blockNumber, nil
}This refactor includes a check to ensure all retries are exhausted before returning an error and improves clarity in event parsing.
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.
| // getRelayBlockNumber fetches the block number of the relay transaction for a given quote request. | |
| func (q *QuoteRequestHandler) getRelayBlockNumber(ctx context.Context, request reldb.QuoteRequest) (blockNumber uint64, err error) { | |
| // fetch the transaction receipt for corresponding tx hash | |
| var receipt *types.Receipt | |
| err = retry.WithBackoff(ctx, func(context.Context) error { | |
| receipt, err = q.Dest.Client.TransactionReceipt(ctx, request.DestTxHash) | |
| if err != nil { | |
| return fmt.Errorf("could not get transaction receipt: %w", err) | |
| } | |
| return nil | |
| }, retry.WithMaxTotalTime(15*time.Second)) | |
| if err != nil { | |
| return blockNumber, fmt.Errorf("could not get get receipt: %w", err) | |
| } | |
| parser, err := fastbridge.NewParser(q.Dest.Bridge.Address()) | |
| if err != nil { | |
| return blockNumber, fmt.Errorf("could not create parser: %w", err) | |
| } | |
| // check that a Relayed event was emitted | |
| for _, log := range receipt.Logs { | |
| if log == nil { | |
| continue | |
| } | |
| _, parsedEvent, ok := parser.ParseEvent(*log) | |
| if !ok { | |
| continue | |
| } | |
| _, ok = parsedEvent.(*fastbridge.FastBridgeBridgeRelayed) | |
| if ok { | |
| return receipt.BlockNumber.Uint64(), nil | |
| } | |
| } | |
| return blockNumber, fmt.Errorf("relayed event not found for dest tx hash: %s", request.DestTxHash.Hex()) | |
| } | |
| func (q *QuoteRequestHandler) getRelayBlockNumber(ctx context.Context, request reldb.QuoteRequest) (blockNumber uint64, err error) { | |
| var receipt *types.Receipt | |
| err = retry.WithBackoff(ctx, func(context.Context) error { | |
| receipt, err = q.Dest.Client.TransactionReceipt(ctx, request.DestTxHash) | |
| if err != nil { | |
| return fmt.Errorf("could not get transaction receipt: %w", err) | |
| } | |
| return nil | |
| }, retry.WithMaxTotalTime(15*time.Second)) | |
| if err != nil { | |
| return blockNumber, fmt.Errorf("could not get receipt after retries: %w", err) | |
| } | |
| parser, err := fastbridge.NewParser(q.Dest.Bridge.Address()) | |
| if err != nil { | |
| return blockNumber, fmt.Errorf("could not create parser: %w", err) | |
| } | |
| found := false | |
| for _, log := range receipt.Logs { | |
| if log == nil { | |
| continue | |
| } | |
| _, parsedEvent, ok := parser.ParseEvent(*log) | |
| if ok { | |
| if _, ok := parsedEvent.(*fastbridge.FastBridgeBridgeRelayed); ok { | |
| found = true | |
| blockNumber = receipt.BlockNumber.Uint64() | |
| break | |
| } | |
| } | |
| } | |
| if !found { | |
| return blockNumber, fmt.Errorf("relayed event not found for dest tx hash: %s", request.DestTxHash.Hex()) | |
| } | |
| return blockNumber, nil | |
| } |
Tools
GitHub Check: codecov/patch
[warning] 430-432: services/rfq/relayer/service/handlers.go#L430-L432
Added lines #L430 - L432 were not covered by tests
[warning] 441-441: services/rfq/relayer/service/handlers.go#L441
Added line #L441 was not covered by tests
[warning] 443-445: services/rfq/relayer/service/handlers.go#L443-L445
Added lines #L443 - L445 were not covered by tests
[warning] 449-458: services/rfq/relayer/service/handlers.go#L449-L458
Added lines #L449 - L458 were not covered by tests
[warning] 461-463: services/rfq/relayer/service/handlers.go#L461-L463
Added lines #L461 - L463 were not covered by tests
GitHub Check: Lint (services/rfq)
[failure] 453-453:
Duplicate words (get) found (dupword)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/relayer/service/handlers.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/service/handlers.go
Bundle ReportChanges will decrease total bundle size by 10.32MB ⬇️
|
Summary by CodeRabbit
New Features
ProveConfirmationsandFinalityConfirmations, for enhanced control over confirmation processes in RFQ operations.ProveConfirmationsandFinalityConfirmationsvalues for specific chain configurations.Improvements