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] #3387

Merged
merged 9 commits into from
Nov 20, 2024
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
530a756
Feat: add v2 contracts to rfq api endpoint
dwasse Nov 12, 2024
c15c47d
Feat: add separate role caches for v1/v2
dwasse Nov 12, 2024
cf4ece0
Fix: build
dwasse Nov 12, 2024
981722c
Feat: add checkRoleParallel()
dwasse Nov 18, 2024
e4867eb
Fix: build
dwasse Nov 18, 2024
b2cfa92
Merge branch 'feat/relayer-arb-call' into feat/rfq-api-v2-contracts
dwasse Nov 19, 2024
c1defaa
Feat: add validation for contract versions
dwasse Nov 19, 2024
fc07ec5
Cleanup: lint
dwasse Nov 20, 2024
6989ad2
Cleaup: remove log
dwasse Nov 20, 2024
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
3 changes: 2 additions & 1 deletion contrib/opbot/botmd/commands.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,8 @@
return nil, fmt.Errorf("error getting chain client: %w", err)
}

contractAddress, ok := contracts.Contracts[req.OriginChainID]
// TODO: handle v2 contract if specified
contractAddress, ok := contracts.ContractsV1[req.OriginChainID]

Check warning on line 397 in contrib/opbot/botmd/commands.go

View check run for this annotation

Codecov / codecov/patch

contrib/opbot/botmd/commands.go#L397 

Added line #L397 was not covered by tests
if !ok {
return nil, errors.New("contract address not found")
}
Expand Down
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"`
ChiTimesChi marked this conversation as resolved.
Show resolved Hide resolved
}

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
118 changes: 84 additions & 34 deletions services/rfq/api/rest/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"github.com/synapsecns/sanguine/services/rfq/api/docs"
"github.com/synapsecns/sanguine/services/rfq/api/model"
"github.com/synapsecns/sanguine/services/rfq/contracts/fastbridge"
"github.com/synapsecns/sanguine/services/rfq/contracts/fastbridgev2"
"github.com/synapsecns/sanguine/services/rfq/relayer/relapi"
)

Expand All @@ -50,15 +51,17 @@ func getCurrentVersion() (string, error) {
// QuoterAPIServer is a struct that holds the configuration, database connection, gin engine, RPC client, metrics handler, and fast bridge contracts.
// It is used to initialize and run the API server.
type QuoterAPIServer struct {
cfg config.Config
db db.APIDB
engine *gin.Engine
upgrader websocket.Upgrader
omnirpcClient omniClient.RPCClient
handler metrics.Handler
meter metric.Meter
fastBridgeContracts map[uint32]*fastbridge.FastBridge
roleCache map[uint32]*ttlcache.Cache[string, bool]
cfg config.Config
db db.APIDB
engine *gin.Engine
upgrader websocket.Upgrader
omnirpcClient omniClient.RPCClient
handler metrics.Handler
meter metric.Meter
fastBridgeContractsV1 map[uint32]*fastbridge.FastBridge
fastBridgeContractsV2 map[uint32]*fastbridgev2.FastBridgeV2
roleCacheV1 map[uint32]*ttlcache.Cache[string, bool]
roleCacheV2 map[uint32]*ttlcache.Cache[string, bool]
Comment on lines +63 to +66
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

// relayAckCache contains a set of transactionID values that reflect
// transactions that have been acked for relay
relayAckCache *ttlcache.Cache[string, string]
Expand Down Expand Up @@ -95,23 +98,47 @@ func NewAPI(

docs.SwaggerInfo.Title = "RFQ Quoter API"

bridges := make(map[uint32]*fastbridge.FastBridge)
roles := make(map[uint32]*ttlcache.Cache[string, bool])
for chainID, bridge := range cfg.Bridges {
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()
}()
}
Comment on lines +103 to +125
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

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


fastBridgeContractsV2 := make(map[uint32]*fastbridgev2.FastBridgeV2)
rolesV2 := make(map[uint32]*ttlcache.Cache[string, bool])
for chainID, contract := range cfg.FastBridgeContractsV2 {
chainClient, err := omniRPCClient.GetChainClient(ctx, int(chainID))
if err != nil {
return nil, fmt.Errorf("could not create omnirpc client: %w", err)
}
fastBridgeContractsV2[chainID], err = fastbridgev2.NewFastBridgeV2(common.HexToAddress(contract), chainClient)
if err != nil {
return nil, fmt.Errorf("could not create bridge contract: %w", err)
}

// create the roles cache
rolesV2[chainID] = ttlcache.New[string, bool](
ttlcache.WithTTL[string, bool](cacheInterval),
)
roleCache := rolesV2[chainID]
go roleCache.Start()
go func() {
<-ctx.Done()
Expand All @@ -131,17 +158,19 @@ func NewAPI(
}()

q := &QuoterAPIServer{
cfg: cfg,
db: store,
omnirpcClient: omniRPCClient,
handler: handler,
meter: handler.Meter(meterName),
fastBridgeContracts: bridges,
roleCache: roles,
relayAckCache: relayAckCache,
ackMux: sync.Mutex{},
wsClients: xsync.NewMapOf[WsClient](),
pubSubManager: NewPubSubManager(),
cfg: cfg,
db: store,
omnirpcClient: omniRPCClient,
handler: handler,
meter: handler.Meter(meterName),
fastBridgeContractsV1: fastBridgeContractsV1,
fastBridgeContractsV2: fastBridgeContractsV2,
roleCacheV1: rolesV1,
roleCacheV2: rolesV2,
relayAckCache: relayAckCache,
ackMux: sync.Mutex{},
wsClients: xsync.NewMapOf[WsClient](),
pubSubManager: NewPubSubManager(),
}

// Prometheus metrics setup
Expand Down Expand Up @@ -246,6 +275,7 @@ func (r *QuoterAPIServer) Run(ctx context.Context) error {
func (r *QuoterAPIServer) AuthMiddleware() gin.HandlerFunc {
return func(c *gin.Context) {
var loggedRequest interface{}
var useV1 bool
var err error
destChainIDs := []uint32{}

Expand All @@ -258,6 +288,7 @@ func (r *QuoterAPIServer) AuthMiddleware() gin.HandlerFunc {
destChainIDs = append(destChainIDs, uint32(req.DestChainID))
loggedRequest = &req
}
useV1 = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we need to derive the useV1 flag from the request's origin/dest fast bridge addresses? Same applies to other cases

case BulkQuotesRoute:
var req model.PutBulkQuotesRequest
err = c.BindJSON(&req)
Expand All @@ -267,13 +298,15 @@ func (r *QuoterAPIServer) AuthMiddleware() gin.HandlerFunc {
}
loggedRequest = &req
}
useV1 = true
case AckRoute:
var req model.PutAckRequest
err = c.BindJSON(&req)
if err == nil {
destChainIDs = append(destChainIDs, uint32(req.DestChainID))
loggedRequest = &req
}
useV1 = true
case RFQRoute, RFQStreamRoute:
chainsHeader := c.GetHeader(ChainsHeader)
if chainsHeader != "" {
Expand All @@ -297,7 +330,7 @@ func (r *QuoterAPIServer) AuthMiddleware() gin.HandlerFunc {
// Authenticate and fetch the address from the request
var addressRecovered *common.Address
for _, destChainID := range destChainIDs {
addr, err := r.checkRole(c, destChainID)
addr, err := r.checkRole(c, destChainID, useV1)
if err != nil {
c.JSON(http.StatusBadRequest, gin.H{"msg": err.Error()})
c.Abort()
Expand All @@ -320,11 +353,28 @@ func (r *QuoterAPIServer) AuthMiddleware() gin.HandlerFunc {
}
}

func (r *QuoterAPIServer) checkRole(c *gin.Context, destChainID uint32) (addressRecovered common.Address, err error) {
bridge, ok := r.fastBridgeContracts[destChainID]
if !ok {
err = fmt.Errorf("dest chain id not supported: %d", destChainID)
return addressRecovered, err
type roleContract interface {
HasRole(opts *bind.CallOpts, role [32]byte, account common.Address) (bool, error)
}

func (r *QuoterAPIServer) checkRole(c *gin.Context, destChainID uint32, useV1 bool) (addressRecovered common.Address, err error) {
var bridge roleContract
var roleCache *ttlcache.Cache[string, bool]
var ok bool
if useV1 {
bridge, ok = r.fastBridgeContractsV1[destChainID]
if !ok {
err = fmt.Errorf("dest chain id not supported: %d", destChainID)
return addressRecovered, err
}
roleCache = r.roleCacheV1[destChainID]
} else {
bridge, ok = r.fastBridgeContractsV2[destChainID]
if !ok {
err = fmt.Errorf("dest chain id not supported: %d", destChainID)
return addressRecovered, err
}
roleCache = r.roleCacheV2[destChainID]
}

ops := &bind.CallOpts{Context: c}
Expand All @@ -339,7 +389,7 @@ func (r *QuoterAPIServer) checkRole(c *gin.Context, destChainID uint32) (address
}

// Check and update cache
cachedRoleItem := r.roleCache[destChainID].Get(addressRecovered.Hex())
cachedRoleItem := roleCache.Get(addressRecovered.Hex())
var hasRole bool

if cachedRoleItem == nil || cachedRoleItem.IsExpired() {
Expand All @@ -349,7 +399,7 @@ func (r *QuoterAPIServer) checkRole(c *gin.Context, destChainID uint32) (address
return addressRecovered, fmt.Errorf("unable to check relayer role on-chain: %w", err)
}
// Update cache
r.roleCache[destChainID].Set(addressRecovered.Hex(), hasRole, cacheInterval)
roleCache.Set(addressRecovered.Hex(), hasRole, cacheInterval)
} else {
// Use cached value
hasRole = cachedRoleItem.Value()
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)
}
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(),
},
Comment on lines +85 to +88
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

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

FastBridgeContractsV2: map[uint32]string{
1: ethFastBridgeAddress.Hex(),
42161: arbFastBridgeAddress.Hex(),
},
Expand Down
Loading