FeePricer: use different multipliers for quote / relay #2663
Conversation
WalkthroughThe recent updates involve modifying the Changes
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 (
|
Deploying sanguine-fe with
|
| Latest commit: |
15e1f73
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cb5b740b.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-profitability-multiplie.sanguine-fe.pages.dev |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2663 +/- ##
===================================================
- Coverage 40.13990% 39.75471% -0.38519%
===================================================
Files 184 180 -4
Lines 14868 14758 -110
Branches 80 80
===================================================
- Hits 5968 5867 -101
+ Misses 8130 8124 -6
+ Partials 770 767 -3
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 (5)
- services/rfq/relayer/pricer/fee_pricer.go (9 hunks)
- services/rfq/relayer/pricer/fee_pricer_test.go (3 hunks)
- services/rfq/relayer/relconfig/config.go (1 hunks)
- services/rfq/relayer/relconfig/config_test.go (3 hunks)
- services/rfq/relayer/relconfig/getters.go (2 hunks)
Additional context used
GitHub Check: codecov/patch
services/rfq/relayer/pricer/fee_pricer.go
[warning] 248-249: services/rfq/relayer/pricer/fee_pricer.go#L248-L249
Added lines #L248 - L249 were not covered by tests
[warning] 253-253: services/rfq/relayer/pricer/fee_pricer.go#L253
Added line #L253 was not covered by testsservices/rfq/relayer/relconfig/getters.go
[warning] 327-327: services/rfq/relayer/relconfig/getters.go#L327
Added line #L327 was not covered by tests
[warning] 330-331: services/rfq/relayer/relconfig/getters.go#L330-L331
Added lines #L330 - L331 were not covered by tests
[warning] 336-340: services/rfq/relayer/relconfig/getters.go#L336-L340
Added lines #L336 - L340 were not covered by tests
[warning] 342-351: services/rfq/relayer/relconfig/getters.go#L342-L351
Added lines #L342 - L351 were not covered by tests
Additional comments not posted (12)
services/rfq/relayer/relconfig/config.go (1)
89-92: The addition ofQuoteFixedFeeMultiplierandRelayFixedFeeMultiplierinChainConfigis consistent with the PR's objective to differentiate fee multipliers for quoting and relaying. This change should provide the intended flexibility in fee calculations. Ensure that these new fields are properly integrated and used in the fee calculation logic throughout the system.services/rfq/relayer/pricer/fee_pricer.go (5)
24-28: The method signatures inFeePricerinterface have been correctly updated to replaceuseMultiplierwithisQuote. This change aligns with the PR's objective and should help in applying different fee multipliers based on the transaction type (quote or relay).
Line range hint
84-109: The implementation ofGetOriginFeemethod correctly utilizes theisQuoteparameter to determine the appropriate fee multiplier. This method also handles the addition of L1 fees when applicable, which is a good practice for ensuring fee accuracy across different network conditions.
Line range hint
120-144: TheGetDestinationFeemethod follows a similar pattern toGetOriginFee, correctly using theisQuoteparameter. The method also handles L1 fees, which is crucial for transactions that span multiple layers or chains.
155-174:GetTotalFeemethod effectively combines the origin and destination fees to compute the total fee, considering whether the transaction is a quote or a relay. This method is central to the fee calculation logic and is implemented as expected.
Line range hint
190-265: ThegetFeemethod is the core function where the actual fee calculation logic is implemented. It correctly uses theisQuoteparameter to fetch the appropriate multiplier. However, there are lines of code here that are not covered by tests, which could lead to potential issues in production.- // Current implementation + // Suggested addition: Include unit tests covering the scenarios when `isQuote` is both true and false.Tools
GitHub Check: codecov/patch
[warning] 248-249: services/rfq/relayer/pricer/fee_pricer.go#L248-L249
Added lines #L248 - L249 were not covered by tests
[warning] 253-253: services/rfq/relayer/pricer/fee_pricer.go#L253
Added line #L253 was not covered by testsservices/rfq/relayer/relconfig/config_test.go (2)
21-35: The test configurations have been updated to reflect the newQuoteFixedFeeMultiplierfield in theChainConfigstruct. This is necessary to ensure that the unit tests are aligned with the new configuration structure and can validate the behavior correctly.Also applies to: 39-53, 59-73
281-292: The test forGetQuoteFixedFeeMultipliercorrectly checks the retrieval of the multiplier from the configuration. This ensures that the new configuration fields are not only correctly implemented but also retrievable as expected in different scenarios (default, base, and specific chain configurations).services/rfq/relayer/pricer/fee_pricer_test.go (3)
264-265: Ensure that the new multipliers are correctly applied in the tests.
281-295: Verify that the fee calculations with the new multipliers are correct.
Line range hint
298-317: Check the behavior when the multiplier is set to negative or zero. Ensure it defaults to 1 as expected.services/rfq/relayer/relconfig/getters.go (1)
335-351: Check theGetRelayFixedFeeMultiplierfunction for correct fallback behavior toQuoteFixedFeeMultiplierwhen the relay multiplier is not set or is less than or equal to zero.- return value, fmt.Errorf("failed to cast RelayFixedFeeMultiplier to int") + return value, fmt.Errorf("failed to cast RelayFixedFeeMultiplier to float64")Tools
GitHub Check: codecov/patch
[warning] 336-340: services/rfq/relayer/relconfig/getters.go#L336-L340
Added lines #L336 - L340 were not covered by tests
[warning] 342-351: services/rfq/relayer/relconfig/getters.go#L342-L351
Added lines #L342 - L351 were not covered by tests
| // GetQuoteFixedFeeMultiplier returns the QuoteFixedFeeMultiplier for the given chainID. | ||
| func (c Config) GetQuoteFixedFeeMultiplier(chainID int) (value float64, err error) { | ||
| rawValue, err := c.getChainConfigValue(chainID, "QuoteFixedFeeMultiplier") | ||
| if err != nil { | ||
| return value, err | ||
| } | ||
|
|
||
| value, ok := rawValue.(float64) | ||
| if !ok { | ||
| return value, fmt.Errorf("failed to cast FixedFeeMultiplier to int") | ||
| return value, fmt.Errorf("failed to cast QuoteFixedFeeMultiplier to int") | ||
| } | ||
| if value <= 0 { | ||
| value = DefaultChainConfig.FixedFeeMultiplier | ||
| value = DefaultChainConfig.QuoteFixedFeeMultiplier | ||
| } | ||
| return value, nil | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The GetQuoteFixedFeeMultiplier function is covered by tests in services/rfq/relayer/relconfig/config_test.go. However, there is a minor issue in the error message within the function.
-
Change the error message from "failed to cast QuoteFixedFeeMultiplier to int" to "failed to cast QuoteFixedFeeMultiplier to float64".
Analysis chain
Ensure that the GetQuoteFixedFeeMultiplier function correctly handles cases where the multiplier is not set or is less than or equal to zero.
- return value, fmt.Errorf("failed to cast QuoteFixedFeeMultiplier to int")
+ return value, fmt.Errorf("failed to cast QuoteFixedFeeMultiplier to float64")Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the `GetQuoteFixedFeeMultiplier` function is covered by tests.
# Test: Search for the function usage in test files. Expect: At least one occurrence.
rg --type go $'GetQuoteFixedFeeMultiplier' --files-with-matches tests/
Length of output: 116
Script:
#!/bin/bash
# Description: Verify that the `GetQuoteFixedFeeMultiplier` function is covered by tests.
# Test: Search for the function usage in Go files across the entire codebase. Expect: At least one occurrence.
rg --type go $'GetQuoteFixedFeeMultiplier'
Length of output: 949
Tools
GitHub Check: codecov/patch
[warning] 327-327: services/rfq/relayer/relconfig/getters.go#L327
Added line #L327 was not covered by tests
[warning] 330-331: services/rfq/relayer/relconfig/getters.go#L330-L331
Added lines #L330 - L331 were not covered by tests
Summary by CodeRabbit
New Features
Refactor
isQuoteparameter instead ofuseMultiplierfor fee calculations.FixedFeeMultiplierintoQuoteFixedFeeMultiplierandRelayFixedFeeMultiplierin configuration settings.Tests