Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(rfq-api): add v2 contracts to rfq api endpoint [SLT-429] #3386

Closed
wants to merge 1 commit into from
Closed

feat(rfq-api): add v2 contracts to rfq api endpoint [SLT-429] #3386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion services/rfq/api/client/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,11 @@ func (c *ClientSuite) SetupTest() {
DSN: filet.TmpFile(c.T(), "", "").Name(),
},
OmniRPCURL: testOmnirpc,
Bridges: map[uint32]string{
FastBridgeContractsV1: map[uint32]string{
1: ethFastBridgeAddress.Hex(),
42161: arbFastBridgeAddress.Hex(),
},
FastBridgeContractsV2: map[uint32]string{
1: ethFastBridgeAddress.Hex(),
42161: arbFastBridgeAddress.Hex(),
},
Expand Down
14 changes: 7 additions & 7 deletions services/rfq/api/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ type DatabaseConfig struct {

// Config is the configuration for the RFQ Quoter.
type Config struct {
Database DatabaseConfig `yaml:"database"`
OmniRPCURL string `yaml:"omnirpc_url"`
// bridges is a map of chainid->address
Bridges map[uint32]string `yaml:"bridges"`
Port string `yaml:"port"`
RelayAckTimeout time.Duration `yaml:"relay_ack_timeout"`
MaxQuoteAge time.Duration `yaml:"max_quote_age"`
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"`
Comment on lines +22 to +28
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Existing configuration files impacted by new fields

  • services/rfq/api/docs/swagger.yaml needs 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

}

const defaultRelayAckTimeout = 30 * time.Second
Expand Down
8 changes: 5 additions & 3 deletions services/rfq/api/model/response.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,12 @@ type PutRelayAckResponse struct {
RelayerAddress string `json:"relayer_address"`
}

// GetContractsResponse contains the schema for a GET /contract response.
// GetContractsResponse contains the schema for a GET /contracts response.
type GetContractsResponse struct {
// Contracts is a map of chain id to contract address
Contracts map[uint32]string `json:"contracts"`
// ContractsV1 is a map of chain id to contract address for v1 fast bridge contracts
ContractsV1 map[uint32]string `json:"contracts_v1"`
// ContractsV2 is a map of chain id to contract address for v2 fast bridge contracts
ContractsV2 map[uint32]string `json:"contracts_v2"`
}

// ActiveRFQMessage represents the general structure of WebSocket messages for Active RFQ.
Expand Down
10 changes: 4 additions & 6 deletions services/rfq/api/rest/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,12 +301,10 @@ func dbActiveQuoteRequestToModel(dbQuote *db.ActiveQuoteRequest) *model.GetOpenQ
// @Header 200 {string} X-Api-Version "API Version Number - See docs for more info"
// @Router /contracts [get].
func (h *Handler) GetContracts(c *gin.Context) {
// Convert quotes from db model to api model
contracts := make(map[uint32]string)
for chainID, address := range h.cfg.Bridges {
contracts[chainID] = address
}
c.JSON(http.StatusOK, model.GetContractsResponse{Contracts: contracts})
c.JSON(http.StatusOK, model.GetContractsResponse{
ContractsV1: h.cfg.FastBridgeContractsV1,
ContractsV2: h.cfg.FastBridgeContractsV2,
})
}

func filterQuoteAge(cfg config.Config, dbQuotes []*db.Quote) []*db.Quote {
Expand Down
9 changes: 5 additions & 4 deletions services/rfq/api/rest/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,14 +95,15 @@ func NewAPI(

docs.SwaggerInfo.Title = "RFQ Quoter API"

bridges := make(map[uint32]*fastbridge.FastBridge)
// TODO: allow role checking for v2 vs v1 contracts; for now default to v1
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)
Comment on lines +99 to +106
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

if err != nil {
return nil, fmt.Errorf("could not create bridge contract: %w", err)
}
Expand Down Expand Up @@ -136,7 +137,7 @@ func NewAPI(
omnirpcClient: omniRPCClient,
handler: handler,
meter: handler.Meter(meterName),
fastBridgeContracts: bridges,
fastBridgeContracts: fastBridgeContracts,
roleCache: roles,
relayAckCache: relayAckCache,
ackMux: sync.Mutex{},
Expand Down
3 changes: 2 additions & 1 deletion services/rfq/api/rest/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,5 +536,6 @@ func (c *ServerSuite) TestContracts() {
contracts, err := client.GetRFQContracts(c.GetTestContext())
c.Require().NoError(err)

c.Require().Len(contracts.Contracts, 2)
c.Require().Len(contracts.ContractsV1, 2)
c.Require().Len(contracts.ContractsV2, 2)
Comment on lines +539 to +540
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

}
6 changes: 5 additions & 1 deletion services/rfq/api/rest/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,11 @@ func (c *ServerSuite) SetupTest() {
DSN: filet.TmpFile(c.T(), "", "").Name(),
},
OmniRPCURL: testOmnirpc,
Bridges: map[uint32]string{
FastBridgeContractsV1: map[uint32]string{
1: ethFastBridgeAddress.Hex(),
42161: arbFastBridgeAddress.Hex(),
},
FastBridgeContractsV2: map[uint32]string{
1: ethFastBridgeAddress.Hex(),
42161: arbFastBridgeAddress.Hex(),
},
Expand Down
Loading