feat(opbot): add additional relay check [SLT-359] #3312
Conversation
|
Warning Rate limit exceeded@golangisfun123 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 53 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 in this pull request focus on enhancing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
golangisfun123
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3312 +/- ##
===================================================
+ Coverage 30.49751% 33.22601% +2.72850%
===================================================
Files 417 544 +127
Lines 27658 34786 +7128
Branches 82 82
===================================================
+ Hits 8435 11558 +3123
- Misses 18437 22209 +3772
- Partials 786 1019 +233
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
contrib/opbot/botmd/commands.go (2)
341-345: Avoid variable shadowing oferrin error handlingIn line 341, the
errvariable is re-declared within the inner scope:_, err := ctx.Response().Reply("error fetching bridge relays")This shadows the outer
errvariable from line 339. Shadowing can lead to confusion and potential bugs, especially in more complex functions. It's advisable to use a different variable name or handle the error without re-declaringerr.Apply this diff to avoid variable shadowing:
- _, err := ctx.Response().Reply("error fetching bridge relays") - if err != nil { - log.Println(err) - } + _, replyErr := ctx.Response().Reply("error fetching bridge relays") + if replyErr != nil { + log.Println(replyErr) + }
348-352: Avoid variable shadowing oferrwhen sending responseSimilarly, in line 348,
erris re-declared when sending the response:_, err := ctx.Response().Reply("transaction has already been relayed")This shadows the outer
errvariable. To improve code clarity and maintainability, consider renaming the innererrvariable.Apply this diff to avoid variable shadowing:
- _, err := ctx.Response().Reply("transaction has already been relayed") - if err != nil { - log.Println(err) - } + _, replyErr := ctx.Response().Reply("transaction has already been relayed") + if replyErr != nil { + log.Println(replyErr) + }
contrib/opbot/botmd/commands.go
Outdated
| isRelayed, err := fastBridgeContract.BridgeRelays(nil, [32]byte(common.Hex2Bytes(rawRequest.TxID))) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply("error fetching bridge relays") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Use common.HexToHash for proper TxID conversion
The current code uses [32]byte(common.Hex2Bytes(rawRequest.TxID)) to convert rawRequest.TxID to a [32]byte array. This approach may lead to incorrect results if rawRequest.TxID is not exactly 64 hexadecimal characters (representing 32 bytes). If the TxID is shorter or longer than expected, this conversion can cause incorrect behavior or even a panic.
To ensure accurate and safe conversion, consider using common.HexToHash(rawRequest.TxID), which returns a common.Hash (an alias for [32]byte) and properly handles the hex string conversion.
Apply this diff to fix the issue:
-isRelayed, err := fastBridgeContract.BridgeRelays(nil, [32]byte(common.Hex2Bytes(rawRequest.TxID)))
+isRelayed, err := fastBridgeContract.BridgeRelays(nil, common.HexToHash(rawRequest.TxID))📝 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.
| isRelayed, err := fastBridgeContract.BridgeRelays(nil, [32]byte(common.Hex2Bytes(rawRequest.TxID))) | |
| if err != nil { | |
| _, err := ctx.Response().Reply("error fetching bridge relays") | |
| if err != nil { | |
| log.Println(err) | |
| } | |
| return | |
| } | |
| isRelayed, err := fastBridgeContract.BridgeRelays(nil, common.HexToHash(rawRequest.TxID)) | |
| if err != nil { | |
| _, err := ctx.Response().Reply("error fetching bridge relays") | |
| if err != nil { | |
| log.Println(err) | |
| } | |
| return | |
| } |
Deploying sanguine-fe with
|
| Latest commit: |
3bc3966
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://18c7ce61.sanguine-fe.pages.dev |
| Branch Preview URL: | https://additional-screening.sanguine-fe.pages.dev |
contrib/opbot/botmd/commands.go
Outdated
| @@ -336,6 +336,22 @@ func (b *Bot) rfqRefund() *slacker.CommandDefinition { | |||
| return | |||
| } | |||
|
|
|||
| isRelayed, err := fastBridgeContract.BridgeRelays(nil, [32]byte(common.Hex2Bytes(rawRequest.TxID))) | |||
There was a problem hiding this comment.
fastBridgeContract is the contract on the origin chain, whereas we need to check if the relay happened on the destination.
I guess one of the possible solutions would involve adjusting makeFastBridge to make both origin and destination instances.
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
contrib/opbot/botmd/commands.go (3)
Line range hint
313-353: Add test coverage for the new relay check logicThe new relay check functionality lacks test coverage. Consider adding test cases for:
- Transaction already relayed scenario
- Error handling for BridgeRelays call
- Integration with both origin and destination contracts
Would you like me to help create test cases for these scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 338-344: contrib/opbot/botmd/commands.go#L338-L344
Added lines #L338 - L344 were not covered by tests
[warning] 346-351: contrib/opbot/botmd/commands.go#L346-L351
Added lines #L346 - L351 were not covered by tests
[warning] 355-355: contrib/opbot/botmd/commands.go#L355
Added line #L355 was not covered by tests
395-396: Fix comment formatting and improve documentationThe function comment should end with a period and provide more context about the return values.
Apply this diff:
-// returns origin and dest fast bridge contracts +// makeFastBridge creates and returns FastBridge contracts for both origin and destination chains.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 396-396: contrib/opbot/botmd/commands.go#L396
Added line #L396 was not covered by tests🪛 GitHub Check: Lint (contrib/opbot)
[failure] 395-395:
Comment should end in a period (godot)
407-429: Consider improving error handling and adding testsThe error handling could be more descriptive and the new functionality needs test coverage. Additionally, consider using named return values for better code clarity.
Apply this diff:
-func (b *Bot) makeFastBridge(ctx context.Context, req *relapi.GetQuoteRequestResponse) (*fastbridge.FastBridge, *fastbridge.FastBridge, error) { +func (b *Bot) makeFastBridge(ctx context.Context, req *relapi.GetQuoteRequestResponse) (originBridge, destBridge *fastbridge.FastBridge, err error) { client, err := rfqClient.NewUnauthenticatedClient(b.handler, b.cfg.RFQApiURL) if err != nil { - return nil, nil, fmt.Errorf("error creating rfq client: %w", err) + return nil, nil, fmt.Errorf("failed to create RFQ client: %w", err) }Would you like me to help create test cases for the updated function?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 407-408: contrib/opbot/botmd/commands.go#L407-L408
Added lines #L407 - L408 were not covered by tests
[warning] 410-414: contrib/opbot/botmd/commands.go#L410-L414
Added lines #L410 - L414 were not covered by tests
[warning] 416-419: contrib/opbot/botmd/commands.go#L416-L419
Added lines #L416 - L419 were not covered by tests
[warning] 421-424: contrib/opbot/botmd/commands.go#L421-L424
Added lines #L421 - L424 were not covered by tests
[warning] 426-426: contrib/opbot/botmd/commands.go#L426
Added line #L426 was not covered by tests
[warning] 429-429: contrib/opbot/botmd/commands.go#L429
Added line #L429 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
contrib/opbot/botmd/commands.go(3 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
contrib/opbot/botmd/commands.go
[warning] 313-313: contrib/opbot/botmd/commands.go#L313
Added line #L313 was not covered by tests
[warning] 338-344: contrib/opbot/botmd/commands.go#L338-L344
Added lines #L338 - L344 were not covered by tests
[warning] 346-351: contrib/opbot/botmd/commands.go#L346-L351
Added lines #L346 - L351 were not covered by tests
[warning] 355-355: contrib/opbot/botmd/commands.go#L355
Added line #L355 was not covered by tests
[warning] 396-396: contrib/opbot/botmd/commands.go#L396
Added line #L396 was not covered by tests
[warning] 399-399: contrib/opbot/botmd/commands.go#L399
Added line #L399 was not covered by tests
[warning] 404-404: contrib/opbot/botmd/commands.go#L404
Added line #L404 was not covered by tests
[warning] 407-408: contrib/opbot/botmd/commands.go#L407-L408
Added lines #L407 - L408 were not covered by tests
[warning] 410-414: contrib/opbot/botmd/commands.go#L410-L414
Added lines #L410 - L414 were not covered by tests
[warning] 416-419: contrib/opbot/botmd/commands.go#L416-L419
Added lines #L416 - L419 were not covered by tests
[warning] 421-424: contrib/opbot/botmd/commands.go#L421-L424
Added lines #L421 - L424 were not covered by tests
[warning] 426-426: contrib/opbot/botmd/commands.go#L426
Added line #L426 was not covered by tests
[warning] 429-429: contrib/opbot/botmd/commands.go#L429
Added line #L429 was not covered by tests
🪛 GitHub Check: Lint (contrib/opbot)
contrib/opbot/botmd/commands.go
[failure] 395-395:
Comment should end in a period (godot)
contrib/opbot/botmd/commands.go
Outdated
| isRelayed, err := fastBridgeContractDest.BridgeRelays(nil, [32]byte(common.Hex2Bytes(rawRequest.TxID))) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply("error fetching bridge relays") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
| if isRelayed { | ||
| _, err := ctx.Response().Reply("transaction has already been relayed") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Use common.HexToHash for safe TxID conversion
The current TxID conversion using [32]byte(common.Hex2Bytes()) is unsafe and could panic if the input is malformed. Use common.HexToHash() for safe conversion.
Apply this diff:
-isRelayed, err := fastBridgeContractDest.BridgeRelays(nil, [32]byte(common.Hex2Bytes(rawRequest.TxID)))
+isRelayed, err := fastBridgeContractDest.BridgeRelays(nil, common.HexToHash(rawRequest.TxID))📝 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.
| isRelayed, err := fastBridgeContractDest.BridgeRelays(nil, [32]byte(common.Hex2Bytes(rawRequest.TxID))) | |
| if err != nil { | |
| _, err := ctx.Response().Reply("error fetching bridge relays") | |
| if err != nil { | |
| log.Println(err) | |
| } | |
| return | |
| } | |
| if isRelayed { | |
| _, err := ctx.Response().Reply("transaction has already been relayed") | |
| if err != nil { | |
| log.Println(err) | |
| } | |
| return | |
| } | |
| isRelayed, err := fastBridgeContractDest.BridgeRelays(nil, common.HexToHash(rawRequest.TxID)) | |
| if err != nil { | |
| _, err := ctx.Response().Reply("error fetching bridge relays") | |
| if err != nil { | |
| log.Println(err) | |
| } | |
| return | |
| } | |
| if isRelayed { | |
| _, err := ctx.Response().Reply("transaction has already been relayed") | |
| if err != nil { | |
| log.Println(err) | |
| } | |
| return | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 338-344: contrib/opbot/botmd/commands.go#L338-L344
Added lines #L338 - L344 were not covered by tests
[warning] 346-351: contrib/opbot/botmd/commands.go#L346-L351
Added lines #L346 - L351 were not covered by tests
contrib/opbot/botmd/commands.go
Outdated
| @@ -377,32 +392,41 @@ func (b *Bot) rfqRefund() *slacker.CommandDefinition { | |||
| } | |||
| } | |||
|
|
|||
| func (b *Bot) makeFastBridge(ctx context.Context, req *relapi.GetQuoteRequestResponse) (*fastbridge.FastBridge, error) { | |||
| // returns origin and dest fast bridge contracts | |||
There was a problem hiding this comment.
Needs a period in the end :)
contrib/opbot/botmd/commands.go
Outdated
| var fastBridges [2]*fastbridge.FastBridge | ||
| chainIDs := []uint32{req.OriginChainID, req.DestChainID} |
There was a problem hiding this comment.
Now that I reread my initial comment about making two contract instances, this does feel a bit clunky. Do you think that passing chain ID instead of GetQuoteRequestResponse would be cleaner/more generic (and making a single FastBridge instance for it)?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
contrib/opbot/botmd/commands.go (2)
Line range hint
404-431: Add test coverage for makeFastBridge method.The new implementation of
makeFastBridgelacks test coverage, which is critical as it's used for both origin and destination chain operations.Would you like me to help create unit tests for this method? The tests should cover:
- Success case with valid chain ID
- Error cases:
- Invalid RFQ client creation
- Missing contract address for chain ID
- Invalid chain client
- Invalid contract initialization
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 415-415: contrib/opbot/botmd/commands.go#L415
Added line #L415 was not covered by tests
[warning] 417-417: contrib/opbot/botmd/commands.go#L417
Added line #L417 was not covered by tests
[warning] 420-420: contrib/opbot/botmd/commands.go#L420
Added line #L420 was not covered by tests
[warning] 422-422: contrib/opbot/botmd/commands.go#L422
Added line #L422 was not covered by tests
[warning] 427-427: contrib/opbot/botmd/commands.go#L427
Added line #L427 was not covered by tests
415-422: Improve error messages for chain client and contract lookup failures.The error messages could be more specific about what went wrong during the chain client initialization and contract lookup.
Apply this diff:
chainClient, err := b.rpcClient.GetChainClient(ctx, int(chainID)) if err != nil { - return nil, fmt.Errorf("error getting chain client for chain ID %d: %w", chainID, err) + return nil, fmt.Errorf("failed to initialize chain client for chain ID %d (check RPC endpoint configuration): %w", chainID, err) } contractAddress, ok := contracts.Contracts[chainID] if !ok { - return nil, fmt.Errorf("contract address not found for chain ID %d", chainID) + return nil, fmt.Errorf("FastBridge contract not deployed on chain ID %d", chainID) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 415-415: contrib/opbot/botmd/commands.go#L415
Added line #L415 was not covered by tests
[warning] 417-417: contrib/opbot/botmd/commands.go#L417
Added line #L417 was not covered by tests
[warning] 420-420: contrib/opbot/botmd/commands.go#L420
Added line #L420 was not covered by tests
[warning] 422-422: contrib/opbot/botmd/commands.go#L422
Added line #L422 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
contrib/opbot/botmd/commands.go(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
contrib/opbot/botmd/commands.go
[warning] 313-313: contrib/opbot/botmd/commands.go#L313
Added line #L313 was not covered by tests
[warning] 338-344: contrib/opbot/botmd/commands.go#L338-L344
Added lines #L338 - L344 were not covered by tests
[warning] 347-353: contrib/opbot/botmd/commands.go#L347-L353
Added lines #L347 - L353 were not covered by tests
[warning] 355-360: contrib/opbot/botmd/commands.go#L355-L360
Added lines #L355 - L360 were not covered by tests
[warning] 364-364: contrib/opbot/botmd/commands.go#L364
Added line #L364 was not covered by tests
[warning] 404-404: contrib/opbot/botmd/commands.go#L404
Added line #L404 was not covered by tests
[warning] 415-415: contrib/opbot/botmd/commands.go#L415
Added line #L415 was not covered by tests
[warning] 417-417: contrib/opbot/botmd/commands.go#L417
Added line #L417 was not covered by tests
[warning] 420-420: contrib/opbot/botmd/commands.go#L420
Added line #L420 was not covered by tests
[warning] 422-422: contrib/opbot/botmd/commands.go#L422
Added line #L422 was not covered by tests
[warning] 427-427: contrib/opbot/botmd/commands.go#L427
Added line #L427 was not covered by tests
🔇 Additional comments (1)
contrib/opbot/botmd/commands.go (1)
347-361:
Use common.HexToHash for safe TxID conversion.
The current TxID conversion using [32]byte(common.Hex2Bytes()) is unsafe and could panic if the input is malformed.
Apply this diff:
-isRelayed, err := fastBridgeContractDest.BridgeRelays(nil, [32]byte(common.Hex2Bytes(rawRequest.TxID)))
+isRelayed, err := fastBridgeContractDest.BridgeRelays(nil, common.HexToHash(rawRequest.TxID))🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 347-353: contrib/opbot/botmd/commands.go#L347-L353
Added lines #L347 - L353 were not covered by tests
[warning] 355-360: contrib/opbot/botmd/commands.go#L355-L360
Added lines #L355 - L360 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
core/bytes.go (1)
28-28: Add documentation for exported function.Following the package's documentation style, please add a comment explaining the function's purpose, parameters, and return values.
+// BytesToArray converts a byte slice of length 32 into a fixed-size array. +// Returns an error if the input slice length is not 32 bytes. func BytesToArray(bz []byte) ([32]byte, error) {🧰 Tools
🪛 GitHub Check: Lint (core)
[failure] 28-28:
exported: exported function BytesToArray should have comment or be unexported (revive)contrib/opbot/botmd/commands.go (2)
347-354: Improve error message for TxID conversion failure.The current error message "error converting tx id" doesn't provide enough context about what went wrong. Consider including the actual error details.
- _, err := ctx.Response().Reply("error converting tx id") + _, err := ctx.Response().Reply(fmt.Sprintf("Invalid transaction ID format: %v", err))
Line range hint
412-439: Add test coverage for makeFastBridge function.The refactored function lacks test coverage. As this is a critical component used by multiple commands, it should be thoroughly tested.
Would you like me to help create test cases covering:
- Successful contract creation
- Error handling for invalid chain IDs
- RPC client errors
- Contract address lookup failures
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 423-423: contrib/opbot/botmd/commands.go#L423
Added line #L423 was not covered by tests
[warning] 425-425: contrib/opbot/botmd/commands.go#L425
Added line #L425 was not covered by tests
[warning] 428-428: contrib/opbot/botmd/commands.go#L428
Added line #L428 was not covered by tests
[warning] 430-430: contrib/opbot/botmd/commands.go#L430
Added line #L430 was not covered by tests
[warning] 435-435: contrib/opbot/botmd/commands.go#L435
Added line #L435 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
contrib/opbot/botmd/commands.go(5 hunks)core/bytes.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
contrib/opbot/botmd/commands.go
[warning] 314-314: contrib/opbot/botmd/commands.go#L314
Added line #L314 was not covered by tests
[warning] 339-345: contrib/opbot/botmd/commands.go#L339-L345
Added lines #L339 - L345 were not covered by tests
[warning] 356-361: contrib/opbot/botmd/commands.go#L356-L361
Added lines #L356 - L361 were not covered by tests
[warning] 363-368: contrib/opbot/botmd/commands.go#L363-L368
Added lines #L363 - L368 were not covered by tests
[warning] 372-372: contrib/opbot/botmd/commands.go#L372
Added line #L372 was not covered by tests
[warning] 412-412: contrib/opbot/botmd/commands.go#L412
Added line #L412 was not covered by tests
[warning] 423-423: contrib/opbot/botmd/commands.go#L423
Added line #L423 was not covered by tests
[warning] 425-425: contrib/opbot/botmd/commands.go#L425
Added line #L425 was not covered by tests
[warning] 428-428: contrib/opbot/botmd/commands.go#L428
Added line #L428 was not covered by tests
[warning] 430-430: contrib/opbot/botmd/commands.go#L430
Added line #L430 was not covered by tests
[warning] 435-435: contrib/opbot/botmd/commands.go#L435
Added line #L435 was not covered by tests
🪛 GitHub Check: Lint (core)
core/bytes.go
[failure] 28-28:
exported: exported function BytesToArray should have comment or be unexported (revive)
🔇 Additional comments (3)
core/bytes.go (1)
29-35: LGTM! Implementation is secure and efficient.
The function correctly handles:
- Input validation
- Safe byte copying
- Proper error handling
contrib/opbot/botmd/commands.go (2)
355-369: LGTM: Relay check implementation is solid.
The implementation correctly:
- Checks if the transaction is already relayed
- Provides clear user feedback
- Prevents duplicate processing
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 356-361: contrib/opbot/botmd/commands.go#L356-L361
Added lines #L356 - L361 were not covered by tests
[warning] 363-368: contrib/opbot/botmd/commands.go#L363-L368
Added lines #L363 - L368 were not covered by tests
Line range hint 412-439: LGTM: Improved makeFastBridge implementation.
The refactored implementation:
- Has a clearer, more focused purpose
- Provides detailed error messages with context
- Properly validates chain ID and contract existence
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 423-423: contrib/opbot/botmd/commands.go#L423
Added line #L423 was not covered by tests
[warning] 425-425: contrib/opbot/botmd/commands.go#L425
Added line #L425 was not covered by tests
[warning] 428-428: contrib/opbot/botmd/commands.go#L428
Added line #L428 was not covered by tests
[warning] 430-430: contrib/opbot/botmd/commands.go#L430
Added line #L430 was not covered by tests
[warning] 435-435: contrib/opbot/botmd/commands.go#L435
Added line #L435 was not covered by tests
contrib/opbot/botmd/commands.go
Outdated
| fastBridgeContractDest, err := b.makeFastBridge(ctx.Context(), rawRequest.DestChainID) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply(err.Error()) | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
| txBz, err := core.BytesToArray(common.Hex2Bytes(rawRequest.TxID)) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply("error converting tx id") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
| isRelayed, err := fastBridgeContractDest.BridgeRelays(nil, txBz) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply("error fetching bridge relays") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
| if isRelayed { | ||
| _, err := ctx.Response().Reply("transaction has already been relayed") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Add test coverage for the relay check functionality.
The newly added relay check is a critical validation that prevents duplicate processing. However, this code path lacks test coverage, which could lead to undetected issues in production.
Could you like me to help create comprehensive test cases covering:
- Successful relay check
- Error scenarios (invalid TxID, contract errors)
- Edge cases
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 339-345: contrib/opbot/botmd/commands.go#L339-L345
Added lines #L339 - L345 were not covered by tests
[warning] 356-361: contrib/opbot/botmd/commands.go#L356-L361
Added lines #L356 - L361 were not covered by tests
[warning] 363-368: contrib/opbot/botmd/commands.go#L363-L368
Added lines #L363 - L368 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
core/bytes.go (2)
29-30: Enhance documentation for exported functionThe current documentation is minimal for an exported function in a core package. Consider expanding it to include:
- Use cases or scenarios where this function should be used
- Example usage
- Description of error conditions
Example improvement:
-// BytesToArray converts a slice to a 32 length byte array. +// BytesToArray converts a byte slice to a fixed-size 32-byte array. +// This function is commonly used when working with fixed-size cryptographic values +// such as hashes or keys. +// +// Returns an error if the input slice length is not exactly 32 bytes. +// +// Example: +// +// input := make([]byte, 32) +// array, err := BytesToArray(input) +// if err != nil { +// // Handle error +// }
30-37: Implementation looks good with a minor suggestionThe function implementation is solid with proper validation and efficient copying. Consider making the error message consistent with Go's standard library style.
- return bytes, fmt.Errorf("invalid length of bytes: %d", len(bz)) + return bytes, fmt.Errorf("bytes: invalid length %d, want 32", len(bz))🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 30-36: core/bytes.go#L30-L36
Added lines #L30 - L36 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
core/bytes.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
core/bytes.go
[warning] 30-36: core/bytes.go#L30-L36
Added lines #L30 - L36 were not covered by tests
🔇 Additional comments (1)
core/bytes.go (1)
30-37: Verify usage of BytesToArray in related changes
Since this is a new utility function, let's verify its usage across the codebase, particularly in the rfqRefund changes mentioned in the PR.
✅ Verification successful
BytesToArray usage appears correct with proper error handling
The function is currently used in only one location (contrib/opbot/botmd/commands.go) where:
- It's called with
common.Hex2Bytes()which produces a byte slice - The error return is properly checked
- The context suggests it's being used for transaction ID handling, which typically requires 32-byte arrays
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential usages of BytesToArray to ensure proper error handling
rg -l "BytesToArray" | while read -r file; do
echo "=== $file ==="
# Show the function calls with surrounding context
rg -B 2 -A 2 "BytesToArray" "$file"
done
Length of output: 626
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 30-36: core/bytes.go#L30-L36
Added lines #L30 - L36 were not covered by tests
| func BytesToArray(bz []byte) ([32]byte, error) { | ||
| var bytes [32]byte | ||
| if len(bz) != 32 { | ||
| return bytes, fmt.Errorf("invalid length of bytes: %d", len(bz)) | ||
| } | ||
| copy(bytes[:], bz) | ||
| return bytes, nil | ||
| } |
There was a problem hiding this comment.
Add unit tests for BytesToArray function
The function lacks test coverage according to codecov report. As this is a core utility function, comprehensive testing is essential to ensure reliable behavior across different scenarios.
Would you like me to help generate unit tests covering:
- Success case with valid 32-byte input
- Error case with invalid length input
- Edge cases with zero-filled arrays
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 30-36: core/bytes.go#L30-L36
Added lines #L30 - L36 were not covered by tests
There was a problem hiding this comment.
LGTM. The failing explorer test is unrelated to the PR and will be fixed once #3353 is merged
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
contrib/opbot/botmd/commands.go (1)
Line range hint
356-383: LGTM! Improved error handling with chain ID context.The refactored
makeFastBridgemethod provides better error messages by including the chain ID in error contexts, making debugging easier.However, the new implementation lacks test coverage. Consider adding tests for:
- Contract initialization with valid chain ID
- Error cases (invalid chain ID, missing contract address)
- RPC client errors
Would you like me to help create test cases for these scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 367-367: contrib/opbot/botmd/commands.go#L367
Added line #L367 was not covered by tests
[warning] 369-369: contrib/opbot/botmd/commands.go#L369
Added line #L369 was not covered by tests
[warning] 372-372: contrib/opbot/botmd/commands.go#L372
Added line #L372 was not covered by tests
[warning] 379-379: contrib/opbot/botmd/commands.go#L379
Added line #L379 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
contrib/opbot/botmd/commands.go(5 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint (contrib/opbot)
contrib/opbot/botmd/commands.go
[failure] 252-252:
G115: integer overflow conversion int -> uint32 (gosec)
[failure] 277-277:
G115: integer overflow conversion int -> uint32 (gosec)
🪛 GitHub Check: codecov/patch
contrib/opbot/botmd/commands.go
[warning] 278-283: contrib/opbot/botmd/commands.go#L278-L283
Added lines #L278 - L283 were not covered by tests
[warning] 286-291: contrib/opbot/botmd/commands.go#L286-L291
Added lines #L286 - L291 were not covered by tests
[warning] 293-299: contrib/opbot/botmd/commands.go#L293-L299
Added lines #L293 - L299 were not covered by tests
[warning] 301-306: contrib/opbot/botmd/commands.go#L301-L306
Added lines #L301 - L306 were not covered by tests
[warning] 356-356: contrib/opbot/botmd/commands.go#L356
Added line #L356 was not covered by tests
[warning] 367-367: contrib/opbot/botmd/commands.go#L367
Added line #L367 was not covered by tests
[warning] 369-369: contrib/opbot/botmd/commands.go#L369
Added line #L369 was not covered by tests
[warning] 372-372: contrib/opbot/botmd/commands.go#L372
Added line #L372 was not covered by tests
[warning] 379-379: contrib/opbot/botmd/commands.go#L379
Added line #L379 was not covered by tests
🔇 Additional comments (2)
contrib/opbot/botmd/commands.go (2)
285-292: LGTM! Proper error handling for BytesToArray conversion.
The code properly handles potential errors from the BytesToArray conversion and provides a clear error message.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 286-291: contrib/opbot/botmd/commands.go#L286-L291
Added lines #L286 - L291 were not covered by tests
293-307: LGTM! Proper relay check implementation.
The relay check is well-implemented with appropriate error handling and clear user feedback.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 293-299: contrib/opbot/botmd/commands.go#L293-L299
Added lines #L293 - L299 were not covered by tests
[warning] 301-306: contrib/opbot/botmd/commands.go#L301-L306
Added lines #L301 - L306 were not covered by tests
| @@ -250,7 +249,7 @@ | |||
| return | |||
| } | |||
|
|
|||
| fastBridgeContract, err := b.makeFastBridge(ctx.Context(), rawRequest) | |||
| fastBridgeContractOrigin, err := b.makeFastBridge(ctx.Context(), uint32(rawRequest.Bridge.OriginChainID)) | |||
There was a problem hiding this comment.
Add bounds checking for chain ID conversions.
The conversions from int to uint32 for both origin and destination chain IDs could lead to integer overflow. Add validation to ensure the values are within uint32 range.
Apply this diff:
-fastBridgeContractOrigin, err := b.makeFastBridge(ctx.Context(), uint32(rawRequest.Bridge.OriginChainID))
+if rawRequest.Bridge.OriginChainID < 0 || rawRequest.Bridge.OriginChainID > math.MaxUint32 {
+ return nil, fmt.Errorf("origin chain ID %d out of uint32 range", rawRequest.Bridge.OriginChainID)
+}
+fastBridgeContractOrigin, err := b.makeFastBridge(ctx.Context(), uint32(rawRequest.Bridge.OriginChainID))
-fastBridgeContractDest, err := b.makeFastBridge(ctx.Context(), uint32(rawRequest.Bridge.DestChainID))
+if rawRequest.Bridge.DestChainID < 0 || rawRequest.Bridge.DestChainID > math.MaxUint32 {
+ return nil, fmt.Errorf("destination chain ID %d out of uint32 range", rawRequest.Bridge.DestChainID)
+}
+fastBridgeContractDest, err := b.makeFastBridge(ctx.Context(), uint32(rawRequest.Bridge.DestChainID))Also applies to: 277-277
🧰 Tools
🪛 GitHub Check: Lint (contrib/opbot)
[failure] 252-252:
G115: integer overflow conversion int -> uint32 (gosec)
| if err != nil { | ||
| _, err := ctx.Response().Reply(err.Error()) | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
| txBz, err := core.BytesToArray(common.Hex2Bytes(rawRequest.Bridge.TransactionID[2:])) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply("error converting tx id") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
| isRelayed, err := fastBridgeContractDest.BridgeRelays(nil, txBz) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply("error fetching bridge relays") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } | ||
| if isRelayed { | ||
| _, err := ctx.Response().Reply("transaction has already been relayed") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for the relay check functionality.
The newly added relay check is a critical validation that prevents duplicate processing. However, this code path lacks test coverage.
Please add test cases covering:
- Successful relay check
- Error scenarios (invalid TxID, contract errors)
- Edge cases (chain ID validation)
Would you like me to help create comprehensive test cases for these scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 278-283: contrib/opbot/botmd/commands.go#L278-L283
Added lines #L278 - L283 were not covered by tests
[warning] 286-291: contrib/opbot/botmd/commands.go#L286-L291
Added lines #L286 - L291 were not covered by tests
[warning] 293-299: contrib/opbot/botmd/commands.go#L293-L299
Added lines #L293 - L299 were not covered by tests
[warning] 301-306: contrib/opbot/botmd/commands.go#L301-L306
Added lines #L301 - L306 were not covered by tests
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
Bug Fixes