feat(synapse-interface): refresh stale quotes #2607
Conversation
bigboydiamonds
requested review from
abtestingalpha and
aureliusbtc
as code owners
WalkthroughThe recent updates to the Changes
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: |
d29218d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d42e3d71.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fe-interval-quotes.sanguine-fe.pages.dev |
Deploying sanguine with
|
| Latest commit: |
d29218d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://51433fec.sanguine.pages.dev |
| Branch Preview URL: | https://fe-interval-quotes.sanguine.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2607 +/- ##
====================================================
+ Coverage 25.93072% 37.60614% +11.67542%
====================================================
Files 750 168 -582
Lines 53265 13426 -39839
Branches 80 80
====================================================
- Hits 13812 5049 -8763
+ Misses 38041 7738 -30303
+ Partials 1412 639 -773
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: 5
Out of diff range and nitpick comments (1)
packages/synapse-interface/utils/hooks/useStaleQuoteRefresher.ts (1)
22-23: Remove or replace debug logging before merging.Debugging statements like
console.logshould be removed or replaced with a more robust logging framework that can be controlled via log levels.
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- packages/synapse-interface/constants/bridge.ts (2 hunks)
- packages/synapse-interface/pages/state-managed-bridge/index.tsx (5 hunks)
- packages/synapse-interface/utils/hooks/useStaleQuoteRefresher.ts (1 hunks)
- packages/synapse-interface/utils/time.ts (1 hunks)
- packages/synapse-interface/utils/types/index.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/synapse-interface/utils/types/index.tsx
Additional comments not posted (2)
packages/synapse-interface/pages/state-managed-bridge/index.tsx (2)
77-77: Verify the relative import path foruseStaleQuoteRefresher.Ensure that the import path is correct and consistent with the project's structure. Relative paths can be error-prone and hard to maintain.
321-321: Ensure the hookuseStaleQuoteRefresheris used effectively.Review the parameters passed to
useStaleQuoteRefresherto ensure they are correctly utilized within the hook. This is crucial for the hook's effectiveness in refreshing stale quotes.
packages/synapse-interface/utils/hooks/useStaleQuoteRefresher.ts
Outdated
Show resolved
Hide resolved
packages/synapse-interface/utils/hooks/useStaleQuoteRefresher.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
packages/synapse-interface/utils/actions/fetchBridgeQuotes.tsx (1)
98-98: The functionlocateBestExchangeRateTokencorrectly identifies the token with the best exchange rate. Consider optimizing this function if the number of quotes is expected to be large.
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- packages/synapse-interface/pages/state-managed-bridge/index.tsx (5 hunks)
- packages/synapse-interface/utils/actions/fetchBridgeQuotes.tsx (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/synapse-interface/pages/state-managed-bridge/index.tsx
Additional comments not posted (4)
packages/synapse-interface/utils/actions/fetchBridgeQuotes.tsx (4)
6-6: Import ofgetTimeMinutesFromNowis correctly added to support the new timestamp functionality.
27-27: The addition ofcurrentTimestampusinggetTimeMinutesFromNow(0)is appropriate for capturing the current time as a timestamp.
98-98: The inclusion oftimestamp: currentTimestampin the return object offetchBridgeQuoteeffectively adds the new timestamp functionality to the bridge quote response. Ensure that all consumers of this function are aware of the new field in the response object.
98-98: Ensure that the error handling infetchBridgeQuotesis robust, especially considering the asynchronous nature of the requests and the potential for partial failures.Verification successful
The error handling within
fetchBridgeQuotesappears to be robust, especially considering the asynchronous nature of the requests and the potential for partial failures. The function uses try-catch blocks to manage errors at both the individual request level and the batch level. Errors are logged, and the function continues processing other requests, effectively managing partial failures by filtering out unsuccessful responses. This implementation aligns with good practices for handling multiple asynchronous operations.
- Location:
packages/synapse-interface/utils/actions/fetchBridgeQuotes.tsxScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify robust error handling in `fetchBridgeQuotes`. # Test: Simulate errors in some requests and ensure the function handles them gracefully. rg --type tsx $'fetchBridgeQuotes'Length of output: 61
Script:
#!/bin/bash # Correcting the previous script to search for 'fetchBridgeQuotes' in TypeScript files rg 'fetchBridgeQuotes' --glob '*.tsx' --glob '*.ts'Length of output: 704
Script:
#!/bin/bash # Extract the implementation of 'fetchBridgeQuotes' from its file rg --context 10 'async function fetchBridgeQuotes' packages/synapse-interface/utils/actions/fetchBridgeQuotes.tsxLength of output: 817
Script:
#!/bin/bash # Extract more details about the handling of batch requests and error management in 'fetchBridgeQuotes' rg --context 20 'batchRequests' packages/synapse-interface/utils/actions/fetchBridgeQuotes.tsxLength of output: 1646
…quote refresher when currentTime changes
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- packages/synapse-interface/utils/formatAmount.ts (1 hunks)
- packages/synapse-interface/utils/hooks/useStaleQuoteRefresher.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/synapse-interface/utils/formatAmount.ts
Files skipped from review as they are similar to previous changes (1)
- packages/synapse-interface/utils/hooks/useStaleQuoteRefresher.ts
…sage for idle pages
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- packages/synapse-interface/utils/hooks/useStaleQuoteRefresher.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/synapse-interface/utils/hooks/useStaleQuoteRefresher.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- packages/synapse-interface/pages/state-managed-bridge/index.tsx (7 hunks)
- packages/synapse-interface/utils/hooks/useStaleQuoteRefresher.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/synapse-interface/pages/state-managed-bridge/index.tsx
- packages/synapse-interface/utils/hooks/useStaleQuoteRefresher.ts
…lback has already initiated
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- packages/synapse-interface/pages/state-managed-bridge/index.tsx (9 hunks)
- packages/synapse-interface/utils/hooks/useStaleQuoteRefresher.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/synapse-interface/utils/hooks/useStaleQuoteRefresher.ts
Additional comments not posted (5)
packages/synapse-interface/pages/state-managed-bridge/index.tsx (5)
75-75: Ensure that the import path foruseStaleQuoteRefresheris correct and that the hook is properly integrated into other parts of the system.
95-95: Confirm that theisLoadingstate is correctly managed across different components and hooks to ensure consistent UI behavior.
Line range hint
157-278: Review the handling ofcurrentTimestampto ensure it is correctly applied and consistent with the intended functionality of timestamp tracking in bridge quotes.Verification successful
The usage of
currentTimestampin bothfetchBridgeQuotes.tsxandstate-managed-bridge/index.tsxappears to be consistent and correctly applied. In both cases,currentTimestampis derived from the functiongetTimeMinutesFromNow(0), which suggests it is intended to capture the current time at the point of execution. This timestamp is then used in various contexts within the bridge quote process, such as logging, setting up transaction details, and error handling.In
fetchBridgeQuotes.tsx,currentTimestampis used to timestamp the bridge quote response, ensuring that each quote has a record of when it was generated. This is crucial for any time-sensitive operations or for debugging purposes.In
state-managed-bridge/index.tsx,currentTimestampis used similarly to mark the initiation of a bridge quote request and to manage transaction states. It is also used as an identifier for pending transactions, which is important for tracking and managing these transactions effectively.Overall, the application of
currentTimestampacross these files supports the intended functionality of timestamp tracking in bridge quotes, ensuring that each operation can be accurately timed and traced.Conclusion:
- The handling of
currentTimestampis consistent and aligns with the intended functionality of timestamp tracking in bridge quotes.- The usage is found across multiple files, indicating a standardized approach within the codebase.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct application of `currentTimestamp` in bridge quotes. # Test: Search for the usage of `currentTimestamp` in the context of bridge quotes. Expect: Correct and consistent application. rg --type tsx --files-with-matches $'currentTimestamp'Length of output: 81
Script:
#!/bin/bash # Description: Re-verify the correct application of `currentTimestamp` in bridge quotes by searching TypeScript files. # Test: Search for the usage of `currentTimestamp` in TypeScript files. Expect: Correct and consistent application. rg --files-with-matches 'currentTimestamp' --glob '*.tsx'Length of output: 184
Script:
#!/bin/bash # Description: Extract sections of code where `currentTimestamp` is used in identified files for detailed review. # Extract relevant code from fetchBridgeQuotes.tsx echo "fetchBridgeQuotes.tsx:" rg 'currentTimestamp' -C 10 packages/synapse-interface/utils/actions/fetchBridgeQuotes.tsx # Extract relevant code from state-managed-bridge/index.tsx echo "state-managed-bridge/index.tsx:" rg 'currentTimestamp' -C 10 packages/synapse-interface/pages/state-managed-bridge/index.tsxLength of output: 5366
320-325: Validate the integration ofuseStaleQuoteRefresherto ensure it is correctly configured with appropriate parameters and effectively refreshes stale quotes as intended.
Line range hint
379-522: Examine the error handling and finalization logic inexecuteBridgeto ensure robustness and proper cleanup, particularly in the context of wallet transactions and UI updates.
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
| }) | ||
| } | ||
| } | ||
| }, [currentTime]) |
There was a problem hiding this comment.
Do we need the other deps in here?:
[currentTime, isValidQuote, isQuoteLoading, isWalletPending, quoteTime, staleTimeout]
There was a problem hiding this comment.
currentTime is the only dep we would want to react to in order to maintain quote freshness for the staleTimeout defined time intervals. Including the other deps may prematurely refresh the quote in this useEffect
There was a problem hiding this comment.
isValidQuote, isQuoteLoading, isWalletPending: If I'm understanding this correctly, these flags determine if an event listener should be added? And we don't want to add them bc we don't want to re-attach if they change?
quoteTime: Is this the timestamp which staleness is calculated? Don't we need to include that bc if quoteTime changes, then the staleness logic needs to re-run?
staleTimeout: Is this the value that determines what's considered stale? So if it changes, the staleness logic would re-run here as well?
There was a problem hiding this comment.
Correct, isValidQuote, isQuoteLoading, isWalletPending are used to determine if an event listener should be attached, but we don't want to react to these props because:
- If wallet action is pending, it would refresh quote in background, but does not actually update the quote used for the transaction (prevents User confusion with output amounts)
- Listening to
isQuoteLoadingcould cause for infinite re-renders - if new quote is being fetched for updated inputs, this would effectively re-trigger another quote fetch isValidQuotemay change frequently if the User is testing out various inputs (token / amount combos) and could effectively re-trigger a quote fetch before interval time has passed
Including quoteTime would also cause infinite re-renders since the change in quoteTime would cause for the quote to refresh in the background, which then updates the quoteTime value again.
Good catch on the staleTimeout! I originally expected for staleTimeout to be a static value when using the hook, but that's an improper assumption to make. Updated to include staleTimeout in the useEffect dependencies array in f8fadd6
Description
Add hook to manage refreshing stale quotes based on
staleTimeoutin ms.Summary by CodeRabbit
New Features
Bug Fixes
formatAmountfunction by disabling a console error output.Refactor
1a0ebf35886568fe41e34413017d80e888d6837c: synapse-interface preview link
8ac4392f17c009ca2ee298436ceccb2d54db6ddc: synapse-interface preview link
a932e3c94d4a399aa326bb210b8e75fb08d9fe64: synapse-interface preview link
56f21dfab051bc6c18fa6274e15f53a22cb40c88: synapse-interface preview link
924eae382dc8718a2491a84f8d92b370d6b08a0c: synapse-interface preview link
14527c1896ad0412601c008d9e7e49c81e121ff4: synapse-interface preview link
252912fc2f7da8b7dc8f38ee361d205bc2bd1f16: synapse-interface preview link
cf34b600339a5799d2195c7b10209a69def32840: synapse-interface preview link
16def59d163793f04be9814df7f6a54593ac700d: synapse-interface preview link
4002ed7a3a05204ef20d02aff805b7365c7caa66: synapse-interface preview link