[SLT-234] feat(rfq): dynamic block conf wait + receipt x event quote check w/ RPC consensus #3190
Conversation
golangisfun123
requested review from
dwasse,
trajan0x and
aureliusbtc
as code owners
WalkthroughThe pull request introduces significant modifications across various files related to the relayer configuration and transaction handling. Key changes include the addition of a Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
golangisfun123
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3190 +/- ##
===================================================
- Coverage 40.97804% 33.07430% -7.90374%
===================================================
Files 459 535 +76
Lines 25643 33715 +8072
Branches 343 82 -261
===================================================
+ Hits 10508 11151 +643
- Misses 14383 21575 +7192
- Partials 752 989 +237
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: |
bfd1761
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://eafc0c77.sanguine-fe.pages.dev |
| Branch Preview URL: | https://rpc-confirmations.sanguine-fe.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (6)
services/rfq/relayer/service/statushandler.go (1)
90-97: LGTM! Consider enhancing error handling.The addition of the
confirmationsClientis a good improvement, aligning with the PR's objective of handling multiple RPC receipts after RFQ confirmation delay. This change provides more precise control over transaction confirmations.A minor suggestion for improvement:
Consider wrapping the error returned from
GetConfirmationsClientwith additional context. This can be done using the%wverb infmt.Errorf. For example:- return nil, fmt.Errorf("could not get confirmations client: %w", err) + return nil, fmt.Errorf("could not get confirmations client for chain %d: %w", req.Transaction.OriginChainId, err)This change would provide more specific information about which chain caused the error, making debugging easier.
Also applies to: 118-118
services/rfq/relayer/relconfig/config.go (1)
111-115: LGTM! Consider clarifying the VolumeLimit comment.The addition of
VolumeLimitandRPCConfirmationsfields to theChainConfigstruct enhances the system's flexibility and security. TheRPCConfirmationsfield, in particular, adds a valuable safeguard against potential reorg issues.Consider updating the comment for
VolumeLimitto clarify the time frame for the volume limit. For example:- // VolumeLimit is the maximum dollar value of relayed transactions in the BlockWindow. + // VolumeLimit is the maximum dollar value of relayed transactions allowed within a specified time frame or block window.This change would provide more context about when and how the volume limit is applied.
services/rfq/relayer/relconfig/getters.go (1)
852-860: LGTM! Consider improving error handling.The new
GetLimitConfirmationsmethod is a good addition for retrieving chain-specific limit confirmations. However, returning 0 for non-existent chain configurations might lead to unexpected behavior if not handled properly by the caller.Consider returning an error along with the value to explicitly indicate when a chain configuration is not found:
-func (c Config) GetLimitConfirmations(chainID int) uint64 { +func (c Config) GetLimitConfirmations(chainID int) (uint64, error) { chainConfig, ok := c.Chains[chainID] if !ok { - return 0 + return 0, fmt.Errorf("chain configuration not found for chainID: %d", chainID) } - return chainConfig.LimitConfirmations + return chainConfig.LimitConfirmations, nil }services/rfq/relayer/limiter/limiter.go (1)
100-100: Update attribute key to reflect volume limit checkThe attribute key
"within_size_limit"may be misleading since the function now checks volume limits. It would be clearer to rename it to"under_volume_limit"to accurately represent what is being measured.Apply this diff to rename the attribute:
100 - attribute.Bool("within_size_limit", underVolumeLimit), 100 + attribute.Bool("under_volume_limit", underVolumeLimit),services/rfq/relayer/limiter/limiter_test.go (2)
257-269: Replace hardcoded hex strings with programmatically generated log data.The hardcoded hex string in lines 262-269 reduces readability and can introduce errors. Additionally, there's a
TODOcomment at line 261 indicating the intent to fix the log data creation. Consider programmatically generatingprefixDatato enhance clarity and maintainability.Would you like assistance in generating the log data programmatically? I can help refactor this section or open a GitHub issue to track this task.
218-219: Ensure error handling forTransactionReceiptin mock client.In lines 218-219, the
TransactionReceiptmethod is mocked without simulating possible errors. Consider adding error scenarios to thoroughly test how your limiter handles failures in retrieving transaction receipts.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- services/rfq/e2e/setup_test.go (2 hunks)
- services/rfq/relayer/limiter/limiter.go (7 hunks)
- services/rfq/relayer/limiter/limiter_test.go (2 hunks)
- services/rfq/relayer/limiter/suite_test.go (2 hunks)
- services/rfq/relayer/relconfig/config.go (1 hunks)
- services/rfq/relayer/relconfig/getters.go (1 hunks)
- services/rfq/relayer/service/statushandler.go (2 hunks)
🔇 Additional comments not posted (10)
services/rfq/relayer/limiter/suite_test.go (5)
48-51: LGTM: New parameters added to chain configuration.The addition of
RPCConfirmations,LimitConfirmations,VolumeLimit, andRFQAddressparameters to the chain configuration is consistent and looks good.
50-50: Verify the difference in VolumeLimit between chains.The VolumeLimit for chain 10 is set to 1000 USD, while for chain 81457 it's set to 10000 USD. Please confirm if this difference is intentional and aligns with the expected behavior for these chains.
Also applies to: 68-68
51-51: Check RFQAddress consistency across chains.The RFQAddress is set to the same value for both chains. Please verify if this is the intended configuration or if each chain should have a unique RFQAddress.
Also applies to: 69-69
Line range hint
36-64: Verify token configurations across chains.The token configurations (USDC and ETH) are identical for both chains. Please confirm if this is correct or if there should be chain-specific token configurations.
35-35: Verify the impact of changing chain ID from 1 to 10.The chain ID has been updated from 1 to 10. Please ensure that this change is consistent with other parts of the codebase and doesn't break any existing functionality.
✅ Verification successful
Chain ID change from 1 to 10 is verified and does not break existing functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for occurrences of chain ID 1 in the codebase rg --type go "chainId.*1" --glob '!*test*'Length of output: 44
Script:
#!/bin/bash # Search for various representations of chain ID in Go files excluding tests rg --type go "(chainID\s*[:=]\s*1)|(chain_id\s*[:=]\s*1)|(CHAIN_ID\s*[:=]\s*1)" --glob '!*test*'Length of output: 98
services/rfq/relayer/relconfig/getters.go (2)
843-851: LGTM! Improved flexibility with chain-specific volume limits.The modification to use
chainCfg.VolumeLimitinstead ofc.VolumeLimitenhances the system's flexibility by allowing different volume limits for different chains. This change aligns well with the evolving requirements of the system.
861-870: LGTM! Well-implemented with a sensible default.The new
GetRPCConfirmationsmethod is a good addition for retrieving chain-specific RPC confirmations. The use of a default value (1) for non-existent chain configurations is a sensible approach, ensuring a minimum of one confirmation.services/rfq/relayer/limiter/limiter.go (2)
215-219: Clarify error handling for transaction receipt retrievalWhen checking for
ethereum.NotFounderror, the logic returnsfalse, nil, indicating that the receipt does not exist, which is acceptable. However, for any other errors, it returnsfalsewith an error. Ensure this behavior is intended and consider logging unexpected errors for better observability.Review if additional error logging is necessary. If so, you might want to add logging for unexpected errors:
218 + l.metrics.Logger().Errorf("Unexpected error when getting transaction receipt: %v", err)
253-260: Review the completeness of therfqFieldsMatchfunctionThe
rfqFieldsMatchfunction compares several fields to determine if the event matches the request. The TODO comment suggests uncertainty about its exhaustiveness. Ensure that all relevant fields are compared to prevent false positives.Would you like assistance in reviewing and updating the
rfqFieldsMatchfunction to ensure it comprehensively compares all necessary fields?services/rfq/relayer/limiter/limiter_test.go (1)
204-204: Verify the correctness ofGetPricemock return value.In line 204, the
GetPricemethod ofmockQuoteris mocked to return a fixed price. Ensure that this price accurately reflects the expected scenarios in your tests.Run the following script to check all usages of
GetPriceand confirm the returned values align with test expectations:
| span.SetAttributes( | ||
| attribute.String("receipt txHash", receipt.TxHash.Hex()), | ||
| attribute.String("receipt log address", receipt.ContractAddress.String()), | ||
| attribute.String("receipt data", hex.EncodeToString(receipt.Logs[0].Data)), |
There was a problem hiding this comment.
Prevent potential index out-of-range panic when accessing receipt logs
Accessing receipt.Logs[0] without checking if receipt.Logs is non-empty can result in a runtime panic if the logs slice is empty. To ensure robustness, add a check to verify that receipt.Logs contains at least one entry before accessing it.
Apply this diff to add the necessary check:
245 + if len(receipt.Logs) > 0 {
245 attribute.String("receipt data", hex.EncodeToString(receipt.Logs[0].Data)),
245 + } else {
245 + attribute.String("receipt data", "no logs available"),
245 + }📝 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.
| attribute.String("receipt data", hex.EncodeToString(receipt.Logs[0].Data)), | |
| if len(receipt.Logs) > 0 { | |
| attribute.String("receipt data", hex.EncodeToString(receipt.Logs[0].Data)), | |
| } else { | |
| attribute.String("receipt data", "no logs available"), | |
| } |
| if err != nil { | ||
| return false, fmt.Errorf("could not get required confirmations from config: %w", err) | ||
| } | ||
| requiredConfirmations := l.cfg.GetLimitConfirmations(int(request.Transaction.OriginChainId)) |
There was a problem hiding this comment.
Handle potential errors when retrieving required confirmations
In the hasEnoughConfirmations function, the retrieval of requiredConfirmations does not handle potential errors from l.cfg.GetLimitConfirmations(). If this function can return an error, it should be properly handled to prevent unexpected behavior.
Modify the code to handle the error:
117 - requiredConfirmations := l.cfg.GetLimitConfirmations(int(request.Transaction.OriginChainId))
+ requiredConfirmations, err := l.cfg.GetLimitConfirmations(int(request.Transaction.OriginChainId))
+ if err != nil {
+ return false, fmt.Errorf("could not get required confirmations: %w", err)
+ }Committable suggestion was skipped due to low confidence.
| originAmount, _ = new(big.Float).SetFloat64(11983199386503458).Int(nil) | ||
| destAmount, _ = new(big.Float).SetFloat64(11980157553261996).Int(nil) |
There was a problem hiding this comment.
Handle potential inaccuracies when converting big.Float to *big.Int.
In lines 20-21, the second return value from (*big.Float).Int(nil) is being ignored using _. This second value indicates the accuracy of the conversion. Ignoring it might lead to unintended results if the conversion is inexact. Consider handling the accuracy explicitly to ensure the integrity of originAmount and destAmount.
| // Basically, I used some random RFQ transaction as the test case for this suite, | ||
| // but mocked the block and price that's returned instead of changing the actual transaction fields. | ||
| // This is why the data is hardcoded. | ||
| // TODO: fix this once I figure out how to properly create the log data |
There was a problem hiding this comment.
Address the TODO comment regarding log data creation.
The TODO at line 261 suggests that the way log data is currently created is temporary. Finalizing this implementation will improve test reliability and code quality.
| mockListener := buildMockListener(6) | ||
| packedBridgeTx, err := packBridgeTransaction(quote.Transaction) | ||
| l.NoError(err) | ||
| mockClient := buildMockEVMClient(quoteRequestToReceipt(quote, packedBridgeTx, false)) |
There was a problem hiding this comment.
Check for potential nil dereference in mockClient.
When calling buildMockEVMClient, ensure that the returned mockClient is not nil before using it to prevent potential runtime panics.
| bridgeStruct, err := abi.NewType( | ||
| "tuple", | ||
| "BridgeTransaction", | ||
| []abi.ArgumentMarshaling{ | ||
| {Name: "originChainId", Type: "uint32"}, | ||
| {Name: "destChainId", Type: "uint32"}, | ||
| {Name: "originSender", Type: "address"}, | ||
| {Name: "destRecipient", Type: "address"}, | ||
| {Name: "originToken", Type: "address"}, | ||
| {Name: "destToken", Type: "address"}, | ||
| {Name: "originAmount", Type: "uint256"}, | ||
| {Name: "destAmount", Type: "uint256"}, | ||
| {Name: "originFeeAmount", Type: "uint256"}, | ||
| {Name: "sendChainGas", Type: "bool"}, | ||
| {Name: "deadline", Type: "uint256"}, | ||
| {Name: "nonce", Type: "uint256"}, | ||
| }) | ||
| if err != nil { | ||
| //nolint: wrapcheck | ||
| return []byte{}, err | ||
| } | ||
|
|
||
| args := abi.Arguments{ | ||
| {Type: bridgeStruct, Name: "param_one"}, | ||
| } | ||
|
|
||
| packed, err := args.Pack(&tx) | ||
| if err != nil { | ||
| //nolint: wrapcheck | ||
| return []byte{}, err | ||
| } | ||
| return packed, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor packBridgeTransaction to improve maintainability.
The packBridgeTransaction function manually constructs the ABI type and packs the transaction data. This approach can be error-prone and hard to maintain. Consider using the ABI generated by abigen or directly referencing the contract's ABI to simplify the process and reduce potential errors.
For example, if you have an ABI binding for your contract, you can modify the function as follows:
-func packBridgeTransaction(tx fastbridge.IFastBridgeBridgeTransaction) ([]byte, error) {
- // manual ABI packing code
- ...
- return packed, nil
+func packBridgeTransaction(tx fastbridge.IFastBridgeBridgeTransaction) ([]byte, error) {
+ return fastbridge.ABI.Pack("YourFunctionName", tx)
+}Replace "YourFunctionName" with the appropriate function name from your contract's ABI.
Committable suggestion was skipped due to low confidence.
golangisfun123
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
services/rfq/relayer/relconfig/getters.go (2)
843-851: Approve change, but test coverage needed.The modification to use
chainCfg.VolumeLimitinstead ofc.VolumeLimitis a logical improvement, allowing for chain-specific volume limits. However, this change is not covered by tests, which is a concern.Would you like assistance in writing test cases to cover this change in the
GetVolumeLimitfunction?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 843-843: services/rfq/relayer/relconfig/getters.go#L843
Added line #L843 was not covered by tests
852-861: Approve new function, but improvements needed.The new
GetRPCConfirmationsfunction is a good addition, providing a way to retrieve chain-specific RPC confirmation settings. However, there are a few points to address:
- Consider adding a comment to explain the purpose and significance of RPC confirmations.
- This new function is not covered by tests, which should be addressed to ensure its correctness and maintain code quality.
Would you like assistance in writing test cases for the new
GetRPCConfirmationsfunction and adding appropriate documentation?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 855-860: services/rfq/relayer/relconfig/getters.go#L855-L860
Added lines #L855 - L860 were not covered by testsservices/rfq/relayer/limiter/limiter.go (5)
Line range hint
113-115: Remove unnecessary error check after callingLatestBlockThe function
l.listener.LatestBlock()returns auint64without an error, but there is an error check immediately after that assumes an error might have occurred. This check is invalid and may lead to confusion.Apply this diff to remove the unnecessary error check:
currentBlockNumber := l.listener.LatestBlock() -if err != nil { - return false, fmt.Errorf("could not get block number: %w", err) -}
181-181: Use consistent attribute naming for clarityIn the span attributes, the key
"within_size_limit"is used. For consistency and clarity, consider renaming it to"under_volume_limit"to match the variableunderVolumeLimitand make the logs more intuitive.Apply this diff to rename the attribute:
attribute.String("token_price", tokenPrice.String()), -attribute.Bool("within_size_limit", underVolumeLimit), +attribute.Bool("under_volume_limit", underVolumeLimit),
240-246: Handle potentialnilreceipt to prevent panicWhile the
TransactionReceiptmethod should return a receipt whenerrisnil, it's good practice to check ifreceiptis notnilbefore using it. This ensures robustness in case of unexpected behavior from the RPC client.Apply this diff to add a nil check:
receipt, err := l.evmClient.TransactionReceipt(ctx, request.OriginTxHash) if err != nil { if errors.Is(err, ethereum.NotFound) { return false, nil } return false, fmt.Errorf("could not get transaction receipt: %w", err) +} else if receipt == nil { + return false, fmt.Errorf("received nil receipt without error") }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 242-245: services/rfq/relayer/limiter/limiter.go#L242-L245
Added lines #L242 - L245 were not covered by tests
76-94: Add unit tests to cover new error handling branchesSeveral new error handling branches have been introduced in the
IsAllowedmethod, specifically when checking volume limits, confirmations, and receipts. Currently, these branches are not covered by tests, as indicated by the static analysis tool. To improve test coverage and ensure robustness, please add unit tests that simulate these error conditions.Would you like assistance in creating these unit tests or opening a GitHub issue to track this task?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 76-76: services/rfq/relayer/limiter/limiter.go#L76
Added line #L76 was not covered by tests
[warning] 85-86: services/rfq/relayer/limiter/limiter.go#L85-L86
Added lines #L85 - L86 were not covered by tests
[warning] 93-94: services/rfq/relayer/limiter/limiter.go#L93-L94
Added lines #L93 - L94 were not covered by tests
146-149: Add context to the error message for better debuggingWhen returning an error from
getRequestVolumeOfToken, including the token information can aid in debugging. This provides more insight into which token caused the error.Apply this diff to enhance the error message:
if err != nil { - return 0, fmt.Errorf("could not get token volume: %w", err) + return 0, fmt.Errorf("could not get volume for token %s: %w", request.Transaction.OriginToken.String(), err) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- services/rfq/relayer/limiter/limiter.go (6 hunks)
- services/rfq/relayer/limiter/limiter_test.go (2 hunks)
- services/rfq/relayer/limiter/suite_test.go (2 hunks)
- services/rfq/relayer/relconfig/config.go (1 hunks)
- services/rfq/relayer/relconfig/getters.go (1 hunks)
- services/rfq/util/util.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- services/rfq/relayer/limiter/limiter_test.go
- services/rfq/relayer/limiter/suite_test.go
- services/rfq/relayer/relconfig/config.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/rfq/relayer/limiter/limiter.go
[warning] 76-76: services/rfq/relayer/limiter/limiter.go#L76
Added line #L76 was not covered by tests
[warning] 85-86: services/rfq/relayer/limiter/limiter.go#L85-L86
Added lines #L85 - L86 were not covered by tests
[warning] 93-94: services/rfq/relayer/limiter/limiter.go#L93-L94
Added lines #L93 - L94 were not covered by tests
[warning] 242-245: services/rfq/relayer/limiter/limiter.go#L242-L245
Added lines #L242 - L245 were not covered by tests
[warning] 250-251: services/rfq/relayer/limiter/limiter.go#L250-L251
Added lines #L250 - L251 were not covered by tests
[warning] 255-256: services/rfq/relayer/limiter/limiter.go#L255-L256
Added lines #L255 - L256 were not covered by tests
[warning] 261-261: services/rfq/relayer/limiter/limiter.go#L261
Added line #L261 was not covered by tests
[warning] 269-275: services/rfq/relayer/limiter/limiter.go#L269-L275
Added lines #L269 - L275 were not covered by testsservices/rfq/relayer/relconfig/getters.go
[warning] 843-843: services/rfq/relayer/relconfig/getters.go#L843
Added line #L843 was not covered by tests
[warning] 855-860: services/rfq/relayer/relconfig/getters.go#L855-L860
Added lines #L855 - L860 were not covered by tests
🔇 Additional comments (3)
services/rfq/util/util.go (1)
15-15: Approved: Performance improvement by using pointer receiverThe change from
func QuoteRequestToAttributes(request reldb.QuoteRequest)tofunc QuoteRequestToAttributes(request *reldb.QuoteRequest)is a good optimization. It avoids unnecessary copying of theQuoteRequeststruct, which can improve performance, especially ifQuoteRequestis a large struct.services/rfq/relayer/relconfig/getters.go (1)
843-861: Summary: Improvements to chain-specific configurations, but test coverage needed.The changes in this file introduce more granular control over chain-specific settings, which is a positive improvement. However, it's crucial to address the lack of test coverage for both the modified
GetVolumeLimitfunction and the newGetRPCConfirmationsfunction.To maintain code quality and ensure the correctness of these changes, please add appropriate test cases covering:
- The use of
chainCfg.VolumeLimitin theGetVolumeLimitfunction.- The entire
GetRPCConfirmationsfunction, including edge cases.To help verify the impact of these changes, you can run the following command to check for existing tests related to these functions:
This will help identify if there are any existing tests that need to be updated or if entirely new tests need to be written.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 843-843: services/rfq/relayer/relconfig/getters.go#L843
Added line #L843 was not covered by tests
[warning] 855-860: services/rfq/relayer/relconfig/getters.go#L855-L860
Added lines #L855 - L860 were not covered by testsservices/rfq/relayer/limiter/limiter.go (1)
226-276: Ensure the receipt parsing handles all expected eventsIn the
checkReceiptfunction, the code currently checks for the event*fastbridge.FastBridgeBridgeRequested. If there are other relevant events that should be considered, make sure to include them or confirm that this is exhaustive.Run the following script to list all events emitted by the contract and verify if additional events need to be handled:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 242-245: services/rfq/relayer/limiter/limiter.go#L242-L245
Added lines #L242 - L245 were not covered by tests
[warning] 250-251: services/rfq/relayer/limiter/limiter.go#L250-L251
Added lines #L250 - L251 were not covered by tests
[warning] 255-256: services/rfq/relayer/limiter/limiter.go#L255-L256
Added lines #L255 - L256 were not covered by tests
[warning] 261-261: services/rfq/relayer/limiter/limiter.go#L261
Added line #L261 was not covered by tests
[warning] 269-275: services/rfq/relayer/limiter/limiter.go#L269-L275
Added lines #L269 - L275 were not covered by tests
| @@ -12,7 +12,7 @@ import ( | |||
| var EthAddress = common.HexToAddress("0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE") | |||
|
|
|||
| // QuoteRequestToAttributes converts a quote request to attributes. | |||
| func QuoteRequestToAttributes(request reldb.QuoteRequest) []attribute.KeyValue { | |||
| func QuoteRequestToAttributes(request *reldb.QuoteRequest) []attribute.KeyValue { | |||
There was a problem hiding this comment.
💡 Codebase verification
Issues Found: Calls to QuoteRequestToAttributes Without Pointers
services/rfq/relayer/limiter/limiter.go:67:trace.WithAttributes(util.QuoteRequestToAttributes(request)...)is passingrequestby value instead of a pointer.
🔗 Analysis chain
Verify all calls to QuoteRequestToAttributes in the codebase
This change in function signature requires updating all calls to QuoteRequestToAttributes throughout the codebase. Ensure that all calling code now passes a pointer to QuoteRequest instead of a value.
Run the following script to find all occurrences of QuoteRequestToAttributes and verify they're passing a pointer:
Please review the output and ensure all calls are updated correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to QuoteRequestToAttributes and verify they're passing a pointer
# Search for QuoteRequestToAttributes calls
echo "Searching for QuoteRequestToAttributes calls:"
rg --type go -n "QuoteRequestToAttributes\s*\(" -A 3
echo "\nVerifying pointer usage:"
# Look for &reldb.QuoteRequest or &variableName patterns in the same line or next line
rg --type go -n "QuoteRequestToAttributes\s*\((\s*&|\n\s*&)"
echo "\nPotential issues (calls without &):"
# Look for calls without & (potential issues)
rg --type go -n "QuoteRequestToAttributes\s*\([^&\n]"
Length of output: 1457
There was a problem hiding this comment.
wrong limiter.go:67 is fine
There was a problem hiding this comment.
@golangisfun123, thank you for the clarification. You're right—limiter.go:67 is correct as is, and passing request without using & is appropriate since request is already a pointer. I'll treat this as a warning rather than an issue in the future.
(•́﹏•̀)
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all calls to QuoteRequestToAttributes are passing pointers
echo "Searching for calls to QuoteRequestToAttributes where the argument is not a pointer:"
rg --type go -n 'QuoteRequestToAttributes\(\s*[^&]'
Length of output: 479
✏️ Learnings added
Learnt from: golangisfun123
PR: synapsecns/sanguine#3190
File: services/rfq/util/util.go:15-15
Timestamp: 2024-09-30T19:02:29.722Z
Learning: In `limiter.go`, the `request` variable passed to `QuoteRequestToAttributes` is already a pointer, so no modification is needed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
|
||
| // isUnderVolumeLimit returns true if the request is under the volume limit, false otherwise. | ||
| func (l *limiterImpl) isUnderVolumeLimit(ctx context.Context, request *reldb.QuoteRequest) (_ bool, err error) { | ||
| ctx, span := l.metrics.Tracer().Start(ctx, "limiter.underVolumeLimitLimit") |
There was a problem hiding this comment.
Correct the tracer name to reflect the function accurately
The tracer name in isUnderVolumeLimit is currently set to "limiter.underVolumeLimitLimit", which seems redundant and may lead to confusion. It should be corrected to "limiter.isUnderVolumeLimit" to match the function name and maintain consistency.
Apply this diff to update the tracer name:
ctx, span := l.metrics.Tracer().Start(ctx,
- "limiter.underVolumeLimitLimit")
+ "limiter.isUnderVolumeLimit")📝 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.
| ctx, span := l.metrics.Tracer().Start(ctx, "limiter.underVolumeLimitLimit") | |
| ctx, span := l.metrics.Tracer().Start(ctx, "limiter.isUnderVolumeLimit") |
| attribute.String("receipt data", hex.EncodeToString(receipt.Logs[0].Data)), | ||
| ) |
There was a problem hiding this comment.
Check for empty receipt logs before accessing the first element
Accessing receipt.Logs[0] without verifying that receipt.Logs is not empty can lead to an index out-of-range panic if the slice is empty. Before accessing receipt.Logs[0].Data, ensure that there is at least one log entry in receipt.Logs.
Apply this diff to add the necessary check:
span.SetAttributes(
attribute.String("receipt txHash", receipt.TxHash.Hex()),
attribute.String("receipt log address", receipt.ContractAddress.String()),
- attribute.String("receipt data", hex.EncodeToString(receipt.Logs[0].Data)),
+ attribute.String("receipt data", func() string {
+ if len(receipt.Logs) > 0 {
+ return hex.EncodeToString(receipt.Logs[0].Data)
+ }
+ return "no logs available"
+ }()),
)📝 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.
| attribute.String("receipt data", hex.EncodeToString(receipt.Logs[0].Data)), | |
| ) | |
| span.SetAttributes( | |
| attribute.String("receipt txHash", receipt.TxHash.Hex()), | |
| attribute.String("receipt log address", receipt.ContractAddress.String()), | |
| attribute.String("receipt data", func() string { | |
| if len(receipt.Logs) > 0 { | |
| return hex.EncodeToString(receipt.Logs[0].Data) | |
| } | |
| return "no logs available" | |
| }()), | |
| ) |
| volumeLimitForChain := l.cfg.GetVolumeLimit(int(request.Transaction.OriginChainId), request.Transaction.OriginToken) | ||
|
|
||
| return uint64(new(big.Int).Div( | ||
| tokenVolume, | ||
| volumeLimitForChain, | ||
| ).Int64()), nil |
There was a problem hiding this comment.
Prevent potential division by zero
In the function getNumberOfConfirmationsToWait, there is a division operation where tokenVolume is divided by volumeLimitForChain. If volumeLimitForChain is zero, this will cause a runtime panic due to division by zero. Ensure that volumeLimitForChain is not zero before performing the division.
Apply this diff to add a check for zero:
volumeLimitForChain := l.cfg.GetVolumeLimit(int(request.Transaction.OriginChainId), request.Transaction.OriginToken)
+if volumeLimitForChain.Cmp(big.NewInt(0)) == 0 {
+ return 0, fmt.Errorf("volume limit for chain is zero, cannot divide by zero")
+}
return uint64(new(big.Int).Div(
tokenVolume,
volumeLimitForChain,
).Int64()), 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.
| volumeLimitForChain := l.cfg.GetVolumeLimit(int(request.Transaction.OriginChainId), request.Transaction.OriginToken) | |
| return uint64(new(big.Int).Div( | |
| tokenVolume, | |
| volumeLimitForChain, | |
| ).Int64()), nil | |
| volumeLimitForChain := l.cfg.GetVolumeLimit(int(request.Transaction.OriginChainId), request.Transaction.OriginToken) | |
| if volumeLimitForChain.Cmp(big.NewInt(0)) == 0 { | |
| return 0, fmt.Errorf("volume limit for chain is zero, cannot divide by zero") | |
| } | |
| return uint64(new(big.Int).Div( | |
| tokenVolume, | |
| volumeLimitForChain, | |
| ).Int64()), nil |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
services/rfq/relayer/limiter/limiter.go (2)
Line range hint
162-187: Renamed and updated isUnderVolumeLimit method is an improvementThe renaming of
withinSizeLimittoisUnderVolumeLimitenhances clarity and better reflects the method's purpose. The updated logic using the newgetRequestVolumeOfTokenmethod is appropriate and consistent with the overall changes.However, there's a minor inconsistency:
Update the attribute name for consistency:
span.SetAttributes( attribute.String("volume_limit", volumeLimit.String()), attribute.String("token_price", tokenPrice.String()), - attribute.Bool("within_size_limit", underVolumeLimit), + attribute.Bool("under_volume_limit", underVolumeLimit), )This change aligns the attribute name with the new method name and improves consistency in logging.
Improve test coverage for critical methods
The static analysis indicates that several important methods in the
limiter.gofile are not covered by existing tests. Specifically, the following methods lack adequate unit tests:
IsAllowedgetNumberOfConfirmationsToWaitcheckReceiptEnsuring comprehensive test coverage for these methods is essential to maintain the reliability and correctness of the rate limiter functionality.
🔗 Analysis chain
Line range hint
1-290: Improve test coverage for new and modified codeThe static analysis tool has flagged several new and modified code sections as not covered by tests. This includes critical parts of the
IsAllowed,getNumberOfConfirmationsToWait, andcheckReceiptmethods.Please add or update unit tests to cover the following areas:
- Error handling in
IsAllowedmethod (lines 76, 85-86, 93-94)- Error handling in
getNumberOfConfirmationsToWaitmethod (lines 148-149)- Various branches in
checkReceiptmethod (lines 245-248, 253-254, 258-259, 264, 272-278)Ensuring good test coverage will help maintain the reliability and correctness of the rate limiter as the codebase evolves.
This will help identify existing test files where new tests can be added.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # List test files related to the limiter package fd -e go -p 'limiter.*test' --exec echo "Test file: {}"Length of output: 352
Script:
#!/bin/bash # Corrected command to list test files related to the limiter package fd -e go -p 'limiter.*test\.go$' --exec echo "Test file: {}"Length of output: 241
Script:
#!/bin/bash # Search for test functions related to specific methods in the limiter package tests rg -i 'func Test.*(IsAllowed|getNumberOfConfirmationsToWait|checkReceipt)' ./services/rfq/relayer/limiter/*_test.goLength of output: 168
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 76-76: services/rfq/relayer/limiter/limiter.go#L76
Added line #L76 was not covered by tests
[warning] 85-86: services/rfq/relayer/limiter/limiter.go#L85-L86
Added lines #L85 - L86 were not covered by tests
[warning] 93-94: services/rfq/relayer/limiter/limiter.go#L93-L94
Added lines #L93 - L94 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- services/rfq/relayer/limiter/limiter.go (6 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/rfq/relayer/limiter/limiter.go
[warning] 76-76: services/rfq/relayer/limiter/limiter.go#L76
Added line #L76 was not covered by tests
[warning] 85-86: services/rfq/relayer/limiter/limiter.go#L85-L86
Added lines #L85 - L86 were not covered by tests
[warning] 93-94: services/rfq/relayer/limiter/limiter.go#L93-L94
Added lines #L93 - L94 were not covered by tests
[warning] 120-120: services/rfq/relayer/limiter/limiter.go#L120
Added line #L120 was not covered by tests
[warning] 148-149: services/rfq/relayer/limiter/limiter.go#L148-L149
Added lines #L148 - L149 were not covered by tests
[warning] 245-248: services/rfq/relayer/limiter/limiter.go#L245-L248
Added lines #L245 - L248 were not covered by tests
[warning] 253-254: services/rfq/relayer/limiter/limiter.go#L253-L254
Added lines #L253 - L254 were not covered by tests
[warning] 258-259: services/rfq/relayer/limiter/limiter.go#L258-L259
Added lines #L258 - L259 were not covered by tests
[warning] 264-264: services/rfq/relayer/limiter/limiter.go#L264
Added line #L264 was not covered by tests
[warning] 272-278: services/rfq/relayer/limiter/limiter.go#L272-L278
Added lines #L272 - L278 were not covered by tests
🔇 Additional comments (3)
services/rfq/relayer/limiter/limiter.go (3)
6-7: New imports and struct field addition look goodThe new imports and the addition of the
evmClientfield to thelimiterImplstruct are well-aligned with the new receipt verification functionality. These changes provide the necessary tools to interact with the Ethereum blockchain and verify transaction receipts.Also applies to: 11-11, 14-15, 42-42
52-52: NewRateLimiter function update is appropriateThe addition of the
evmClientparameter to theNewRateLimiterfunction and its assignment to the struct field is consistent with the new functionality. This change ensures that the rate limiter has access to the Ethereum client for receipt verification.Also applies to: 60-60
Line range hint
1-290: Summary of limiter.go changesThe changes to
limiter.gosignificantly enhance the rate limiting logic and introduce important new features like receipt verification. Key improvements include:
- More comprehensive checks in the
IsAllowedmethod.- Dynamic confirmation waiting based on transaction volume.
- Receipt verification to handle potential reorg scenarios.
While these changes are generally positive, please address the following points:
- Improve error handling and error messages for better debugging.
- Add safeguards against potential panics (division by zero, nil pointer dereference).
- Ensure consistency in naming (e.g., attribute names in spans).
- Review and possibly expand the RFQ field matching in
rfqFieldsMatch.- Significantly improve test coverage for new and modified code.
Addressing these points will further improve the robustness and maintainability of the rate limiter implementation.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 76-76: services/rfq/relayer/limiter/limiter.go#L76
Added line #L76 was not covered by tests
[warning] 85-86: services/rfq/relayer/limiter/limiter.go#L85-L86
Added lines #L85 - L86 were not covered by tests
[warning] 93-94: services/rfq/relayer/limiter/limiter.go#L93-L94
Added lines #L93 - L94 were not covered by tests
| // if not enough confirmations, check volume. if under limit, wait 0 confirmations. | ||
| underVolumeLimit, err := l.isUnderVolumeLimit(ctx, request) | ||
| if err != nil { | ||
| return false, fmt.Errorf("could not check confirmations: %w", err) | ||
| return false, fmt.Errorf("could not check volume limit: %w", err) | ||
| } | ||
| if hasEnoughConfirmations { | ||
| if underVolumeLimit { | ||
| return true, nil | ||
| } | ||
|
|
||
| // if not enough confirmations, check volume | ||
| withinSize, err := l.withinSizeLimit(ctx, request) | ||
| // if enough confirmations, allow because reorgs are rare at this point. | ||
| hasEnoughConfirmations, err := l.hasEnoughConfirmations(ctx, request) | ||
| if err != nil { | ||
| return false, fmt.Errorf("could not check volume limit: %w", err) | ||
| return false, fmt.Errorf("could not check confirmations: %w", err) | ||
| } | ||
| if hasEnoughConfirmations { | ||
| // we need to check if the receipt exists, parse the events from it, check for possible reverts, | ||
| // and has the correct fields in case of a reorg. then, and only then, we can be sure a reorg will not | ||
| // revert this. | ||
| receiptFieldsMatch, err := l.checkReceipt(ctx, request) | ||
| if err != nil { | ||
| return false, fmt.Errorf("could not check receipt: %w", err) | ||
| } | ||
| return receiptFieldsMatch, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improved IsAllowed method logic
The new logic in the IsAllowed method provides a more comprehensive check for request validation. The sequence of checks (volume limit, confirmations, and receipt verification) enhances the security and reliability of the rate limiting process.
However, consider improving error handling for better debugging:
Consider wrapping errors with more context:
- return false, fmt.Errorf("could not check volume limit: %w", err)
+ return false, fmt.Errorf("IsAllowed failed to check volume limit: %w", err)
- return false, fmt.Errorf("could not check confirmations: %w", err)
+ return false, fmt.Errorf("IsAllowed failed to check confirmations: %w", err)
- return false, fmt.Errorf("could not check receipt: %w", err)
+ return false, fmt.Errorf("IsAllowed failed to check receipt: %w", err)This change will make it easier to trace the origin of errors in logs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // if not enough confirmations, check volume. if under limit, wait 0 confirmations. | |
| underVolumeLimit, err := l.isUnderVolumeLimit(ctx, request) | |
| if err != nil { | |
| return false, fmt.Errorf("could not check confirmations: %w", err) | |
| return false, fmt.Errorf("could not check volume limit: %w", err) | |
| } | |
| if hasEnoughConfirmations { | |
| if underVolumeLimit { | |
| return true, nil | |
| } | |
| // if not enough confirmations, check volume | |
| withinSize, err := l.withinSizeLimit(ctx, request) | |
| // if enough confirmations, allow because reorgs are rare at this point. | |
| hasEnoughConfirmations, err := l.hasEnoughConfirmations(ctx, request) | |
| if err != nil { | |
| return false, fmt.Errorf("could not check volume limit: %w", err) | |
| return false, fmt.Errorf("could not check confirmations: %w", err) | |
| } | |
| if hasEnoughConfirmations { | |
| // we need to check if the receipt exists, parse the events from it, check for possible reverts, | |
| // and has the correct fields in case of a reorg. then, and only then, we can be sure a reorg will not | |
| // revert this. | |
| receiptFieldsMatch, err := l.checkReceipt(ctx, request) | |
| if err != nil { | |
| return false, fmt.Errorf("could not check receipt: %w", err) | |
| } | |
| return receiptFieldsMatch, nil | |
| // if not enough confirmations, check volume. if under limit, wait 0 confirmations. | |
| underVolumeLimit, err := l.isUnderVolumeLimit(ctx, request) | |
| if err != nil { | |
| return false, fmt.Errorf("IsAllowed failed to check volume limit: %w", err) | |
| } | |
| if underVolumeLimit { | |
| return true, nil | |
| } | |
| // if enough confirmations, allow because reorgs are rare at this point. | |
| hasEnoughConfirmations, err := l.hasEnoughConfirmations(ctx, request) | |
| if err != nil { | |
| return false, fmt.Errorf("IsAllowed failed to check confirmations: %w", err) | |
| } | |
| if hasEnoughConfirmations { | |
| // we need to check if the receipt exists, parse the events from it, check for possible reverts, | |
| // and has the correct fields in case of a reorg. then, and only then, we can be sure a reorg will not | |
| // revert this. | |
| receiptFieldsMatch, err := l.checkReceipt(ctx, request) | |
| if err != nil { | |
| return false, fmt.Errorf("IsAllowed failed to check receipt: %w", err) | |
| } | |
| return receiptFieldsMatch, nil | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 76-76: services/rfq/relayer/limiter/limiter.go#L76
Added line #L76 was not covered by tests
[warning] 85-86: services/rfq/relayer/limiter/limiter.go#L85-L86
Added lines #L85 - L86 were not covered by tests
[warning] 93-94: services/rfq/relayer/limiter/limiter.go#L93-L94
Added lines #L93 - L94 were not covered by tests
| // getNumberOfConfirmationsToWait returns the number of confirmations to wait for the request. | ||
| // confirmations = requestVolume / volumeLimitForChain, e.g. we wait 1 confirmation per every `volumeLimitForChain` USD. | ||
| func (l *limiterImpl) getNumberOfConfirmationsToWait(ctx context.Context, request *reldb.QuoteRequest) (_ uint64, err error) { | ||
| ctx, span := l.metrics.Tracer().Start(ctx, "limiter.getNumberOfConfirmationsToWait") | ||
|
|
||
| defer func() { | ||
| metrics.EndSpanWithErr(span, err) | ||
| }() | ||
|
|
||
| tokenVolume, err := l.getRequestVolumeOfToken(ctx, request) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("could not get token volume: %w", err) | ||
| } | ||
|
|
||
| volumeLimitForChain := l.cfg.GetVolumeLimit(int(request.Transaction.OriginChainId), request.Transaction.OriginToken) | ||
| if volumeLimitForChain.Cmp(big.NewInt(-1)) == 0 { | ||
| return 0, nil | ||
| } | ||
|
|
||
| return uint64(new(big.Int).Div( | ||
| tokenVolume, | ||
| volumeLimitForChain, | ||
| ).Int64()), nil | ||
| } |
There was a problem hiding this comment.
New getNumberOfConfirmationsToWait method looks good, but needs a safeguard
The new getNumberOfConfirmationsToWait method provides a dynamic way to determine the required confirmations based on the request volume and chain-specific volume limit. The logic is sound, including the special case handling for no limit (when volumeLimitForChain is -1).
However, there's a potential issue:
Add a check to prevent division by zero:
volumeLimitForChain := l.cfg.GetVolumeLimit(int(request.Transaction.OriginChainId), request.Transaction.OriginToken)
if volumeLimitForChain.Cmp(big.NewInt(-1)) == 0 {
return 0, nil
}
+if volumeLimitForChain.Cmp(big.NewInt(0)) == 0 {
+ return 0, fmt.Errorf("volume limit for chain %d and token %s is zero", request.Transaction.OriginChainId, request.Transaction.OriginToken)
+}
return uint64(new(big.Int).Div(
tokenVolume,
volumeLimitForChain,
).Int64()), nilThis change will prevent a potential panic if the volume limit is set to zero for a chain/token combination.
📝 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.
| // getNumberOfConfirmationsToWait returns the number of confirmations to wait for the request. | |
| // confirmations = requestVolume / volumeLimitForChain, e.g. we wait 1 confirmation per every `volumeLimitForChain` USD. | |
| func (l *limiterImpl) getNumberOfConfirmationsToWait(ctx context.Context, request *reldb.QuoteRequest) (_ uint64, err error) { | |
| ctx, span := l.metrics.Tracer().Start(ctx, "limiter.getNumberOfConfirmationsToWait") | |
| defer func() { | |
| metrics.EndSpanWithErr(span, err) | |
| }() | |
| tokenVolume, err := l.getRequestVolumeOfToken(ctx, request) | |
| if err != nil { | |
| return 0, fmt.Errorf("could not get token volume: %w", err) | |
| } | |
| volumeLimitForChain := l.cfg.GetVolumeLimit(int(request.Transaction.OriginChainId), request.Transaction.OriginToken) | |
| if volumeLimitForChain.Cmp(big.NewInt(-1)) == 0 { | |
| return 0, nil | |
| } | |
| return uint64(new(big.Int).Div( | |
| tokenVolume, | |
| volumeLimitForChain, | |
| ).Int64()), nil | |
| } | |
| // getNumberOfConfirmationsToWait returns the number of confirmations to wait for the request. | |
| // confirmations = requestVolume / volumeLimitForChain, e.g. we wait 1 confirmation per every `volumeLimitForChain` USD. | |
| func (l *limiterImpl) getNumberOfConfirmationsToWait(ctx context.Context, request *reldb.QuoteRequest) (_ uint64, err error) { | |
| ctx, span := l.metrics.Tracer().Start(ctx, "limiter.getNumberOfConfirmationsToWait") | |
| defer func() { | |
| metrics.EndSpanWithErr(span, err) | |
| }() | |
| tokenVolume, err := l.getRequestVolumeOfToken(ctx, request) | |
| if err != nil { | |
| return 0, fmt.Errorf("could not get token volume: %w", err) | |
| } | |
| volumeLimitForChain := l.cfg.GetVolumeLimit(int(request.Transaction.OriginChainId), request.Transaction.OriginToken) | |
| if volumeLimitForChain.Cmp(big.NewInt(-1)) == 0 { | |
| return 0, nil | |
| } | |
| if volumeLimitForChain.Cmp(big.NewInt(0)) == 0 { | |
| return 0, fmt.Errorf("volume limit for chain %d and token %s is zero", request.Transaction.OriginChainId, request.Transaction.OriginToken) | |
| } | |
| return uint64(new(big.Int).Div( | |
| tokenVolume, | |
| volumeLimitForChain, | |
| ).Int64()), nil | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 148-149: services/rfq/relayer/limiter/limiter.go#L148-L149
Added lines #L148 - L149 were not covered by tests
| // checkReceipt checks if the receipt exists and has the correct fields in the case a reorg happened. | ||
| func (l *limiterImpl) checkReceipt(ctx context.Context, request *reldb.QuoteRequest) (_ bool, err error) { | ||
| // Make sure receipt exists and has the correct fields in case of a reorg. | ||
| // Note: https://community.infura.io/t/does-eth-gettransactionreceipt-respond-to-re-orged-transactions/7765 | ||
| // "You will get a tx receipt back but, as you note, there is a small chance of a reorg. | ||
| // In a re-org the shorter side chain (one block usually, two occasionally) will have all its tx’s reverted and | ||
| // placed back in the mempool. Calling eth_getTransactionReceipt at this point will return null, until the tx is | ||
| // added to a new block and validated." | ||
| ctx, span := l.metrics.Tracer().Start(ctx, "limiter.checkReceipt") | ||
| defer func() { | ||
| metrics.EndSpanWithErr(span, err) | ||
| }() | ||
|
|
||
| receipt, err := l.evmClient.TransactionReceipt(ctx, request.OriginTxHash) | ||
| if err != nil { | ||
| if errors.Is(err, ethereum.NotFound) { | ||
| return false, nil | ||
| } | ||
| return false, fmt.Errorf("could not get transaction receipt: %w", err) | ||
| } | ||
|
|
||
| rfqAddr, err := l.cfg.GetRFQAddress(int(request.Transaction.OriginChainId)) | ||
| if err != nil { | ||
| return false, fmt.Errorf("could not get RFQ address: %w", err) | ||
| } | ||
|
|
||
| parser, err := fastbridge.NewParser(rfqAddr) | ||
| if err != nil { | ||
| return false, fmt.Errorf("could not create parser: %w", err) | ||
| } | ||
|
|
||
| for _, log := range receipt.Logs { | ||
| _, parsedEvent, ok := parser.ParseEvent(*log) | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| event, ok := parsedEvent.(*fastbridge.FastBridgeBridgeRequested) | ||
| if ok { | ||
| return rfqFieldsMatch(request, event) && !log.Removed, nil | ||
| } | ||
| } | ||
| span.SetAttributes( | ||
| attribute.String("receipt txHash", receipt.TxHash.Hex()), | ||
| attribute.String("receipt log address", receipt.ContractAddress.String()), | ||
| attribute.String("receipt data", hex.EncodeToString(receipt.Logs[0].Data)), | ||
| ) | ||
|
|
||
| return false, nil | ||
| } |
There was a problem hiding this comment.
New checkReceipt method enhances security, but needs a safeguard
The checkReceipt method is a valuable addition that verifies transaction receipts and handles potential reorg scenarios. The event parsing and field matching logic provide a robust check for transaction validity.
However, there's a potential issue:
Add a check before accessing log data to prevent a potential nil pointer dereference:
span.SetAttributes(
attribute.String("receipt txHash", receipt.TxHash.Hex()),
attribute.String("receipt log address", receipt.ContractAddress.String()),
- attribute.String("receipt data", hex.EncodeToString(receipt.Logs[0].Data)),
+ attribute.String("receipt data", func() string {
+ if len(receipt.Logs) > 0 {
+ return hex.EncodeToString(receipt.Logs[0].Data)
+ }
+ return "no logs available"
+ }()),
)This change will prevent a panic if the receipt has no logs, which could happen in certain edge cases.
📝 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.
| // checkReceipt checks if the receipt exists and has the correct fields in the case a reorg happened. | |
| func (l *limiterImpl) checkReceipt(ctx context.Context, request *reldb.QuoteRequest) (_ bool, err error) { | |
| // Make sure receipt exists and has the correct fields in case of a reorg. | |
| // Note: https://community.infura.io/t/does-eth-gettransactionreceipt-respond-to-re-orged-transactions/7765 | |
| // "You will get a tx receipt back but, as you note, there is a small chance of a reorg. | |
| // In a re-org the shorter side chain (one block usually, two occasionally) will have all its tx’s reverted and | |
| // placed back in the mempool. Calling eth_getTransactionReceipt at this point will return null, until the tx is | |
| // added to a new block and validated." | |
| ctx, span := l.metrics.Tracer().Start(ctx, "limiter.checkReceipt") | |
| defer func() { | |
| metrics.EndSpanWithErr(span, err) | |
| }() | |
| receipt, err := l.evmClient.TransactionReceipt(ctx, request.OriginTxHash) | |
| if err != nil { | |
| if errors.Is(err, ethereum.NotFound) { | |
| return false, nil | |
| } | |
| return false, fmt.Errorf("could not get transaction receipt: %w", err) | |
| } | |
| rfqAddr, err := l.cfg.GetRFQAddress(int(request.Transaction.OriginChainId)) | |
| if err != nil { | |
| return false, fmt.Errorf("could not get RFQ address: %w", err) | |
| } | |
| parser, err := fastbridge.NewParser(rfqAddr) | |
| if err != nil { | |
| return false, fmt.Errorf("could not create parser: %w", err) | |
| } | |
| for _, log := range receipt.Logs { | |
| _, parsedEvent, ok := parser.ParseEvent(*log) | |
| if !ok { | |
| continue | |
| } | |
| event, ok := parsedEvent.(*fastbridge.FastBridgeBridgeRequested) | |
| if ok { | |
| return rfqFieldsMatch(request, event) && !log.Removed, nil | |
| } | |
| } | |
| span.SetAttributes( | |
| attribute.String("receipt txHash", receipt.TxHash.Hex()), | |
| attribute.String("receipt log address", receipt.ContractAddress.String()), | |
| attribute.String("receipt data", hex.EncodeToString(receipt.Logs[0].Data)), | |
| ) | |
| return false, nil | |
| } | |
| // checkReceipt checks if the receipt exists and has the correct fields in the case a reorg happened. | |
| func (l *limiterImpl) checkReceipt(ctx context.Context, request *reldb.QuoteRequest) (_ bool, err error) { | |
| // Make sure receipt exists and has the correct fields in case of a reorg. | |
| // Note: https://community.infura.io/t/does-eth-gettransactionreceipt-respond-to-re-orged-transactions/7765 | |
| // "You will get a tx receipt back but, as you note, there is a small chance of a reorg. | |
| // In a re-org the shorter side chain (one block usually, two occasionally) will have all its tx's reverted and | |
| // placed back in the mempool. Calling eth_getTransactionReceipt at this point will return null, until the tx is | |
| // added to a new block and validated." | |
| ctx, span := l.metrics.Tracer().Start(ctx, "limiter.checkReceipt") | |
| defer func() { | |
| metrics.EndSpanWithErr(span, err) | |
| }() | |
| receipt, err := l.evmClient.TransactionReceipt(ctx, request.OriginTxHash) | |
| if err != nil { | |
| if errors.Is(err, ethereum.NotFound) { | |
| return false, nil | |
| } | |
| return false, fmt.Errorf("could not get transaction receipt: %w", err) | |
| } | |
| rfqAddr, err := l.cfg.GetRFQAddress(int(request.Transaction.OriginChainId)) | |
| if err != nil { | |
| return false, fmt.Errorf("could not get RFQ address: %w", err) | |
| } | |
| parser, err := fastbridge.NewParser(rfqAddr) | |
| if err != nil { | |
| return false, fmt.Errorf("could not create parser: %w", err) | |
| } | |
| for _, log := range receipt.Logs { | |
| _, parsedEvent, ok := parser.ParseEvent(*log) | |
| if !ok { | |
| continue | |
| } | |
| event, ok := parsedEvent.(*fastbridge.FastBridgeBridgeRequested) | |
| if ok { | |
| return rfqFieldsMatch(request, event) && !log.Removed, nil | |
| } | |
| } | |
| span.SetAttributes( | |
| attribute.String("receipt txHash", receipt.TxHash.Hex()), | |
| attribute.String("receipt log address", receipt.ContractAddress.String()), | |
| attribute.String("receipt data", func() string { | |
| if len(receipt.Logs) > 0 { | |
| return hex.EncodeToString(receipt.Logs[0].Data) | |
| } | |
| return "no logs available" | |
| }()), | |
| ) | |
| return false, nil | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 245-248: services/rfq/relayer/limiter/limiter.go#L245-L248
Added lines #L245 - L248 were not covered by tests
[warning] 253-254: services/rfq/relayer/limiter/limiter.go#L253-L254
Added lines #L253 - L254 were not covered by tests
[warning] 258-259: services/rfq/relayer/limiter/limiter.go#L258-L259
Added lines #L258 - L259 were not covered by tests
[warning] 264-264: services/rfq/relayer/limiter/limiter.go#L264
Added line #L264 was not covered by tests
[warning] 272-278: services/rfq/relayer/limiter/limiter.go#L272-L278
Added lines #L272 - L278 were not covered by tests
| // TODO: is this exhaustive? | ||
| func rfqFieldsMatch(request *reldb.QuoteRequest, event *fastbridge.FastBridgeBridgeRequested) bool { | ||
| transactionIDMatch := request.TransactionID == event.TransactionId | ||
| senderMatch := request.Sender.String() == event.Sender.String() | ||
| originAmountMatch := request.Transaction.OriginAmount.String() == event.OriginAmount.String() | ||
| destAmountMatch := request.Transaction.DestAmount.String() == event.DestAmount.String() | ||
| originTokenMatch := request.Transaction.OriginToken.String() == event.OriginToken.String() | ||
|
|
||
| return transactionIDMatch && senderMatch && originAmountMatch && destAmountMatch && originTokenMatch | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
rfqFieldsMatch function is not exhaustive
The rfqFieldsMatch function currently verifies transaction ID, sender, origin amount, destination amount, and origin token. However, the BridgeRequested event includes additional fields that should also be matched to ensure comprehensive validation:
- Destination Chain ID (
destChainId) - Destination Token (
destToken) - Send Chain Gas (
sendChainGas)
Please update the rfqFieldsMatch function to include these fields to address the TODO comment and enhance the reliability of the verification process.
🔗 Analysis chain
New rfqFieldsMatch function is comprehensive, but needs review
The rfqFieldsMatch function provides a thorough comparison of RFQ fields with event data, which is crucial for verifying the integrity of the transaction. The checks cover all essential fields: transaction ID, sender, amounts, and token.
However, there's an open question:
The TODO comment suggests that the field matching might not be exhaustive. Please review if there are any additional fields that should be checked. Consider adding checks for:
- Destination chain ID
- Destination token
- Recipient address
If these fields are not available in the event data, update the comment to explain why the current checks are sufficient.
This will help ensure that all necessary fields are being verified and improve the robustness of the check.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the BridgeRequested event definition to verify available fields
ast-grep --lang solidity --pattern 'event BridgeRequested(
$$$
)'
Length of output: 352
Script:
#!/bin/bash
# Search for the BridgeRequested event definition to verify available fields
ast-grep --lang solidity --pattern 'event BridgeRequested(
$$$
)'
Length of output: 186
Script:
#!/bin/bash
# Search for the BridgeRequested event definition in Solidity files
rg 'event BridgeRequested\(' --glob '*.sol'
Length of output: 648
Script:
#!/bin/bash
# Extract full definitions of the BridgeRequested event in Solidity files
rg -A 10 'event BridgeRequested\(' --glob '*.sol'
Length of output: 6623
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Description
A clear and concise description of the features you're adding in this pull request.
Additional context
Add any other context about the problem you're solving.
Metadata
Summary by CodeRabbit
New Features
VolumeLimitconfiguration for specific backend chains to enhance transaction management.Bug Fixes
Tests
Chores