activeQuote depositAmount [SLT-464] #3372
Conversation
parodime
requested review from
dwasse,
trajan0x and
aureliusbtc
as code owners
WalkthroughThe changes in this pull request focus on enhancing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lintlevel=warning msg="[config_reader] The configuration option 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (2)services/rfq/api/docs/docs.go (2)
The rename from Let's verify the consistency of this rename across the codebase:
The rename from Let's verify the request/response handling for this property: ✅ Verification successfulProperty rename verified successfully. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for proper request/response handling of the renamed property
# Search for QuoteData struct definition and usage
ast-grep --pattern 'type QuoteData struct {
$$$
origin_amount_exact $$$
$$$
}'
# Check for any JSON marshaling/unmarshaling of this property
rg -g '!*.go.swp' '(json\.Marshal|json\.Unmarshal).*QuoteData'
Length of output: 151 Script: #!/bin/bash
# Description: Search for all usages of 'origin_amount_exact' in the codebase
rg 'origin_amount_exact'
Length of output: 576 Script: #!/bin/bash
# Description: Verify that 'origin_amount' is no longer used in the QuoteData struct
ast-grep --pattern 'type QuoteData struct {
$$$
origin_amount $$$
$$$
}'
Length of output: 80 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Deploying sanguine-fe with
|
| Latest commit: |
1a32365
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://11d4aec1.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-activequotedepositamoun.sanguine-fe.pages.dev |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3372 +/- ##
====================================================
- Coverage 43.84981% 25.00380% -18.84601%
====================================================
Files 74 199 +125
Lines 2317 13142 +10825
Branches 82 82
====================================================
+ Hits 1016 3286 +2270
- Misses 1295 9577 +8282
- Partials 6 279 +273
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
services/rfq/relayer/quoter/quoter.go(4 hunks)
🔇 Additional comments (4)
services/rfq/relayer/quoter/quoter.go (4)
380-380: Include DepositAmount in QuoteInput
Adding DepositAmount: depositAmount to the QuoteInput initialization ensures that the deposit amount is correctly passed into the quote generation process.
550-550: Set DepositAmount to nil for bulk quote generation
Setting DepositAmount to nil here indicates that no deposit amount should be considered during bulk quote generation. This is appropriate if deposit amounts are only applicable in specific contexts like active RFQ requests.
587-587: Add DepositAmount field to QuoteInput struct
Including DepositAmount *big.Int in the QuoteInput struct definition allows the system to handle deposit amounts during quote generation effectively.
776-788: Correctly adjust quoteAmount based on DepositAmount
The logic to adjust quoteAmount ensures that if the calculated quoteAmount is sufficient to cover the DepositAmount, it is clipped to DepositAmount. Otherwise, setting quoteAmount to zero appropriately indicates the inability to cover the requested deposit amount.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
services/rfq/relayer/quoter/quoter.go (1)
775-788: Consider improving the deposit amount handling.While the logic is correct, consider these improvements:
- Enhance the comment to explain why we return 0 when the quote amount is insufficient.
- Simplify the logic block by combining conditions.
- // If input included a depositAmount, and our calculated quoteAmount is sufficient to cover it, - // then clip to the depositAmount. Otherwise return 0 to indicate inability to cover the requested amount. + // If a deposit amount is specified, we either: + // - Return the exact deposit amount if we can cover it + // - Return 0 if we cannot cover it, indicating insufficient liquidity if input.DepositAmount != nil { - if quoteAmount.Cmp(input.DepositAmount) >= 0 { - quoteAmount = input.DepositAmount - } else { + if quoteAmount.Cmp(input.DepositAmount) < 0 { span.AddEvent("quote amount insufficient to cover deposit amount", trace.WithAttributes( attribute.String("quote_amount", quoteAmount.String()), attribute.String("deposit_amount", input.DepositAmount.String()), )) quoteAmount = big.NewInt(0) + } else { + quoteAmount = input.DepositAmount } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
services/rfq/relayer/quoter/quoter.go(4 hunks)
🔇 Additional comments (3)
services/rfq/relayer/quoter/quoter.go (3)
367-370: LGTM! Deposit amount parsing looks good.
The code correctly parses the deposit amount using SetString and includes appropriate error handling for invalid input.
379-379: LGTM! QuoteInput struct changes are consistent.
The DepositAmount field is correctly added to the QuoteInput struct and consistently used in the codebase.
Also applies to: 586-586
549-549: LGTM! Proper initialization for backward compatibility.
Setting DepositAmount to nil in generateQuotes maintains backward compatibility for regular quote generation.
| @@ -540,6 +546,7 @@ func (m *Manager) generateQuotes(parentCtx context.Context, chainID int, address | |||
| OriginBalance: originBalance, | |||
| DestBalance: balance, | |||
| DestRFQAddr: destRFQAddr.Hex(), | |||
| DepositAmount: nil, | |||
There was a problem hiding this comment.
Minor nit: let's add a small comment explaining why this is nil.
| // If input included a depositAmount, and our calculated quoteAmount is sufficient to cover it, | ||
| // then clip to the depositAmount. Otherwise return 0 to indicate inability to cover the requested amount. | ||
| if input.DepositAmount != nil { | ||
| if quoteAmount.Cmp(input.DepositAmount) >= 0 { | ||
| quoteAmount = input.DepositAmount | ||
| } else { | ||
| span.AddEvent("quote amount insufficient to cover deposit amount", trace.WithAttributes( | ||
| attribute.String("quote_amount", quoteAmount.String()), | ||
| attribute.String("deposit_amount", input.DepositAmount.String()), | ||
| )) | ||
| quoteAmount = big.NewInt(0) | ||
| } |
There was a problem hiding this comment.
Let's add some cases for this in TestGetOriginAmount
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
services/rfq/relayer/quoter/quoter_test.go (2)
245-405: Consider adding error case assertions.While the test coverage is comprehensive for valid scenarios, consider adding explicit test cases for error conditions such as:
- Invalid quote percentage values
- Malformed configuration values
- Edge cases around decimal precision
🧰 Tools
🪛 GitHub Check: Lint (services/rfq)
[failure] 265-265:
commentFormatting: put a space between//and comment text (gocritic)
[failure] 346-346:
accomodatesis a misspelling ofaccommodates(misspell)
265-265: Fix minor style issues.There are two minor issues to address:
- Add a space after
//in the comment on line 265- Fix the spelling of "accommodates" in the comment on line 346
Apply these fixes:
-//should return zero even though we can partially cover -- because deposit amount is all-or-nothing. +// should return zero even though we can partially cover -- because deposit amount is all-or-nothing. -// output s/b 0 as neither ceiling setting accomodates the deposit amount +// output s/b 0 as neither ceiling setting accommodates the deposit amountAlso applies to: 346-346
🧰 Tools
🪛 GitHub Check: Lint (services/rfq)
[failure] 265-265:
commentFormatting: put a space between//and comment text (gocritic)services/rfq/relayer/quoter/quoter.go (2)
367-371: Add test coverage for deposit amount parsing.The deposit amount parsing logic looks good, but it lacks test coverage. Consider adding test cases to verify successful parsing and error handling scenarios.
Would you like me to help generate test cases for the deposit amount parsing logic?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 368-370: services/rfq/relayer/quoter/quoter.go#L368-L370
Added lines #L368 - L370 were not covered by tests
Line range hint
367-796: Enhance test coverage for deposit amount functionality.The changes introduce important deposit amount handling functionality across multiple components:
- Parsing deposit amounts from RFQ requests
- Integrating deposit amounts into quote generation
- Handling deposit amounts in origin amount calculations
Consider creating a comprehensive test suite that covers all these aspects to ensure reliability.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 787-795: services/rfq/relayer/quoter/quoter.go#L787-L795
Added lines #L787 - L795 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
services/rfq/relayer/quoter/quoter.go(8 hunks)services/rfq/relayer/quoter/quoter_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/rfq/relayer/quoter/quoter.go
[warning] 368-370: services/rfq/relayer/quoter/quoter.go#L368-L370
Added lines #L368 - L370 were not covered by tests
[warning] 379-379: services/rfq/relayer/quoter/quoter.go#L379
Added line #L379 was not covered by tests
[warning] 787-795: services/rfq/relayer/quoter/quoter.go#L787-L795
Added lines #L787 - L795 were not covered by tests
🪛 GitHub Check: Lint (services/rfq)
services/rfq/relayer/quoter/quoter_test.go
[failure] 265-265:
commentFormatting: put a space between // and comment text (gocritic)
[failure] 346-346:
accomodates is a misspelling of accommodates (misspell)
🔇 Additional comments (4)
services/rfq/relayer/quoter/quoter_test.go (1)
205-244: LGTM! Well-structured test setup.
The test method is well-organized with clear helper functions, meaningful variable names, and proper test data setup.
services/rfq/relayer/quoter/quoter.go (3)
379-379: LGTM!
The addition of the DepositAmount field to QuoteInput struct is well-placed and correctly typed.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 379-379: services/rfq/relayer/quoter/quoter.go#L379
Added line #L379 was not covered by tests
549-549: LGTM!
Setting DepositAmount to nil for non-active quotes is correct and well-documented.
783-796: Add test coverage for deposit amount handling logic.
The deposit amount handling logic is well-implemented and properly documented. However, it lacks test coverage for critical scenarios:
- When quote amount is sufficient to cover deposit
- When quote amount is insufficient to cover deposit
- When deposit amount is nil (backward compatibility)
Would you like me to help generate comprehensive test cases for these scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 787-795: services/rfq/relayer/quoter/quoter.go#L787-L795
Added lines #L787 - L795 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
services/rfq/relayer/quoter/quoter_test.go (4)
210-211: Consider using constants for magic numbersThe balance and deposit amount values could be more readable if defined as named constants with clear units.
- balance := big.NewInt(1000_000_000) // 1000 USDC - depositAmount := big.NewInt(50_000_000) // 50 USDC + const ( + USDC_DECIMALS = 6 + BALANCE_USDC = 1000 + DEPOSIT_USDC = 50 + ) + balance := big.NewInt(BALANCE_USDC * int64(math.Pow10(USDC_DECIMALS))) + depositAmount := big.NewInt(DEPOSIT_USDC * int64(math.Pow10(USDC_DECIMALS)))
213-219: Add documentation for the quoteParams structThe struct could benefit from documentation explaining the purpose of each field.
+ // quoteParams encapsulates configuration parameters for quote generation + // quotePct: Percentage of balance available for quoting + // quoteOffset: Offset in basis points for quote adjustment + // minQuoteAmount: Minimum amount that can be quoted + // maxBalance: Maximum balance allowed + // maxQuoteAmount: Maximum amount that can be quoted type quoteParams struct { quotePct float64 quoteOffset float64 minQuoteAmount string maxBalance string maxQuoteAmount string }
245-445: Consider using subtests for better organizationThe test cases could be more organized and maintainable using Go's subtests feature. This would make it easier to run specific test cases and provide better failure output.
Here's an example of how to refactor using subtests:
- // Set default quote params; should return the depositAmount. - quoteAmount, err := s.manager.GetOriginAmount(s.GetTestContext(), input) - s.NoError(err) - expectedAmount := depositAmount - s.Equal(expectedAmount, quoteAmount) + testCases := []struct { + name string + params quoteParams + depositAmount *big.Int + expected *big.Int + }{ + { + name: "default_params", + params: quoteParams{}, + depositAmount: depositAmount, + expected: depositAmount, + }, + // Add other test cases... + } + + for _, tc := range testCases { + s.Run(tc.name, func() { + setQuoteParams(tc.params) + input.DepositAmount = tc.depositAmount + quoteAmount, err := s.manager.GetOriginAmount(s.GetTestContext(), input) + s.NoError(err) + s.Equal(tc.expected, quoteAmount) + }) + }
399-405: Add explicit error assertions for gas sufficiencyThe test case for insufficient gas should explicitly verify that the error is nil, as this is an expected condition rather than an error case.
// Toggle insufficient gas; should be 0. s.setGasSufficiency(false) quoteAmount, err = s.manager.GetOriginAmount(s.GetTestContext(), input) - s.NoError(err) + s.NoError(err, "GetOriginAmount should not return error for insufficient gas") + s.Zero(quoteAmount.Int64(), "Quote amount should be zero when gas is insufficient") - expectedAmount = big.NewInt(0) - s.Equal(expectedAmount, quoteAmount)
| @@ -202,6 +202,248 @@ func (s *QuoterSuite) TestIsProfitable() { | |||
| s.False(s.manager.IsProfitable(s.GetTestContext(), quote)) | |||
| } | |||
|
|
|||
| func (s *QuoterSuite) TestGetOriginAmountActiveQuotes() { | |||
There was a problem hiding this comment.
May be easier to add some test cases with the DepositAmount to the existing TestGetOriginAmount() test, but up to you
There was a problem hiding this comment.
i originally did that, but the function was huge & mostly cloned from the existing ones. felt cleaner to separate
| @@ -364,13 +364,19 @@ func (m *Manager) generateActiveRFQ(ctx context.Context, msg *model.ActiveRFQMes | |||
| } | |||
| span.SetAttributes(attribute.String("request_id", rfqRequest.RequestID)) | |||
|
|
|||
| depositAmount, ok := new(big.Int).SetString(rfqRequest.Data.OriginAmount, 10) | |||
There was a problem hiding this comment.
Could we keep this as originAmount instead of depositAmount for consistency?
To be clear I don't really have preference between originAmount and depositAmount but I do think it's cleaner to keep the naming consistent across the codebase. If you do prefer depositAmount we can run with that, but we should change it everywhere
There was a problem hiding this comment.
yeah youre right - actually for even more clarity i'd like to go with "originAmountExact" which is also consistent with swap interfaces
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (13)
services/rfq/api/model/request.go (1)
54-62: Consider aligning JSON tag with field name for clarityWhile the rename from
OriginAmounttoOriginAmountExactimproves semantic clarity, the JSON tag remains as"origin_amount". Although this maintains API compatibility, it could lead to confusion during maintenance.Consider updating both or adding a comment explaining the discrepancy:
- OriginAmountExact string `json:"origin_amount"` + // OriginAmountExact represents the exact deposit amount. + // Note: JSON tag remains "origin_amount" for backward compatibility + OriginAmountExact string `json:"origin_amount"`services/rfq/api/model/response.go (1)
82-89: Add documentation for the GetOpenQuoteRequestsResponse struct and its fields.Consider adding documentation comments to describe the purpose of the struct and each field, especially explaining the significance of
OriginAmountExactin the context of deposit amounts.+// GetOpenQuoteRequestsResponse represents the response for open quote requests, +// containing details about the quote request including the exact origin amount for deposit. type GetOpenQuoteRequestsResponse struct { + // UserAddress is the address of the user who created the quote request UserAddress string `json:"user_address"` + // OriginChainID is the ID of the source chain OriginChainID uint64 `json:"origin_chain_id"` + // OriginTokenAddr is the token address on the source chain OriginTokenAddr string `json:"origin_token"` + // DestChainID is the ID of the destination chain DestChainID uint64 `json:"dest_chain_id"` + // DestTokenAddr is the token address on the destination chain DestTokenAddr string `json:"dest_token"` + // OriginAmountExact is the precise deposit amount in origin token OriginAmountExact string `json:"origin_amount_exact"` + // ExpirationWindow is the duration in which the quote request is valid ExpirationWindow int `json:"expiration_window"` + // CreatedAt is the timestamp when the quote request was created CreatedAt time.Time `json:"created_at"` }services/rfq/api/rest/rfq.go (2)
229-231: Enhance error message for better debuggingConsider providing more context in the error message to help with debugging.
- return nil, errors.New("invalid origin amount exact") + return nil, fmt.Errorf("invalid origin amount exact: unable to parse '%s' as base-10 number", request.Data.OriginAmountExact)
Line range hint
232-266: Consider extracting complex calculations into helper functionsThe quote calculation logic involves multiple big number operations. Consider extracting these into helper functions to improve readability and maintainability.
Example refactor:
func calculateQuoteDestAmount(originAmount *big.Int, quote *db.Quote) (*big.Int, error) { quotePrice := new(big.Float).Quo( new(big.Float).SetInt(quote.DestAmount.BigInt()), new(big.Float).SetInt(quote.MaxOriginAmount.BigInt()), ) rawDestAmount := new(big.Float).Mul( new(big.Float).SetInt(originAmount), quotePrice, ) rawDestAmountInt, _ := rawDestAmount.Int(nil) if rawDestAmountInt.Cmp(quote.FixedFee.BigInt()) < 0 { return nil, errors.New("destination amount less than fixed fee") } return new(big.Int).Sub(rawDestAmountInt, quote.FixedFee.BigInt()), nil }services/rfq/api/rest/rfq_test.go (2)
95-100: Consider adding assertions for exact amount handlingWhile the test verifies basic functionality, consider adding assertions to ensure that the
OriginAmountExactis strictly respected in the quote generation process. This would validate that the system properly handles the exact deposit amount requirement.// Assert the response c.Assert().True(userQuoteResp.Success) c.Assert().Equal("active", userQuoteResp.QuoteType) c.Assert().Equal(destAmount, userQuoteResp.DestAmount) +// Verify that the origin amount is exactly as requested +c.Assert().Equal(userRequestAmount.String(), userQuoteResp.OriginAmount)
196-201: Enhance test coverage for multiple relayersConsider adding assertions to verify that all relayer quotes respect the
OriginAmountExactrequirement. This would ensure that the exact deposit amount constraint is enforced across all relayers.// Assert the response c.Assert().True(userQuoteResp.Success) c.Assert().Equal("active", userQuoteResp.QuoteType) c.Assert().Equal(destAmount, userQuoteResp.DestAmount) + +// Verify that all relayer quotes use the exact origin amount +quotes, err := c.database.GetQuotesForRequest(c.GetTestContext(), userQuoteResp.RequestID) +c.Require().NoError(err) +for _, quote := range quotes { + c.Assert().Equal(userRequestAmount.String(), quote.OriginAmount.String()) +}services/rfq/api/rest/server_test.go (1)
212-217: Consider diversifying test data for better coverage.All three test requests use identical values (
OriginAmountExact: "100.0"). Consider using different amounts to test various scenarios:
- Edge cases (very small/large amounts)
- Different decimal places
- Different number formats
Consider refactoring the test data setup:
testRequests := []*model.PutRFQRequest{ { Data: model.QuoteData{ OriginChainID: 1, DestChainID: 42161, OriginTokenAddr: "0xOriginTokenAddr", DestTokenAddr: "0xDestTokenAddr", - OriginAmountExact: "100.0", + OriginAmountExact: "0.000001", // Test minimum amount ExpirationWindow: 100, }, }, { Data: model.QuoteData{ OriginChainID: 1, DestChainID: 42161, OriginTokenAddr: "0xOriginTokenAddr", DestTokenAddr: "0xDestTokenAddr", - OriginAmountExact: "100.0", + OriginAmountExact: "1000000.0", // Test large amount ExpirationWindow: 100, }, }, { Data: model.QuoteData{ OriginChainID: 1, DestChainID: 42161, OriginTokenAddr: "0xOriginTokenAddr", DestTokenAddr: "0xDestTokenAddr", - OriginAmountExact: "100.0", + OriginAmountExact: "123.456789", // Test precise decimal amount ExpirationWindow: 100, }, }, }Also applies to: 222-227, 232-237
services/rfq/relayer/quoter/quoter_test.go (3)
205-244: Consider enhancing test documentation with a test table.While the test setup is well-structured, consider using a table-driven test approach to make the test cases more maintainable and easier to understand. This would help document the various scenarios being tested in a more organized manner.
func (s *QuoterSuite) TestGetOriginAmountActiveQuotes() { + testCases := []struct { + name string + params quoteParams + depositAmount *big.Int + expectedAmount *big.Int + }{ + { + name: "default_params", + params: quoteParams{ + quotePct: 100, + quoteOffset: 0, + minQuoteAmount: "0", + maxBalance: "0", + }, + depositAmount: big.NewInt(50_000_000), + expectedAmount: big.NewInt(50_000_000), + }, + // Add more test cases... + }
245-444: Consider adding more edge cases and error scenarios.While the test coverage is good, consider adding these additional test cases:
- Error handling for invalid quote parameters (e.g., negative quote percentages)
- Behavior with extremely large numbers (near max int)
- Concurrent quote requests with the same deposit amount
Also, some test cases have similar setups that could be consolidated using the suggested table-driven approach to reduce code duplication.
205-445: LGTM! Consider organizing test cases by category.The test implementation is thorough and well-structured. To further improve maintainability, consider organizing the test cases into logical groups using subtests:
- Basic functionality tests
- Parameter boundary tests
- Error condition tests
- Edge case tests
services/rfq/relayer/quoter/quoter.go (3)
367-370: Add validation for negative deposit amounts.The current implementation correctly parses the deposit amount but doesn't validate if it's negative. Consider adding a check to ensure the deposit amount is positive.
originAmountExact, ok := new(big.Int).SetString(rfqRequest.Data.OriginAmountExact, 10) if !ok { return nil, fmt.Errorf("invalid rfq request deposit amount: %s", rfqRequest.Data.OriginAmountExact) } +if originAmountExact.Sign() < 0 { + return nil, fmt.Errorf("negative deposit amount not allowed: %s", rfqRequest.Data.OriginAmountExact) +}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 368-368: services/rfq/relayer/quoter/quoter.go#L368
Added line #L368 was not covered by tests
[warning] 370-370: services/rfq/relayer/quoter/quoter.go#L370
Added line #L370 was not covered by tests
783-796: Enhance comment clarity for exact amount handling.The current implementation is correct, but the comment could be more descriptive about the business logic and implications of returning 0 for insufficient quotes.
-// If input included a OriginAmountExact, and our calculated ceiling at this point is sufficient to cover it, -// then clip to the OriginAmountExact to indicate ability to cover that exact amount, as requested. -// Otherwise return 0 to indicate inability to cover the requested amount. +// Handle exact amount requirements for Active Quotes: +// 1. If the calculated quote amount is sufficient to cover the exact amount requested, +// clip to the exact amount to provide precise quote matching. +// 2. If the quote amount is insufficient, return 0 to reject the quote entirely, +// preventing partial fills that might not meet user expectations.
368-370: Add test coverage for deposit amount parsing errors.The error handling for invalid deposit amounts lacks test coverage. This could lead to undetected issues with malformed inputs.
Would you like me to help generate test cases that cover:
- Invalid numeric formats
- Non-decimal strings
- Empty strings
- Extremely large numbers
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 368-368: services/rfq/relayer/quoter/quoter.go#L368
Added line #L368 was not covered by tests
[warning] 370-370: services/rfq/relayer/quoter/quoter.go#L370
Added line #L370 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
services/rfq/api/db/api_db.go(1 hunks)services/rfq/api/model/request.go(1 hunks)services/rfq/api/model/response.go(1 hunks)services/rfq/api/rest/handler.go(1 hunks)services/rfq/api/rest/rfq.go(3 hunks)services/rfq/api/rest/rfq_test.go(6 hunks)services/rfq/api/rest/server_test.go(1 hunks)services/rfq/relayer/quoter/quoter.go(8 hunks)services/rfq/relayer/quoter/quoter_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/rfq/relayer/quoter/quoter.go
[warning] 368-368: services/rfq/relayer/quoter/quoter.go#L368
Added line #L368 was not covered by tests
[warning] 370-370: services/rfq/relayer/quoter/quoter.go#L370
Added line #L370 was not covered by tests
🔇 Additional comments (11)
services/rfq/api/model/request.go (1)
54-62: Verify the impact of QuoteData changes
The field rename in QuoteData struct might affect various parts of the codebase that construct or parse this structure.
services/rfq/api/model/response.go (1)
82-89: LGTM! Verify consistency across the codebase.
The change from OriginAmount to OriginAmountExact is appropriate for precise deposit amount handling.
✅ Verification successful
Inconsistent usage of OriginAmount vs OriginAmountExact detected
The codebase shows mixed usage of field names:
OriginAmountExactis used in API models and request/response structuresOriginAmountis still widely used in internal services, database models, and business logic
Let's verify if this is an intentional separation between external API and internal implementations:
Intentional API/Internal field name distinction confirmed
The codebase shows a clear pattern:
- External API interfaces (request/response models) consistently use
OriginAmountExact - Internal implementations maintain
OriginAmountfor business logic - Proper conversion between the two occurs at the API boundary in
services/rfq/api
This separation appears to be an intentional design choice to distinguish between the external API contract and internal implementation details. The change from OriginAmount to OriginAmountExact in the API models is consistent and properly handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of OriginAmountExact across the codebase
# Expected: All related structs should use OriginAmountExact instead of OriginAmount
# Search for any remaining instances of OriginAmount
rg "OriginAmount[^E]" --type go
# Search for consistent usage of OriginAmountExact
rg "OriginAmountExact" --type go
Length of output: 19782
Script:
#!/bin/bash
# Check if there's a pattern where OriginAmountExact is used for external APIs
# and OriginAmount for internal implementations
# Check API-related files for the pattern
rg -C 2 "OriginAmountExact" --type go services/rfq/api/
# Check if there are any direct connections between OriginAmount and OriginAmountExact
ast-grep --pattern 'OriginAmount = $_.OriginAmountExact'
ast-grep --pattern 'OriginAmountExact = $_.OriginAmount'
Length of output: 7046
services/rfq/api/rest/rfq.go (1)
149-155: LGTM: Field mapping is correct and consistent
The change from OriginAmount to OriginAmountExact aligns with the PR objective to handle deposit amounts properly.
services/rfq/api/rest/handler.go (1)
281-288: LGTM! Verify consistency of OriginAmountExact usage
The changes correctly map the deposit amount from the database model to the API response using OriginAmountExact, which aligns with the PR objectives of incorporating deposit amounts into ActiveQuoting.
Let's verify the consistency of this change across related files:
✅ Verification successful
Based on the search results, I can see that OriginAmountExact is consistently used across the codebase in the RFQ service, particularly in:
- API models (
services/rfq/api/model/) - Database models (
services/rfq/api/db/) - Quoter service (
services/rfq/relayer/quoter/) - REST handlers (
services/rfq/api/rest/)
The old field OriginAmount is still present but primarily in different contexts (explorer service, contracts, etc.) and serves different purposes. The changes in the review are consistent with the codebase's usage of OriginAmountExact for active quote requests.
Changes are consistent with codebase patterns ✅
The mapping of dbQuote.OriginAmountExact to the response model is correctly implemented and follows the established pattern across the RFQ service.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of OriginAmountExact across the codebase
# Check for any remaining references to the old field name
rg "OriginAmount[^E]" --type go
# Check the usage of the new field name
rg "OriginAmountExact" --type go
# Check related model definitions
ast-grep --pattern 'type $_ struct {
$$$
OriginAmountExact $_
$$$
}'
Length of output: 23775
services/rfq/api/db/api_db.go (2)
145-157: LGTM! Verify database migration for the renamed column.
The struct changes look good. The renamed field OriginAmountExact properly reflects the deposit amount requirement from the PR objectives.
Let's verify if a database migration exists for this column rename:
#!/bin/bash
# Description: Check for database migration files related to the column rename
# Expected: Find migration files that rename 'origin_amount' to 'origin_amount_exact'
# Search for migration files
fd -e sql -e go migration | xargs rg -l 'origin_amount'162-177: LGTM! Verify consistency of the field rename across the codebase.
The function changes correctly handle the renamed field and maintain proper error handling.
Let's verify the consistency of this change across the codebase:
services/rfq/api/rest/rfq_test.go (4)
69-69: LGTM: Verification assertion updated correctly
The assertion has been properly updated to verify the new OriginAmountExact field, maintaining consistency across test cases.
146-151: LGTM: Expired request test updated correctly
The test case properly validates the expired request scenario while maintaining the new OriginAmountExact field structure.
279-284: LGTM: Fallback test updated correctly
The test case properly validates the fallback to passive quotes while maintaining the exact amount requirement.
349-354: LGTM: Best quote comparison test updated correctly
The test case effectively validates the quote comparison logic while properly handling the exact amount requirement.
services/rfq/relayer/quoter/quoter.go (1)
542-549: Implementation looks good!
The changes correctly implement deposit amount handling for active quotes while maintaining backward compatibility for regular quotes by setting OriginAmountExact to nil. The code is well-structured with appropriate error handling and debug logging.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
services/rfq/api/model/request.go (1)
54-62: LGTM! Consider adding documentation for clarity.The field name change from
OriginAmounttoOriginAmountExactbetter reflects its purpose for handling exact deposit amounts. However, consider adding documentation to explain the distinction between exact and non-exact amounts.Add a comment above the field:
type QuoteData struct { OriginChainID int `json:"origin_chain_id"` DestChainID int `json:"dest_chain_id"` OriginTokenAddr string `json:"origin_token_addr"` DestTokenAddr string `json:"dest_token_addr"` + // OriginAmountExact represents the exact deposit amount specified by the user OriginAmountExact string `json:"origin_amount"` ExpirationWindow int64 `json:"expiration_window"` DestAmount *string `json:"dest_amount"` RelayerAddress *string `json:"relayer_address"` QuoteID *string `json:"quote_id"` }services/rfq/api/model/response.go (1)
87-87: Consider consistent JSON field namingThe JSON tag
origin_amount_exactdiffers from the naming pattern used in other fields (e.g.,user_address,origin_chain_id). Consider usingorigin_exact_amountto maintain consistency with the established field naming convention.- OriginAmountExact string `json:"origin_amount_exact"` + OriginAmountExact string `json:"origin_exact_amount"`services/rfq/api/rest/rfq.go (1)
229-231: Enhance error message for invalid amountThe current error message "invalid origin amount exact" could be more descriptive to help users understand the issue.
- return nil, errors.New("invalid origin amount exact") + return nil, fmt.Errorf("invalid origin amount exact: %q is not a valid decimal string", request.Data.OriginAmountExact)services/rfq/api/rest/rfq_test.go (1)
95-100: LGTM: Consistent field updates across test casesThe
OriginAmountExactfield is consistently updated across all test cases, maintaining comprehensive test coverage for various RFQ scenarios.Consider reducing duplication by extracting the common
QuoteDatasetup into a helper function:+func createTestQuoteData(originChainID, destChainID int, amount *big.Int, expirationWindow int64) model.QuoteData { + return model.QuoteData{ + OriginChainID: originChainID, + OriginTokenAddr: originTokenAddr, + DestChainID: destChainID, + DestTokenAddr: destTokenAddr, + OriginAmountExact: amount.String(), + ExpirationWindow: expirationWindow, + } +}This would simplify the test cases and make future updates easier to maintain.
Also applies to: 146-151, 196-201, 279-284, 349-354
services/rfq/api/rest/server_test.go (1)
212-217: Enhance test coverage with varied test dataThe test data uses identical values ("100.0") for
OriginAmountExactacross all test cases. Consider adding test cases with:
- Different decimal places
- Edge cases (very large/small amounts)
- Invalid formats
This would provide better coverage of the deposit amount handling functionality.testRequests := []*model.PutRFQRequest{ { Data: model.QuoteData{ OriginChainID: 1, DestChainID: 42161, OriginTokenAddr: "0xOriginTokenAddr", DestTokenAddr: "0xDestTokenAddr", - OriginAmountExact: "100.0", + OriginAmountExact: "100.0", // Standard case ExpirationWindow: 100, }, }, { Data: model.QuoteData{ OriginChainID: 1, DestChainID: 42161, OriginTokenAddr: "0xOriginTokenAddr", DestTokenAddr: "0xDestTokenAddr", - OriginAmountExact: "100.0", + OriginAmountExact: "999999.999999", // Large amount with decimals ExpirationWindow: 100, }, }, { Data: model.QuoteData{ OriginChainID: 1, DestChainID: 42161, OriginTokenAddr: "0xOriginTokenAddr", DestTokenAddr: "0xDestTokenAddr", - OriginAmountExact: "100.0", + OriginAmountExact: "0.000001", // Small amount ExpirationWindow: 100, }, }, }Also applies to: 222-227, 232-237
services/rfq/relayer/quoter/quoter_test.go (2)
245-370: Improve consistency in test case comments.While the test cases are comprehensive, consider standardizing the comment format for better readability. For example:
-// Set QuotePct to 50 with MinQuoteAmount of 0; should still be 100% of deposit amount -// IE: we're only willing to use 50% of our balance, but deposit amount is well below that. +// Test: QuotePct=50%, MinQuoteAmount=0 +// Expect: 100% of deposit amount (deposit amount is below 50% balance threshold)
371-445: Consider adding more error case assertions.While the edge cases are well covered, consider adding assertions for error scenarios:
- Invalid configuration combinations
- Overflow scenarios with large numbers
- Error responses from dependent services
Example:
// Test: Invalid configuration - negative QuotePct setQuoteParams(quoteParams{ quotePct: -1, // ... other params }) _, err = s.manager.GetOriginAmount(s.GetTestContext(), input) s.Error(err) s.Contains(err.Error(), "invalid quote percentage")services/rfq/relayer/quoter/quoter.go (1)
Line range hint
710-796: Consider adding a high-level overview comment for the quote amount calculation flow.While individual sections are well-documented, a high-level comment explaining the sequence and purpose of each clipping operation would improve maintainability.
Add a comment like:
+// getOriginAmount calculates the quote amount through a series of clipping operations: +// 1. Apply quote percentage limit +// 2. Enforce minimum quote amount +// 3. Clip by maximum origin balance +// 4. Clip by destination balance +// 5. Clip by maximum quote amount +// 6. Deduct gas costs +// 7. Handle exact amount requirements func (m *Manager) getOriginAmount(parentCtx context.Context, input QuoteInput) (quoteAmount *big.Int, err error) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
services/rfq/api/db/api_db.go(1 hunks)services/rfq/api/model/request.go(1 hunks)services/rfq/api/model/response.go(1 hunks)services/rfq/api/rest/handler.go(1 hunks)services/rfq/api/rest/rfq.go(3 hunks)services/rfq/api/rest/rfq_test.go(6 hunks)services/rfq/api/rest/server_test.go(1 hunks)services/rfq/relayer/quoter/quoter.go(8 hunks)services/rfq/relayer/quoter/quoter_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/rfq/relayer/quoter/quoter.go
[warning] 368-368: services/rfq/relayer/quoter/quoter.go#L368
Added line #L368 was not covered by tests
[warning] 370-370: services/rfq/relayer/quoter/quoter.go#L370
Added line #L370 was not covered by tests
🔇 Additional comments (11)
services/rfq/api/model/request.go (1)
58-58: Verify the impact of the field rename.
The field has been renamed from OriginAmount to OriginAmountExact while keeping the same JSON tag. Let's verify all references are updated.
✅ Verification successful
Based on the search results, I can see that OriginAmountExact is used consistently in the RFQ API model and related components. The field rename from OriginAmount to OriginAmountExact in the request model is properly reflected in the codebase. The occurrences of OriginAmount found in the search results are actually different fields used in other contexts (like database models, contracts, and event structures) and are not related to this specific JSON field rename.
Field rename is properly implemented and has no unintended impact.
The JSON tag origin_amount remains unchanged, ensuring backward compatibility with API clients, while the Go struct field name has been updated from OriginAmount to OriginAmountExact for better clarity in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining references to the old field name that might have been missed
# Search for struct field references
ast-grep --pattern 'OriginAmount'
# Search for string references including comments and tests
rg 'OriginAmount' -g '!{*.mod,*.sum}'
Length of output: 38676
services/rfq/api/rest/rfq.go (1)
149-155: LGTM: Clean struct initialization with exact deposit amount
The change from OriginAmount to OriginAmountExact aligns with the PR objective and maintains a clean structure.
services/rfq/api/db/api_db.go (2)
162-177: LGTM! Clean implementation of the deposit amount changes
The conversion from string to decimal is properly handled with appropriate error checking, and the implementation aligns well with the PR's objective of incorporating deposit amounts into the ActiveQuoting process.
145-157: Verify DB schema migration for the renamed field
The renaming of OriginAmount to OriginAmountExact in the struct requires a corresponding DB schema migration to prevent runtime issues.
services/rfq/api/rest/rfq_test.go (1)
69-69: LGTM: Verification updated for exact amount assertion
The assertion change correctly aligns with the new OriginAmountExact field, maintaining proper verification of quote requests.
services/rfq/api/rest/server_test.go (1)
212-237: Verify the field rename across the codebase
The change from OriginAmount to OriginAmountExact should be consistently applied across all related components.
services/rfq/relayer/quoter/quoter_test.go (2)
205-244: LGTM! Well-structured test setup.
The test setup is clear, organized, and follows good testing practices with reusable helper functions and meaningful variable names.
205-205: Well-organized test separation.
Good decision to keep TestGetOriginAmountActiveQuotes separate from TestGetOriginAmount. This separation improves test clarity and maintainability by keeping distinct test scenarios in focused test methods.
services/rfq/relayer/quoter/quoter.go (3)
542-549: LGTM! Clear initialization of quote input parameters.
The code correctly initializes OriginAmountExact to nil for non-active quotes, maintaining backward compatibility.
783-796: LGTM! Well-implemented exact amount handling.
The code correctly implements the deposit amount requirements:
- If the calculated quote amount is sufficient, it's clipped to the exact amount requested
- If insufficient, it returns 0 to indicate inability to cover the requested amount
- Clear logging helps with debugging and monitoring
367-370: Add test coverage for deposit amount parsing.
This is a critical path in the active quote generation flow that requires test coverage to ensure proper handling of valid and invalid deposit amounts.
Would you like me to help generate test cases for:
- Valid deposit amount parsing
- Invalid deposit amount handling
- Edge cases (empty string, negative values, etc.)
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 368-368: services/rfq/relayer/quoter/quoter.go#L368
Added line #L368 was not covered by tests
[warning] 370-370: services/rfq/relayer/quoter/quoter.go#L370
Added line #L370 was not covered by tests
| UserAddress string `json:"user_address"` | ||
| OriginChainID uint64 `json:"origin_chain_id"` | ||
| OriginTokenAddr string `json:"origin_token"` | ||
| DestChainID uint64 `json:"dest_chain_id"` | ||
| DestTokenAddr string `json:"dest_token"` | ||
| OriginAmountExact string `json:"origin_amount_exact"` | ||
| ExpirationWindow int `json:"expiration_window"` | ||
| CreatedAt time.Time `json:"created_at"` |
There was a problem hiding this comment.
💡 Codebase verification
Breaking change detected in API response field
The field OriginAmountExact in GetOpenQuoteRequestsResponse struct is using json:"origin_amount_exact" while the client code and existing implementations are using "origin_amount". This mismatch will break API compatibility.
services/rfq/api/model/response.go: Change the JSON tag fromorigin_amount_exacttoorigin_amountto maintain backward compatibilitycontrib/restclient/client.gen.go: Client code expectsorigin_amountin the response- Multiple services are using
origin_amountin their logging and processing:services/rfq/util/util.go,services/rfq/relayer/quoter/quoter.go, etc.
🔗 Analysis chain
Semantic improvement but potential breaking change
The renaming from OriginAmount to OriginAmountExact better reflects the precise nature of deposit amounts in the ActiveQuoting system. However, this change might affect existing API clients.
Let's verify the impact on API clients:
Let me gather more context about the JSON field usage to understand the full impact.
Let me check the RFQ API swagger documentation to understand the API contract.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct references to the old JSON field name
rg -l 'origin_amount"' --type go
# Search for struct field usage
ast-grep --pattern 'OriginAmount'
Length of output: 2794
Script:
#!/bin/bash
# Check the actual JSON field usage in the codebase
rg '"origin_amount"' -A 2 -B 2 --type go
# Check for any API documentation or OpenAPI specs
fd swagger.json
fd openapi.yaml
Length of output: 2919
Script:
#!/bin/bash
# Check the RFQ API swagger documentation
cat services/rfq/api/docs/swagger.json | jq '.paths[][] | select(.description != null) | .description'
# Look for the field in the actual response struct implementation
rg -A 5 -B 5 'GetOpenQuoteRequestsResponse struct' --type go
Length of output: 2899
ActiveQuoting needs to consider the deposit amount when preparing an Active RFQ response and utilize it to calculate a corresponding dest amount.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
origin_amounttoorigin_amount_exactfor clarity and precision in data representation.