RFQ Relayer: configure chain confirmations #2967
Conversation
|
Warning Rate limit exceeded@dwasse has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 27 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. WalkthroughThis update significantly enhances the Changes
Possibly related issues
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://github.com/avatars/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
The pull request introduces a new configuration parameter for chain confirmations, enhancing the flexibility and configurability of the RFQ relayer.
services/rfq/relayer/chain/chain.go:NewChainfunction now acceptsrelconfig.Configto retrieve RFQ address and confirmations.services/rfq/relayer/relapi/server.go: ModifiedNewChaincall to pass the entire configuration object (cfg).services/rfq/relayer/service/statushandler.go: RefactoredchainIDToChainto pass the configuration object directly tochain.NewChain.
Ensure thorough testing of new error handling paths and configuration validation to avoid runtime errors.
3 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings
| chainID, err := chainClient.ChainID(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get chain id: %w", err) |
There was a problem hiding this comment.
logic: Consider caching the chain ID to avoid repeated calls to chainClient.ChainID(ctx).
| addr, err := cfg.GetRFQAddress(int(chainID.Int64())) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get rfq address: %w", err) |
There was a problem hiding this comment.
style: Ensure cfg.GetRFQAddress handles all possible edge cases for chain ID mappings.
| confirmations, err := cfg.GetConfirmations(int(chainID.Int64())) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get chain id: %w", err) | ||
| return nil, fmt.Errorf("could not get confirmations: %w", err) |
There was a problem hiding this comment.
check: Validate that cfg.GetConfirmations returns a reasonable default if the value is not set.
| @@ -80,7 +80,7 @@ func NewRelayerAPI( | |||
| if err != nil { | |||
| return nil, fmt.Errorf("could not get chain listener: %w", err) | |||
| } | |||
| chains[uint32(chainID)], err = chain.NewChain(ctx, chainClient, common.HexToAddress(chainCfg.RFQAddress), chainListener, submitter) | |||
| chains[uint32(chainID)], err = chain.NewChain(ctx, cfg, chainClient, chainListener, submitter) | |||
There was a problem hiding this comment.
style: Ensure NewChain handles the entire configuration object correctly to avoid potential issues.
| return nil, fmt.Errorf("could not get rfq address: %w", err) | ||
| } | ||
| chain, err := chain.NewChain(ctx, chainClient, common.HexToAddress(rfqAddr), r.chainListeners[id], r.submitter) | ||
| chain, err := chain.NewChain(ctx, r.cfg, chainClient, r.chainListeners[id], r.submitter) |
There was a problem hiding this comment.
style: Ensure the configuration object includes all necessary parameters previously fetched separately.
![coderabbitai[bot]](https://github.com/avatars/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/chain/chain.go (2 hunks)
- services/rfq/relayer/relapi/server.go (1 hunks)
- services/rfq/relayer/service/statushandler.go (1 hunks)
Additional comments not posted (8)
services/rfq/relayer/chain/chain.go (6)
17-17: Import statement forrelconfiglooks good.The new import statement is necessary for the changes in the
NewChainfunction.
34-34: Function signature change looks good.The new parameter
cfgenhances the flexibility and configurability of the function.
35-38: Error handling forchainIDretrieval looks good.The error message is clear and provides context.
39-42: Error handling for RFQ address retrieval looks good.The error message is clear and provides context.
47-49: Error handling for confirmations retrieval looks good.The error message is clear and provides context.
52-55: Return statement looks good.The function returns a properly initialized
Chainobject.services/rfq/relayer/relapi/server.go (1)
83-83: Simplified configuration handling looks good.The new implementation uses the configuration object directly, eliminating the intermediate step of fetching the RFQ address.
Verify that the new configuration object is correctly utilized in the codebase.
Verification successful
Simplified configuration handling looks good.
The new implementation uses the configuration object directly, eliminating the intermediate step of fetching the RFQ address. The
cfgparameter is correctly utilized in theNewChainfunction to fetch necessary configuration details such as the RFQ address and confirmations.
services/rfq/relayer/chain/chain.go: Thecfgparameter is used to get the RFQ address and confirmations.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new configuration object in the codebase. # Test: Search for the usage of the `NewChain` function. Expect: Only occurrences of the new signature. rg --type go -A 5 $'NewChain'Length of output: 73189
Script:
#!/bin/bash # Description: Extract the implementation of the `NewChain` function to verify how the `cfg` parameter is utilized. # Extract the `NewChain` function implementation from the `chain` package. ast-grep --lang go --pattern $'func NewChain(ctx context.Context, cfg $_, chainClient $_, chainListener $_, ts $_) (*Chain, error) { $$$ }' --jsonLength of output: 2517
services/rfq/relayer/service/statushandler.go (1)
203-203: Simplified configuration handling looks good.The new implementation uses the configuration object directly, eliminating the intermediate step of fetching the RFQ address.
Verify that the new configuration object is correctly utilized in the codebase.
Deploying sanguine-fe with
|
| Latest commit: |
c411078
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f59138d1.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-configure-confs.sanguine-fe.pages.dev |
There was a problem hiding this comment.
PR Summary
(updates since last review)
The pull request enhances the configurability and error handling of the RFQ relayer service, focusing on chain initialization and address retrieval.
services/rfq/relayer/relapi/server.go: Improved chain initialization with a new configuration parameter.services/rfq/relayer/relapi/server.go: Enhanced error handling during chain setup for better failure messages.services/rfq/relayer/relapi/server.go: Streamlined logic for retrieving RFQ addresses, reducing errors and improving performance.
Ensure thorough testing of the new configuration paths and error handling to prevent runtime issues.
1 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
| @@ -59,7 +59,7 @@ func NewRelayerAPI( | |||
| } | |||
|
|
|||
| chains := make(map[uint32]*chain.Chain) | |||
| for chainID, chainCfg := range cfg.Chains { | |||
| for chainID := range cfg.Chains { | |||
There was a problem hiding this comment.
check: Consider validating the chain configuration before iterating over it to catch potential misconfigurations early.
| @@ -59,7 +59,7 @@ func NewRelayerAPI( | |||
| } | |||
|
|
|||
| chains := make(map[uint32]*chain.Chain) | |||
| for chainID, chainCfg := range cfg.Chains { | |||
| for chainID := range cfg.Chains { | |||
| chainClient, err := omniRPCClient.GetChainClient(ctx, chainID) | |||
There was a problem hiding this comment.
style: Check if chainClient is nil before proceeding to avoid dereferencing a nil pointer.
| @@ -80,7 +80,7 @@ func NewRelayerAPI( | |||
| if err != nil { | |||
| return nil, fmt.Errorf("could not get chain listener: %w", err) | |||
| } | |||
| chains[uint32(chainID)], err = chain.NewChain(ctx, chainClient, common.HexToAddress(chainCfg.RFQAddress), chainListener, submitter) | |||
| chains[uint32(chainID)], err = chain.NewChain(ctx, cfg, chainClient, chainListener, submitter) | |||
There was a problem hiding this comment.
style: Ensure NewChain handles the entire configuration object correctly to avoid potential issues.
There was a problem hiding this comment.
PR Summary
(updates since last review)
The pull request enhances the configurability and error handling of the RFQ relayer service, focusing on chain initialization and address retrieval.
services/rfq/relayer/relapi/server.go: Improved chain initialization with a new configuration parameter.services/rfq/relayer/relapi/server.go: Enhanced error handling during chain setup for better failure messages.services/rfq/relayer/relapi/server.go: Streamlined logic for retrieving RFQ addresses, reducing errors and improving performance.
No major changes found since the last review.
No file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
Enhanced configurability and error handling for RFQ relayer service, focusing on chain initialization and address retrieval.
packages/explorer-ui/components/BridgeTransaction/BridgeTransactionTable.tsx: ReplacedformattedValuewithvalueforIconAndAmountcomponent, simplifying data handling.packages/explorer-ui/components/misc/IconAndAmount.tsx: IntroducedformatBigIntToStringutility, improving token amount formatting.packages/explorer-ui/patch.ts: Added method to serializeBigIntto JSON, enhancing large integer handling.packages/explorer-ui/tsconfig.json: Updated TypeScript target toES2020, leveraging modern JS features.packages/explorer-ui/utils/formatBigIntToString.ts: Added utility function for robust BigInt to string conversion, crucial for financial calculations.
8 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2967 +/- ##
===================================================
- Coverage 25.70725% 25.70179% -0.00546%
===================================================
Files 771 771
Lines 55603 55572 -31
Branches 80 80
===================================================
- Hits 14294 14283 -11
+ Misses 39823 39804 -19
+ Partials 1486 1485 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Bundle ReportChanges will decrease total bundle size by 3.22MB ⬇️
|
Summary by CodeRabbit
New Features
Bug Fixes