Add relayer API #1785
Add relayer API #1785
Conversation
WalkthroughThe changes encompass modifications to the RFQ system to introduce new API endpoints for checking the status of an RFQ relay and retrying transactions via the API. These updates involve adjustments to backend logic, addition of new configuration parameters, enhancements to testing suites, and structural changes to support the new functionality. Changes
Assessment against linked issues
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 Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (8)
- services/rfq/relayer/relapi/handler.go (1 hunks)
- services/rfq/relayer/relapi/model.go (1 hunks)
- services/rfq/relayer/relapi/server.go (1 hunks)
- services/rfq/relayer/reldb/base/model.go (6 hunks)
- services/rfq/relayer/reldb/base/quote.go (1 hunks)
- services/rfq/relayer/reldb/db.go (3 hunks)
- services/rfq/relayer/service/handlers.go (2 hunks)
- services/rfq/relayer/service/statushandler.go (1 hunks)
Additional comments: 16
services/rfq/relayer/relapi/model.go (2)
3-9: The
GetQuoteRequestStatusResponsestruct is well-defined and includes all necessary fields to represent the status of a quote request. The JSON tags are correctly assigned, ensuring proper serialization.11-17: The
PutTxRetryResponsestruct is also well-defined. It includes fields that are relevant for a response after a transaction retry attempt. The JSON tags are present and correctly formatted.services/rfq/relayer/reldb/base/quote.go (2)
37-44: The error message in
GetQuoteRequestByIDhas been updated to a more generic "could not get quote" which is appropriate as it does not assume the nature of the error (previously "could not get quote id"). This change improves the clarity of the error message.47-56: The new method
GetQuoteRequestByOriginTxHashis added to retrieve a quote request by its origin transaction hash. The method correctly handles the case where the quote request is not found by returningErrNoQuoteForTxHash. The error message for other errors is consistent with the rest of the file.services/rfq/relayer/relapi/handler.go (2)
32-60: The
GetQuoteRequestStatusByTxIDhandler is correctly implemented. It validates the input, decodes the transaction ID, retrieves the quote request, and sends a well-structured response. Error handling is present for each step where an error might occur.65-86: The
GetQuoteRequestStatusByTxHashhandler is similar to theGetQuoteRequestStatusByTxIDhandler and is correctly implemented. It validates the input, converts the hash to the common.Hash type, retrieves the quote request, and sends a response. Error handling is also correctly implemented.services/rfq/relayer/relapi/server.go (2)
38-82: The
NewAPIfunction is well-structured and includes error handling for nil arguments. It initializes thechainsmap and sets up each chain with the necessary components. The error messages are descriptive and will aid in debugging if initialization fails.92-115: The
Runmethod sets up the Gin engine and routes, and starts the server. The use ofginhelper.Newsuggests that there is a wrapper around the Gin setup which is not shown here. Ensure that this wrapper is correctly implemented and tested. The logging of the API start is a good practice for observability.services/rfq/relayer/reldb/db.go (3)
35-36: The addition of the
GetQuoteRequestByOriginTxHashmethod to theReaderinterface is consistent with the new functionality introduced in thequote.gofile. This ensures that the interface remains accurate with the capabilities of the implementing struct.53-55: The updated error declarations
ErrNoQuoteForIDandErrNoQuoteForTxHashprovide clear differentiation between the types of retrieval errors, which is a good practice for error handling and debugging.68-70: The
QuoteRequeststruct has been updated to include theOriginTxHashandDestTxHashfields of typecommon.Hash. This change aligns with the new functionality of retrieving and retrying quote requests by transaction hash.services/rfq/relayer/reldb/base/model.go (4)
25-25: The addition of
originTxHashFieldNameis consistent with the other field name declarations and follows the same naming convention.67-67: The
UpdatedAtfield has been modified. Ensure that this change is reflected in all places whereRequestForQuoteis used to avoid any potential issues with database operations.Verification successful
The ast-grep search for usages of the
RequestForQuotestruct with theUpdatedAtfield did not yield any results. This could mean that there are no explicit initializations of theRequestForQuotestruct with theUpdatedAtfield in the codebase, or that the field is not being directly manipulated in the code.To ensure thorough verification, I will perform a broader search to find any references to the
RequestForQuotestruct, which will help us understand how theUpdatedAtfield is being used throughout the codebase.
The search results indicate several references to the
RequestForQuotestruct within the codebase, particularly inquote.goandmodel.go. The references inquote.goseem to be related to database operations, such as querying and modeling data. The explicit construction of aRequestForQuotestruct is found inmodel.go, where various fields are being set, but there is no mention of theUpdatedAtfield in this construction.Given that the
UpdatedAtfield is typically managed by the database layer (often automatically updated to the current timestamp when a record is modified), the absence of explicit references in the construction or queries may not be an issue. However, it's important to ensure that the database schema and any ORM (Object-Relational Mapping) configurations are in sync with this field change.To further verify the correct handling of the
UpdatedAtfield, I will search for any database migration files or ORM configuration that might reference this field, to ensure that it is being automatically managed as expected.
The
rgsearch results show several references to theUpdatedAtfield across different files in the codebase. Notably, the field is mentioned inmodel.goas part of theRequestForQuotestruct, and it is also referenced in various files within theservices/rfq/apidirectory, indicating its use in API response models and database operations.The presence of the
UpdatedAtfield inapi_db_test.gosuggests that there are tests that check the non-nil nature of this field, which implies that it is expected to be set during database operations. Additionally, theUpdatedAtfield is formatted for API responses inutil.go, and it is used inclient_test.gofor constructing test cases.The
UpdatedAtfield is also referenced inethergo/debug/source.go, where it is explicitly set to the current time. This indicates that there might be instances where the field is manually set, although this particular reference seems to be in a debug file and may not reflect the typical usage pattern in production code.Given the information from the search results, it appears that the
UpdatedAtfield is indeed used throughout the codebase and is likely managed correctly by the database layer and the application code. There is no indication of a mismatch or mismanagement of this field that would cause issues due to the changes in theRequestForQuotestruct.Based on the evidence gathered, the modification to the
UpdatedAtfield in theRequestForQuotestruct does not seem to introduce any issues, and the field is being handled as expected in various parts of the codebase.* 92-92: The changes to the `Deadline`, `OriginTxHash`, and `DestTxHash` fields in the `RequestForQuote` struct are consistent with the PR objectives to track and retrieve quote requests by transaction hash. Ensure that these fields are properly indexed and used throughout the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of the RequestForQuote struct to verify the UpdatedAt field change. ast-grep --lang go --pattern $'RequestForQuote { $$$ UpdatedAt: $_ $$$ }'Length of output: 74
Script:
#!/bin/bash # Search for references to the RequestForQuote struct to understand how it's used. ast-grep --lang go --pattern $'RequestForQuote { $$$ }'Length of output: 3048
Script:
#!/bin/bash # Search for database migration files or ORM configurations that reference the UpdatedAt field. rg "UpdatedAt"Length of output: 1257
Also applies to: 97-97, 100-100
Verification successful
The output from the
rgcommand shows numerous instances of theOriginTxHash,DestTxHash, andDeadlinefields being used across various files in the codebase. This indicates that the new fields are indeed being utilized throughout the codebase, which aligns with the PR objectives and the review comment.Based on the output, it is evident that the fields are not only present in the
RequestForQuotestruct but are also integrated into the logic of the application, including database operations, API responses, and internal processing logic.Given the extensive use of these fields across the codebase, it can be concluded that the changes to the
Deadline,OriginTxHash, andDestTxHashfields in theRequestForQuotestruct are properly indexed and used, which supports the approval of the changes.* 126-126: The `FromQuoteRequest` and `ToQuoteRequest` functions have been updated to include the `OriginTxHash` field. Verify that the conversion between the struct and the `QuoteRequest` object is correct and that error handling is appropriate.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new fields are properly indexed and used. rg --type go 'OriginTxHash|DestTxHash|Deadline'Length of output: 239236
Also applies to: 182-183
Verification successful
The review comment is correct in stating that the
FromQuoteRequestandToQuoteRequestfunctions have been updated to include theOriginTxHashfield. The conversion between the struct and theQuoteRequestobject is handled correctly, with theOriginTxHashbeing converted to a string inFromQuoteRequestand to acommon.HashinToQuoteRequest. There is no specific error handling for theOriginTxHashfield shown in the output, which is expected as the field is a simple data type conversion.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the conversion functions handle the new field correctly. ast-grep --lang go --pattern $'func FromQuoteRequest($_) $_ { $$$ OriginTxHash: $_ $$$ }' ast-grep --lang go --pattern $'func ($_ RequestForQuote) ToQuoteRequest() $_ { $$$ OriginTxHash: $_ $$$ }'Length of output: 198
Script:
#!/bin/bash # Search for the existence of FromQuoteRequest and ToQuoteRequest functions. ast-grep --lang go --pattern $'func FromQuoteRequest($_) $_ { $$$ }' ast-grep --lang go --pattern $'func ($_ RequestForQuote) ToQuoteRequest() $_ { $$$ }'Length of output: 6467
services/rfq/relayer/service/handlers.go (1)
- 84-84: The assignment of
OriginTxHashin thehandleBridgeRequestedLogfunction aligns with the PR objectives to track the origin transaction hash. Ensure that this new assignment is used correctly in subsequent processing steps.
| // PutTxRetry retries a transaction based on tx hash. | ||
| func (h *Handler) PutTxRetry(c *gin.Context) { | ||
| txHashStr := c.Query("hash") | ||
| if txHashStr == "" { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": unspecifiedTxHash}) | ||
| return | ||
| } | ||
|
|
||
| txHash := common.HexToHash(txHashStr) | ||
| quoteRequest, err := h.db.GetQuoteRequestByOriginTxHash(c, txHash) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) | ||
| return | ||
| } | ||
|
|
||
| chainID := quoteRequest.Transaction.DestChainId | ||
| chain, ok := h.chains[uint32(chainID)] | ||
| if !ok { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("No contract found for chain: %d", chainID)}) | ||
| return | ||
| } | ||
|
|
||
| // TODO: this can be deduped with handlers.go code | ||
| gasAmount := big.NewInt(0) | ||
| if quoteRequest.Transaction.SendChainGas { | ||
| gasAmount, err = chain.Bridge.ChainGasAmount(&bind.CallOpts{Context: c}) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("could not get chain gas amount: %s", err.Error())}) | ||
| return | ||
| } | ||
| } | ||
| nonce, err := chain.SubmitTransaction(c, func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||
| transactor.Value = core.CopyBigInt(gasAmount) | ||
|
|
||
| tx, err = chain.Bridge.Relay(transactor, quoteRequest.RawRequest) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not relay: %w", err) | ||
| } | ||
|
|
||
| return tx, nil | ||
| }) | ||
|
|
||
| resp := PutTxRetryResponse{ | ||
| TxID: hexutil.Encode(quoteRequest.TransactionID[:]), | ||
| ChainID: chainID, | ||
| Nonce: nonce, | ||
| GasAmount: gasAmount.String(), | ||
| } | ||
| c.JSON(http.StatusOK, resp) |
There was a problem hiding this comment.
The PutTxRetry handler is responsible for retrying a transaction. It validates the input, retrieves the quote request, and attempts to submit the transaction. The error handling is comprehensive, and the response structure is appropriate. However, there is a TODO comment indicating that some code duplication might exist with handlers.go. This should be addressed to ensure DRY principles are followed.
| // AuthMiddleware is the Gin authentication middleware that authenticates requests using EIP191. | ||
| func (r *APIServer) AuthMiddleware() gin.HandlerFunc { | ||
| return func(c *gin.Context) { | ||
| var req model.PutQuoteRequest | ||
| if err := c.BindJSON(&req); err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) | ||
| c.Abort() | ||
| return | ||
| } | ||
|
|
||
| chain, ok := r.chains[uint32(req.DestChainID)] | ||
| if !ok { | ||
| c.JSON(http.StatusBadRequest, gin.H{"msg": "dest chain id not supported"}) | ||
| c.Abort() | ||
| return | ||
| } | ||
|
|
||
| ops := &bind.CallOpts{Context: c} | ||
| relayerRole := crypto.Keccak256Hash([]byte("RELAYER_ROLE")) | ||
|
|
||
| // authenticate relayer signature with EIP191 | ||
| deadline := time.Now().Unix() - 1000 // TODO: Replace with some type of r.cfg.AuthExpiryDelta | ||
| addressRecovered, err := rest.EIP191Auth(c, deadline) | ||
| if err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"msg": fmt.Sprintf("unable to authenticate relayer: %v", err)}) | ||
| c.Abort() | ||
| return | ||
| } | ||
|
|
||
| has, err := chain.Bridge.HasRole(ops, relayerRole, addressRecovered) | ||
| if err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"msg": "unable to check relayer role on-chain"}) | ||
| c.Abort() | ||
| return | ||
| } else if !has { | ||
| c.JSON(http.StatusBadRequest, gin.H{"msg": "q.Relayer not an on-chain relayer"}) | ||
| c.Abort() | ||
| return | ||
| } | ||
|
|
||
| // Log and pass to the next middleware if authentication succeeds | ||
| // Store the request in context after binding and validation | ||
| c.Set("putRequest", &req) | ||
| c.Set("relayerAddr", addressRecovered.Hex()) | ||
| c.Next() | ||
| } |
There was a problem hiding this comment.
The AuthMiddleware function is responsible for authenticating requests using EIP191. It correctly binds the JSON request, validates the destination chain ID, and recovers the address from the signature. The error handling is comprehensive, and the middleware correctly aborts the request if authentication fails. The TODO comment about replacing the hardcoded deadline with a configurable value should be addressed to make the middleware more flexible.
| // NewChain creates a new chain helper. | ||
| func NewChain(ctx context.Context, chainClient client.EVM, addr common.Address, chainListener listener.ContractListener, ts submitter.TransactionSubmitter) (*Chain, error) { | ||
| bridge, err := fastbridge.NewFastBridgeRef(addr, chainClient) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create bridge contract: %w", err) | ||
| } | ||
| chainID, err := chainClient.ChainID(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get chain id: %w", err) | ||
| } | ||
| return &Chain{ | ||
| ChainID: uint32(chainID.Int64()), | ||
| Bridge: bridge, | ||
| Client: chainClient, | ||
| // TODO: configure | ||
| Confirmations: 1, | ||
| listener: chainListener, | ||
| submitter: ts, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
The NewChain function is a new addition that creates a chain helper. It correctly initializes the fastbridge.FastBridgeRef and retrieves the chain ID. The function includes error handling for each step that could fail. The TODO comment about configuring the number of confirmations indicates that this value should not be hardcoded and should be addressed before finalizing the implementation.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (6)
- services/rfq/relayer/relapi/handler.go (1 hunks)
- services/rfq/relayer/relapi/model.go (1 hunks)
- services/rfq/relayer/relapi/server.go (1 hunks)
- services/rfq/relayer/relapi/server_test.go (1 hunks)
- services/rfq/relayer/relapi/suite_test.go (1 hunks)
- services/rfq/relayer/service/handlers.go (3 hunks)
Files skipped from review as they are similar to previous changes (4)
- services/rfq/relayer/relapi/handler.go
- services/rfq/relayer/relapi/model.go
- services/rfq/relayer/relapi/server.go
- services/rfq/relayer/service/handlers.go
Additional comments: 3
services/rfq/relayer/relapi/server_test.go (1)
- 145-160: The
startAPIServerhelper function is used to start the API server. It uses a retry mechanism to wait for the server to start. This is a good practice to ensure the server is ready before proceeding with tests. No issues found here.services/rfq/relayer/relapi/suite_test.go (2)
34-50: The
RelayerServerSuitestruct is well-defined with clear fields for the test suite. It includes necessary components such as the API server, database, and wallet, which are essential for the tests. No issues found here.185-188: The
TestRelayerServerSuitefunction is the entry point for running the test suite. It uses thesuitepackage to run the tests, which is a standard practice in Go for structuring test suites. No issues found here.
| func (c *RelayerServerSuite) TestGetQuoteRequestByTxHash() { | ||
| c.startAPIServer() | ||
|
|
||
| // Insert quote request to db | ||
| quoteRequest := c.getTestQuoteRequest(reldb.Seen) | ||
| err := c.database.StoreQuoteRequest(c.GetTestContext(), quoteRequest) | ||
| c.Require().NoError(err) | ||
|
|
||
| // Fetch the quote request by tx hash | ||
| client := &http.Client{} | ||
| req, err := http.NewRequestWithContext(c.GetTestContext(), http.MethodGet, fmt.Sprintf("http://localhost:%d/status?hash=%s", c.port, quoteRequest.OriginTxHash), nil) | ||
| c.Require().NoError(err) | ||
| resp, err := client.Do(req) | ||
| c.Require().NoError(err) | ||
| defer func() { | ||
| err = resp.Body.Close() | ||
| c.Require().NoError(err) | ||
| }() | ||
| c.Equal(http.StatusOK, resp.StatusCode) | ||
|
|
||
| // Compare to expected result | ||
| var result relapi.GetQuoteRequestStatusResponse | ||
| err = json.NewDecoder(resp.Body).Decode(&result) | ||
| c.Require().NoError(err) | ||
| expectedResult := relapi.GetQuoteRequestStatusResponse{ | ||
| Status: quoteRequest.Status.String(), | ||
| TxID: hexutil.Encode(quoteRequest.TransactionID[:]), | ||
| OriginTxHash: quoteRequest.OriginTxHash.String(), | ||
| DestTxHash: quoteRequest.DestTxHash.String(), | ||
| } | ||
| c.Equal(expectedResult, result) | ||
| c.GetTestContext().Done() |
There was a problem hiding this comment.
The test TestGetQuoteRequestByTxHash is well-structured and checks the status endpoint by transaction hash. It includes proper setup, execution, and validation steps. However, the test could be improved by adding assertions for the content of the response to ensure that the API returns the correct data.
| func (c *RelayerServerSuite) TestGetQuoteRequestByTxID() { | ||
| c.startAPIServer() | ||
|
|
||
| // Insert quote request to db | ||
| quoteRequest := c.getTestQuoteRequest(reldb.Seen) | ||
| err := c.database.StoreQuoteRequest(c.GetTestContext(), quoteRequest) | ||
| c.Require().NoError(err) | ||
|
|
||
| // Fetch the quote request by tx hash | ||
| client := &http.Client{} | ||
| txIDStr := hexutil.Encode(quoteRequest.TransactionID[:]) | ||
| req, err := http.NewRequestWithContext(c.GetTestContext(), http.MethodGet, fmt.Sprintf("http://localhost:%d/status/by_tx_id?id=%s", c.port, txIDStr), nil) | ||
| c.Require().NoError(err) | ||
| resp, err := client.Do(req) | ||
| c.Require().NoError(err) | ||
| defer func() { | ||
| err = resp.Body.Close() | ||
| c.Require().NoError(err) | ||
| }() | ||
| c.Equal(http.StatusOK, resp.StatusCode) | ||
|
|
||
| // Compare to expected result | ||
| var result relapi.GetQuoteRequestStatusResponse | ||
| err = json.NewDecoder(resp.Body).Decode(&result) | ||
| c.Require().NoError(err) | ||
| expectedResult := relapi.GetQuoteRequestStatusResponse{ | ||
| Status: quoteRequest.Status.String(), | ||
| TxID: hexutil.Encode(quoteRequest.TransactionID[:]), | ||
| OriginTxHash: quoteRequest.OriginTxHash.String(), | ||
| DestTxHash: quoteRequest.DestTxHash.String(), | ||
| } | ||
| c.Equal(expectedResult, result) | ||
| c.GetTestContext().Done() |
There was a problem hiding this comment.
The test TestGetQuoteRequestByTxID is similar to TestGetQuoteRequestByTxHash and shares the same strengths and areas for improvement. It would be beneficial to assert the content of the response for completeness.
| func (c *RelayerServerSuite) getTestQuoteRequest(status reldb.QuoteRequestStatus) reldb.QuoteRequest { | ||
| txIDRaw := hexutil.Encode(crypto.Keccak256([]byte("test"))) | ||
| var txID [32]byte | ||
| copy(txID[:], txIDRaw) | ||
| return reldb.QuoteRequest{ | ||
| OriginTokenDecimals: 6, | ||
| DestTokenDecimals: 6, | ||
| TransactionID: txID, | ||
| Status: status, | ||
| Transaction: fastbridge.IFastBridgeBridgeTransaction{ | ||
| OriginChainId: c.originChainID, | ||
| DestChainId: c.destChainID, | ||
| OriginAmount: big.NewInt(100), | ||
| DestAmount: big.NewInt(100), | ||
| Deadline: big.NewInt(time.Now().Unix()), | ||
| Nonce: big.NewInt(0), | ||
| }, | ||
| OriginTxHash: common.HexToHash("0x0000000"), | ||
| DestTxHash: common.HexToHash("0x0000001"), | ||
| } |
There was a problem hiding this comment.
The getTestQuoteRequest helper function creates a test quote request with predefined values. It's important to ensure that these values are representative of real-world scenarios to make the tests as realistic as possible. If these values are placeholders, consider using more realistic test data.
| // NewRelayerServerSuite creates a end-to-end test suite. | ||
| func NewRelayerServerSuite(tb testing.TB) *RelayerServerSuite { | ||
| tb.Helper() | ||
| return &RelayerServerSuite{ | ||
| TestSuite: testsuite.NewTestSuite(tb), | ||
| } | ||
| } | ||
|
|
||
| func (c *RelayerServerSuite) SetupTest() { | ||
| c.TestSuite.SetupTest() | ||
|
|
||
| testOmnirpc := omnirpcHelper.NewOmnirpcServer(c.GetTestContext(), c.T(), c.omniRPCTestBackends...) | ||
| omniRPCClient := omniClient.NewOmnirpcClient(testOmnirpc, c.handler, omniClient.WithCaptureReqRes()) | ||
| c.omniRPCClient = omniRPCClient | ||
|
|
||
| arbFastBridgeAddress, ok := c.fastBridgeAddressMap.Load(42161) | ||
| c.True(ok) | ||
| ethFastBridgeAddress, ok := c.fastBridgeAddressMap.Load(1) | ||
| c.True(ok) | ||
| port, err := freeport.GetFreePort() | ||
| c.port = uint16(port) | ||
| c.Require().NoError(err) | ||
|
|
||
| c.originChainID = 1 | ||
| c.destChainID = 42161 | ||
|
|
||
| testConfig := config.Config{ | ||
| Database: config.DatabaseConfig{ | ||
| Type: "sqlite", | ||
| DSN: filet.TmpFile(c.T(), "", "").Name(), | ||
| }, | ||
| OmniRPCURL: testOmnirpc, | ||
| Bridges: map[uint32]string{ | ||
| c.originChainID: ethFastBridgeAddress.Hex(), | ||
| c.destChainID: arbFastBridgeAddress.Hex(), | ||
| }, | ||
| Port: fmt.Sprintf("%d", port), | ||
| } | ||
| c.cfg = testConfig | ||
|
|
||
| c.wallet, err = wallet.FromRandom() | ||
| c.Require().NoError(err) | ||
| signer := localsigner.NewSigner(c.wallet.PrivateKey()) | ||
| submitterCfg := &submitterConfig.Config{} | ||
| ts := submitter.NewTransactionSubmitter(c.handler, signer, omniRPCClient, c.database.SubmitterDB(), submitterCfg) | ||
|
|
||
| server, err := relapi.NewRelayerAPI(c.GetTestContext(), c.cfg, c.handler, c.omniRPCClient, c.database, ts) | ||
| c.Require().NoError(err) | ||
| c.RelayerAPIServer = server | ||
| } |
There was a problem hiding this comment.
The SetupTest function properly initializes the test environment, including the Omni RPC client, wallet, and API server. It also handles errors appropriately. However, the test suite should clean up any resources it creates, such as temporary files or network listeners, to avoid side effects on other tests or the system.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- services/rfq/relayer/relapi/server.go (1 hunks)
- services/rfq/relayer/relapi/server_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- services/rfq/relayer/relapi/server.go
- services/rfq/relayer/relapi/server_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- services/rfq/relayer/relapi/server_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/relapi/server_test.go
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1785 +/- ##
===================================================
- Coverage 51.41789% 50.98475% -0.43315%
===================================================
Files 407 397 -10
Lines 27823 27012 -811
Branches 298 298
===================================================
- Hits 14306 13772 -534
+ Misses 12119 11889 -230
+ Partials 1398 1351 -47
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- services/rfq/relayer/relapi/server_test.go (1 hunks)
- services/rfq/relayer/reldb/base/model.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/relapi/server_test.go
Additional comments: 1
services/rfq/relayer/reldb/base/model.go (1)
- 26-26: The addition of the
originTxHashFieldNamevariable seems to be consistent with the existing pattern of defining field names using thedbcommon.NewNamerutility. This change is likely intended to standardize the database field name across the application.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (10)
- services/rfq/e2e/rfq_test.go (1 hunks)
- services/rfq/e2e/setup_test.go (3 hunks)
- services/rfq/relayer/chain/chain.go (1 hunks)
- services/rfq/relayer/relapi/handler.go (1 hunks)
- services/rfq/relayer/relapi/server.go (1 hunks)
- services/rfq/relayer/relapi/suite_test.go (1 hunks)
- services/rfq/relayer/relconfig/config.go (3 hunks)
- services/rfq/relayer/service/handlers.go (3 hunks)
- services/rfq/relayer/service/relayer.go (5 hunks)
- services/rfq/relayer/service/statushandler.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/relapi/suite_test.go
Additional comments: 26
services/rfq/relayer/chain/chain.go (3)
20-30: The
Chainstruct is well-documented, with clear comments about the visibility of fields and future plans for the struct. The fields seem to be appropriately named and typed.53-57: The
SubmitTransactionmethod correctly delegates the transaction submission to thesubmitterfield of theChainstruct. The use ofnolint: wrapcheckon line 55 suggests that the error fromSubmitTransactionshould not be wrapped, which is acceptable if the error is meant to be handled as is by the caller.64-90: The
SubmitRelaymethod is responsible for submitting a relay transaction. It checks if theSendChainGasflag is set on the request and fetches the gas amount if necessary. The error handling is consistent, and the method returns detailed errors. The use ofcore.CopyBigInton line 77 ensures that theValuefield of thetransactoris not inadvertently modified elsewhere.services/rfq/relayer/relapi/server.go (2)
23-31: The
RelayerAPIServerstruct is well-defined with clear comments. The fields are private, which is good for encapsulation, and the types seem appropriate for their purposes.33-77: The
NewRelayerAPIfunction performs thorough nil checks on its parameters, which is good for preventing nil dereference errors. The error messages are clear and provide context. The creation of thechainsmap is done correctly, and the error handling within the loop is consistent.services/rfq/relayer/relapi/handler.go (6)
14-18: The
Handlerstruct is simple and straightforward, storing a database connection and a map ofChaininstances. The naming is clear, and the struct serves its purpose without unnecessary complexity.20-25: The
NewHandlerfunction is a constructor for theHandlerstruct. It is concise and correctly assigns the provided arguments to the struct fields.28-31: The
GetHealthmethod is a simple health check endpoint. It returns a JSON response with a status of "ok", which is a common practice for health endpoints.35-57: The
GetQuoteRequestStatusByTxHashmethod retrieves the status of a quote request by transaction hash. It correctly handles the case where the 'hash' query parameter is not provided and returns an appropriate error response. The method also handles database errors correctly.59-88: The
GetQuoteRequestStatusByTxIDmethod is similar to the previous method but retrieves the status by transaction ID. It includes proper error handling for missing or invalid 'txID' query parameters and database errors.90-125: The
GetTxRetrymethod allows users to retry a transaction based on its hash. It includes error handling for missing hash parameters, database errors, and errors during the submission of the relay. The response includes the transaction ID, chain ID, nonce, and gas amount, which is useful information for the client.services/rfq/relayer/service/statushandler.go (2)
28-30: The
QuoteRequestHandlerstruct has been updated to use thechain.Chaintype for itsOriginandDestfields. This change aligns with the refactoring mentioned in the AI-generated summary and suggests a consolidation of chain-related functionality.107-115: The
chainIDToChainmethod has been refactored to use the newchain.NewChainfunction. This change is consistent with the refactoring strategy and simplifies the creation ofChaininstances.services/rfq/e2e/rfq_test.go (2)
30-37: The
IntegrationSuitestruct has been updated to include arelayerApiServerfield, which is consistent with the changes made to the relayer service. The renaming of therelayerfield torelayerApiServerreflects the introduction of the new API server functionality.30-37: The setup and test methods in the
IntegrationSuitestruct appear to be correctly updated to accommodate the newrelayerApiServerfield. The test setup includes the initialization of necessary services and clients, which is appropriate for an integration test suite.services/rfq/relayer/relconfig/config.go (3)
28-29: The addition of the
RelayerAPIURLfield to theConfigstruct is consistent with the PR objectives to enhance the relayer service with new endpoints. The field is properly named, typed as a string, and has an appropriate yaml tag.40-41: The addition of the
APIConfigfield to theConfigstruct is also consistent with the PR objectives. The field is properly named, typed asAPIConfig, and has an appropriate yaml tag.72-77: The new
APIConfigstruct is introduced to encapsulate the configuration for the relayer API server. The fieldsDatabase,OmniRPCURL, andPortare well-named and have appropriate data types and yaml tags. This struct is a good addition for modularity and clarity in configuration management.services/rfq/relayer/service/relayer.go (5)
22-22: The import of the
relapipackage is necessary for the use of theRelayerAPIServerwithin theRelayerstruct and is consistent with the changes made in this PR.36-36: The addition of the
apiServerfield to theRelayerstruct is consistent with the PR's objective to introduce a new API server. The field is properly named and typed as a pointer toRelayerAPIServer.99-102: The initialization of the
apiServerwithin theNewRelayerfunction is correctly done, with error handling in place to return an error if the API server cannot be initialized.116-116: The
apiServeris correctly assigned to theRelayerstruct within theNewRelayerfunction.189-195: The invocation of
apiServer.Runwithin a goroutine in theStartmethod is correct and follows the pattern of concurrent initialization for other components of theRelayer. Proper error handling is in place.services/rfq/e2e/setup_test.go (1)
- 231-245: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [192-242]
The declaration of
relayerApiPortand its usage to construct theRelayerAPIURLandAPIConfigin thesetupRelayerfunction is correct. The port is dynamically obtained and properly used to set up the configuration for the relayer API server. This is a good practice for test setup to avoid port conflicts.services/rfq/relayer/service/handlers.go (2)
- 4-11: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1-1]
The import
math/bighas been removed. Please ensure that there are no remaining usages of big.Int or related functions that would require this import, as this could lead to compilation errors.Verification successful
The removal of the
math/bigimport fromhandlers.goappears to be justified as there are no remaining usages ofbig.Intor related functions in that file. This suggests that the review comment has been addressed correctly.* 4-11: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1-1]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usages of big.Int or related functions in handlers.go rg --type go 'big\.Int' services/rfq/relayer/service/handlers.goLength of output: 66
Overall, the changes in the file seem to align with the PR's objectives. However, please address the issues mentioned regarding the
handleCommitConfirmedfunction and the removal of themath/bigimport.
| err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.RelayStarted) | ||
| if err != nil { | ||
| return fmt.Errorf("could not submit transaction: %w", err) | ||
| return fmt.Errorf("could not update quote request status: %w", err) | ||
| } | ||
|
|
||
| err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.RelayStarted) | ||
|
|
||
| // TODO: store the dest txhash connected to the nonce | ||
| nonce, _, err := q.Dest.SubmitRelay(ctx, request) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
There seems to be a logical error in the handleCommitConfirmed function. The variable nonce is retrieved but not used, and there is a redundant error check after the nonce retrieval that will never be executed because the function will return on the first error encountered.
- _ = nonce
- if err != nil {
- return fmt.Errorf("could not update request status: %w", err)
- }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.
| err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.RelayStarted) | |
| if err != nil { | |
| return fmt.Errorf("could not submit transaction: %w", err) | |
| return fmt.Errorf("could not update quote request status: %w", err) | |
| } | |
| err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.RelayStarted) | |
| // TODO: store the dest txhash connected to the nonce | |
| nonce, _, err := q.Dest.SubmitRelay(ctx, request) | |
| if err != nil { | |
| return err | |
| } | |
| err = q.db.UpdateQuoteRequestStatus(ctx, request.TransactionID, reldb.RelayStarted) | |
| if err != nil { | |
| return fmt.Errorf("could not update quote request status: %w", err) | |
| } | |
| // TODO: store the dest txhash connected to the nonce | |
| nonce, _, err := q.Dest.SubmitRelay(ctx, request) | |
| if err != nil { | |
| return err | |
| } |
| // NewChain creates a new chain. | ||
| func NewChain(ctx context.Context, chainClient client.EVM, addr common.Address, chainListener listener.ContractListener, ts submitter.TransactionSubmitter) (*Chain, error) { | ||
| bridge, err := fastbridge.NewFastBridgeRef(addr, chainClient) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create bridge contract: %w", err) | ||
| } | ||
| chainID, err := chainClient.ChainID(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get chain id: %w", err) | ||
| } | ||
| return &Chain{ | ||
| ChainID: uint32(chainID.Int64()), | ||
| Bridge: bridge, | ||
| Client: chainClient, | ||
| // TODO: configure | ||
| Confirmations: 1, | ||
| listener: chainListener, | ||
| submitter: ts, | ||
| }, nil |
There was a problem hiding this comment.
The NewChain function is responsible for creating a new Chain instance. It performs error checking and returns a new Chain object or an error. The error messages are wrapped for better context. The TODO comment on line 46 indicates that the Confirmations field should be configurable in the future.
- // TODO: configure
- Confirmations: 1,
+ // TODO: This value should be made configurable.
+ Confirmations: defaultConfirmations, // Assuming `defaultConfirmations` is a constant defined elsewhere.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.
| // NewChain creates a new chain. | |
| func NewChain(ctx context.Context, chainClient client.EVM, addr common.Address, chainListener listener.ContractListener, ts submitter.TransactionSubmitter) (*Chain, error) { | |
| bridge, err := fastbridge.NewFastBridgeRef(addr, chainClient) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not create bridge contract: %w", err) | |
| } | |
| chainID, err := chainClient.ChainID(ctx) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get chain id: %w", err) | |
| } | |
| return &Chain{ | |
| ChainID: uint32(chainID.Int64()), | |
| Bridge: bridge, | |
| Client: chainClient, | |
| // TODO: configure | |
| Confirmations: 1, | |
| listener: chainListener, | |
| submitter: ts, | |
| }, nil | |
| // NewChain creates a new chain. | |
| func NewChain(ctx context.Context, chainClient client.EVM, addr common.Address, chainListener listener.ContractListener, ts submitter.TransactionSubmitter) (*Chain, error) { | |
| bridge, err := fastbridge.NewFastBridgeRef(addr, chainClient) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not create bridge contract: %w", err) | |
| } | |
| chainID, err := chainClient.ChainID(ctx) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not get chain id: %w", err) | |
| } | |
| return &Chain{ | |
| ChainID: uint32(chainID.Int64()), | |
| Bridge: bridge, | |
| Client: chainClient, | |
| // TODO: This value should be made configurable. | |
| Confirmations: defaultConfirmations, // Assuming `defaultConfirmations` is a constant defined elsewhere. | |
| listener: chainListener, | |
| submitter: ts, | |
| }, nil | |
| } |
| // Run runs the rest api server. | ||
| func (r *RelayerAPIServer) Run(ctx context.Context) error { | ||
| // TODO: Use Gin Helper | ||
| engine := ginhelper.New(logger) | ||
| h := NewHandler(r.db, r.chains) | ||
|
|
||
| // Assign GET routes | ||
| engine.GET(getHealthRoute, h.GetHealth) | ||
| engine.GET(getQuoteStatusByTxHashRoute, h.GetQuoteRequestStatusByTxHash) | ||
| engine.GET(getQuoteStatusByTxIDRoute, h.GetQuoteRequestStatusByTxID) | ||
| engine.GET(getRetryRoute, h.GetTxRetry) | ||
|
|
||
| r.engine = engine | ||
|
|
||
| connection := baseServer.Server{} | ||
| fmt.Printf("starting api at http://localhost:%s\n", r.cfg.APIConfig.Port) | ||
| err := connection.ListenAndServe(ctx, fmt.Sprintf(":%s", r.cfg.APIConfig.Port), r.engine) | ||
| if err != nil { | ||
| return fmt.Errorf("could not start relayer api server: %w", err) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
The Run method sets up the Gin engine and routes, then starts the server. The TODO comment on line 91 suggests using a Gin Helper, which should be addressed in the future. The server start-up message on line 104 is printed to stdout, which might not be ideal for production environments where logging to a file or a logging system is preferred.
- fmt.Printf("starting api at http://localhost:%s\n", r.cfg.APIConfig.Port)
+ // TODO: Replace with structured logging
+ log.Infof("Starting API at http://localhost:%s", r.cfg.APIConfig.Port)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.
| // Run runs the rest api server. | |
| func (r *RelayerAPIServer) Run(ctx context.Context) error { | |
| // TODO: Use Gin Helper | |
| engine := ginhelper.New(logger) | |
| h := NewHandler(r.db, r.chains) | |
| // Assign GET routes | |
| engine.GET(getHealthRoute, h.GetHealth) | |
| engine.GET(getQuoteStatusByTxHashRoute, h.GetQuoteRequestStatusByTxHash) | |
| engine.GET(getQuoteStatusByTxIDRoute, h.GetQuoteRequestStatusByTxID) | |
| engine.GET(getRetryRoute, h.GetTxRetry) | |
| r.engine = engine | |
| connection := baseServer.Server{} | |
| fmt.Printf("starting api at http://localhost:%s\n", r.cfg.APIConfig.Port) | |
| err := connection.ListenAndServe(ctx, fmt.Sprintf(":%s", r.cfg.APIConfig.Port), r.engine) | |
| if err != nil { | |
| return fmt.Errorf("could not start relayer api server: %w", err) | |
| } | |
| return nil | |
| // Run runs the rest api server. | |
| func (r *RelayerAPIServer) Run(ctx context.Context) error { | |
| // TODO: Use Gin Helper | |
| engine := ginhelper.New(logger) | |
| h := NewHandler(r.db, r.chains) | |
| // Assign GET routes | |
| engine.GET(getHealthRoute, h.GetHealth) | |
| engine.GET(getQuoteStatusByTxHashRoute, h.GetQuoteRequestStatusByTxHash) | |
| engine.GET(getQuoteStatusByTxIDRoute, h.GetQuoteRequestStatusByTxID) | |
| engine.GET(getRetryRoute, h.GetTxRetry) | |
| r.engine = engine | |
| connection := baseServer.Server{} | |
| // TODO: Replace with structured logging | |
| log.Infof("Starting API at http://localhost:%s", r.cfg.APIConfig.Port) | |
| err := connection.ListenAndServe(ctx, fmt.Sprintf(":%s", r.cfg.APIConfig.Port), r.engine) | |
| if err != nil { | |
| return fmt.Errorf("could not start relayer api server: %w", err) | |
| } | |
| return nil |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- services/rfq/relayer/relapi/server.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/relapi/server.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- services/rfq/relayer/relapi/server.go (1 hunks)
- services/rfq/relayer/relapi/suite_test.go (1 hunks)
- services/rfq/relayer/relconfig/config.go (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- services/rfq/relayer/relapi/server.go
- services/rfq/relayer/relapi/suite_test.go
- services/rfq/relayer/relconfig/config.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (7)
- services/rfq/api/client/suite_test.go (2 hunks)
- services/rfq/api/rest/server.go (5 hunks)
- services/rfq/api/rest/server_test.go (7 hunks)
- services/rfq/api/rest/suite_test.go (2 hunks)
- services/rfq/e2e/rfq_test.go (2 hunks)
- services/rfq/e2e/setup_test.go (4 hunks)
- services/rfq/relayer/relapi/server_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- services/rfq/api/rest/suite_test.go
Files skipped from review as they are similar to previous changes (3)
- services/rfq/e2e/rfq_test.go
- services/rfq/e2e/setup_test.go
- services/rfq/relayer/relapi/server_test.go
Additional comments: 4
services/rfq/api/rest/server.go (2)
26-28: The renaming of the
APIServerstruct toQuoterAPIServeris consistent with the PR objectives to enhance the blockchain relayer service and provide more descriptive naming.42-48: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [45-71]
The
NewAPIfunction has been correctly updated to return a pointer toQuoterAPIServer. The error handling within this function is appropriate, checking for nil arguments before proceeding with the initialization.services/rfq/api/client/suite_test.go (1)
- 45-45: The
ClientSuitestruct has been updated to include theQuoterAPIServerfield instead ofAPIServer. This change is consistent with the renaming throughout the codebase.services/rfq/api/rest/server_test.go (1)
- 18-18: The test function
TestNewQuoterAPIServerhas been correctly renamed and thestartQuoterAPIServermethod is called to start the server. This is consistent with the changes in the codebase.
| } | ||
|
|
||
| // AuthMiddleware is the Gin authentication middleware that authenticates requests using EIP191. | ||
| func (r *APIServer) AuthMiddleware() gin.HandlerFunc { | ||
| func (r *QuoterAPIServer) AuthMiddleware() gin.HandlerFunc { | ||
| return func(c *gin.Context) { | ||
| var req model.PutQuoteRequest | ||
| if err := c.BindJSON(&req); err != nil { |
There was a problem hiding this comment.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [87-117]
The Run method of QuoterAPIServer has been updated to use the ginhelper.New function to create a new Gin engine. The TODO comment suggests that there is an intention to replace the hardcoded deadline with a configurable value. This should be addressed to avoid potential security issues with stale authentication tokens.
| @@ -112,7 +112,7 @@ | |||
| } | |||
|
|
|||
| // AuthMiddleware is the Gin authentication middleware that authenticates requests using EIP191. | |||
| func (r *APIServer) AuthMiddleware() gin.HandlerFunc { | |||
| func (r *QuoterAPIServer) AuthMiddleware() gin.HandlerFunc { | |||
There was a problem hiding this comment.
The AuthMiddleware method correctly implements the EIP191 authentication, but the hardcoded deadline (time.Now().Unix() - 1000) should be replaced with a configurable value from r.cfg.AuthExpiryDelta as indicated by the TODO comment. This is important for security reasons and to allow for easy adjustments without code changes.
| QuoterAPIServer, err := rest.NewAPI(c.GetTestContext(), c.cfg, c.handler, c.omniRPCClient, c.database) | ||
| c.Require().NoError(err) | ||
| c.APIServer = APIServer | ||
| c.QuoterAPIServer = QuoterAPIServer | ||
|
|
||
| go func() { | ||
| err := c.APIServer.Run(c.GetTestContext()) | ||
| err := c.QuoterAPIServer.Run(c.GetTestContext()) |
There was a problem hiding this comment.
The SetupTest method correctly initializes the QuoterAPIServer and starts it in a goroutine. The use of time.Sleep to wait for the server to start is not ideal for test reliability. Consider using a more deterministic approach to wait for the server to be ready, such as polling on a health check endpoint.
- time.Sleep(2 * time.Second) // Wait for the server to start.
+ // Implement a more reliable way to wait for the server to start, such as polling a health endpoint.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.
| QuoterAPIServer, err := rest.NewAPI(c.GetTestContext(), c.cfg, c.handler, c.omniRPCClient, c.database) | |
| c.Require().NoError(err) | |
| c.APIServer = APIServer | |
| c.QuoterAPIServer = QuoterAPIServer | |
| go func() { | |
| err := c.APIServer.Run(c.GetTestContext()) | |
| err := c.QuoterAPIServer.Run(c.GetTestContext()) | |
| QuoterAPIServer, err := rest.NewAPI(c.GetTestContext(), c.cfg, c.handler, c.omniRPCClient, c.database) | |
| c.Require().NoError(err) | |
| c.QuoterAPIServer = QuoterAPIServer | |
| go func() { | |
| err := c.QuoterAPIServer.Run(c.GetTestContext()) | |
| // Implement a more reliable way to wait for the server to start, such as polling a health endpoint. |
| // startQuoterAPIServer starts the API server and waits for it to initialize. | ||
| func (c *ServerSuite) startQuoterAPIServer() { | ||
| go func() { | ||
| err := c.APIServer.Run(c.GetTestContext()) | ||
| err := c.QuoterAPIServer.Run(c.GetTestContext()) |
There was a problem hiding this comment.
The startQuoterAPIServer method starts the QuoterAPIServer in a goroutine. As mentioned earlier, using time.Sleep to wait for the server to start is not ideal. A more deterministic approach should be used to ensure the server is ready before proceeding with tests.
- time.Sleep(2 * time.Second) // Wait for the server to start.
+ // Implement a more reliable way to wait for the server to start, such as polling a health endpoint.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.
| // startQuoterAPIServer starts the API server and waits for it to initialize. | |
| func (c *ServerSuite) startQuoterAPIServer() { | |
| go func() { | |
| err := c.APIServer.Run(c.GetTestContext()) | |
| err := c.QuoterAPIServer.Run(c.GetTestContext()) | |
| // startQuoterAPIServer starts the API server and waits for it to initialize. | |
| func (c *ServerSuite) startQuoterAPIServer() { | |
| go func() { | |
| err := c.QuoterAPIServer.Run(c.GetTestContext()) | |
| // Implement a more reliable way to wait for the server to start, such as polling a health endpoint. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- services/rfq/relayer/listener/listener_test.go (3 hunks)
- services/rfq/relayer/service/statushandler.go (4 hunks)
Additional comments: 6
services/rfq/relayer/listener/listener_test.go (3)
- 2-17: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-8]
The reordering of imports follows the standard Go convention of grouping standard library imports separately from third-party libraries, which is good for readability.
44-44: Adding
l.NotNil(bridgeRequestTX)ensures that theMockBridgeRequestfunction returns a non-nil transaction, which is a good practice for verifying the expected behavior of the mocked function.68-68: Similarly, adding
l.NotNil(bridgeResponseTX)ensures that theMockBridgeRelayerfunction returns a non-nil transaction. This is a good addition for the same reasons as the previous assertion.services/rfq/relayer/service/statushandler.go (3)
13-13: The import of the new
chainpackage is correctly placed with other internal package imports, which is consistent with Go conventions.28-30: The
Chaintype from thechainpackage is now used for theOriginandDestfields in theQuoteRequestHandlerstruct. This change likely reflects a refactoring effort to consolidate chain-related functionality. Ensure that all references to these fields throughout the codebase have been updated accordingly.Verification successful
The
Chaintype from thechainpackage is correctly used in theQuoteRequestHandlerstruct, with theOriginandDestfields being properly assigned. The code snippet provided in the output shows the instantiation of theQuoteRequestHandlerwith bothOriginandDestfields set, indicating that the refactoring has been consistently applied where this struct is initialized.* 107-116: The `chainIDToChain` method now uses the `NewChain` constructor from the `chain` package. This change centralizes the creation of `Chain` instances and is a good practice for maintainability. However, the `//nolint: wrapcheck` directive on line 115 suggests that error wrapping is being suppressed. It's important to ensure that this suppression is intentional and justified, as error wrapping can provide valuable context for debugging.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to the Origin and Dest fields in the QuoteRequestHandler struct. ast-grep --lang go --pattern $'QuoteRequestHandler { $$$ Origin: $_, $$$ }' ast-grep --lang go --pattern $'QuoteRequestHandler { $$$ Dest: $_, $$$ }'Length of output: 1031
Verification successful
The review comment regarding the use of the
NewChainconstructor in thechainIDToChainmethod has been verified. Thergcommand output confirms that theNewChainconstructor is indeed used within thechainpackage and is referenced in theservices/rfq/relayer/chain/chain.goandservices/rfq/relayer/service/statushandler.gofiles, among others. This aligns with the changes made in the PR and suggests that the constructor is consistently used where appropriate.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the NewChain constructor is used consistently across the codebase. ast-grep --lang go --pattern $'NewChain($_, $_, $_, $_, $_)'Length of output: 61
Script:
#!/bin/bash # Search for the usage of the NewChain constructor across the codebase without specifying the number of arguments. ast-grep --lang go --pattern $'NewChain($$$)'Length of output: 46
Script:
#!/bin/bash # Perform a text-based search for the usage of the NewChain constructor across the codebase. rg 'NewChain' --type goLength of output: 24280
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- services/rfq/relayer/service/handlers.go (3 hunks)
Additional comments: 3
services/rfq/relayer/service/handlers.go (3)
175-184: The
handleCommitConfirmedfunction has been updated to use theSubmitRelaymethod for submitting the relay transaction. Ensure that theSubmitRelaymethod is correctly implemented and that all error cases are handled appropriately. Also, the variablenonceis retrieved but not used, which could be an oversight or leftover from a refactor.225-230: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [230-248]
In the
handleProofPostedfunction, there is a checkq.shouldCheckClaim(request)to determine if the claim should be checked. Ensure that this function is correctly implemented and that it accounts for all necessary conditions before proceeding with the claim.Verification successful
The
shouldCheckClaimfunction is implemented in thestatushandler.gofile. It uses aclaimCacheto determine if a claim check should be performed, avoiding too frequent RPC hits. If theTransactionIDis present in theclaimCache, the function returnsfalse, indicating that the claim should not be checked. Otherwise, it sets theTransactionIDin theclaimCachewith a timeout of 30 seconds and returnstrue, indicating that the claim should be checked.* 4-11: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [250-260]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of shouldCheckClaim method and its conditions. ast-grep --lang go --pattern $'func ($_ *$_) shouldCheckClaim($_) $_ { $$$ }'Length of output: 848
The
handleNotEnoughInventoryfunction updates the status toCommittedPendingif there is now enough inventory. This is a good use of the optimistic update pattern, assuming that inventory levels are being updated elsewhere in the system. Confirm that inventory levels are being tracked and updated correctly to avoid any race conditions.
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "math/big" | ||
| "strings" | ||
|
|
||
| "github.com/synapsecns/sanguine/core" | ||
|
|
||
| "github.com/ethereum/go-ethereum/accounts/abi/bind" | ||
| "github.com/ethereum/go-ethereum/common/hexutil" | ||
| "github.com/ethereum/go-ethereum/core/types" |
There was a problem hiding this comment.
Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [204-214]
In the handleRelayLog function, there is a comment about the potential for re-orgs but no code to handle such a scenario. If re-orgs are a concern, consider implementing logic to handle them or create a task to address this in the future.
| func (q *QuoteRequestHandler) handleRelayCompleted(ctx context.Context, _ trace.Span, request reldb.QuoteRequest) (err error) { | ||
| // relays been completed, it's time to go back to the origin chain and try to prove | ||
| _, err = q.Origin.SubmitTransaction(ctx, func(transactor *bind.TransactOpts) (tx *types.Transaction, err error) { | ||
| // MAJO MAJOR TODO should be dest tx hash | ||
| tx, err = q.Origin.Bridge.Prove(transactor, request.RawRequest, request.DestTxHash) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not relay: %w", err) |
There was a problem hiding this comment.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [216-228]
The handleRelayCompleted function appears to be correctly updating the status after submitting a claim transaction. However, there is no check to ensure that the previous status is RelayCompleted before proceeding. This could lead to incorrect state transitions if the function is called out of order.
if request.Status != reldb.RelayCompleted {
- return fmt.Errorf("incorrect status for proving posting: %w", reldb.ErrInvalidStatus)
+ return nil
}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- services/rfq/relayer/pricer/fee_pricer.go (1 hunks)
Additional comments: 2
services/rfq/relayer/pricer/fee_pricer.go (2)
80-80: The
originparameter in theGetDestinationFeefunction signature has been replaced with an underscore, indicating it is unused. Ensure that this change does not affect any existing calls to this function where theoriginparameter was previously being used.77-83: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-122]
The rest of the file appears to be unchanged and there are no other modifications to the public interface of the
feePricerstruct. The logic within theGetDestinationFeemethod remains the same, and the removal of theoriginparameter seems to be the only change. Ensure that the rest of the codebase is consistent with this change and that no other parts of the code rely on theoriginparameter within theGetDestinationFeemethod.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- services/rfq/e2e/rfq_test.go (5 hunks)
- services/rfq/e2e/setup_test.go (4 hunks)
- services/rfq/relayer/relapi/server.go (1 hunks)
- services/rfq/relayer/relapi/suite_test.go (1 hunks)
- services/rfq/relayer/relconfig/config.go (1 hunks)
Files skipped from review due to trivial changes (1)
- services/rfq/relayer/relconfig/config.go
Description
Adds a separate
RelayerAPIServerfor exposing endpoints with information about bridge event statuses as well as an interface for retrying transaction submission.Notes:
rest.APIServerhas been renamed toQuoterAPIServer(as opposed to the newRelayerAPIServer) for clarityChainhelper struct now lives in its own package to avoid circular depsMetadata
Summary by CodeRabbit
New Features
Enhancements
chainpackage for submitting and handling transactions.Refactor
Bug Fixes
Documentation
Chores
depguardlinter into the project configuration to enforce dependency best practices.