feat(synapse-interface): validate rfq quotes #2971
Conversation
bigboydiamonds
requested review from
abtestingalpha and
aureliusbtc
as code owners
WalkthroughThe recent changes across several components primarily focus on removing the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Component
participant Analytics
User->>Component: Mounts Component
Component->>Analytics: Log Event (without address)
Analytics-->>Component: Acknowledge
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
This pull request enhances the quote selection logic in the StateManagedBridge component, introducing a comparison between RFQ and SynapseBridge quotes for improved bridge transaction rates.
- Added comparison logic between RFQ and SynapseBridge quotes in
packages/synapse-interface/pages/state-managed-bridge/index.tsx - Implemented a 30% threshold for quote selection, favoring RFQ quotes when within the threshold
- Introduced console logging for all quotes to aid in debugging and monitoring
- Integrated RFQ module functionality, leveraging the existing Synapse Router infrastructure
1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
| @@ -166,6 +166,8 @@ const StateManagedBridge = () => { | |||
| stringToBigInt(debouncedFromValue, fromToken?.decimals[fromChainId]) | |||
| ) | |||
|
|
|||
| console.log('allQuotes: ', allQuotes) | |||
There was a problem hiding this comment.
style: Remove this console.log statement before merging to production
packages/synapse-interface/pages/state-managed-bridge/index.tsx
Outdated
Show resolved
Hide resolved
Bundle ReportChanges will decrease total bundle size by 123.94kB ⬇️
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2971 +/- ##
====================================================
+ Coverage 25.70802% 90.64961% +64.94158%
====================================================
Files 771 54 -717
Lines 55613 1016 -54597
Branches 80 82 +2
====================================================
- Hits 14297 921 -13376
+ Misses 39830 93 -39737
+ Partials 1486 2 -1484
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Deploying sanguine-fe with
|
| Latest commit: |
ebf52df
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c62f6c15.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fe-validate-rfq-quotes.sanguine-fe.pages.dev |
There was a problem hiding this comment.
PR Summary
(updates since last review)
This update refines the quote selection process in the StateManagedBridge component, focusing on improved efficiency and clarity in handling RFQ and non-RFQ quotes.
- Renamed 'bridgeQuote' to 'nonRfqQuote' in
packages/synapse-interface/pages/state-managed-bridge/index.tsxfor better clarity - Removed unnecessary console.log statement, reducing clutter in the code
- Adjusted quote selection logic to ensure more accurate comparisons between RFQ and non-RFQ quotes
1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
packages/synapse-interface/pages/state-managed-bridge/index.tsx
Outdated
Show resolved
Hide resolved
packages/synapse-interface/pages/state-managed-bridge/index.tsx
Outdated
Show resolved
Hide resolved
Deploying sanguine with
|
| Latest commit: |
ebf52df
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f66495e3.sanguine.pages.dev |
| Branch Preview URL: | https://fe-validate-rfq-quotes.sanguine.pages.dev |
There was a problem hiding this comment.
PR Summary
(updates since last review)
The recent changes further refine the RFQ and non-RFQ quote selection process in the StateManagedBridge component, enhancing decision-making and logging.
- Updated
packages/synapse-interface/pages/state-managed-bridge/index.tsxto comparemaxAmountOutvalues between RFQ and non-RFQ quotes. - Introduced logging to capture events when a non-RFQ quote is chosen over an RFQ quote.
- Set a hardcoded
maxAmountOutvalue for RFQ quotes for testing purposes. - Added additional logging for debugging, which should be reviewed for removal before production.
1 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings
packages/synapse-interface/pages/state-managed-bridge/index.tsx
Outdated
Show resolved
Hide resolved
packages/synapse-interface/pages/state-managed-bridge/index.tsx
Outdated
Show resolved
Hide resolved
packages/synapse-interface/pages/state-managed-bridge/index.tsx
Outdated
Show resolved
Hide resolved
packages/synapse-interface/pages/state-managed-bridge/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
PR Summary
(updates since last review)
The PR introduces validation logic for RFQ quotes in the StateManagedBridge component, enhancing the accuracy and reliability of the bridge quote selection process.
- Removed console log statement and hardcoded value for
rfqQuote.maxAmountOutin/packages/synapse-interface/pages/state-managed-bridge/index.tsx. - Added logic to validate and compare RFQ and non-RFQ quotes, ensuring the best quote is selected based on a 30% difference threshold.
- Improved logging to capture events when a non-RFQ quote is chosen over an RFQ quote.
- Potential performance impacts due to added complexity in the quote selection logic should be reviewed.
1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
| `[Bridge] use non-RFQ quote over RFQ`, | ||
| { | ||
| address, | ||
| rfqQuote, | ||
| nonRfqQuote, | ||
| timestamp: currentTimestamp, | ||
| }, | ||
| true | ||
| ) |
There was a problem hiding this comment.
logic: The condition here still references 'bridgeQuote' which was renamed to 'nonRfqQuote'. This might cause unexpected behavior.
packages/synapse-interface/pages/state-managed-bridge/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
PR Summary
(updates since last review)
The PR enhances the StateManagedBridge component by adding validation logic for RFQ quotes, ensuring more accurate and reliable bridge quote selection.
- Removed
timestampfield from segment analytics event when using non-RFQ quotes. - Updated logic to validate and compare RFQ and non-RFQ quotes, selecting the best quote based on a 30% difference threshold.
- Improved logging to capture events when a non-RFQ quote is chosen over an RFQ quote.
- Removed console log statement and hardcoded value for
rfqQuote.maxAmountOutin/packages/synapse-interface/pages/state-managed-bridge/index.tsx. - Potential performance impacts due to added complexity in the quote selection logic should be reviewed.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
The PR enhances the StateManagedBridge component by adding validation logic for RFQ quotes, ensuring more accurate and reliable bridge quote selection.
- Removed
addressfield from severalsegmentAnalyticsEventcalls. - Updated bridge quote fetching logic to handle RFQ and non-RFQ quotes more accurately.
- Improved error handling and state management during the bridge transaction process.
- Enhanced logging to capture events when a non-RFQ quote is chosen over an RFQ quote.
- Removed console log statement and hardcoded value for
rfqQuote.maxAmountOutin/packages/synapse-interface/pages/state-managed-bridge/index.tsx.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
PR Summary
(updates since last review)
The PR focuses on refining the StateManagedBridge component by adding validation logic for RFQ quotes and removing the address field from analytics events.
- Removed
useAccounthook andaddressparameter fromsegmentAnalyticsEventinpackages/synapse-interface/components/WipWrapperComponents/Wrapper.tsx. - Simplified
packages/synapse-interface/pages/landing/index.tsxby removinguseAccountandaddressfrom analytics events. - Updated
packages/synapse-interface/pages/swap/index.tsxto excludeaddressfrom swap-related analytics events. - Removed
useAccountandaddressfromsegmentAnalyticsEventinpackages/synapse-interface/pages/teaser/index.tsx.
Ensure analytics tracking remains accurate without the address parameter.
4 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
bigboydiamonds
changed the title
There was a problem hiding this comment.
PR Summary
(updates since last review)
The PR enhances the StateManagedBridge component by refining the logic for selecting between RFQ and non-RFQ quotes and updating the segmentAnalyticsEvent call to include more detailed information.
- Updated
packages/synapse-interface/pages/state-managed-bridge/index.tsxto capture specific details in segmentAnalyticsEvent when a non-RFQ quote is chosen over an RFQ quote. - Ensures analytics event now includes bridge module name, origin and destination chain IDs, token symbols, and quote amounts.
- Aims to improve tracking and analysis of quote selection decisions for better optimization of bridge functionality.
1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
packages/synapse-interface/pages/state-managed-bridge/index.tsx
Outdated
Show resolved
Hide resolved
![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)
- packages/synapse-interface/components/WipWrapperComponents/Wrapper.tsx (2 hunks)
- packages/synapse-interface/pages/landing/index.tsx (1 hunks)
- packages/synapse-interface/pages/state-managed-bridge/index.tsx (4 hunks)
- packages/synapse-interface/pages/swap/index.tsx (2 hunks)
- packages/synapse-interface/pages/teaser/#index.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/synapse-interface/pages/swap/index.tsx
Additional comments not posted (7)
packages/synapse-interface/pages/teaser/#index.tsx (1)
11-13: Simplification of Analytics Event Tracking.The removal of
currentAddressfrom the analytics event simplifies the code and enhances user privacy by not tracking the user's address.packages/synapse-interface/pages/landing/index.tsx (1)
20-22: Consistent Simplification of Analytics Tracking.The removal of the
currentAddressfrom the analytics payload aligns with the goal of simplifying data collection and enhancing privacy.packages/synapse-interface/components/WipWrapperComponents/Wrapper.tsx (1)
9-11: Focus on Routing Information for Analytics.By removing the
currentAddress, the analytics event now focuses solely on routing information, which simplifies the event logging mechanism.packages/synapse-interface/pages/state-managed-bridge/index.tsx (4)
190-192: LGTM! The introduction ofnonRfqQuoteis logical.The addition of
nonRfqQuoteeffectively differentiates between RFQ and non-RFQ quotes, aligning with the PR objectives.
196-219: Verify the default behavior for selecting the first active quote.The fallback to selecting the first active quote when neither RFQ nor non-RFQ quotes are available is reasonable. Ensure this is a valid default behavior.
202-206: Verify the business requirement for the 30% variance.The logic correctly compares the RFQ and non-RFQ quotes with a 30% variance. Ensure that this variance aligns with business requirements.
207-217: Ensure no sensitive data is logged in analytics events.The analytics event logs relevant information when a non-RFQ quote is selected. Ensure that no sensitive data is inadvertently included.
| if (rfqQuote && nonRfqQuote) { | ||
| const rfqMaxAmountOut = BigInt(rfqQuote.maxAmountOut.toString()) | ||
| const nonRfqMaxAmountOut = BigInt(nonRfqQuote.maxAmountOut.toString()) | ||
|
|
||
| const maxDifference = (nonRfqMaxAmountOut * 30n) / 100n | ||
|
|
||
| if (rfqMaxAmountOut > nonRfqMaxAmountOut - maxDifference) { | ||
| quote = rfqQuote | ||
| } else { | ||
| quote = nonRfqQuote | ||
|
|
||
| segmentAnalyticsEvent(`[Bridge] use non-RFQ quote over RFQ`, { | ||
| bridgeModuleName: nonRfqQuote.bridgeModuleName, | ||
| originChainId: fromChainId, | ||
| originToken: fromToken.symbol, | ||
| originTokenAddress: fromToken.addresses[fromChainId], | ||
| destinationChainId: toChainId, | ||
| destinationToken: toToken.symbol, | ||
| destinationTokenAddress: toToken.addresses[toChainId], | ||
| rfqQuoteAmountOut: rfqQuote.maxAmountOut.toString(), | ||
| nonRfqMaxAmountOut: nonRfqQuote.maxAmountOut.toString(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
Consider extracting quote selection logic into a separate function.
The logic for selecting between RFQ and non-RFQ quotes could be extracted into a separate function for improved readability and testability.
function selectQuote(rfqQuote, nonRfqQuote) {
const rfqMaxAmountOut = BigInt(rfqQuote.maxAmountOut.toString());
const nonRfqMaxAmountOut = BigInt(nonRfqQuote.maxAmountOut.toString());
const maxDifference = (nonRfqMaxAmountOut * 30n) / 100n;
if (rfqMaxAmountOut > nonRfqMaxAmountOut - maxDifference) {
return rfqQuote;
} else {
segmentAnalyticsEvent(`[Bridge] use non-RFQ quote over RFQ`, {
bridgeModuleName: nonRfqQuote.bridgeModuleName,
originChainId: fromChainId,
originToken: fromToken.symbol,
originTokenAddress: fromToken.addresses[fromChainId],
destinationChainId: toChainId,
destinationToken: toToken.symbol,
destinationTokenAddress: toToken.addresses[toChainId],
rfqQuoteAmountOut: rfqQuote.maxAmountOut.toString(),
nonRfqMaxAmountOut: nonRfqQuote.maxAmountOut.toString(),
});
return nonRfqQuote;
}
}
packages/synapse-interface/pages/state-managed-bridge/index.tsx
Outdated
Show resolved
Hide resolved
packages/synapse-interface/pages/state-managed-bridge/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
PR Summary
(updates since last review)
The PR enhances the logic in the State Managed Bridge component to validate RFQ quotes more effectively. It introduces a percentile difference for comparing RFQ and non-RFQ quotes and updates the selection logic accordingly.
- Updated
packages/synapse-interface/pages/state-managed-bridge/index.tsxto refine the logic for selecting between RFQ and non-RFQ quotes. - Introduced a percentile difference for comparing RFQ and non-RFQ quotes.
- Simplified analytics event tracking by removing user account address, focusing on routing and transaction details.
- Corrected the logic for comparing quotes to ensure accurate selection.
- Enhanced error handling and logging for better debugging and user feedback.
1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
| const maxDifference = | ||
| (nonRfqMaxAmountOut * BigInt(allowedPercentileDifference)) / 100n | ||
|
|
||
| if (rfqMaxAmountOut > nonRfqMaxAmountOut - maxDifference) { |
There was a problem hiding this comment.
logic: The condition here still references 'bridgeQuote' which was renamed to 'nonRfqQuote'. This might cause unexpected behavior.
| segmentAnalyticsEvent(`[Bridge] use non-RFQ quote over RFQ`, { | ||
| bridgeModuleName: nonRfqQuote.bridgeModuleName, | ||
| originChainId: fromChainId, | ||
| originToken: fromToken.symbol, | ||
| originTokenAddress: fromToken.addresses[fromChainId], | ||
| destinationChainId: toChainId, | ||
| destinationToken: toToken.symbol, | ||
| destinationTokenAddress: toToken.addresses[toChainId], | ||
| rfqQuoteAmountOut: rfqQuote.maxAmountOut.toString(), | ||
| nonRfqMaxAmountOut: nonRfqQuote.maxAmountOut.toString(), | ||
| }) |
There was a problem hiding this comment.
style: Consider extracting this quote selection logic into a separate function for better readability and testability
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/pages/state-managed-bridge/index.tsx (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/synapse-interface/pages/state-managed-bridge/index.tsx
There was a problem hiding this comment.
PR Summary
(updates since last review)
The recent changes in the state-managed-bridge/index.tsx file focus on refining the decision-making process for selecting between RFQ and non-RFQ quotes, ensuring the most favorable quote is chosen based on a 30% allowable difference.
- Introduced percentile difference check for quote selection.
- Enhanced logging for analytics events.
- Adjusted logic for setting bridge quotes.
- Improved error handling and user feedback.
- Simplified analytics event tracking by removing user account address.
Ensure thorough testing of the quote selection logic to avoid unexpected behavior.
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
| segmentAnalyticsEvent(`[Bridge] use non-RFQ quote over RFQ`, { | ||
| bridgeModuleName: nonRfqQuote.bridgeModuleName, | ||
| originChainId: fromChainId, | ||
| originToken: fromToken.symbol, | ||
| originTokenAddress: fromToken.addresses[fromChainId], | ||
| destinationChainId: toChainId, | ||
| destinationToken: toToken.symbol, | ||
| destinationTokenAddress: toToken.addresses[toChainId], | ||
| rfqQuoteAmountOut: rfqQuote.maxAmountOut.toString(), | ||
| nonRfqMaxAmountOut: nonRfqQuote.maxAmountOut.toString(), | ||
| }) |
There was a problem hiding this comment.
style: Consider extracting this quote selection logic into a separate function for better readability and testability.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- packages/synapse-interface/pages/state-managed-bridge/index.tsx (4 hunks)
Additional comments not posted (4)
packages/synapse-interface/pages/state-managed-bridge/index.tsx (4)
190-192: LGTM!The introduction of
nonRfqQuotealigns well with the RFQ handling logic.
209-219: LGTM!The analytics event captures relevant details when a non-RFQ quote is selected.
222-222: LGTM!The use of the nullish coalescing operator (
??) simplifies the fallback logic for quote selection.
Line range hint
108-108: Remove console.log statement.Ensure that this console.log statement is removed before merging to production.
| if (rfqQuote && nonRfqQuote) { | ||
| const rfqMaxAmountOut = BigInt(rfqQuote.maxAmountOut.toString()) | ||
| const nonRfqMaxAmountOut = BigInt(nonRfqQuote.maxAmountOut.toString()) | ||
|
|
||
| const allowedPercentileDifference = 30n | ||
| const maxDifference = | ||
| (nonRfqMaxAmountOut * allowedPercentileDifference) / 100n | ||
|
|
||
| if (rfqMaxAmountOut > nonRfqMaxAmountOut - maxDifference) { | ||
| quote = rfqQuote | ||
| } else { | ||
| quote = nonRfqQuote | ||
|
|
||
| segmentAnalyticsEvent(`[Bridge] use non-RFQ quote over RFQ`, { | ||
| bridgeModuleName: nonRfqQuote.bridgeModuleName, | ||
| originChainId: fromChainId, | ||
| originToken: fromToken.symbol, | ||
| originTokenAddress: fromToken.addresses[fromChainId], | ||
| destinationChainId: toChainId, | ||
| destinationToken: toToken.symbol, | ||
| destinationTokenAddress: toToken.addresses[toChainId], | ||
| rfqQuoteAmountOut: rfqQuote.maxAmountOut.toString(), | ||
| nonRfqMaxAmountOut: nonRfqQuote.maxAmountOut.toString(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
Consider extracting quote selection logic into a separate function.
The logic for selecting between RFQ and non-RFQ quotes is sound. Extracting it into a separate function could improve readability and testability.
function selectQuote(rfqQuote, nonRfqQuote) {
const rfqMaxAmountOut = BigInt(rfqQuote.maxAmountOut.toString());
const nonRfqMaxAmountOut = BigInt(nonRfqQuote.maxAmountOut.toString());
const allowedPercentileDifference = 30n;
const maxDifference = (nonRfqMaxAmountOut * allowedPercentileDifference) / 100n;
if (rfqMaxAmountOut > nonRfqMaxAmountOut - maxDifference) {
return rfqQuote;
} else {
segmentAnalyticsEvent(`[Bridge] use non-RFQ quote over RFQ`, {
bridgeModuleName: nonRfqQuote.bridgeModuleName,
originChainId: fromChainId,
originToken: fromToken.symbol,
originTokenAddress: fromToken.addresses[fromChainId],
destinationChainId: toChainId,
destinationToken: toToken.symbol,
destinationTokenAddress: toToken.addresses[toChainId],
rfqQuoteAmountOut: rfqQuote.maxAmountOut.toString(),
nonRfqMaxAmountOut: nonRfqQuote.maxAmountOut.toString(),
});
return nonRfqQuote;
}
}
null
9c76e85: synapse-interface preview link
0fe57cd: synapse-interface preview link
9bc4102: synapse-interface preview link
5c7b8d3: synapse-interface preview link
35b1df0: synapse-interface preview link
9aff122: synapse-interface preview link
7b6206b: synapse-interface preview link
Summary by CodeRabbit
New Features
Improvements
Chores
d1e3f29: synapse-interface preview link
8081ae0: synapse-interface preview link
beb82df: synapse-interface preview link