feat(rfq-api): add v2 contracts to rfq api endpoint [SLT-429] #3387
Conversation
dwasse
requested review from
trajan0x,
aureliusbtc and
ChiTimesChi
as code owners
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request focus on enhancing the handling of fast bridge contracts within 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/relayer-arb-call #3387 +/- ##
===============================================================
- Coverage 26.70871% 24.83818% -1.87053%
===============================================================
Files 215 208 -7
Lines 13636 13286 -350
Branches 82 82
===============================================================
- Hits 3642 3300 -342
+ Misses 9711 9704 -7
+ Partials 283 282 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Deploying sanguine-fe with
|
| Latest commit: |
6989ad2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5a02a62e.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-rfq-api-v2-contracts.sanguine-fe.pages.dev |
github-actions
bot
added
size/s
needs-go-generate-contrib/opbot
needs-go-generate-contrib/promexporter
needs-go-generate-services/rfq
and removed
size/l
labels
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
services/rfq/api/config/config.go (2)
24-25: Add documentation for the new contract fieldsPlease add documentation comments to explain:
- The purpose of these contract maps
- The expected format of contract addresses
- Whether these fields are required or optional
- The relationship between V1 and V2 contracts
Example:
+// FastBridgeContractsV1 maps chain IDs to their respective V1 bridge contract addresses. +// The addresses should be provided in their canonical format (0x...). FastBridgeContractsV1 map[uint32]string `yaml:"fast_bridge_contracts_v1"` +// FastBridgeContractsV2 maps chain IDs to their respective V2 bridge contract addresses. +// The addresses should be provided in their canonical format (0x...). FastBridgeContractsV2 map[uint32]string `yaml:"fast_bridge_contracts_v2"`
24-25: Consider adding contract address validationThe configuration loader should validate that the provided contract addresses are properly formatted to catch configuration errors early.
Consider:
- Adding validation in the
LoadConfigfunction- Creating a custom type for contract addresses with validation
- Adding helper methods to verify chain ID and contract address pairs
Would you like me to provide an example implementation for any of these approaches?
services/rfq/api/model/response.go (1)
46-49: Consider enhancing documentation and validationWhile the structure is good, consider these improvements:
- Document the expected format of contract addresses (e.g., "0x" prefixed hex strings)
- Add validation methods to ensure non-nil maps and valid contract address formats
Example validation method:
func (r *GetContractsResponse) Validate() error { if r.ContractsV1 == nil && r.ContractsV2 == nil { return errors.New("at least one contract version map must be non-nil") } return nil }contrib/opbot/botmd/commands.go (2)
396-397: Consider implementing comprehensive version handling strategyThe TODO comment indicates a transition to versioned contracts, but the current implementation might need a more robust approach.
Consider:
- Adding version validation in the request
- Implementing a version-aware contract factory
- Documenting the version compatibility matrix
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 397-397: contrib/opbot/botmd/commands.go#L397
Added line #L397 was not covered by tests
Line range hint
396-401: Enhance error handling for contract versionsThe current implementation silently falls back to v1 contracts without explicit version validation or clear error messaging.
Consider this approach:
- // TODO: handle v2 contract if specified - contractAddress, ok := contracts.ContractsV1[req.OriginChainID] + var contractAddress string + var ok bool + + // Explicit version handling + switch req.ContractVersion { + case "v2": + return nil, errors.New("v2 contracts are not yet supported") + case "v1", "": + contractAddress, ok = contracts.ContractsV1[req.OriginChainID] + default: + return nil, fmt.Errorf("unsupported contract version: %s", req.ContractVersion) + } + if !ok { return nil, errors.New("contract address not found") }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 397-397: contrib/opbot/botmd/commands.go#L397
Added line #L397 was not covered by testsservices/rfq/api/rest/server_test.go (1)
539-540: Enhance test coverage for contract validationThe test only verifies the length of contract lists. Consider enhancing it to:
- Verify contract addresses match the configuration
- Validate contract properties and structure
- Test error scenarios (e.g., missing configuration)
c.Require().Len(contracts.ContractsV1, 2) c.Require().Len(contracts.ContractsV2, 2) + // Verify contract addresses match configuration + c.Assert().Equal(c.cfg.FastBridgeContractsV1, contracts.ContractsV1) + c.Assert().Equal(c.cfg.FastBridgeContractsV2, contracts.ContractsV2) + + // Verify contract structure + for _, contract := range append(contracts.ContractsV1, contracts.ContractsV2...) { + c.Assert().NotEmpty(contract.Address) + c.Assert().NotEmpty(contract.ChainID) + }services/rfq/api/rest/server.go (3)
Line range hint
278-309: Use an enumerated type instead of a boolean flag for contract versionsCurrently,
useV1is a boolean flag used to determine the contract version based on the request path:var useV1 bool // ... useV1 = trueUsing a boolean may not scale well if additional versions are introduced. Consider using an enumerated type or constants to represent the contract version, enhancing code clarity and extensibility.
Apply this diff to introduce an enumerated type for contract versions:
+type ContractVersion string + +const ( + Version1 ContractVersion = "v1" + Version2 ContractVersion = "v2" +) + func (r *QuoterAPIServer) AuthMiddleware() gin.HandlerFunc { return func(c *gin.Context) { - var useV1 bool + var version ContractVersion var err error destChainIDs := []uint32{} // Parse the dest chain id from the request switch c.Request.URL.Path { case QuoteRoute: var req model.PutRelayerQuoteRequest err = c.BindJSON(&req) if err == nil { destChainIDs = append(destChainIDs, uint32(req.DestChainID)) loggedRequest = &req } - useV1 = true + version = Version1 case BulkQuotesRoute: var req model.PutBulkQuotesRequest err = c.BindJSON(&req) if err == nil { for _, quote := range req.Quotes { destChainIDs = append(destChainIDs, uint32(quote.DestChainID)) } loggedRequest = &req } - useV1 = true + version = Version1 case AckRoute: var req model.PutAckRequest err = c.BindJSON(&req) if err == nil { destChainIDs = append(destChainIDs, uint32(req.DestChainID)) loggedRequest = &req } - useV1 = true + version = Version1 case RFQRoute, RFQStreamRoute: // ... handle parsing for these routes + version = Version2 default: err = fmt.Errorf("unexpected request path: %s", c.Request.URL.Path) } if err != nil { c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) c.Abort() return } // Authenticate and fetch the address from the request var addressRecovered *common.Address for _, destChainID := range destChainIDs { - addr, err := r.checkRole(c, destChainID, useV1) + addr, err := r.checkRole(c, destChainID, version) if err != nil { c.JSON(http.StatusBadRequest, gin.H{"msg": err.Error()}) c.Abort() return } // ... } // ... } }
Line range hint
360-391: UpdatecheckRolemethod to use the enumerated contract versionFollowing the previous suggestion, modify the
checkRolemethod to accept theContractVersiontype. This change enhances readability and prepares the codebase for additional versions in the future.Apply this diff to update the
checkRolemethod:-func (r *QuoterAPIServer) checkRole(c *gin.Context, destChainID uint32, useV1 bool) (addressRecovered common.Address, err error) { +func (r *QuoterAPIServer) checkRole(c *gin.Context, destChainID uint32, version ContractVersion) (addressRecovered common.Address, err error) { var bridge roleContract var roleCache *ttlcache.Cache[string, bool] var ok bool - if useV1 { + switch version { + case Version1: bridge, ok = r.fastBridgeContractsV1[destChainID] if !ok { err = fmt.Errorf("dest chain id not supported in V1: %d", destChainID) return addressRecovered, err } roleCache = r.roleCacheV1[destChainID] - } else { + case Version2: bridge, ok = r.fastBridgeContractsV2[destChainID] if !ok { err = fmt.Errorf("dest chain id not supported in V2: %d", destChainID) return addressRecovered, err } roleCache = r.roleCacheV2[destChainID] + default: + err = fmt.Errorf("unsupported contract version: %s", version) + return addressRecovered, err } // ... rest of the method remains unchanged }
Line range hint
333-353: Enhance error handling for multiple destination chain IDsIn the
AuthMiddleware, when looping overdestChainIDs, if an error occurs duringcheckRole, the function returns a400 Bad Requestwith a generic error message.Consider providing more specific error messages to aid debugging and user experience. Also, ensure consistency in error responses across different parts of the middleware.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
contrib/opbot/botmd/commands.go(1 hunks)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(11 hunks)services/rfq/api/rest/server_test.go(1 hunks)services/rfq/api/rest/suite_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
contrib/opbot/botmd/commands.go
[warning] 397-397: contrib/opbot/botmd/commands.go#L397
Added line #L397 was not covered by tests
🔇 Additional comments (7)
services/rfq/api/model/response.go (1)
44-49: Well-structured versioning approach for contract addresses!
The separation of contracts into ContractsV1 and ContractsV2 provides a clean and backward-compatible way to support both versions while maintaining clear distinction between them.
services/rfq/api/client/suite_test.go (2)
88-91: Verify chain ID type consistency
The test configuration uses uint32 for chain IDs, which matches the config package. However, let's verify this is consistent across the codebase to prevent potential type conversion issues.
#!/bin/bash
# Search for chain ID type declarations
rg -A 2 "type.*Config.*struct" --type go
# Search for chain ID usage in maps
ast-grep --pattern 'map[$_]$_ {
$$$
}'88-95: Consider differentiating V1 and V2 contract addresses in tests
Both FastBridgeContractsV1 and FastBridgeContractsV2 are using identical addresses. While this might work for basic testing, consider:
- Using different addresses to ensure the system correctly routes requests to the appropriate version
- Adding test cases that verify version-specific behavior
Let's verify the contract version handling across the codebase:
services/rfq/api/rest/suite_test.go (1)
85-92: 🛠️ Refactor suggestion
Enhance test coverage for V1 and V2 contracts
Both V1 and V2 contract maps are using identical addresses. This might not adequately test the differences between contract versions.
Let's verify if there are any behavioral differences between V1 and V2 contracts that should be tested:
Consider:
- Using different contract addresses for V1 and V2 to ensure proper version handling
- Adding test cases that specifically verify version-specific behaviors
services/rfq/api/rest/handler.go (1)
304-307: LGTM! Clean implementation of versioned contracts response.
The changes effectively implement the versioned contracts support while maintaining a clean and straightforward implementation. The structured response with separate ContractsV1 and ContractsV2 fields provides good clarity and maintainability.
Let's verify the response model and config structure alignment:
✅ Verification successful
Verified: Response model and configuration structures are correctly aligned.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the alignment between response model and config structure
# Check response model structure
echo "Checking response model structure..."
rg -A 5 "type GetContractsResponse struct" services/rfq/api/model/
# Check config structure
echo "Checking config structure..."
rg -A 5 "FastBridgeContracts[V1|V2]" services/rfq/api/config/
Length of output: 1385
contrib/opbot/botmd/commands.go (1)
396-397: Add test coverage for contract version handling
The new contract version handling code lacks test coverage, which is critical for this core functionality.
Let's verify the existing test coverage:
Would you like me to help create test cases for:
- V1 contract happy path
- V2 contract handling (currently unsupported)
- Invalid version handling
- Missing contract address scenarios
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 397-397: contrib/opbot/botmd/commands.go#L397
Added line #L397 was not covered by tests
services/rfq/api/rest/server.go (1)
Line range hint 392-407: Ensure thread safety of the role cache
In the checkRole method, access to roleCache may be subject to concurrent access if multiple requests are processed simultaneously. Verify that the ttlcache being used is thread-safe or protect cache access with synchronization mechanisms like mutexes to prevent race conditions.
To verify if ttlcache is thread-safe, you can check the documentation or the implementation details. If it's not thread-safe, consider using a thread-safe cache implementation or add synchronization.
| FastBridgeContractsV1: map[uint32]string{ | ||
| 1: ethFastBridgeAddress.Hex(), | ||
| 42161: arbFastBridgeAddress.Hex(), | ||
| }, |
There was a problem hiding this comment.
Address type inconsistency in chain ID mappings
The FastBridge contract maps in the test config use uint32 for chain IDs, but the implementation uses uint64 (see fastBridgeAddressMap field). This type mismatch could cause issues in production code.
Apply this diff to fix the type inconsistency:
- FastBridgeContractsV1: map[uint32]string{
+ FastBridgeContractsV1: map[uint64]string{
},
- FastBridgeContractsV2: map[uint32]string{
+ FastBridgeContractsV2: map[uint64]string{
},Also applies to: 89-92
| fastBridgeContractsV1 map[uint32]*fastbridge.FastBridge | ||
| fastBridgeContractsV2 map[uint32]*fastbridgev2.FastBridgeV2 | ||
| roleCacheV1 map[uint32]*ttlcache.Cache[string, bool] | ||
| roleCacheV2 map[uint32]*ttlcache.Cache[string, bool] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to reduce duplication between V1 and V2 contract fields
The QuoterAPIServer struct introduces separate fields for V1 and V2 contracts and role caches:
fastBridgeContractsV1 map[uint32]*fastbridge.FastBridge
fastBridgeContractsV2 map[uint32]*fastbridgev2.FastBridgeV2
roleCacheV1 map[uint32]*ttlcache.Cache[string, bool]
roleCacheV2 map[uint32]*ttlcache.Cache[string, bool]Consider refactoring these fields to reduce duplication. One approach is to use a unified structure or map that can handle multiple versions, possibly by introducing a version key or encapsulating the contract and cache data together.
There was a problem hiding this comment.
@dwasse thoughts on this? Not sure if worth it, but I see that the new roleContract type could be potentially used here, as we only check roles anyway
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
My plan here was to keep these as separate fields temporarily while we support backwards compatibility, then remove the old fields. We could opt for a more generic approach if we want to support multiple versions down the line- do you think that will be the case?
| fastBridgeContractsV1 map[uint32]*fastbridge.FastBridge | ||
| fastBridgeContractsV2 map[uint32]*fastbridgev2.FastBridgeV2 | ||
| roleCacheV1 map[uint32]*ttlcache.Cache[string, bool] | ||
| roleCacheV2 map[uint32]*ttlcache.Cache[string, bool] |
There was a problem hiding this comment.
@dwasse thoughts on this? Not sure if worth it, but I see that the new roleContract type could be potentially used here, as we only check roles anyway
services/rfq/api/rest/server.go
Outdated
| @@ -258,6 +288,7 @@ func (r *QuoterAPIServer) AuthMiddleware() gin.HandlerFunc { | |||
| destChainIDs = append(destChainIDs, uint32(req.DestChainID)) | |||
| loggedRequest = &req | |||
| } | |||
| useV1 = true | |||
There was a problem hiding this comment.
Don't we need to derive the useV1 flag from the request's origin/dest fast bridge addresses? Same applies to other cases
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
services/rfq/api/rest/server.go (3)
62-65: Consider using a version-agnostic data structure.The separate maps for V1 and V2 contracts and role caches introduce code duplication. Consider using a version-agnostic structure to improve maintainability.
type VersionedContracts struct { contracts map[uint32]roleContract roleCache map[uint32]*ttlcache.Cache[string, bool] } type QuoterAPIServer struct { // ... bridgeContracts map[string]VersionedContracts // key: "v1" or "v2" // ... }
Line range hint
386-440: Consider adding metrics for role check performance.The role checking function is critical for authentication but lacks observability.
Add metrics to track:
- Cache hit/miss rates
- On-chain role check latency
- Error rates by version and chain ID
// Add these metrics in the NewAPI function roleCheckDuration := r.meter.Float64Histogram("role_check_duration") roleCheckErrors := r.meter.Int64Counter("role_check_errors") roleCacheHits := r.meter.Int64Counter("role_cache_hits") // Use them in checkRole start := time.Now() defer func() { roleCheckDuration.Record(time.Since(start).Seconds(), metric.WithAttributes( attribute.Bool("useV1", useV1), attribute.Int64("chainID", int64(destChainID)), )) }()
Line range hint
418-428: Add context timeout for on-chain role checks.The
HasRolecall to the blockchain lacks a timeout, which could lead to hanging requests.Add a timeout context:
if cachedRoleItem == nil || cachedRoleItem.IsExpired() { + ctx, cancel := context.WithTimeout(ops.Context, 5*time.Second) + defer cancel() + ops.Context = ctx hasRole, err = bridge.HasRole(ops, relayerRole, addressRecovered) if err != nil { return addressRecovered, fmt.Errorf("unable to check relayer role on-chain: %w", err) } roleCache.Set(addressRecovered.Hex(), hasRole, cacheInterval) }services/rfq/e2e/setup_test.go (1)
Line range hint
346-350: Consider refactoring to eliminate duplication inQuotableTokensconfigurationThe addition of
ETHtoQuotableTokensfor both origin and destination backends is currently duplicated. To enhance maintainability and reduce code duplication, consider refactoring this section by iterating over the backends or utilizing the existinggetOtherBackendmethod.Suggested refactor:
- cfg.QuotableTokens[fmt.Sprintf("%d-%s", originBackendChainID, util.EthAddress)] = []string{ - fmt.Sprintf("%d-%s", destBackendChainID, util.EthAddress), - } - cfg.QuotableTokens[fmt.Sprintf("%d-%s", destBackendChainID, util.EthAddress)] = []string{ - fmt.Sprintf("%d-%s", originBackendChainID, util.EthAddress), - } + for _, backend := range core.ToSlice(i.originBackend, i.destBackend) { + otherBackend := i.getOtherBackend(backend) + cfg.QuotableTokens[fmt.Sprintf("%d-%s", backend.GetChainID(), util.EthAddress)] = []string{ + fmt.Sprintf("%d-%s", otherBackend.GetChainID(), util.EthAddress), + } + }This refactor reduces repetition and utilizes existing methods to determine the other backend dynamically.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
services/rfq/api/rest/server.go(9 hunks)services/rfq/e2e/setup_test.go(1 hunks)
🔇 Additional comments (2)
services/rfq/e2e/setup_test.go (2)
62-65: Addition of FastBridgeContractsV1 to apiCfg is appropriate
The inclusion of FastBridgeContractsV1 in the apiCfg configuration correctly maps the origin and destination chain IDs to their respective contract addresses. This aligns with the requirement to handle versioned fast bridge contracts.
Line range hint 250-256: Proper addition of Tokens configuration for destBackendChainID
The Tokens map for destBackendChainID is correctly added under the Chains configuration in the getRelayerConfig method. This ensures that the ETH token is properly configured with its address, price, and decimals, which is essential for accurate token handling on the destination backend.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
services/rfq/api/rest/server.go (2)
Line range hint
387-441: Consider adding metrics for role check cache hits/missesThe role checking implementation could benefit from monitoring cache effectiveness.
Consider adding metrics to track cache hits and misses:
+func (r *QuoterAPIServer) recordRoleCacheMetrics(useV1 bool, hit bool) { + labels := []attribute.KeyValue{ + attribute.Bool("v1", useV1), + attribute.Bool("hit", hit), + } + r.meter.Counter("role_cache_hits").Add(context.Background(), 1, metric.WithAttributes(labels...)) +} func (r *QuoterAPIServer) checkRole(c *gin.Context, destChainID uint32, useV1 bool) (addressRecovered common.Address, err error) { // ... existing code ... cachedRoleItem := roleCache.Get(addressRecovered.Hex()) var hasRole bool if cachedRoleItem == nil || cachedRoleItem.IsExpired() { + r.recordRoleCacheMetrics(useV1, false) hasRole, err = bridge.HasRole(ops, relayerRole, addressRecovered) if err != nil { return addressRecovered, fmt.Errorf("unable to check relayer role on-chain: %w", err) } roleCache.Set(addressRecovered.Hex(), hasRole, cacheInterval) } else { + r.recordRoleCacheMetrics(useV1, true) hasRole = cachedRoleItem.Value() }
Line range hint
419-429: Consider adding context timeout for role checksThe role checking operation lacks a timeout, which could lead to hanging requests.
Consider adding a timeout context:
+const roleCheckTimeout = 5 * time.Second func (r *QuoterAPIServer) checkRole(c *gin.Context, destChainID uint32, useV1 bool) (addressRecovered common.Address, err error) { - ops := &bind.CallOpts{Context: c} + ctx, cancel := context.WithTimeout(c, roleCheckTimeout) + defer cancel() + ops := &bind.CallOpts{Context: ctx}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
contrib/opbot/botmd/commands.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(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- services/rfq/api/config/config.go
- services/rfq/api/model/response.go
- services/rfq/api/rest/handler.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
contrib/opbot/botmd/commands.go
[warning] 375-375: contrib/opbot/botmd/commands.go#L375
Added line #L375 was not covered by tests
🔇 Additional comments (2)
contrib/opbot/botmd/commands.go (1)
374-375:
Implement v2 contract handling before deployment
The TODO comment indicates incomplete implementation for v2 contracts. Since this function is used by critical operations like refunds, we should ensure proper handling of both contract versions.
Consider implementing proper version handling:
-// TODO: handle v2 contract if specified
-contractAddress, ok := contracts.ContractsV1[chainID]
+var contractAddress string
+var ok bool
+
+// Check v2 contracts first if specified
+if useV2 {
+ contractAddress, ok = contracts.ContractsV2[chainID]
+ if !ok {
+ return nil, fmt.Errorf("no v2 contract address for chain ID %d", chainID)
+ }
+} else {
+ contractAddress, ok = contracts.ContractsV1[chainID]
+ if !ok {
+ return nil, fmt.Errorf("no v1 contract address for chain ID %d", chainID)
+ }
+}Would you like me to help create a GitHub issue to track this implementation?
Let's verify the usage of this function:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 375-375: contrib/opbot/botmd/commands.go#L375
Added line #L375 was not covered by tests
services/rfq/api/rest/server.go (1)
63-66: LGTM: Clear separation of v1 and v2 contract fields
The struct fields are well-organized with clear naming conventions, separating v1 and v2 contracts and their respective role caches.
| // TODO: handle v2 contract if specified | ||
| contractAddress, ok := contracts.ContractsV1[chainID] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for contract version handling
The new contract version logic lacks test coverage. This is critical as it affects core functionality like refunds.
Consider adding these test cases:
- Test successful v1 contract retrieval
- Test successful v2 contract retrieval (after implementation)
- Test fallback behavior
- Test error cases for missing contracts
Would you like me to help generate the test cases?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 375-375: contrib/opbot/botmd/commands.go#L375
Added line #L375 was not covered by tests
| fastBridgeContractsV1 := make(map[uint32]*fastbridge.FastBridge) | ||
| rolesV1 := make(map[uint32]*ttlcache.Cache[string, bool]) | ||
| 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) | ||
| fastBridgeContractsV1[chainID], err = fastbridge.NewFastBridge(common.HexToAddress(contract), chainClient) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create bridge contract: %w", err) | ||
| } | ||
|
|
||
| // create the roles cache | ||
| roles[chainID] = ttlcache.New[string, bool]( | ||
| rolesV1[chainID] = ttlcache.New[string, bool]( | ||
| ttlcache.WithTTL[string, bool](cacheInterval), | ||
| ) | ||
| roleCache := roles[chainID] | ||
| roleCache := rolesV1[chainID] | ||
| go roleCache.Start() | ||
| go func() { | ||
| <-ctx.Done() | ||
| roleCache.Stop() | ||
| }() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in contract initialization
The initialization code for v1 and v2 contracts is nearly identical, which could lead to maintenance challenges.
Consider consolidating the initialization logic into a helper function:
+func initializeContracts[T roleContract](
+ ctx context.Context,
+ cfg map[uint32]string,
+ newContract func(common.Address, bind.ContractBackend) (T, error),
+ omniRPCClient omniClient.RPCClient,
+) (map[uint32]T, map[uint32]*ttlcache.Cache[string, bool], error) {
+ contracts := make(map[uint32]T)
+ roles := make(map[uint32]*ttlcache.Cache[string, bool])
+
+ for chainID, contract := range cfg {
+ chainClient, err := omniRPCClient.GetChainClient(ctx, int(chainID))
+ if err != nil {
+ return nil, nil, fmt.Errorf("could not create omnirpc client: %w", err)
+ }
+
+ contractInstance, err := newContract(common.HexToAddress(contract), chainClient)
+ if err != nil {
+ return nil, nil, fmt.Errorf("could not create bridge contract: %w", err)
+ }
+ contracts[chainID] = contractInstance
+
+ roles[chainID] = ttlcache.New[string, bool](
+ ttlcache.WithTTL[string, bool](cacheInterval),
+ )
+ roleCache := roles[chainID]
+ go roleCache.Start()
+ go func() {
+ <-ctx.Done()
+ roleCache.Stop()
+ }()
+ }
+ return contracts, roles, nil
+}Usage:
-fastBridgeContractsV1 := make(map[uint32]*fastbridge.FastBridge)
-rolesV1 := make(map[uint32]*ttlcache.Cache[string, bool])
-for chainID, contract := range cfg.FastBridgeContractsV1 {
- // ... initialization code
-}
+fastBridgeContractsV1, rolesV1, err := initializeContracts(
+ ctx,
+ cfg.FastBridgeContractsV1,
+ fastbridge.NewFastBridge,
+ omniRPCClient,
+)
+if err != nil {
+ return nil, err
+}Also applies to: 127-141
| func (r *QuoterAPIServer) checkRoleParallel(c *gin.Context, destChainID uint32) (addressRecovered common.Address, err error) { | ||
| g := new(errgroup.Group) | ||
| var v1Addr, v2Addr common.Address | ||
| var v1Err, v2Err error | ||
|
|
||
| g.Go(func() error { | ||
| v1Addr, v1Err = r.checkRole(c, destChainID, true) | ||
| return v1Err | ||
| }) | ||
|
|
||
| g.Go(func() error { | ||
| v2Addr, v2Err = r.checkRole(c, destChainID, false) | ||
| return v2Err | ||
| }) | ||
|
|
||
| err = g.Wait() | ||
| if v1Addr != (common.Address{}) { | ||
| return v1Addr, nil | ||
| } | ||
| if v2Addr != (common.Address{}) { | ||
| return v2Addr, nil | ||
| } | ||
| if err != nil { | ||
| return common.Address{}, fmt.Errorf("role check failed: %w", err) | ||
| } | ||
|
|
||
| return common.Address{}, fmt.Errorf("role check failed for both v1 and v2") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in parallel role checking
The current implementation might mask specific errors when both v1 and v2 checks fail.
Consider enhancing error reporting:
func (r *QuoterAPIServer) checkRoleParallel(c *gin.Context, destChainID uint32) (addressRecovered common.Address, err error) {
g := new(errgroup.Group)
var v1Addr, v2Addr common.Address
var v1Err, v2Err error
g.Go(func() error {
v1Addr, v1Err = r.checkRole(c, destChainID, true)
return v1Err
})
g.Go(func() error {
v2Addr, v2Err = r.checkRole(c, destChainID, false)
return v2Err
})
- err = g.Wait()
+ _ = g.Wait() // We want to collect both errors regardless of g.Wait()
if v1Addr != (common.Address{}) {
return v1Addr, nil
}
if v2Addr != (common.Address{}) {
return v2Addr, nil
}
- if err != nil {
- return common.Address{}, fmt.Errorf("role check failed: %w", err)
- }
- return common.Address{}, fmt.Errorf("role check failed for both v1 and v2")
+ return common.Address{}, fmt.Errorf("role check failed: v1 error: %v, v2 error: %v", v1Err, v2Err)
}📝 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.
| func (r *QuoterAPIServer) checkRoleParallel(c *gin.Context, destChainID uint32) (addressRecovered common.Address, err error) { | |
| g := new(errgroup.Group) | |
| var v1Addr, v2Addr common.Address | |
| var v1Err, v2Err error | |
| g.Go(func() error { | |
| v1Addr, v1Err = r.checkRole(c, destChainID, true) | |
| return v1Err | |
| }) | |
| g.Go(func() error { | |
| v2Addr, v2Err = r.checkRole(c, destChainID, false) | |
| return v2Err | |
| }) | |
| err = g.Wait() | |
| if v1Addr != (common.Address{}) { | |
| return v1Addr, nil | |
| } | |
| if v2Addr != (common.Address{}) { | |
| return v2Addr, nil | |
| } | |
| if err != nil { | |
| return common.Address{}, fmt.Errorf("role check failed: %w", err) | |
| } | |
| return common.Address{}, fmt.Errorf("role check failed for both v1 and v2") | |
| } | |
| func (r *QuoterAPIServer) checkRoleParallel(c *gin.Context, destChainID uint32) (addressRecovered common.Address, err error) { | |
| g := new(errgroup.Group) | |
| var v1Addr, v2Addr common.Address | |
| var v1Err, v2Err error | |
| g.Go(func() error { | |
| v1Addr, v1Err = r.checkRole(c, destChainID, true) | |
| return v1Err | |
| }) | |
| g.Go(func() error { | |
| v2Addr, v2Err = r.checkRole(c, destChainID, false) | |
| return v2Err | |
| }) | |
| _ = g.Wait() // We want to collect both errors regardless of g.Wait() | |
| if v1Addr != (common.Address{}) { | |
| return v1Addr, nil | |
| } | |
| if v2Addr != (common.Address{}) { | |
| return v2Addr, nil | |
| } | |
| return common.Address{}, fmt.Errorf("role check failed: v1 error: %v, v2 error: %v", v1Err, v2Err) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
services/rfq/api/rest/handler.go (1)
Line range hint
138-156: Consider enhancing error messages for better debuggingThe validation integration looks good, but the error messages could be more specific to help with debugging.
Consider this enhancement:
- return nil, fmt.Errorf("invalid fast bridge addresses: %w", err) + return nil, fmt.Errorf("fast bridge address validation failed for chainIDs %d->%d: %w", + putRequest.OriginChainID, putRequest.DestChainID, err)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
services/rfq/api/rest/handler.go(7 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint (services/rfq)
services/rfq/api/rest/handler.go
[failure] 175-175:
G115: integer overflow conversion int -> uint32 (gosec)
[failure] 176-176:
G115: integer overflow conversion int -> uint32 (gosec)
[failure] 179-179:
G115: integer overflow conversion int -> uint32 (gosec)
🔇 Additional comments (2)
services/rfq/api/rest/handler.go (2)
73-73: LGTM: Consistent config parameter addition
The addition of the config parameter to parseDBQuote calls is consistent across both ModifyQuote and ModifyBulkQuotes functions.
Also applies to: 121-121
328-331: LGTM: Clean contract version separation
The updated response structure clearly separates V1 and V2 contracts, making it easier for clients to handle different versions.
| func validateFastBridgeAddresses(cfg config.Config, putRequest model.PutRelayerQuoteRequest) error { | ||
| // Check V1 contracts | ||
| isV1Origin := common.HexToAddress(cfg.FastBridgeContractsV1[uint32(putRequest.OriginChainID)]) == common.HexToAddress(putRequest.OriginFastBridgeAddress) | ||
| isV1Dest := common.HexToAddress(cfg.FastBridgeContractsV1[uint32(putRequest.DestChainID)]) == common.HexToAddress(putRequest.DestFastBridgeAddress) | ||
|
|
||
| // Check V2 contracts | ||
| isV2Origin := common.HexToAddress(cfg.FastBridgeContractsV2[uint32(putRequest.OriginChainID)]) == common.HexToAddress(putRequest.OriginFastBridgeAddress) | ||
| isV2Dest := common.HexToAddress(cfg.FastBridgeContractsV2[uint32(putRequest.DestChainID)]) == common.HexToAddress(putRequest.DestFastBridgeAddress) | ||
|
|
||
| // Valid if both addresses match either V1 or V2 | ||
| if (isV1Origin && isV1Dest) || (isV2Origin && isV2Dest) { | ||
| return nil | ||
| } | ||
|
|
||
| return fmt.Errorf("origin and destination fast bridge addresses must match either V1 or V2") | ||
| } |
There was a problem hiding this comment.
Address potential integer overflow and optimize validation logic
- There's a risk of integer overflow when converting chainIDs from int to uint32.
- The validation logic could be more efficient.
Fix the integer overflow risk:
- isV1Origin := common.HexToAddress(cfg.FastBridgeContractsV1[uint32(putRequest.OriginChainID)]) == common.HexToAddress(putRequest.OriginFastBridgeAddress)
+ if putRequest.OriginChainID < 0 || putRequest.DestChainID < 0 {
+ return fmt.Errorf("chain IDs must be non-negative")
+ }
+ originChainID := uint32(putRequest.OriginChainID)
+ destChainID := uint32(putRequest.DestChainID)
+ isV1Origin := common.HexToAddress(cfg.FastBridgeContractsV1[originChainID]) == common.HexToAddress(putRequest.OriginFastBridgeAddress)Optimize the validation logic:
- isV1Origin := common.HexToAddress(cfg.FastBridgeContractsV1[uint32(putRequest.OriginChainID)]) == common.HexToAddress(putRequest.OriginFastBridgeAddress)
- isV1Dest := common.HexToAddress(cfg.FastBridgeContractsV1[uint32(putRequest.DestChainID)]) == common.HexToAddress(putRequest.DestFastBridgeAddress)
- isV2Origin := common.HexToAddress(cfg.FastBridgeContractsV2[uint32(putRequest.OriginChainID)]) == common.HexToAddress(putRequest.OriginFastBridgeAddress)
- isV2Dest := common.HexToAddress(cfg.FastBridgeContractsV2[uint32(putRequest.DestChainID)]) == common.HexToAddress(putRequest.DestFastBridgeAddress)
+ originAddr := common.HexToAddress(putRequest.OriginFastBridgeAddress)
+ destAddr := common.HexToAddress(putRequest.DestFastBridgeAddress)
+
+ v1OriginAddr := common.HexToAddress(cfg.FastBridgeContractsV1[originChainID])
+ v1DestAddr := common.HexToAddress(cfg.FastBridgeContractsV1[destChainID])
+ if originAddr == v1OriginAddr && destAddr == v1DestAddr {
+ return nil
+ }
+
+ v2OriginAddr := common.HexToAddress(cfg.FastBridgeContractsV2[originChainID])
+ v2DestAddr := common.HexToAddress(cfg.FastBridgeContractsV2[destChainID])
+ if originAddr == v2OriginAddr && destAddr == v2DestAddr {
+ return nil
+ }
+
+ return fmt.Errorf("origin and destination fast bridge addresses must match either V1 or V2")📝 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.
| func validateFastBridgeAddresses(cfg config.Config, putRequest model.PutRelayerQuoteRequest) error { | |
| // Check V1 contracts | |
| isV1Origin := common.HexToAddress(cfg.FastBridgeContractsV1[uint32(putRequest.OriginChainID)]) == common.HexToAddress(putRequest.OriginFastBridgeAddress) | |
| isV1Dest := common.HexToAddress(cfg.FastBridgeContractsV1[uint32(putRequest.DestChainID)]) == common.HexToAddress(putRequest.DestFastBridgeAddress) | |
| // Check V2 contracts | |
| isV2Origin := common.HexToAddress(cfg.FastBridgeContractsV2[uint32(putRequest.OriginChainID)]) == common.HexToAddress(putRequest.OriginFastBridgeAddress) | |
| isV2Dest := common.HexToAddress(cfg.FastBridgeContractsV2[uint32(putRequest.DestChainID)]) == common.HexToAddress(putRequest.DestFastBridgeAddress) | |
| // Valid if both addresses match either V1 or V2 | |
| if (isV1Origin && isV1Dest) || (isV2Origin && isV2Dest) { | |
| return nil | |
| } | |
| return fmt.Errorf("origin and destination fast bridge addresses must match either V1 or V2") | |
| } | |
| func validateFastBridgeAddresses(cfg config.Config, putRequest model.PutRelayerQuoteRequest) error { | |
| if putRequest.OriginChainID < 0 || putRequest.DestChainID < 0 { | |
| return fmt.Errorf("chain IDs must be non-negative") | |
| } | |
| originChainID := uint32(putRequest.OriginChainID) | |
| destChainID := uint32(putRequest.DestChainID) | |
| originAddr := common.HexToAddress(putRequest.OriginFastBridgeAddress) | |
| destAddr := common.HexToAddress(putRequest.DestFastBridgeAddress) | |
| v1OriginAddr := common.HexToAddress(cfg.FastBridgeContractsV1[originChainID]) | |
| v1DestAddr := common.HexToAddress(cfg.FastBridgeContractsV1[destChainID]) | |
| if originAddr == v1OriginAddr && destAddr == v1DestAddr { | |
| return nil | |
| } | |
| v2OriginAddr := common.HexToAddress(cfg.FastBridgeContractsV2[originChainID]) | |
| v2DestAddr := common.HexToAddress(cfg.FastBridgeContractsV2[destChainID]) | |
| if originAddr == v2OriginAddr && destAddr == v2DestAddr { | |
| return nil | |
| } | |
| return fmt.Errorf("origin and destination fast bridge addresses must match either V1 or V2") | |
| } |
🧰 Tools
🪛 GitHub Check: Lint (services/rfq)
[failure] 175-175:
G115: integer overflow conversion int -> uint32 (gosec)
[failure] 176-176:
G115: integer overflow conversion int -> uint32 (gosec)
[failure] 179-179:
G115: integer overflow conversion int -> uint32 (gosec)
Summary by CodeRabbit
New Features
/contractsendpoint to include separate contract lists for V1 and V2.Bug Fixes
Tests
Chores