feat(withdrawal): print txhash of withdrawal #2845
Conversation
golangisfun123
requested review from
dwasse,
trajan0x,
aureliusbtc and
ChiTimesChi
as code owners
WalkthroughThe updates streamline dependencies across modules, primarily focusing on switching assertion and color libraries. There's a notable enhancement in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2845 +/- ##
===================================================
- Coverage 25.65579% 25.35979% -0.29600%
===================================================
Files 768 785 +17
Lines 55239 56491 +1252
Branches 80 80
===================================================
+ Hits 14172 14326 +154
- Misses 39594 40687 +1093
- Partials 1473 1478 +5
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: |
4c475e9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://77a91e9f.sanguine-fe.pages.dev |
| Branch Preview URL: | https://withdrawal-print.sanguine-fe.pages.dev |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
- Added spinner and print functionality for transaction hash in
services/rfq/relayer/cmd/commands.go - Introduced
GetTxHashByNoncemethod inservices/rfq/relayer/relapi/client.go - Added new endpoint
GetTxHashByNonceinservices/rfq/relayer/relapi/handler.go - Updated tests for ETH and ERC20 withdrawals in
services/rfq/relayer/relapi/client_test.go - Removed unnecessary dependencies in multiple
go.modfiles
14 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
![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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (6)
contrib/promexporter/go.sumis excluded by!**/*.sumcontrib/screener-api/go.sumis excluded by!**/*.sumcore/go.sumis excluded by!**/*.sumethergo/go.sumis excluded by!**/*.sumservices/omnirpc/go.sumis excluded by!**/*.sumservices/rfq/go.sumis excluded by!**/*.sum
Files selected for processing (14)
- agents/go.mod (1 hunks)
- core/go.mod (2 hunks)
- core/metrics/base.go (2 hunks)
- core/metrics/metrics.go (2 hunks)
- core/metrics/metrics_test.go (1 hunks)
- services/explorer/go.mod (1 hunks)
- services/rfq/go.mod (7 hunks)
- services/rfq/relayer/cmd/commands.go (3 hunks)
- services/rfq/relayer/relapi/client.go (3 hunks)
- services/rfq/relayer/relapi/client_test.go (2 hunks)
- services/rfq/relayer/relapi/handler.go (1 hunks)
- services/rfq/relayer/relapi/server.go (2 hunks)
- services/rfq/relayer/relapi/server_test.go (3 hunks)
- services/scribe/go.mod (1 hunks)
Files skipped from review due to trivial changes (5)
- agents/go.mod
- core/metrics/base.go
- core/metrics/metrics.go
- services/explorer/go.mod
- services/scribe/go.mod
Additional context used
Learnings (1)
services/rfq/relayer/relapi/client_test.go (2)
Learnt from: trajan0x PR: synapsecns/sanguine#2815 File: services/rfq/relayer/relapi/handler.go:154-156 Timestamp: 2024-06-29T05:11:50.952Z Learning: When adding new validation logic in the `Withdraw` function, ensure to cover it with unit tests to handle various scenarios.Learnt from: trajan0x PR: synapsecns/sanguine#2815 File: services/rfq/relayer/relapi/handler.go:153-156 Timestamp: 2024-06-29T05:12:50.268Z Learning: When adding new validation logic in the `Withdraw` function, ensure to cover it with unit tests to handle various scenarios.
GitHub Check: Lint (services/rfq)
services/rfq/relayer/cmd/commands.go
[failure] 124-124:
the variable nameclientErrshould conform to theerrXxxformat (errname)services/rfq/relayer/relapi/client.go
[failure] 128-128:
exported typeTxHashByNonceResponseshould have comment or be unexported (golint)services/rfq/relayer/relapi/handler.go
[failure] 215-215:
exported typeGetTxByNonceRequestshould have comment or be unexported (golint)
[failure] 220-220:
exported methodHandler.GetTxHashByNonceshould have comment or be unexported (golint)
Additional comments not posted (20)
core/metrics/metrics_test.go (1)
7-7: Verify compatibility of the new assertion library.Ensure that
github.com/alecthomas/assertprovides equivalent functionality asgithub.com/stretchr/testify/assert.services/rfq/relayer/cmd/commands.go (2)
146-146: Ensure correct printing of the transaction hash.The addition of printing the transaction hash enhances the functionality. Verify that the hash is printed correctly.
92-93: Ensure correct handling ofchainIDasbig.Int.The conversion from
uinttobig.Intshould handle larger chain IDs correctly. Verify that this change does not introduce any issues.Verification successful
Ensure correct handling of
chainIDasbig.Int.The verification process confirms that
chainIDis properly initialized, compared, and converted using appropriate methods for handling large values withbig.Int. The codebase shows consistent usage ofbig.NewInt,Cmp, andUint64conversions, ensuring that larger chain IDs are correctly managed.
- Initialization:
chainIDis initialized usingbig.NewInt, which correctly handles large integer values.- Comparisons:
chainIDis compared usingCmp, ensuring accurate comparisons ofbig.Intvalues.- Conversions:
chainIDis converted touint64where necessary, which is appropriate for handling large chain IDs within the limits ofuint64.No issues were found with the handling of
chainIDasbig.Int.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct handling of `chainID` as `big.Int`. # Test: Run the test suite to ensure all tests pass with the new `chainID` handling. go test ./...Length of output: 85
Script:
#!/bin/bash # Search for the usage of `chainID` in the codebase to verify correct handling. # Find all instances where `chainID` is used. ast-grep --lang go --pattern 'chainID'Length of output: 294490
Script:
#!/bin/bash # Verify correct initialization, comparisons, and conversions of `chainID`. # Search for initialization of `chainID` using `big.NewInt` ast-grep --lang go --pattern 'big.NewInt($_)' # Search for comparisons involving `chainID` ast-grep --lang go --pattern '$_.Cmp($_)' # Search for conversions of `chainID` to other types ast-grep --lang go --pattern '$_ = $_.Uint64()'Length of output: 1176
services/rfq/relayer/relapi/server.go (1)
104-104: Ensure correct registration of the new route.The new route
getTxHashByNonceRouteshould be correctly registered and handled. Verify that the route is accessible and functions as expected.Verification successful
The new route
/tx_hash/by_nonceis correctly registered and handled.
- The route is registered with the handler
GetTxHashByNonceinservices/rfq/relayer/relapi/server.go.- The handler function
GetTxHashByNonceis implemented inservices/rfq/relayer/relapi/handler.go.- There is test coverage for the
GetTxHashByNoncefunctionality inservices/rfq/relayer/relapi/client_test.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct registration of the new route. # Test: Use curl to test the new route and ensure it returns the expected response. # Note: Replace `localhost:your_port` with the actual server address and port. curl -X GET "http://localhost:your_port/tx_hash/by_nonce"Length of output: 111
Script:
#!/bin/bash # Search for the registration of the route `getTxHashByNonceRoute` in the codebase. rg "getTxHashByNonceRoute" # Search for the handler function associated with the route `getTxHashByNonceRoute`. rg "/tx_hash/by_nonce" # Check the implementation of the handler function. # Assuming we find the handler function name, we will search for its implementation. # For now, let's assume the handler function is named `GetTxHashByNonceHandler`. # If the actual name is different, we will adjust accordingly. rg "func GetTxHashByNonceHandler" # Search for any tests that cover the route `getTxHashByNonceRoute`. rg "tx_hash/by_nonce" --glob "*test*"Length of output: 462
Script:
#!/bin/bash # Search for the implementation of the handler function `GetTxHashByNonce`. rg "func GetTxHashByNonce" # Search for any tests that might cover `GetTxHashByNonce`. rg "GetTxHashByNonce" --glob "*test*"Length of output: 155
Script:
#!/bin/bash # Search for the method `GetTxHashByNonce` within struct definitions. rg "GetTxHashByNonce" --type go # Review the test in `services/rfq/relayer/relapi/client_test.go` to ensure it tests the `/tx_hash/by_nonce` route directly. rg "GetTxHashByNonce" services/rfq/relayer/relapi/client_test.go -A 10 -B 10Length of output: 1342
services/rfq/relayer/relapi/client.go (1)
132-149: Verify correctness of the new methodGetTxHashByNonce.Ensure that the new method
GetTxHashByNonceis correctly implemented and returns the expected response.services/rfq/relayer/relapi/client_test.go (3)
133-167: LGTM!The test for retrieving the transaction hash by nonce is well-structured and ensures proper functionality.
Line range hint
16-20:
LGTM!The health check test is straightforward and ensures the API is up and running.
Line range hint
24-37:
LGTM!The test for retrieving quote request status by transaction hash is well-structured and ensures proper functionality.
core/go.mod (3)
13-13: LGTM!The addition of
github.com/alecthomas/assert v1.0.0is appropriate and aligns with the changes incore/metrics/metrics_test.go.
73-73: LGTM!The addition of
github.com/alecthomas/colour v0.1.0is appropriate and aligns with the changes in the project.
74-74: LGTM!The addition of
github.com/alecthomas/repr v0.0.0-20210801044451-80ca428c5142is appropriate and aligns with the changes in the project.services/rfq/relayer/relapi/server_test.go (4)
Line range hint
17-26:
LGTM!The test for initializing the Quoter API server is well-structured and ensures proper functionality.
Line range hint
28-55:
LGTM!The test for retrieving quote request by transaction hash is well-structured and ensures proper functionality.
Line range hint
57-83:
LGTM!The test for retrieving quote request by transaction ID is well-structured and ensures proper functionality.
Line range hint
85-114:
LGTM!The test for retrying a transaction based on transaction hash is well-structured and ensures proper functionality.
services/rfq/relayer/relapi/handler.go (2)
Line range hint
30-44:
LGTM!The method for retrieving quote request status by transaction hash is well-structured and ensures proper functionality.
Tools
GitHub Check: Lint (services/rfq)
[failure] 215-215:
exported typeGetTxByNonceRequestshould have comment or be unexported (golint)
[failure] 220-220:
exported methodHandler.GetTxHashByNonceshould have comment or be unexported (golint)
Line range hint
46-60:
LGTM!The method for retrieving quote request status by transaction ID is well-structured and ensures proper functionality.
Tools
GitHub Check: Lint (services/rfq)
[failure] 215-215:
exported typeGetTxByNonceRequestshould have comment or be unexported (golint)
[failure] 220-220:
exported methodHandler.GetTxHashByNonceshould have comment or be unexported (golint)services/rfq/go.mod (3)
11-11: Verify the necessity and suitability of the new spinner dependency.The dependency
github.com/charmbracelet/huh/spinneris added. Ensure it is necessary for the new functionality and is a suitable choice for the project.
Line range hint
83-104:
Verify the necessity and appropriateness of new indirect dependencies.The following indirect dependencies are added:
github.com/charmbracelet/bubblesgithub.com/charmbracelet/bubbleteagithub.com/charmbracelet/lipglossgithub.com/charmbracelet/x/ansigithub.com/charmbracelet/x/inputgithub.com/charmbracelet/x/termgithub.com/charmbracelet/x/windowsEnsure these dependencies are necessary for the new functionality and are appropriate for the project.
214-218: Verify the necessity and appropriateness of additional indirect dependencies.The following indirect dependencies are added:
github.com/lucasb-eyer/go-colorfulgithub.com/mattn/go-colorablegithub.com/mattn/go-isattygithub.com/mattn/go-localereadergithub.com/muesli/ansigithub.com/muesli/cancelreadergithub.com/muesli/termenvEnsure these dependencies are necessary for the new functionality and are appropriate for the project.
Also applies to: 231-233
| func (c *RelayerClientSuite) TestEthWithdraw() { | ||
| backend := c.underlying.testBackends[uint64(c.underlying.originChainID)] | ||
|
|
||
| startBalance, err := backend.BalanceAt(c.GetTestContext(), testWithdrawalAddress, nil) | ||
| c.Require().NoError(err) | ||
|
|
||
| withdrawalAmount := big.NewInt(50) | ||
|
|
||
| _, err = c.Client.Withdraw(c.GetTestContext(), &relapi.WithdrawRequest{ | ||
| ChainID: uint32(backend.GetChainID()), | ||
| To: testWithdrawalAddress, | ||
| Amount: withdrawalAmount.String(), | ||
| TokenAddress: chain.EthAddress, | ||
| }) | ||
| c.Require().NoError(err) | ||
|
|
||
| // Wait for the transaction to be mined | ||
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | ||
| balance, err := backend.BalanceAt(ctx, testWithdrawalAddress, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("could not fetch balance %w", err) | ||
| } | ||
|
|
||
| expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount) | ||
|
|
||
| if balance.Cmp(expectedBalance) != 0 { | ||
| return fmt.Errorf("balance not updated") | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
| c.Require().NoError(err) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure proper test coverage for new validation logic.
The current test suite does not seem to cover scenarios for invalid token addresses and non-whitelisted addresses. Please add test cases to cover these scenarios to ensure robust validation.
- Add a test case for invalid token addresses.
- Add a test case for non-whitelisted addresses.
Analysis chain
Ensure proper test coverage for new validation logic.
The withdrawal functionality has new validation logic. Ensure that the test cases cover various scenarios, including invalid token addresses and non-whitelisted addresses.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for new validation logic in the Withdraw function.
# Test: Search for test cases covering invalid token addresses and non-whitelisted addresses.
rg --type go 'invalid token address|withdrawal address is not whitelisted'
Length of output: 254
| func (c *RelayerClientSuite) TestERC20WithdrawCLI() { | ||
| backend := c.underlying.testBackends[uint64(c.underlying.originChainID)] | ||
|
|
||
| _, erc20 := c.underlying.deployManager.GetMockERC20(c.GetTestContext(), backend) | ||
|
|
||
| startBalance, err := erc20.BalanceOf(&bind.CallOpts{Context: c.GetTestContext()}, testWithdrawalAddress) | ||
| c.Require().NoError(err) | ||
|
|
||
| withdrawalAmount := big.NewInt(1000000000000000000) | ||
|
|
||
| res, err := c.Client.Withdraw(c.GetTestContext(), &relapi.WithdrawRequest{ | ||
| ChainID: c.underlying.originChainID, | ||
| To: common.HexToAddress(testWithdrawalAddress.String()), | ||
| Amount: withdrawalAmount.String(), | ||
| TokenAddress: erc20.Address(), | ||
| }) | ||
| c.Require().NoError(err) | ||
|
|
||
| // Wait for the transaction to be mined | ||
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | ||
| balance, err := erc20.BalanceOf(&bind.CallOpts{Context: ctx}, testWithdrawalAddress) | ||
| if err != nil { | ||
| return fmt.Errorf("could not fetch balance %w", err) | ||
| } | ||
|
|
||
| expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount) | ||
|
|
||
| if balance.Cmp(expectedBalance) != 0 { | ||
| return fmt.Errorf("balance not updated") | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
|
|
||
| c.Require().NoError(err) | ||
| c.Require().NotNil(res) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure proper test coverage for new validation logic.
The withdrawal functionality has new validation logic. Ensure that the test cases cover various scenarios, including invalid token addresses and non-whitelisted addresses.
- Add test cases in
services/rfq/relayer/relapi/client_test.goor appropriate test files to cover invalid token addresses. - Add test cases to cover scenarios where the withdrawal address is not whitelisted.
Analysis chain
Ensure proper test coverage for new validation logic.
The withdrawal functionality has new validation logic. Ensure that the test cases cover various scenarios, including invalid token addresses and non-whitelisted addresses.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for new validation logic in the Withdraw function.
# Test: Search for test cases covering invalid token addresses and non-whitelisted addresses.
rg --type go 'invalid token address|withdrawal address is not whitelisted'
Length of output: 254
Script:
#!/bin/bash
# Description: Verify comprehensive test coverage for new validation logic in the Withdraw function.
# Test: Search for all test cases related to the Withdraw function.
rg --type go 'Withdraw' | grep '_test.go'
Length of output: 5682
| func (c *RelayerClientSuite) TestEthWithdrawCLI() { | ||
| res, err := c.Client.Withdraw(c.GetTestContext(), &relapi.WithdrawRequest{ | ||
| ChainID: c.underlying.originChainID, | ||
| To: common.HexToAddress(testWithdrawalAddress.String()), | ||
| Amount: "1000000000000000000", | ||
| TokenAddress: chain.EthAddress, | ||
| }) | ||
| c.Require().NoError(err) | ||
|
|
||
| // Wait for the transaction to be mined | ||
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | ||
| status, err := c.underlying.database.SubmitterDB(). | ||
| GetNonceStatus( | ||
| ctx, | ||
| c.underlying.wallet.Address(), | ||
| big.NewInt(int64(c.underlying.originChainID)), | ||
| res.Nonce, | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get status %w", err) | ||
| } | ||
|
|
||
| if status != submitterdb.Stored { | ||
| return fmt.Errorf("transaction not mined") | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
| c.Require().NoError(err) | ||
| c.Require().NotNil(res) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure proper test coverage for new validation logic.
The withdrawal functionality has new validation logic. Ensure that the test cases cover various scenarios, including invalid token addresses and non-whitelisted addresses. Currently, there are no test cases covering these scenarios.
- Add test cases for invalid token addresses.
- Add test cases for non-whitelisted addresses.
Analysis chain
Ensure proper test coverage for new validation logic.
The withdrawal functionality has new validation logic. Ensure that the test cases cover various scenarios, including invalid token addresses and non-whitelisted addresses.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for new validation logic in the Withdraw function.
# Test: Search for test cases covering invalid token addresses and non-whitelisted addresses.
rg --type go 'invalid token address|withdrawal address is not whitelisted'
Length of output: 254
Script:
#!/bin/bash
# Description: Search for test cases covering invalid token addresses and non-whitelisted addresses.
# Find all test files in the repository.
test_files=$(fd --type f --extension go --search-path services/rfq/relayer/relapi)
# Search for test cases covering invalid token addresses and non-whitelisted addresses.
for file in $test_files; do
echo "Checking $file for invalid token address and non-whitelisted address test cases..."
rg 'invalid token address|withdrawal address is not whitelisted' "$file"
done
Length of output: 3997
| func (c *RelayerClientSuite) TestERC20Withdraw() { | ||
| backend := c.underlying.testBackends[uint64(c.underlying.originChainID)] | ||
|
|
||
| _, erc20 := c.underlying.deployManager.GetMockERC20(c.GetTestContext(), backend) | ||
|
|
||
| startBalance, err := erc20.BalanceOf(&bind.CallOpts{Context: c.GetTestContext()}, testWithdrawalAddress) | ||
| c.Require().NoError(err) | ||
|
|
||
| withdrawalAmount := big.NewInt(50) | ||
|
|
||
| _, err = c.Client.Withdraw(c.GetTestContext(), &relapi.WithdrawRequest{ | ||
| ChainID: uint32(backend.GetChainID()), | ||
| To: testWithdrawalAddress, | ||
| Amount: withdrawalAmount.String(), | ||
| TokenAddress: erc20.Address(), | ||
| }) | ||
| c.Require().NoError(err) | ||
|
|
||
| // Wait for the transaction to be mined | ||
| err = retry.WithBackoff(c.GetTestContext(), func(ctx context.Context) error { | ||
| balance, err := erc20.BalanceOf(&bind.CallOpts{Context: ctx}, testWithdrawalAddress) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get balance %w", err) | ||
| } | ||
|
|
||
| expectedBalance := new(big.Int).Add(startBalance, withdrawalAmount) | ||
|
|
||
| if balance.Cmp(expectedBalance) != 0 { | ||
| return fmt.Errorf("balance not updated") | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
| c.Require().NoError(err) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure proper test coverage for new validation logic.
The withdrawal functionality has new validation logic. Ensure that the test cases cover various scenarios, including invalid token addresses and non-whitelisted addresses.
- Add test cases for invalid token addresses.
- Add test cases for non-whitelisted addresses.
Analysis chain
Ensure proper test coverage for new validation logic.
The withdrawal functionality has new validation logic. Ensure that the test cases cover various scenarios, including invalid token addresses and non-whitelisted addresses.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for new validation logic in the Withdraw function.
# Test: Search for test cases covering invalid token addresses and non-whitelisted addresses.
rg --type go 'invalid token address|withdrawal address is not whitelisted'
Length of output: 254
Script:
#!/bin/bash
# Description: Verify test coverage for new validation logic in the Withdraw function.
# Test: Search for test cases covering invalid token addresses and non-whitelisted addresses in the test files.
rg --type go 'invalid token address|withdrawal address is not whitelisted' --tests
Length of output: 516
Script:
#!/bin/bash
# Description: Verify test coverage for new validation logic in the Withdraw function.
# Test: Search for test cases covering invalid token addresses and non-whitelisted addresses in the test files.
rg --type go 'invalid token address|withdrawal address is not whitelisted' --glob '*_test.go'
Length of output: 95
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Improved error handling in
services/rfq/relayer/cmd/commands.gofor thewithdrawcommand, including printing the transaction hash. - Added
GetTxHashByNoncemethod inservices/rfq/relayer/relapi/client.gofor retrieving transaction hash by nonce. - Introduced
GetTxHashByNonceendpoint inservices/rfq/relayer/relapi/handler.gowith a new request structGetTxByNonceRequest. - Minor cleanup in
services/rfq/relayer/relapi/suite_test.goby removing redundant variable declaration.
4 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- services/rfq/relayer/cmd/commands.go (3 hunks)
- services/rfq/relayer/relapi/client.go (4 hunks)
- services/rfq/relayer/relapi/handler.go (1 hunks)
- services/rfq/relayer/relapi/suite_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- services/rfq/relayer/relapi/suite_test.go
Files skipped from review as they are similar to previous changes (3)
- services/rfq/relayer/cmd/commands.go
- services/rfq/relayer/relapi/client.go
- services/rfq/relayer/relapi/handler.go
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Modified error message for invalid
chainIDinservices/rfq/relayer/relapi/handler.goto exclude string representation of the invalidchainID.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added new API endpoint to retrieve transaction hash by nonce in
/services/rfq/relayer/relapi/suite_test.go - Configured dependencies for the new endpoint in
/services/rfq/relayer/relapi/suite_test.go - Ensured test suite coverage for the new functionality in
/services/rfq/relayer/relapi/suite_test.go - Reorganized import statements for better readability in
/services/rfq/relayer/relapi/suite_test.go
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
core/metrics/metrics_test.go
Outdated
| "github.com/stretchr/testify/assert" | ||
| "testing" | ||
|
|
||
| "github.com/alecthomas/assert" |
There was a problem hiding this comment.
if i edit the file, undo the changes, then save, gopls still reorders the imports
There was a problem hiding this comment.
btw I meant using "github.com/alecthomas/assert" instead of "github.com/stretchr/testify/assert"
There was a problem hiding this comment.
Wrong import inference I think, wasn't intentional. Removed now
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Reorganized import statements for better readability in
/core/metrics/metrics_test.go
No major changes found since last review.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- .vscode/settings.json (1 hunks)
- services/rfq/relayer/relapi/handler.go (1 hunks)
- services/rfq/relayer/relapi/suite_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- .vscode/settings.json
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/relapi/suite_test.go
Additional context used
GitHub Check: codecov/patch
services/rfq/relayer/relapi/handler.go
[warning] 228-230: services/rfq/relayer/relapi/handler.go#L228-L230
Added lines #L228 - L230 were not covered by tests
[warning] 234-236: services/rfq/relayer/relapi/handler.go#L234-L236
Added lines #L234 - L236 were not covered by tests
[warning] 240-242: services/rfq/relayer/relapi/handler.go#L240-L242
Added lines #L240 - L242 were not covered by tests
Additional comments not posted (2)
services/rfq/relayer/relapi/handler.go (2)
215-215: Add a comment for the exported struct.The struct
GetTxByNonceRequestis exported but lacks a comment.
221-221: Add a comment for the exported method.The method
GetTxHashByNonceis exported but lacks a comment.
| c.JSON(http.StatusBadRequest, gin.H{"error": "invalid chainID"}) | ||
| return | ||
| } |
There was a problem hiding this comment.
Add test coverage for invalid chainID.
The error handling for invalid chainID is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 228-230: services/rfq/relayer/relapi/handler.go#L228-L230
Added lines #L228 - L230 were not covered by tests
| c.JSON(http.StatusBadRequest, gin.H{"error": "invalid nonce"}) | ||
| return | ||
| } |
There was a problem hiding this comment.
Add test coverage for invalid nonce.
The error handling for invalid nonce is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 234-236: services/rfq/relayer/relapi/handler.go#L234-L236
Added lines #L234 - L236 were not covered by tests
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("could not get tx hash: %s", err.Error())}) | ||
| return | ||
| } |
There was a problem hiding this comment.
Add test coverage for internal server error.
The error handling for internal server errors in GetTxHashByNonce is not covered by tests.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 240-242: services/rfq/relayer/relapi/handler.go#L240-L242
Added lines #L240 - L242 were not covered by tests
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Removed dependencies:
github.com/alecthomas/assert,github.com/alecthomas/colour,github.com/alecthomas/reprin/core/go.mod - Reorganized import statements in
/core/metrics/metrics_test.go
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (3)
contrib/opbot/go.sumis excluded by!**/*.sumservices/cctp-relayer/go.sumis excluded by!**/*.sumservices/rfq/go.sumis excluded by!**/*.sum
Files selected for processing (9)
- core/metrics/base.go (2 hunks)
- services/cctp-relayer/go.mod (2 hunks)
- services/cctp-relayer/relayer/util_test.go (1 hunks)
- services/rfq/go.mod (8 hunks)
- services/rfq/relayer/relapi/client.go (4 hunks)
- services/rfq/relayer/relapi/client_test.go (2 hunks)
- services/rfq/relayer/relapi/handler.go (1 hunks)
- services/rfq/relayer/relapi/server.go (2 hunks)
- services/rfq/relayer/relconfig/config_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- services/cctp-relayer/go.mod
Files skipped from review as they are similar to previous changes (5)
- core/metrics/base.go
- services/rfq/go.mod
- services/rfq/relayer/relapi/client.go
- services/rfq/relayer/relapi/handler.go
- services/rfq/relayer/relapi/server.go
Additional context used
Learnings (1)
services/rfq/relayer/relapi/client_test.go (6)
Learnt from: trajan0x PR: synapsecns/sanguine#2815 File: services/rfq/relayer/relapi/handler.go:162-165 Timestamp: 2024-07-02T14:43:19.311Z Learning: When adding new validation logic in the `Withdraw` function, ensure to cover it with unit tests to handle various scenarios, including valid and invalid addresses.Learnt from: trajan0x PR: synapsecns/sanguine#2815 File: services/rfq/relayer/relapi/handler.go:154-156 Timestamp: 2024-06-29T05:11:50.952Z Learning: When adding new validation logic in the `Withdraw` function, ensure to cover it with unit tests to handle various scenarios.Learnt from: trajan0x PR: synapsecns/sanguine#2815 File: services/rfq/relayer/relapi/handler.go:153-156 Timestamp: 2024-06-29T05:12:50.268Z Learning: When adding new validation logic in the `Withdraw` function, ensure to cover it with unit tests to handle various scenarios.Learnt from: trajan0x PR: synapsecns/sanguine#2815 File: services/rfq/relayer/relapi/handler.go:162-165 Timestamp: 2024-07-02T14:49:46.636Z Learning: When adding new validation logic in the `Withdraw` function, ensure to export helper functions like `toAddressIsWhitelisted` for direct unit testing.Learnt from: trajan0x PR: synapsecns/sanguine#2815 File: services/rfq/relayer/relapi/handler.go:280-282 Timestamp: 2024-07-02T14:43:41.835Z Learning: When the `UnmarshalJSON` method is added or modified in the `WithdrawRequest` struct, ensure to cover it with unit tests to handle various scenarios, including valid and invalid JSON inputs.Learnt from: trajan0x PR: synapsecns/sanguine#2815 File: services/rfq/relayer/relconfig/config.go:163-181 Timestamp: 2024-07-02T14:44:18.051Z Learning: When adding the `DecodeTokenID` function in `services/rfq/relayer/relconfig/config.go`, ensure to cover it with unit tests to handle various scenarios.
Additional comments not posted (7)
services/cctp-relayer/relayer/util_test.go (1)
4-4: Import Library Change: Ensure ConsistencyThe assertion library has been switched from
github.com/alecthomas/asserttogithub.com/stretchr/testify/assert. Ensure this change is consistent across the codebase.Verification successful
Import Library Change: Consistency Verified
The assertion library has been switched to
github.com/stretchr/testify/assertconsistently across the codebase. No instances ofgithub.com/alecthomas/assertwere found.
- Verified consistent use of
github.com/stretchr/testify/assertScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent use of the assertion library across the codebase. # Test: Search for imports of `github.com/alecthomas/assert` and `github.com/stretchr/testify/assert`. rg 'import "github.com/alecthomas/assert"' --type go rg 'import "github.com/stretchr/testify/assert"' --type goLength of output: 115
Script:
#!/bin/bash # Description: Verify consistent use of the assertion library across the codebase. # Test: Search for imports of `github.com/alecthomas/assert` and `github.com/stretchr/testify/assert`. rg 'github.com/alecthomas/assert' --type go rg 'github.com/stretchr/testify/assert' --type goLength of output: 14871
services/rfq/relayer/relapi/client_test.go (5)
64-96: Ensure Proper Test Coverage forTestEthWithdrawThe test function
TestEthWithdrawis well-implemented, covering the withdrawal process and balance verification. Ensure similar coverage for other test cases.
98-132: Ensure Proper Test Coverage forTestERC20WithdrawThe test function
TestERC20Withdrawis well-implemented, covering the ERC20 withdrawal process and balance verification. Ensure similar coverage for other test cases.
134-168: Ensure Proper Test Coverage forTestGetTxHashThe test function
TestGetTxHashis well-implemented, covering the retrieval of transaction hashes by nonce. Ensure similar coverage for other test cases.
170-200: Ensure Proper Test Coverage forTestEthWithdrawCLIThe test function
TestEthWithdrawCLIis well-implemented, covering the CLI-based Ethereum withdrawal process and transaction status verification. Ensure similar coverage for other test cases.
202-238: Ensure Proper Test Coverage forTestERC20WithdrawCLIThe test function
TestERC20WithdrawCLIis well-implemented, covering the CLI-based ERC20 withdrawal process and balance verification. Ensure similar coverage for other test cases.services/rfq/relayer/relconfig/config_test.go (1)
4-4: Import Library Change: Ensure ConsistencyThe assertion library has been switched from
github.com/alecthomas/asserttogithub.com/stretchr/testify/assert. Ensure this change is consistent across the codebase.Verification successful
Import Library Change: Verified Consistency
The assertion library
github.com/stretchr/testify/assertis used consistently across the codebase, with no instances ofgithub.com/alecthomas/assertfound.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent use of the assertion library across the codebase. # Test: Search for imports of `github.com/alecthomas/assert` and `github.com/stretchr/testify/assert`. rg 'import "github.com/alecthomas/assert"' --type go rg 'import "github.com/stretchr/testify/assert"' --type goLength of output: 115
Script:
#!/bin/bash # Description: Verify consistent use of the assertion library across the codebase. # Test: Search for any mentions of `github.com/alecthomas/assert` and `github.com/stretchr/testify/assert` in Go files. rg 'github.com/alecthomas/assert' --type go rg 'github.com/stretchr/testify/assert' --type goLength of output: 14871
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added
carryforward: trueto 'cctp-relayer' and 'rfq' paths in.codecov.yml - Introduced
rfqRefundcommand incontrib/opbot/botmd/commands.go - Updated dependencies in
agents/go.modandcontrib/git-changes-action/go.mod - Added new methods and routes for transaction handling in
services/rfq/relayer/relapi/client.goandhandler.go - Introduced new configuration fields and methods in
services/rfq/guard/guardconfig/config.goandguarddb/base/store.go
119 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
|
|
||
| chainID, ok := new(big.Int).SetString(chainIDStr, 10) | ||
| if !ok { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "invalid chainID"}) |
There was a problem hiding this comment.
🪶 style: Potentially misleading error message: chainID is invalid but the error message includes its string representation.
| nonce, ok := new(big.Int).SetString(nonceStr, 10) | ||
| if !ok { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "invalid nonce"}) |
There was a problem hiding this comment.
🪶 style: Consider adding more specific error handling for invalid nonce values.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
Files selected for processing (2)
- services/rfq/go.mod (8 hunks)
- services/rfq/relayer/relapi/client_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- services/rfq/go.mod
- services/rfq/relayer/relapi/client_test.go
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Added
status_countmetric todocs/bridge/docs/rfq/Relayer/Relayer.md - Introduced OpenTelemetry recorder in
ethergo/listener/listener.goandotel.go - Enhanced error handling in
services/cctp-relayer/relayer/relayer.go - Removed
github.com/cornelk/hashmapdependency inservices/rfq/go.mod - Added
GetStatusCountsmethod inservices/rfq/relayer/reldb/db.goandquote.go
14 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
- Updated
services/rfq/go.modto includegithub.com/cornelk/hashmapdependency. - Enhanced
services/rfq/relayer/relapi/client_test.gowith new test cases for Ethereum and ERC20 token withdrawals. - Improved test coverage for transaction hash retrieval by nonce.
- Added spinner for visual feedback during transaction status checks.
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
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
Tests
Chores
editor.formatOnSavesetting in the VSCode configuration.