RFQ Relayer: support pointers for config values #2913
Conversation
|
Warning Rate limit exceeded@trajan0x has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 56 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes enhance the configuration management in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Config
participant Getter
Client->>Getter: Request Configuration
Getter->>Config: Retrieve ChainConfig
Config->>Getter: Return ChainConfig
Getter->>Client: Provide Configuration
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 Configuration File (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Support for pointers in config values has been added to prevent zero values from being overridden by default values.
- Updated
services/rfq/relayer/relconfig/config.goto use pointers forQuotePct,QuoteFixedFeeMultiplier, andRelayFixedFeeMultiplierfields. - Modified
services/rfq/relayer/relconfig/config_test.goto align test cases with pointer-based config fields. - Enhanced
services/rfq/relayer/relconfig/getters.gowithNewFloatPtrfunction and updated utility functions for pointer handling.
3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
![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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- services/rfq/relayer/relconfig/config.go (1 hunks)
- services/rfq/relayer/relconfig/config_test.go (7 hunks)
- services/rfq/relayer/relconfig/getters.go (4 hunks)
Additional comments not posted (11)
services/rfq/relayer/relconfig/config.go (3)
93-93: Approved: ChangeQuotePctto*float64.Changing
QuotePctto a pointer type allows for better handling of optional values by enabling the representation ofnil, indicating unset or absent values.
98-98: Approved: ChangeQuoteFixedFeeMultiplierto*float64.Changing
QuoteFixedFeeMultiplierto a pointer type allows for better handling of optional values by enabling the representation ofnil, indicating unset or absent values.
100-100: Approved: ChangeRelayFixedFeeMultiplierto*float64.Changing
RelayFixedFeeMultiplierto a pointer type allows for better handling of optional values by enabling the representation ofnil, indicating unset or absent values.services/rfq/relayer/relconfig/config_test.go (3)
34-34: Approved: Utilizerelconfig.NewFloatPtrforQuotePct.Using
relconfig.NewFloatPtrto create a pointer to a float64 value ensures that the test cases correctly validate the new pointer-based structure of the configuration.
36-36: Approved: Utilizerelconfig.NewFloatPtrforQuoteFixedFeeMultiplier.Using
relconfig.NewFloatPtrto create a pointer to a float64 value ensures that the test cases correctly validate the new pointer-based structure of the configuration.
285-293: Approved: Dereference pointer values inGetQuoteFixedFeeMultiplierassertions.Dereferencing pointer values in the assertions ensures that the tests correctly validate the new pointer-based structure of the configuration.
services/rfq/relayer/relconfig/getters.go (5)
27-29: Approved: AddNewFloatPtrfunction.The
NewFloatPtrfunction is straightforward and correctly implemented, returning a pointer to a float64 value.
101-106: Approved: AddisNilOrZerofunction.The
isNilOrZerofunction correctly handles both nil and zero checks, replacing the previousisNonZerofunction.
93-99: Approved: AddderefPointerfunction.The
derefPointerfunction is correctly implemented, ensuring that the correct underlying value is accessed if the pointer is not nil.
42-59: Approved: UpdategetChainConfigValuefunction.The updates to the
getChainConfigValuefunction ensure that it can handle cases where configuration values are not set, providing a more accurate representation of the configuration state.
347-347: Approved: UpdateGetQuoteFixedFeeMultiplierfunction.The updates to the
GetQuoteFixedFeeMultiplierfunction ensure that it correctly handles the new pointer-based structure and provides a default value if the configuration value is not set.
Deploying sanguine-fe with
|
| Latest commit: |
e178edb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dc4e4d26.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-config-zero.sanguine-fe.pages.dev |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2913 +/- ##
===================================================
+ Coverage 25.71336% 25.72432% +0.01095%
===================================================
Files 770 770
Lines 55512 55535 +23
Branches 80 80
===================================================
+ Hits 14274 14286 +12
- Misses 39760 39771 +11
Partials 1478 1478
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.
PR Summary
(updates since last review)
Recent changes enhance configuration management by supporting pointer types for certain fields, ensuring zero values are not overridden by defaults.
- Updated
services/rfq/relayer/pricer/fee_pricer_test.goto use pointer-based configuration forQuoteFixedFeeMultiplierandRelayFixedFeeMultiplier. - Modified
services/rfq/relayer/quoter/quoter_test.goto validate the new pointer-based configuration approach. - Ensured test cases reflect the new pointer-based configuration handling, particularly for fields that can represent uninitialized states.
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
services/rfq/relayer/pricer/fee_pricer_test.go (1)
265-266: Ensure proper handling of nil pointers for fixed fee multipliers.The fixed fee multipliers are now pointers. Ensure that the code correctly handles nil values to avoid potential nil pointer dereference errors.
- In
services/rfq/relayer/relconfig/getters.go, bothGetQuoteFixedFeeMultiplierandGetRelayFixedFeeMultiplierfunctions dereference the default multiplier without checking for nil. This could lead to nil pointer dereference errors ifDefaultChainConfig.QuoteFixedFeeMultiplieris nil.- In
services/rfq/relayer/pricer/fee_pricer.go, the multipliers are used directly after being fetched, but the nil check is implicit through error handling from the getter functions.Recommendations:
- Add nil checks before dereferencing
DefaultChainConfig.QuoteFixedFeeMultiplierandDefaultChainConfig.RelayFixedFeeMultiplierin the getter functions.- Ensure that any default values set in the configuration are not nil.
Code Locations:
services/rfq/relayer/relconfig/getters.go:
func (c Config) GetQuoteFixedFeeMultiplier(chainID int) (value float64, err error) {(line 336)func (c Config) GetRelayFixedFeeMultiplier(chainID int) (value float64, err error) {(line 353)Analysis chain
Ensure proper handling of nil pointers for fixed fee multipliers.
The fixed fee multipliers are now pointers. Ensure that the code correctly handles nil values to avoid potential nil pointer dereference errors.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of fixed fee multipliers handle nil values correctly. # Test: Search for usages of `QuoteFixedFeeMultiplier` and `RelayFixedFeeMultiplier`. Expect: Proper nil checks. rg --type go 'QuoteFixedFeeMultiplier|RelayFixedFeeMultiplier'Length of output: 3862
Script:
#!/bin/bash # Extract the implementations of functions using QuoteFixedFeeMultiplier and RelayFixedFeeMultiplier to verify nil pointer handling. # Get the implementation of GetQuoteFixedFeeMultiplier function ast-grep --lang go --pattern 'func (c Config) GetQuoteFixedFeeMultiplier($_) ($_, $_) { $$$ }' services/rfq/relayer/relconfig/getters.go # Get the implementation of GetRelayFixedFeeMultiplier function ast-grep --lang go --pattern 'func (c Config) GetRelayFixedFeeMultiplier($_) ($_, $_) { $$$ }' services/rfq/relayer/relconfig/getters.go # Get the usage of multipliers in fee_pricer.go rg -A 10 'GetQuoteFixedFeeMultiplier|GetRelayFixedFeeMultiplier' services/rfq/relayer/pricer/fee_pricer.goLength of output: 3583
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- services/rfq/relayer/pricer/fee_pricer_test.go (4 hunks)
- services/rfq/relayer/quoter/quoter_test.go (1 hunks)
Additional comments not posted (3)
services/rfq/relayer/quoter/quoter_test.go (1)
168-168: Ensure proper handling of nil pointers forQuotePct.The
QuotePctparameter is now a pointer. Ensure that the code correctly handles nil values to avoid potential nil pointer dereference errors.services/rfq/relayer/pricer/fee_pricer_test.go (2)
318-318: Ensure proper handling of zero values for fixed fee multipliers.The fixed fee multipliers are set to 0. Ensure that the code correctly handles zero values and defaults to 1 as expected.
Verification successful
Zero values for fixed fee multipliers are handled correctly.
The code ensures that when a zero or negative value is encountered for
QuoteFixedFeeMultiplierorRelayFixedFeeMultiplier, it defaults to a predefined value, ensuring proper handling.
services/rfq/relayer/relconfig/getters.go:
GetQuoteFixedFeeMultiplierGetRelayFixedFeeMultiplierScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of fixed fee multipliers handle zero values correctly. # Test: Search for usages of `QuoteFixedFeeMultiplier` and `RelayFixedFeeMultiplier`. Expect: Proper handling of zero values. rg --type go 'QuoteFixedFeeMultiplier|RelayFixedFeeMultiplier'Length of output: 3862
Script:
#!/bin/bash # Extract relevant code blocks from fee_pricer.go and getters.go to check handling of zero values for fixed fee multipliers. # Extract code from fee_pricer.go where QuoteFixedFeeMultiplier and RelayFixedFeeMultiplier are used rg --type go -A 10 'QuoteFixedFeeMultiplier|RelayFixedFeeMultiplier' services/rfq/relayer/pricer/fee_pricer.go # Extract code from getters.go where QuoteFixedFeeMultiplier and RelayFixedFeeMultiplier are used rg --type go -A 10 'QuoteFixedFeeMultiplier|RelayFixedFeeMultiplier' services/rfq/relayer/relconfig/getters.goLength of output: 2608
299-299: Ensure proper handling of negative values for fixed fee multipliers.The fixed fee multipliers are set to -1. Ensure that the code correctly handles negative values and defaults to 1 as expected.
There was a problem hiding this comment.
PR Summary
(updates since last review)
Recent changes enhance configuration management by supporting pointer types for certain fields, ensuring zero values are not overridden by defaults.
- Updated
services/rfq/relayer/relconfig/getters.goto handle pointer dereferencing for configuration values. - Introduced
core.PtrToinNewFloatPtrfor creating float64 pointers. - Enhanced
getChainConfigValueto differentiate between zero values and uninitialized states. - Thorough testing is essential to ensure the new pointer-based approach does not introduce bugs or unexpected behavior.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
If a zero value is specified in a config field, it won't be overridden as a base or default value (assuming the field is a pointer).
Summary by CodeRabbit
New Features
Bug Fixes
Refactor