feat(rfq-relayer): make QuoteWidthBps a token-level param [SLT-354] #3305
Conversation
WalkthroughThe pull request introduces a new field, Changes
Possibly related PRs
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
|
Deploying sanguine-fe with
|
| Latest commit: |
103b5aa
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://111517d2.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-quote-width-token.sanguine-fe.pages.dev |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3305 +/- ##
===================================================
- Coverage 33.27199% 24.99048% -8.28151%
===================================================
Files 543 198 -345
Lines 34777 13125 -21652
Branches 82 82
===================================================
- Hits 11571 3280 -8291
+ Misses 22183 9563 -12620
+ Partials 1023 282 -741
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: 0
🧹 Outside diff range and nitpick comments (3)
services/rfq/relayer/relconfig/config.go (2)
142-144: LGTM. Consider adding validation for QuoteWidthBps.The addition of
QuoteWidthBpsto theTokenConfigstruct allows for more granular control over quote width for individual tokens, which is a good improvement.Consider adding a validation check in the
Validatemethod to ensure thatQuoteWidthBpsis always positive, as per the comment. This would prevent configuration errors at runtime.
Add validation for
QuoteWidthBpsin theValidatemethod.The
validatemethod inservices/rfq/relayer/relconfig/config.godoes not currently include validation for the newly addedQuoteWidthBpsfield inTokenConfig. To ensure configuration integrity, please update theValidatemethod to include appropriate checks forQuoteWidthBps.
- File:
services/rfq/relayer/relconfig/config.go- Lines to Update: Within the
Validatemethod, add validation logic forQuoteWidthBpssimilar to existing field validations.🔗 Analysis chain
Line range hint
279-321: Update Validate method and review QuoteWidthBps usage.With the addition of
QuoteWidthBpstoTokenConfig, theValidatemethod of theConfigstruct may need to be updated to include validation for this new field. Additionally, the usage ofQuoteWidthBpsthroughout the codebase should be reviewed to ensure it's now being accessed from the correct location.
- Update the
Validatemethod to include checks forQuoteWidthBpsin eachTokenConfig.- Review and update any code that previously accessed
QuoteWidthBpsfromChainConfigto now use it fromTokenConfig.Please run the following script to identify potential areas that need updating:
Review the results and make necessary updates to align with the new configuration structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of QuoteWidthBps in the codebase rg -i 'quotewidthbps' --type go # Search for the Validate method rg -i 'func \(c config\) validate' --type goLength of output: 146
Script:
#!/bin/bash # Search for QuoteWidthBps within the validate method rg -i 'func \(c Config\) validate[^}]*quotewidthbps' --type go # Search for any remaining usages of QuoteWidthBps outside of test files rg -i 'quotewidthbps' --type go -g '!*.test.go'Length of output: 1758
services/rfq/relayer/quoter/quoter.go (1)
821-824: LGTM! Consider enhancing error handling.The changes look good and align with the PR objective of making QuoteWidthBps a token-level parameter. This allows for more granular control over quote widths based on specific tokens.
Consider wrapping the error returned from
m.config.GetQuoteWidthBpswith additional context:quoteWidthBps, err := m.config.GetQuoteWidthBps(input.DestChainID, tokenName) if err != nil { - return nil, fmt.Errorf("error getting quote width bps: %w", err) + return nil, fmt.Errorf("error getting quote width bps for token %s on chain %d: %w", tokenName, input.DestChainID, err) }This will provide more detailed information in case of an error, making debugging easier.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- services/rfq/relayer/quoter/quoter.go (1 hunks)
- services/rfq/relayer/relconfig/config.go (1 hunks)
- services/rfq/relayer/relconfig/config_test.go (0 hunks)
- services/rfq/relayer/relconfig/getters.go (1 hunks)
💤 Files with no reviewable changes (1)
- services/rfq/relayer/relconfig/config_test.go
🧰 Additional context used
🔇 Additional comments (3)
services/rfq/relayer/relconfig/config.go (1)
Line range hint
65-106: Verify usage of QuoteWidthBps in other parts of the codebase.The
QuoteWidthBpsfield has been removed from theChainConfigstruct and added to theTokenConfigstruct. This change allows for more granular control but may require updates in other parts of the codebase.Please run the following script to check for any remaining references to
QuoteWidthBpsin the context ofChainConfig:#!/bin/bash # Search for references to QuoteWidthBps in the context of ChainConfig rg -i 'chainconfig.*quotewidthbps' --type goEnsure that all occurrences are updated to use the new
TokenConfig.QuoteWidthBpsinstead.services/rfq/relayer/relconfig/getters.go (2)
442-458: Function implementation looks goodThe updated
GetQuoteWidthBpsfunction correctly retrieves theQuoteWidthBpsvalue from the token configuration based on the providedchainIDandtokenName. Error handling is appropriately managed for missing chain or token configurations, and negative width values.
442-458: Ensure all calls toGetQuoteWidthBpsare updated with the new parameterThe method
GetQuoteWidthBpsnow includes an additional parametertokenName stringin its signature. Please verify that all invocations of this method throughout the codebase have been updated to pass bothchainIDandtokenNameto prevent any potential runtime errors.Run the following script to identify any usages of
GetQuoteWidthBpsthat may not have been updated:
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- services/rfq/relayer/quoter/quoter_test.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
services/rfq/relayer/quoter/quoter_test.go (1)
401-404: Test correctly validates error handling for negativeQuoteWidthBpsThe test case appropriately verifies that setting
QuoteWidthBpsto a negative value results in an error fromGetDestAmount. This strengthens the validation logic by ensuring that invalid configuration values are properly handled, enhancing the robustness of the system.
| tokenCfg := s.config.Chains[origin].Tokens["USDC"] | ||
| tokenCfg.QuoteOffsetBps = originQuoteOffsetBps | ||
| s.config.Chains[origin].Tokens["USDC"] = tokenCfg | ||
| tokenCfg = s.config.Chains[dest].Tokens["USDC"] | ||
| tokenCfg.QuoteOffsetBps = destQuoteOffsetBps | ||
| tokenCfg.QuoteWidthBps = quoteWidthBps |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider setting QuoteWidthBps for the origin token configuration
In the setQuoteParams function, QuoteWidthBps is being set only for the destination token configuration. If QuoteWidthBps is a token-level parameter that affects both origin and destination tokens, consider setting it for the origin token configuration as well. This ensures comprehensive testing and consistent behavior across all tokens.
🛠️ Refactor suggestion
Use integer types for basis points parameters
The setQuoteParams function accepts basis points parameters (originQuoteOffsetBps, destQuoteOffsetBps, quoteWidthBps) as float64. Since basis points represent integer values (1 basis point = 0.01%), consider using integer types (e.g., int or uint) for these parameters. This prevents potential floating-point precision issues and more accurately represents the discrete nature of basis points.
dwasse
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
services/rfq/relayer/quoter/quoter_test.go (1)
Line range hint
251-326: Add tests forQuoteWidthBpsinTestGetOriginAmountCurrently, the
TestGetOriginAmountfunction does not include test cases that assess the impact ofQuoteWidthBpson origin amount calculations. Adding such tests would enhance coverage and verify thatQuoteWidthBpsis consistently applied in both origin and destination amount computations.Would you like assistance in adding these test cases?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- services/rfq/relayer/quoter/quoter_test.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
services/rfq/relayer/quoter/quoter_test.go (2)
362-365: Consider settingQuoteWidthBpsfor the origin token configurationCurrently,
QuoteWidthBpsis being set only for the destination token configuration in thesetQuoteParamsfunction. IfQuoteWidthBpsshould also affect the origin token calculations, consider setting it for the origin token configuration to ensure consistent behavior across both origin and destination tokens.
401-405: Proper error handling for negativeQuoteWidthBpsvaluesThe test case correctly verifies that a negative
QuoteWidthBpsvalue results in an error. This ensures that the system robustly handles invalid input forQuoteWidthBps.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests