feat(rfq-api): add v2 contracts to rfq api endpoint [SLT-429] #3386
Conversation
WalkthroughThe pull request introduces significant changes to the configuration and handling of fast bridge contracts within the RFQ service. Key modifications include the restructuring of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lintlevel=warning msg="[config_reader] The configuration option 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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3386 +/- ##
===================================================
- Coverage 33.20876% 33.18956% -0.01920%
===================================================
Files 544 544
Lines 34786 34776 -10
Branches 82 82
===================================================
- Hits 11552 11542 -10
Misses 22216 22216
Partials 1018 1018
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: |
e5d9dcc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1596657e.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-rfq-api-contracts.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: 3
🧹 Outside diff range and nitpick comments (7)
services/rfq/api/config/config.go (2)
24-25: Add documentation for the new contract fieldsPlease add comments explaining:
- The purpose of these contract maps
- The expected format of contract addresses
- The relationship between v1 and v2 contracts
Example:
+ // FastBridgeContractsV1 maps chain IDs to their respective v1 fast bridge contract addresses + // Format: map[chainID]contractAddress FastBridgeContractsV1 map[uint32]string `yaml:"fast_bridge_contracts_v1"` + // FastBridgeContractsV2 maps chain IDs to their respective v2 fast bridge contract addresses + // Format: map[chainID]contractAddress FastBridgeContractsV2 map[uint32]string `yaml:"fast_bridge_contracts_v2"`
24-25: Consider adding validation for contract addressesThe current implementation doesn't validate the contract addresses. Consider adding validation to:
- Ensure contract addresses are valid hex strings
- Check for duplicate chain IDs across v1 and v2 maps
- Verify contract addresses meet minimum length requirements
Would you like me to provide an implementation for a validation method?
services/rfq/api/model/response.go (1)
44-44: Enhance API DocumentationConsider adding API version information to the struct documentation. For example:
-// GetContractsResponse contains the schema for a GET /contracts response. +// GetContractsResponse contains the schema for a GET /contracts response (API v2). +// Breaking changes from v1: +// - Replaced single Contracts map with separate ContractsV1 and ContractsV2 mapsservices/rfq/api/client/suite_test.go (1)
88-95: Consider adding integration tests for version-specific behaviors.While the basic configuration is set up for both v1 and v2 contracts, it would be valuable to add test cases that specifically verify:
- Version-specific contract interactions
- Version compatibility checks
- Upgrade scenarios from v1 to v2
services/rfq/api/rest/handler.go (1)
304-307: Update API documentation to reflect new response structureThe implementation looks good with proper separation of v1 and v2 contracts. However, the Swagger documentation needs to be updated.
Update the @success annotation to reflect the actual response structure:
- @Success 200 {array} model.GetContractsResponse + @Success 200 {object} model.GetContractsResponseservices/rfq/api/rest/server.go (2)
98-99: Address TODO comment: Implement role checking for v2 contractsThe TODO comment indicates missing role checking functionality for v2 contracts. This could lead to security issues if v2 contracts have different role requirements.
Would you like me to help implement the role checking logic for v2 contracts?
Line range hint
140-146: Add validation and consider thread safetyThe struct initialization has two potential issues:
- No validation for required fields before initialization
- Concurrent access to
fastBridgeContractsandroleCachemaps could lead to race conditionsConsider these improvements:
- Add validation:
+if len(fastBridgeContracts) == 0 { + return nil, fmt.Errorf("no fast bridge contracts initialized") +} +if len(roles) == 0 { + return nil, fmt.Errorf("no role caches initialized") +} q := &QuoterAPIServer{
- Use sync.Map for thread-safe operations:
-fastBridgeContracts: fastBridgeContracts, -roleCache: roles, +fastBridgeContracts: &sync.Map{}, +roleCache: &sync.Map{},Then initialize the maps:
for k, v := range fastBridgeContracts { q.fastBridgeContracts.Store(k, v) } for k, v := range roles { q.roleCache.Store(k, v) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
services/rfq/api/client/suite_test.go(1 hunks)services/rfq/api/config/config.go(1 hunks)services/rfq/api/model/response.go(1 hunks)services/rfq/api/rest/handler.go(1 hunks)services/rfq/api/rest/server.go(2 hunks)services/rfq/api/rest/server_test.go(1 hunks)services/rfq/api/rest/suite_test.go(1 hunks)
🔇 Additional comments (4)
services/rfq/api/model/response.go (1)
44-49:
Breaking API Change: Review Client Impact
The restructuring from a single Contracts map to separate ContractsV1 and ContractsV2 maps is a breaking change. Ensure that:
- All API clients are notified of this change
- API versioning is in place
- A migration path is provided for existing clients
✅ Verification successful
Verification Successful: No Impact on External Clients
No external API clients reference the old Contracts field. The restructuring to ContractsV1 and ContractsV2 does not impact external consumers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for API clients that might be affected by this change
rg -A 2 "Contracts\""
Length of output: 1332
Script:
#!/bin/bash
# Search for references to the old 'Contracts' field in the codebase
rg 'Contracts"' --glob '*.go' --glob '*.ts' --context 5
Length of output: 3000
services/rfq/api/client/suite_test.go (2)
92-95: Verify the intention of using identical addresses for v1 and v2 contracts.
The configuration uses the same addresses for both v1 and v2 contracts. If this is intentional for testing purposes, please add a comment explaining why. If not, consider using different addresses to ensure proper separation and testing of both contract versions.
Let's check if this pattern exists in other test files:
#!/bin/bash
# Search for other test files that might show the intended pattern
ast-grep --pattern 'FastBridgeContractsV1.*FastBridgeContractsV2'88-91: Consider adding test cases for invalid v1 contract addresses.
The test configuration for v1 fast bridge contracts looks good. However, consider adding negative test cases to verify the behavior when invalid addresses are provided.
Let's verify the error handling for invalid addresses:
services/rfq/api/rest/suite_test.go (1)
85-88: 🛠️ Refactor suggestion
Address type mismatch and potential code duplication.
Two issues to consider:
-
There's a type mismatch between the configuration maps (using
uint32) andfastBridgeAddressMap(usinguint64), which could lead to issues when working with chain IDs. -
Both v1 and v2 maps are using identical addresses. If this is intentional, consider extracting these common values to reduce duplication.
Consider these improvements:
+ // Common addresses for both v1 and v2 contracts
+ commonBridgeContracts := map[uint64]string{
+ 1: ethFastBridgeAddress.Hex(),
+ 42161: arbFastBridgeAddress.Hex(),
+ }
testConfig := config.Config{
// ...
- FastBridgeContractsV1: map[uint32]string{
- 1: ethFastBridgeAddress.Hex(),
- 42161: arbFastBridgeAddress.Hex(),
- },
- FastBridgeContractsV2: map[uint32]string{
- 1: ethFastBridgeAddress.Hex(),
- 42161: arbFastBridgeAddress.Hex(),
- },
+ FastBridgeContractsV1: commonBridgeContracts,
+ FastBridgeContractsV2: commonBridgeContracts,Let's verify the chain ID type usage across the codebase:
Also applies to: 89-92
| Database DatabaseConfig `yaml:"database"` | ||
| OmniRPCURL string `yaml:"omnirpc_url"` | ||
| FastBridgeContractsV1 map[uint32]string `yaml:"fast_bridge_contracts_v1"` | ||
| FastBridgeContractsV2 map[uint32]string `yaml:"fast_bridge_contracts_v2"` | ||
| Port string `yaml:"port"` | ||
| RelayAckTimeout time.Duration `yaml:"relay_ack_timeout"` | ||
| MaxQuoteAge time.Duration `yaml:"max_quote_age"` |
There was a problem hiding this comment.
💡 Codebase verification
Existing configuration files impacted by new fields
services/rfq/api/docs/swagger.yamlneeds to be updated to include the new fast_bridge configurations.
🔗 Analysis chain
Verify the impact on existing configurations
The addition of new fields might require updates to existing configuration files and deployment processes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing config files that might need updates
# Search for YAML config files
echo "Searching for config files..."
fd -e yaml -e yml | while read -r file; do
echo "Checking $file for fast bridge configurations..."
# Look for existing bridge contract configurations
rg -l "fast_bridge|bridge_contract" "$file" || true
done
Length of output: 27176
| c.Require().Len(contracts.ContractsV1, 2) | ||
| c.Require().Len(contracts.ContractsV2, 2) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for contract validation
The test only verifies the length of contract lists. Consider enhancing it to validate:
- The actual contract addresses and their properties
- Correct categorization of contracts as v1 or v2
- Uniqueness of contracts across v1 and v2 lists
- Contract deployment status on expected chains
Apply this diff to improve the test:
- c.Require().Len(contracts.ContractsV1, 2)
- c.Require().Len(contracts.ContractsV2, 2)
+ // Verify v1 contracts
+ c.Require().Len(contracts.ContractsV1, 2, "Expected 2 v1 contracts")
+ for _, contract := range contracts.ContractsV1 {
+ c.Require().NotEmpty(contract.Address, "Contract address should not be empty")
+ c.Require().NotZero(contract.ChainID, "Chain ID should not be zero")
+ // Add more specific assertions based on v1 contract properties
+ }
+
+ // Verify v2 contracts
+ c.Require().Len(contracts.ContractsV2, 2, "Expected 2 v2 contracts")
+ for _, contract := range contracts.ContractsV2 {
+ c.Require().NotEmpty(contract.Address, "Contract address should not be empty")
+ c.Require().NotZero(contract.ChainID, "Chain ID should not be zero")
+ // Add more specific assertions based on v2 contract properties
+ }
+
+ // Verify uniqueness across v1 and v2
+ allContracts := append(contracts.ContractsV1, contracts.ContractsV2...)
+ seen := make(map[string]bool)
+ for _, contract := range allContracts {
+ key := fmt.Sprintf("%d-%s", contract.ChainID, contract.Address)
+ c.Require().False(seen[key], "Duplicate contract found: %s", key)
+ seen[key] = true
+ }Committable suggestion skipped: line range outside the PR's diff.
| fastBridgeContracts := make(map[uint32]*fastbridge.FastBridge) | ||
| roles := make(map[uint32]*ttlcache.Cache[string, bool]) | ||
| for chainID, bridge := range cfg.Bridges { | ||
| for chainID, contract := range cfg.FastBridgeContractsV1 { | ||
| chainClient, err := omniRPCClient.GetChainClient(ctx, int(chainID)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create omnirpc client: %w", err) | ||
| } | ||
| bridges[chainID], err = fastbridge.NewFastBridge(common.HexToAddress(bridge), chainClient) | ||
| fastBridgeContracts[chainID], err = fastbridge.NewFastBridge(common.HexToAddress(contract), chainClient) |
There was a problem hiding this comment.
Initialize both v1 and v2 fast bridge contracts
The code only initializes v1 contracts despite the config supporting both v1 and v2 contracts. This could lead to v2 contracts being inaccessible.
Apply this diff to initialize both contract versions:
fastBridgeContracts := make(map[uint32]*fastbridge.FastBridge)
roles := make(map[uint32]*ttlcache.Cache[string, bool])
-for chainID, contract := range cfg.FastBridgeContractsV1 {
+// Initialize v1 contracts
+for chainID, contract := range cfg.FastBridgeContractsV1 {
+ if err := initializeContract(ctx, chainID, contract, fastBridgeContracts, roles, omniRPCClient); err != nil {
+ return nil, fmt.Errorf("could not initialize v1 contract: %w", err)
+ }
+}
+
+// Initialize v2 contracts
+for chainID, contract := range cfg.FastBridgeContractsV2 {
+ if err := initializeContract(ctx, chainID, contract, fastBridgeContracts, roles, omniRPCClient); err != nil {
+ return nil, fmt.Errorf("could not initialize v2 contract: %w", err)
+ }
+}
+
+// Helper function to initialize contracts
+func initializeContract(ctx context.Context, chainID uint32, contract string, contracts map[uint32]*fastbridge.FastBridge, roles map[uint32]*ttlcache.Cache[string, bool], omniRPCClient omniClient.RPCClient) error {
chainClient, err := omniRPCClient.GetChainClient(ctx, int(chainID))
if err != nil {
- return nil, fmt.Errorf("could not create omnirpc client: %w", err)
+ return fmt.Errorf("could not create omnirpc client: %w", err)
}
- fastBridgeContracts[chainID], err = fastbridge.NewFastBridge(common.HexToAddress(contract), chainClient)
+ contracts[chainID], err = fastbridge.NewFastBridge(common.HexToAddress(contract), chainClient)
if err != nil {
- return nil, fmt.Errorf("could not create bridge contract: %w", err)
+ return fmt.Errorf("could not create bridge contract: %w", err)
}
+ return nil
+}Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests