refactor(rfq): move chain utils to new util pkg
#2982
Conversation
WalkthroughThe changes primarily involve refactoring the source of Ethereum address constants from the Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant Util
TestSuite->>Util: Request EthAddress
Util-->>TestSuite: Provide EthAddress
TestSuite->>Util: Check IsGasToken
Util-->>TestSuite: GasToken Validation Result
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 Configuration File (
|
Deploying sanguine-fe with
|
| Latest commit: |
13319d5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8fbe4c16.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-rfq-util.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: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (15)
- services/rfq/e2e/rfq_test.go (4 hunks)
- services/rfq/e2e/setup_test.go (4 hunks)
- services/rfq/relayer/chain/chain.go (2 hunks)
- services/rfq/relayer/inventory/manager.go (7 hunks)
- services/rfq/relayer/quoter/quoter.go (2 hunks)
- services/rfq/relayer/quoter/quoter_test.go (6 hunks)
- services/rfq/relayer/quoter/suite_test.go (3 hunks)
- services/rfq/relayer/relapi/client_test.go (3 hunks)
- services/rfq/relayer/relapi/handler.go (2 hunks)
- services/rfq/relayer/relapi/suite_test.go (2 hunks)
- services/rfq/relayer/relconfig/config.go (2 hunks)
- services/rfq/relayer/service/chainindexer.go (2 hunks)
- services/rfq/relayer/service/statushandler.go (2 hunks)
- services/rfq/util/doc.go (1 hunks)
- services/rfq/util/util.go (1 hunks)
Files skipped from review due to trivial changes (2)
- services/rfq/util/doc.go
- services/rfq/util/util.go
Additional context used
Learnings (2)
services/rfq/relayer/relapi/client_test.go (1)
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.services/rfq/relayer/relapi/handler.go (1)
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.
Additional comments not posted (26)
services/rfq/relayer/chain/chain.go (2)
19-19: LGTM!The addition of the
utilimport aligns with the refactoring objectives.
79-81: LGTM! But verify the function usage in the codebase.The change enhances modularity and maintainability by centralizing the logic for determining gas tokens within the
utilpackage.However, ensure that all function calls to
IsGasTokenare correctly updated to referenceutil.IsGasToken.Verification successful
Verification successful!
All function calls to
IsGasTokenare correctly updated to referenceutil.IsGasToken. The remaining instances are part of struct fields or variable names and do not require changes.
services/rfq/relayer/relapi/handler.goservices/rfq/relayer/quoter/quoter.goservices/rfq/relayer/chain/chain.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `IsGasToken` reference `util.IsGasToken`. # Test: Search for the function usage. Expect: Only occurrences of `util.IsGasToken`. rg --type go -A 5 $'IsGasToken'Length of output: 3921
services/rfq/relayer/quoter/suite_test.go (2)
19-19: LGTM!The import statement change aligns with the refactoring objectives and centralizes the utility functions.
56-56: LGTM! But verify the function usage in the codebase.The change aligns with the refactoring objectives and centralizes the utility functions.
However, ensure that all references to
EthAddress.String()are correctly updated to referenceutil.EthAddress.String().Also applies to: 81-81
Verification successful
All references to
EthAddress.String()have been correctly updated toutil.EthAddress.String().The refactoring aligns with the objectives and centralizes the utility functions.
services/rfq/relayer/quoter/suite_test.goservices/rfq/relayer/quoter/quoter_test.goservices/rfq/relayer/relconfig/config.goservices/rfq/e2e/setup_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `EthAddress.String()` reference `util.EthAddress.String()`. # Test: Search for the function usage. Expect: Only occurrences of `util.EthAddress.String()`. rg --type go -A 5 $'EthAddress.String()'Length of output: 3238
services/rfq/relayer/service/chainindexer.go (2)
17-17: LGTM!The import statement change aligns with the refactoring objectives and centralizes the utility functions.
183-183: LGTM! But verify the function usage in the codebase.The change aligns with the refactoring objectives and centralizes the utility functions.
However, ensure that all references to
EthAddressare correctly updated to referenceutil.EthAddress.Verification successful
All references to
EthAddresshave been correctly updated toutil.EthAddress.The search results confirm that the change aligns with the refactoring objectives, and the references are consistent across the codebase.
services/rfq/util/util.goservices/rfq/e2e/setup_test.goservices/rfq/relayer/service/statushandler.goservices/rfq/relayer/quoter/suite_test.goservices/rfq/relayer/quoter/quoter_test.goservices/rfq/relayer/relapi/client_test.goservices/rfq/e2e/rfq_test.goservices/rfq/relayer/relconfig/config.goservices/rfq/relayer/service/chainindexer.goservices/rfq/relayer/relapi/suite_test.goservices/rfq/relayer/inventory/manager.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `EthAddress` reference `util.EthAddress`. # Test: Search for the function usage. Expect: Only occurrences of `util.EthAddress`. rg --type go -A 5 $'EthAddress'Length of output: 13068
services/rfq/relayer/relapi/client_test.go (2)
75-75: LGTM!The reference to
chain.EthAddresshas been correctly updated toutil.EthAddress.
174-174: LGTM!The reference to
chain.EthAddresshas been correctly updated toutil.EthAddress.services/rfq/relayer/relapi/suite_test.go (2)
107-107: LGTM!The reference to
chain.EthAddresshas been correctly updated toutil.EthAddressin theQuotableTokensmap.
110-110: LGTM!The reference to
chain.EthAddresshas been correctly updated toutil.EthAddressin theQuotableTokensmap.services/rfq/relayer/service/statushandler.go (2)
19-19: LGTM!The import for
utilhas been correctly added to the file.
179-179: LGTM!The reference to
chain.EthAddresshas been correctly updated toutil.EthAddressin thegasMiddlewarefunction.services/rfq/relayer/relapi/handler.go (1)
220-220: Verify the usage ofutil.IsGasToken.Ensure that the
util.IsGasTokenfunction behaves as expected and is correctly integrated. This change centralizes the gas token identification logic, which is a good practice.Verification successful
Verify the definition and usage of
EthAddress.Ensure that
EthAddressis correctly defined and used within theutilpackage to ensure theIsGasTokenfunction behaves as expected.
The usage of
util.IsGasTokenis verified and correct.The
util.IsGasTokenfunction andEthAddressare correctly implemented and consistently used across the codebase. The change centralizes the gas token identification logic, which is a good practice.
util.IsGasTokenis correctly defined inservices/rfq/util/util.go.EthAddressis correctly defined and used throughout the project.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `util.IsGasToken` function. # Test: Search for the function definition. Expect: Correct implementation. rg --type go -A 5 $'func IsGasToken'Length of output: 190
Script:
#!/bin/bash # Description: Verify the definition and usage of `EthAddress`. # Test: Search for the definition of `EthAddress`. Expect: Correct definition. rg --type go -A 5 $'EthAddress'Length of output: 13068
services/rfq/relayer/quoter/quoter_test.go (1)
46-46: Verify the usage ofutil.EthAddress.Ensure that the
util.EthAddressconstant is correctly integrated and behaves as expected. This change centralizes the Ethereum address reference, which is a good practice.Also applies to: 57-57, 59-59, 71-71, 82-82, 84-84, 96-96
Verification successful
The
util.EthAddressconstant is correctly defined and integrated.The change centralizes the Ethereum address reference, which is a good practice and ensures consistency across the codebase.
util.EthAddressis defined ascommon.HexToAddress("0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE")inservices/rfq/util/util.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition of `util.EthAddress` constant. # Test: Search for the constant definition. Expect: Correct implementation. rg --type go -A 5 $'var EthAddress'Length of output: 415
services/rfq/relayer/relconfig/config.go (1)
262-262: Verify the usage ofutil.EthAddress.String().Ensure that the
util.EthAddress.String()constant is correctly integrated and behaves as expected. This change centralizes the Ethereum address reference, which is a good practice.Verification successful
Verified the usage of
util.EthAddress.String().The
util.EthAddressis defined usingcommon.HexToAddress, which returns an Ethereum address type from thego-ethereumlibrary. This type inherently has aStringmethod. The change centralizes the Ethereum address reference correctly.
services/rfq/util/util.go:var EthAddress = common.HexToAddress("0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE")Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition of `util.EthAddress` constant and its `String` method. # Test: Search for the constant definition and usage of the `String` method. Expect: Correct implementation. rg --type go -A 5 $'var EthAddress'Length of output: 415
Script:
#!/bin/bash # Description: Verify the implementation and usage of the String method for EthAddress. # Test: Search for the String method implementation related to EthAddress. Expect: Correct implementation and usage. rg --type go 'EthAddress.String()'Length of output: 800
Script:
#!/bin/bash # Description: Verify the implementation of the String method for EthAddress. # Test: Search for the String method implementation in the codebase. Expect: Correct implementation. rg --type go 'func (e EthAddress) String()'Length of output: 45
Script:
#!/bin/bash # Description: Verify the definition of common.HexToAddress and the type it returns. # Test: Search for the definition of HexToAddress in the common package. Expect: Identification of the type returned. rg --type go 'func HexToAddress' -A 5Length of output: 39
Script:
#!/bin/bash # Description: Verify the definition of common.HexToAddress and the type it returns. # Test: Search for the definition of HexToAddress in the common package with a broader search pattern. Expect: Identification of the type returned. rg --type go 'HexToAddress'Length of output: 35644
services/rfq/e2e/setup_test.go (4)
244-246: LGTM!The change from
chain.EthAddresstoutil.EthAddressis consistent and correct.
257-259: LGTM!The change from
chain.EthAddresstoutil.EthAddressis consistent and correct.
382-384: LGTM!The change from
chain.EthAddresstoutil.EthAddressis consistent and correct.
385-387: LGTM!The change from
chain.EthAddresstoutil.EthAddressis consistent and correct.services/rfq/e2e/rfq_test.go (3)
291-293: LGTM!The change from
chain.EthAddresstoutil.EthAddressis consistent and correct.
311-313: LGTM!The change from
chain.EthAddresstoutil.EthAddressis consistent and correct.
356-358: LGTM!The change from
chain.EthAddresstoutil.EthAddressis consistent and correct.services/rfq/relayer/quoter/quoter.go (1)
542-544: LGTM!The change from
chain.IsGasTokentoutil.IsGasTokenis consistent and correct.services/rfq/relayer/inventory/manager.go (3)
Line range hint
98-103:
LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
GetCommittableBalancematch the new reference toutil.EthAddress.Verification successful
Verification successful: All function calls to
GetCommittableBalancematch the new reference toutil.EthAddress.The changes are consistent across the codebase.
services/rfq/relayer/service/statushandler.goservices/rfq/relayer/service/handlers.goservices/rfq/relayer/quoter/quoter.goservices/rfq/relayer/inventory/manager.goservices/rfq/relayer/inventory/mocks/manager.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `GetCommittableBalance` match the new reference to `util.EthAddress`. # Test: Search for the function usage. Expect: Only occurrences of the new reference. rg --type go -A 5 $'GetCommittableBalance'Length of output: 7134
544-546: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
initializeTokensmatch the new reference toutil.EthAddress.Verification successful
Verified: The function usage matches the new reference to
util.EthAddress.The references to
util.EthAddressare correctly used within the fileservices/rfq/relayer/inventory/manager.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `initializeTokens` match the new reference to `util.EthAddress`. # Test: Search for the function usage. Expect: Only occurrences of the new reference. rg --type go -A 5 $'initializeTokens'Length of output: 1513
Script:
#!/bin/bash # Description: Verify the reference to `util.EthAddress` in the file `services/rfq/relayer/inventory/manager.go`. # Test: Search for the reference to `util.EthAddress` in the file. rg --type go 'util.EthAddress' services/rfq/relayer/inventory/manager.goLength of output: 519
Line range hint
314-352:
LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
ApproveAllTokensmatch the new reference toutil.EthAddress.Verification successful
All instances of
util.EthAddressare correctly referenced within theApproveAllTokensmethod.The verification confirms that the method
ApproveAllTokensinservices/rfq/relayer/inventory/manager.gocorrectly usesutil.EthAddressin all necessary places.
services/rfq/relayer/inventory/manager.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `ApproveAllTokens` match the new reference to `util.EthAddress`. # Test: Search for the function usage. Expect: Only occurrences of the new reference. rg --type go -A 5 $'ApproveAllTokens'Length of output: 2465
Script:
#!/bin/bash # Description: Verify the usage of `util.EthAddress` within the `ApproveAllTokens` method. # Test: Search for `util.EthAddress` within the method definition. rg --type go -A 10 'func (i *inventoryManagerImpl) ApproveAllTokens(ctx context.Context) error' | rg 'util\.EthAddress'Length of output: 121
Script:
#!/bin/bash # Description: Verify the usage of `util.EthAddress` within the `ApproveAllTokens` method. # Test: Search for `util.EthAddress` within the entire method definition. rg --type go -A 50 'func (i *inventoryManagerImpl) ApproveAllTokens(ctx context.Context) error' services/rfq/relayer/inventory/manager.go | rg 'util\.EthAddress'Length of output: 163
Script:
#!/bin/bash # Description: Verify the usage of `util.EthAddress` within the entire `services/rfq/relayer/inventory/manager.go` file. # Test: Search for `util.EthAddress` within the entire file. rg 'util\.EthAddress' services/rfq/relayer/inventory/manager.goLength of output: 512
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This pull request refactors the RFQ (Request for Quote) service by moving chain utilities from the 'chain' package to a new 'util' package, improving code organization and potential reusability.
- Created new
services/rfq/utilpackage withEthAddressandIsGasTokenfunctions - Updated import statements across multiple files to use the new
utilpackage - Replaced
chain.EthAddressandchain.IsGasTokenreferences withutil.EthAddressandutil.IsGasToken - Removed
IsGasTokenfunction fromservices/rfq/relayer/chain/chain.go - Added
services/rfq/util/doc.goto describe the purpose of the new utility package
15 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
No major changes found since last review. The previous summary accurately captures the key points of the refactoring, and there are no significant new changes to report.
No 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 selected for processing (1)
- .github/workflows/go.yml (6 hunks)
Additional comments not posted (4)
.github/workflows/go.yml (4)
7-8: Verify exclusion offe-releasebranch.Ensure that excluding the
fe-releasebranch from the workflow is intentional and aligns with the overall workflow strategy.
80-81: LGTM! Verify the correctness of updated conditions.The changes simplify the
runs-oncondition and refine theifcondition, enhancing readability and maintainability.Ensure that the updated conditions work as expected.
112-112: Verify exclusion of runners containing 'nsc' from caching.Ensure that excluding runners with names containing 'nsc' from caching is intentional and aligns with the overall caching strategy.
234-234: LGTM! Cleanup approved.Removing the commented-out
ifcondition cleans up the configuration file without impacting functionality.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2982 +/- ##
===================================================
+ Coverage 25.70802% 25.71691% +0.00888%
===================================================
Files 771 771
Lines 55613 55551 -62
Branches 80 82 +2
===================================================
- Hits 14297 14286 -11
+ Misses 39830 39781 -49
+ Partials 1486 1484 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
PR Summary
(updates since last review)
This PR introduces improvements to the token approval process in the RFQ (Request for Quote) system's end-to-end tests, focusing on error handling and retry logic.
- Enhanced
Approvefunction inservices/rfq/e2e/setup_test.gowith retry mechanism usingretry.WithBackoff - Added context and time imports to support the new retry logic
- Improved error handling with detailed error messages in the
Approvefunction - Introduced a 15-second maximum retry time for token approvals
- Utilized
core.CopyBigIntfor safer big integer operations in the approval process
These changes significantly improve the reliability and robustness of the e2e test setup for the RFQ system.
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
| if err != nil { | ||
| return fmt.Errorf("could not get token at %s: %w", token.Address().String(), err) | ||
| } |
There was a problem hiding this comment.
style: Ensure error messages are consistent with project standards
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/e2e/setup_test.go (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/e2e/setup_test.go
Summary by CodeRabbit