feat(rfq-relayer): add MaxRelayAmount #3259
Conversation
WalkthroughThe pull request introduces several modifications across multiple files to enhance the quote handling functionality in the RFQ service. Key changes include the addition of a Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3259 +/- ##
============================================
Coverage ? 24.98855%
============================================
Files ? 198
Lines ? 13106
Branches ? 82
============================================
Hits ? 3275
Misses ? 9549
Partials ? 282
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://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
services/rfq/relayer/quoter/quoter_test.go (1)
Line range hint
184-196: LGTM with a minor suggestion: Consider clarifying the function nameThe updated
setQuoteParamsfunction now uses the newquoteParamsstruct, which improves consistency and readability. However, the function name might not fully convey that it's setting parameters for both destination and origin tokens. Consider renaming it tosetQuoteParamsForBothChainsor adding a comment to clarify its behavior.services/rfq/relayer/relconfig/getters.go (2)
749-750: Consider initializingdefaultMaxQuoteAmountThe
defaultMaxQuoteAmountis declared but not initialized. Whilenilis a valid default for this case (signifying no maximum limit), it might be beneficial to explicitly initialize it for clarity.Consider modifying the declaration as follows:
-var defaultMaxQuoteAmount *big.Int // nil +var defaultMaxQuoteAmount *big.Int = nil // Explicitly set to nil, signifying no maximum limit
751-782: Consider adding error loggingWhile the function handles error cases gracefully by returning the default value, it might be beneficial to log these errors for debugging purposes.
Consider adding error logging in cases where the configuration is not found or parsing fails. For example:
if !ok { + log.Printf("Error: Chain configuration not found for chainID: %d", chainID) return defaultMaxQuoteAmount }if !ok { + log.Printf("Error: Failed to parse MaxQuoteAmount for chainID: %d, token: %s", chainID, addr.Hex()) return defaultMaxQuoteAmount }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- services/rfq/relayer/quoter/quoter.go (2 hunks)
- services/rfq/relayer/quoter/quoter_test.go (2 hunks)
- services/rfq/relayer/relconfig/config.go (1 hunks)
- services/rfq/relayer/relconfig/getters.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
services/rfq/relayer/quoter/quoter_test.go (3)
176-182: LGTM: Introduction ofquoteParamsstruct improves code organizationThe new
quoteParamsstruct effectively groups related quote parameters, enhancing the readability and maintainability of the test cases.
214-219: LGTM: Well-structured test cases using the newquoteParamsstructThe updated test cases effectively utilize the new
quoteParamsstruct, improving readability and maintainability. They cover a good range of scenarios, including edge cases, with clear expected results and assertions.Also applies to: 226-231, 238-243, 250-255, 262-267
273-284: LGTM: New test case for MaxQuoteAmountThe new test case effectively verifies the behavior of the MaxQuoteAmount parameter. It's a valuable addition to ensure the new functionality is working as expected and maintains consistency with the existing test style.
services/rfq/relayer/relconfig/getters.go (1)
751-782: LGTM: NewGetMaxQuoteAmountfunction looks goodThe new
GetMaxQuoteAmountfunction is well-implemented and follows the existing patterns in the file. It correctly handles various edge cases and scales the quote amount by the token decimals.A few observations:
- The function properly checks for the existence of chain and token configurations.
- It correctly handles cases where the max quote amount is not set or is invalid.
- The scaling of the quote amount by token decimals is implemented correctly.
services/rfq/relayer/quoter/quoter.go (2)
Line range hint
716-723: LGTM: ClippingquoteAmountby Destination BalanceThe code correctly ensures that
quoteAmountdoes not exceed the availableDestBalance, preventing over-allocation of funds beyond what is currently available on the destination chain.
725-733: LGTM: AddingmaxQuoteAmountClipping for Maximum Limit EnforcementThe new logic appropriately limits
quoteAmountbymaxQuoteAmountas defined in the configuration. This enhances the robustness of the quote generation by enforcing an upper bound on the quote amounts.
| // MaxQuoteAmount is the maximum amount to quote for this token in human-readable units. | ||
| MaxQuoteAmount string `yaml:"max_quote_amount"` |
There was a problem hiding this comment.
💡 Codebase verification
Action Required: MaxQuoteAmount is not properly utilized in GetMaxQuoteAmount.
The GetMaxQuoteAmount function currently returns defaultMaxQuoteAmount without using the configured MaxQuoteAmount. This means the maximum quote limit is not being enforced as intended.
Files to address:
services/rfq/relayer/relconfig/getters.goservices/rfq/relayer/quoter/quoter.go
🔗 Analysis chain
LGTM! Consider updating related quote generation logic.
The addition of the MaxQuoteAmount field to the TokenConfig struct is a good improvement. It complements the existing MinQuoteAmount field and provides more control over the quoting process. The field is consistently named and tagged for YAML serialization.
To ensure this change is properly integrated, please verify that the quote generation logic is updated to respect this new maximum limit. You can use the following script to check for potential usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for files that might need updating to use MaxQuoteAmount
echo "Files potentially needing updates for MaxQuoteAmount:"
rg -l "MinQuoteAmount" --type go | grep -v "config.go"
Length of output: 292
Script:
#!/bin/bash
# Search for 'MaxQuoteAmount' usage in identified files
rg 'MaxQuoteAmount' services/rfq/relayer/relconfig/getters.go services/rfq/relayer/quoter/quoter_test.go services/rfq/relayer/quoter/quoter.go
Length of output: 1329
Script:
#!/bin/bash
# Extract the implementation of GetMaxQuoteAmount from getters.go
ast-grep --lang go --pattern 'func (c Config) GetMaxQuoteAmount($_, $_) $_ { $$$ }' services/rfq/relayer/relconfig/getters.go
Length of output: 2397
| // GetMaxQuoteAmount returns the quote amount for the given chain and address. | ||
| // Note that this getter returns the value in native token decimals. | ||
| func (c Config) GetMaxQuoteAmount(chainID int, addr common.Address) *big.Int { | ||
| chainCfg, ok := c.Chains[chainID] | ||
| if !ok { | ||
| return defaultMaxQuoteAmount | ||
| } | ||
|
|
||
| var tokenCfg *TokenConfig | ||
| for _, cfg := range chainCfg.Tokens { | ||
| if common.HexToAddress(cfg.Address).Hex() == addr.Hex() { | ||
| cfgCopy := cfg | ||
| tokenCfg = &cfgCopy | ||
| break | ||
| } | ||
| } | ||
| if tokenCfg == nil { | ||
| return defaultMaxQuoteAmount | ||
| } | ||
| quoteAmountFlt, ok := new(big.Float).SetString(tokenCfg.MaxQuoteAmount) | ||
| if !ok { | ||
| return defaultMaxQuoteAmount | ||
| } | ||
| if quoteAmountFlt.Cmp(big.NewFloat(0)) <= 0 { | ||
| return defaultMaxQuoteAmount | ||
| } | ||
|
|
||
| // Scale the minQuoteAmount by the token decimals. | ||
| denomDecimalsFactor := new(big.Int).Exp(big.NewInt(10), big.NewInt(int64(tokenCfg.Decimals)), nil) | ||
| quoteAmountScaled, _ := new(big.Float).Mul(quoteAmountFlt, new(big.Float).SetInt(denomDecimalsFactor)).Int(nil) | ||
| return quoteAmountScaled | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Impact of the new GetMaxQuoteAmount function confirmed
The addition of the GetMaxQuoteAmount function affects multiple parts of the codebase where maximum quote amounts are utilized. Ensure the following areas are reviewed and updated accordingly:
services/rfq/api/rest/handler.goservices/rfq/api/db/api_db_test.goservices/rfq/relayer/relconfig/getters.goservices/rfq/relayer/relconfig/config.goservices/rfq/relayer/quoter/quoter.goservices/rfq/relayer/quoter/quoter_test.go
🔗 Analysis chain
Verify the impact of the new GetMaxQuoteAmount function
The addition of this new function might require updates in other parts of the codebase where maximum quote amounts are used.
Let's check for any existing usages of maximum quote amounts that might need to be updated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential usages of maximum quote amounts
rg -i "max.*quote.*amount" --type go
Length of output: 2676
Deploying sanguine-fe with
|
| Latest commit: |
2639cd2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3769f41f.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-max-quote-amount.sanguine-fe.pages.dev |
dwasse
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
services/rfq/relayer/quoter/quoter_test.go (3)
139-145: LGTM! Consider adding an additional test case.The new test case for max relay amount is well-implemented and checks the correct behavior. It's a good addition to ensure the quoter respects the max relay amount constraint.
Consider adding another test case where the max relay amount is greater than the balance to ensure the quoter behaves correctly in both scenarios.
Line range hint
191-203: Effective update of setQuoteParams function.The
setQuoteParamsfunction has been successfully updated to use the newquoteParamsstruct. It correctly sets the configuration for both destination and origin tokens, improving the test setup process.Consider adding a comment explaining the purpose of this function, as it plays a crucial role in setting up the test environment.
221-291: Comprehensive test cases for GetOriginAmount.The new test cases for
GetOriginAmountare well-implemented and cover a wide range of scenarios. They effectively test different combinations of quote percentages, offsets, and constraints, ensuring the function behaves correctly under various conditions.To improve test readability, consider adding brief comments before each test case explaining the specific scenario being tested. This would make it easier for other developers to understand the purpose of each test at a glance.
services/rfq/relayer/relconfig/getters.go (2)
751-782: Minor improvements forGetMaxRelayAmountfunctionThe implementation of
GetMaxRelayAmountis well-structured and consistent with other getter functions. However, there are a few minor issues that could be addressed:
- The comment on line 752 should be updated to mention "relay amount" instead of "quote amount".
- The variable name
quoteAmountFlton line 770 could be more specific, e.g.,maxRelayAmountFlt.- The comment on line 778 should be updated to mention "maxRelayAmount" instead of "minQuoteAmount".
Consider applying these changes to improve clarity and consistency:
-// Note that this getter returns the value in native token decimals. +// Note that this getter returns the max relay amount in native token decimals. func (c Config) GetMaxRelayAmount(chainID int, addr common.Address) *big.Int { // ... - quoteAmountFlt, ok := new(big.Float).SetString(tokenCfg.MaxRelayAmount) + maxRelayAmountFlt, ok := new(big.Float).SetString(tokenCfg.MaxRelayAmount) if !ok { return defaultMaxRelayAmount } - if quoteAmountFlt.Cmp(big.NewFloat(0)) <= 0 { + if maxRelayAmountFlt.Cmp(big.NewFloat(0)) <= 0 { return defaultMaxRelayAmount } - // Scale the minQuoteAmount by the token decimals. + // Scale the maxRelayAmount by the token decimals. denomDecimalsFactor := new(big.Int).Exp(big.NewInt(10), big.NewInt(int64(tokenCfg.Decimals)), nil) - quoteAmountScaled, _ := new(big.Float).Mul(quoteAmountFlt, new(big.Float).SetInt(denomDecimalsFactor)).Int(nil) + maxRelayAmountScaled, _ := new(big.Float).Mul(maxRelayAmountFlt, new(big.Float).SetInt(denomDecimalsFactor)).Int(nil) - return quoteAmountScaled + return maxRelayAmountScaled }These changes will improve the readability and maintainability of the code.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 754-755: services/rfq/relayer/relconfig/getters.go#L754-L755
Added lines #L754 - L755 were not covered by tests
[warning] 757-757: services/rfq/relayer/relconfig/getters.go#L757
Added line #L757 was not covered by tests
[warning] 759-764: services/rfq/relayer/relconfig/getters.go#L759-L764
Added lines #L759 - L764 were not covered by tests
[warning] 767-767: services/rfq/relayer/relconfig/getters.go#L767
Added line #L767 was not covered by tests
[warning] 769-769: services/rfq/relayer/relconfig/getters.go#L769
Added line #L769 was not covered by tests
[warning] 771-771: services/rfq/relayer/relconfig/getters.go#L771
Added line #L771 was not covered by tests
[warning] 773-774: services/rfq/relayer/relconfig/getters.go#L773-L774
Added lines #L773 - L774 were not covered by tests
[warning] 776-776: services/rfq/relayer/relconfig/getters.go#L776
Added line #L776 was not covered by tests
[warning] 779-781: services/rfq/relayer/relconfig/getters.go#L779-L781
Added lines #L779 - L781 were not covered by tests
751-782: Improve test coverage forGetMaxRelayAmountfunctionThe static analysis tool indicates that several lines in the
GetMaxRelayAmountfunction are not covered by tests. To ensure the reliability and correctness of this new function, it's important to add comprehensive unit tests that cover various scenarios, including:
- Retrieving a valid max relay amount
- Handling cases where the chain configuration is not found
- Handling cases where the token configuration is not found
- Dealing with invalid
MaxRelayAmountvalues- Verifying the correct scaling of the amount based on token decimals
Would you like assistance in generating unit tests for the
GetMaxRelayAmountfunction to improve code coverage?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 754-755: services/rfq/relayer/relconfig/getters.go#L754-L755
Added lines #L754 - L755 were not covered by tests
[warning] 757-757: services/rfq/relayer/relconfig/getters.go#L757
Added line #L757 was not covered by tests
[warning] 759-764: services/rfq/relayer/relconfig/getters.go#L759-L764
Added lines #L759 - L764 were not covered by tests
[warning] 767-767: services/rfq/relayer/relconfig/getters.go#L767
Added line #L767 was not covered by tests
[warning] 769-769: services/rfq/relayer/relconfig/getters.go#L769
Added line #L769 was not covered by tests
[warning] 771-771: services/rfq/relayer/relconfig/getters.go#L771
Added line #L771 was not covered by tests
[warning] 773-774: services/rfq/relayer/relconfig/getters.go#L773-L774
Added lines #L773 - L774 were not covered by tests
[warning] 776-776: services/rfq/relayer/relconfig/getters.go#L776
Added line #L776 was not covered by tests
[warning] 779-781: services/rfq/relayer/relconfig/getters.go#L779-L781
Added lines #L779 - L781 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- services/rfq/relayer/quoter/quoter.go (3 hunks)
- services/rfq/relayer/quoter/quoter_test.go (3 hunks)
- services/rfq/relayer/relconfig/config.go (1 hunks)
- services/rfq/relayer/relconfig/getters.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/relconfig/config.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
services/rfq/relayer/relconfig/getters.go
[warning] 754-755: services/rfq/relayer/relconfig/getters.go#L754-L755
Added lines #L754 - L755 were not covered by tests
[warning] 757-757: services/rfq/relayer/relconfig/getters.go#L757
Added line #L757 was not covered by tests
[warning] 759-764: services/rfq/relayer/relconfig/getters.go#L759-L764
Added lines #L759 - L764 were not covered by tests
[warning] 767-767: services/rfq/relayer/relconfig/getters.go#L767
Added line #L767 was not covered by tests
[warning] 769-769: services/rfq/relayer/relconfig/getters.go#L769
Added line #L769 was not covered by tests
[warning] 771-771: services/rfq/relayer/relconfig/getters.go#L771
Added line #L771 was not covered by tests
[warning] 773-774: services/rfq/relayer/relconfig/getters.go#L773-L774
Added lines #L773 - L774 were not covered by tests
[warning] 776-776: services/rfq/relayer/relconfig/getters.go#L776
Added line #L776 was not covered by tests
[warning] 779-781: services/rfq/relayer/relconfig/getters.go#L779-L781
Added lines #L779 - L781 were not covered by tests
🔇 Additional comments (5)
services/rfq/relayer/quoter/quoter_test.go (2)
183-189: Well-structured quoteParams struct.The introduction of the
quoteParamsstruct is a good design choice. It encapsulates related quote parameters, improving code organization and readability. This will make it easier to manage and update quote parameters in the test cases.
294-299: Good addition of MaxBalance test case.The new test case effectively checks the behavior of
GetOriginAmountwhen a MaxBalance constraint is applied. It ensures that the quoter correctly respects the MaxBalance limit, even when it's less than the actual balance. This is an important edge case to test.services/rfq/relayer/relconfig/getters.go (1)
749-750: LGTM: Appropriate default value fordefaultMaxRelayAmountThe declaration of
defaultMaxRelayAmountasnilis consistent with other similar default variables in the file. This is an appropriate default value, as it effectively represents the absence of a maximum relay amount limit.services/rfq/relayer/quoter/quoter.go (2)
Line range hint
725-732: Properly clippingquoteAmountby destination balanceClipping the
quoteAmountbyinput.DestBalanceensures that the quoted amount does not exceed the available destination balance. This is a correct implementation that prevents overcommitting funds and potential failures during transaction execution.
734-743: Adding maximum quote amount constraint enhances controlIntroducing the clipping of
quoteAmountbymaxQuoteAmountadds a crucial constraint that prevents quoting amounts higher than a predefined maximum. This enhancement improves the robustness and compliance of the quote generation logic by enforcing configuration-defined limits.
| maxRelayAmount := m.config.GetMaxRelayAmount(int(quote.Transaction.OriginChainId), quote.Transaction.OriginToken) | ||
| if maxRelayAmount != nil { | ||
| if quote.Transaction.OriginAmount.Cmp(maxRelayAmount) > 0 { | ||
| span.AddEvent("origin amount is greater than max relay amount") | ||
| return false, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential integer overflow when casting OriginChainId to int
In line 211, you are casting quote.Transaction.OriginChainId to an int using int(quote.Transaction.OriginChainId). If OriginChainId is a uint32, casting it directly to int could lead to incorrect negative values on systems where int is a 32-bit signed integer and the OriginChainId exceeds 2,147,483,647. This may cause unexpected behavior when fetching the maxRelayAmount.
Consider using uint32 consistently or adjusting the GetMaxRelayAmount method to accept the appropriate unsigned integer type to prevent potential overflows.
…nds (#3287) * feat(rfq-relayer): add MaxRelayAmount (#3259) * Feat: add quoteParams helper for test * Feat: add MaxQuoteAmount to relconfig * Feat: use MaxQuoteAmount * Feat: handle MaxQuoteAmount in quoter test * Replace: MaxQuoteAmount -> MaxRelayAmount * Feat: shouldProcess() returns false if max relay amount exceeded * Feat: add test for MaxRelayAmount * add request field for refunds * adding to events typing --------- Co-authored-by: dwasse <wassermandaniel8@gmail.com> Co-authored-by: defi-moses <jakedinero@protonmail.com>
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests