FE Release 2024-12-13 #3462
FE Release 2024-12-13 #3462
Conversation
* feat: scaffold new prover management * test: define unit tests for prover management * feat: implement prover management * feat: deprecate prover role constant * test: update to constant deprecation * feat: additional prover getters * feat: prover timeout management * test: coverage for timeout management * test: should save proverID for origin txs * feat: save proverID, pack BridgeTxDetails * test: define expected behavior for prover timeouts * feat: implement timeout penalty in dispute * fix: Role Admin should be handling provers, as before * refactor: "prover timeout" -> "dispute penalty time"
- @synapsecns/[email protected]
* feat: configurable `deployBlock` * test: add coverage
* feat: scaffold IntentRouter * feat: initial implementation * test: single step * feat: scaffold checked/unchecked `completeIntent` * feat: implement balance checks * test: add cases where TokenZap has non-zero initial amount * test: double step cases (erc20 and/or native combos) * feat: SIR, TokenZap deployment script * deploy: test deploys for SIR * test: update to #3434 changes * feat: scaffold SIP * test: coverage for SIP * feat: Synapse Intent Previewer * feat: update deploy script, deploy SIP * fix(contracts-rfq): add `forwardTo` to ZapData for Zap Actions that don't forward assets to the user (#3451) * feat: scaffold finalToken, forwardTo in ZapDataV1 * feat: add finalToken, forwardTo * feat: scaffold TokenZap.encodeZapData * test: update existing tests * test: forwardTo scenarios * feat: token zeor address checks * feat:scaffold token forwarding * test: more revert cases * feat: final token forwarding * test: forwardTo behaviour in SIP * feat: support optional `forwardTo` in SIP * deploy: redeploy SIP, TokenZap
- @synapsecns/[email protected]
* test: add TokenZapV1 uncovered cases * test: remove test contracts from the coverage report
- @synapsecns/[email protected]
- @synapsecns/[email protected] - @synapsecns/[email protected]
* new blog post * nits
- @synapsecns/[email protected]
…3461) * Hyperliquid bridge * Step indicator
abtestingalpha
requested review from
bigboydiamonds,
aureliusbtc,
ChiTimesChi and
trajan0x
as code owners
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/synapse-interface/components/HyperliquidDepositInfo.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@babel/eslint-parser' declared in 'packages/synapse-interface/.eslintrc.js': Cannot find module '@babel/core/package.json'
WalkthroughThe pull request introduces several updates across various files, primarily focusing on enhancing the functionality related to the Hyperliquid bridge and deposit features. Key changes include the addition of new words for spell checking, updates to changelogs, the introduction of new components for user interactions, and modifications to existing contracts and interfaces to support new functionalities. Additionally, several localization files have been updated to include new translations for improved user accessibility. Overall, these changes reflect a significant enhancement in the Synapse interface and its underlying contracts. Changes
Suggested labels
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
|
github-actions
bot
added
M-synapse-interface
Sol
Typescript
M-docs
size/l
fe-release
labels
- @synapsecns/[email protected] - @synapsecns/[email protected]
Deploying sanguine-fe with
|
| Latest commit: |
fb55d90
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5b78439e.sanguine-fe.pages.dev |
| Branch Preview URL: | https://master.sanguine-fe.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (45)
packages/contracts-rfq/test/mocks/VaultMock.sol (1)
17-18: Consider enhancing the test function documentation.While the current documentation explains the purpose, it could be more specific about why coverage should be excluded for this mock contract.
- /// @notice We include an empty "test" function so that this contract does not appear in the coverage report. + /// @notice Empty function to exclude this mock contract from coverage reports since it's only used as a testing utility. function testVaultMock() external {}packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol (1)
14-14: Consider using explicit seconds for DISPUTE_PENALTY_TIMEFor better clarity and to avoid potential floating-point confusion, consider expressing the time in seconds explicitly.
- uint256 public constant DISPUTE_PENALTY_TIME = 4.2 minutes; + uint256 public constant DISPUTE_PENALTY_TIME = 252 seconds; // 4.2 minutespackages/contracts-rfq/CHANGELOG.md (2)
20-20: Fix heading level inconsistencyThe heading levels should only increment by one level at a time. All feature and bug fix sections should be h2 (##) instead of h3 (###).
Apply this change to all similar headings in the file:
-### Bug Fixes +## Bug FixesAlso applies to: 36-36
🧰 Tools
🪛 Markdownlint (0.37.0)
20-20: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
7-17: Remove excessive blank linesThere are too many consecutive blank lines between changelog entries. One blank line is sufficient for readability.
Remove the excessive blank lines, keeping only one blank line between sections.
Also applies to: 29-31
packages/contracts-rfq/test/harnesses/BridgeTransactionV2Harness.sol (1)
Line range hint
1-85: Well-structured test harness with good architectural patterns.The contract demonstrates good practices:
- Clear separation of concerns with individual wrapper functions
- Consistent use of pure functions for deterministic testing
- Proper documentation and test utility patterns
Consider adding explicit error messages in the library functions that this harness wraps, to improve debugging experience during testing.
packages/contracts-rfq/contracts/interfaces/ISynapseIntentRouterErrors.sol (2)
1-4: LGTM! Consider pinning the Solidity version.The interface follows good practices with proper license and naming conventions. However, for production contracts, it's recommended to pin the exact Solidity version rather than using the caret range.
-pragma solidity ^0.8.4; +pragma solidity 0.8.4;
5-12: Consider adding parameters and NatSpec documentation to errors.While the error names are descriptive, adding parameters would provide more context during reverts. Additionally, NatSpec documentation would improve developer experience.
Example improvements:
+ /// @notice Thrown when the amount provided is less than required + /// @dev This includes cases where token amounts or ETH value is insufficient - error SIR__AmountInsufficient(); + error SIR__AmountInsufficient(uint256 provided, uint256 required); + /// @notice Thrown when the operation exceeds its deadline - error SIR__DeadlineExceeded(); + error SIR__DeadlineExceeded(uint256 deadline, uint256 currentTimestamp); + /// @notice Thrown when msg.value doesn't match the expected amount - error SIR__MsgValueIncorrect(); + error SIR__MsgValueIncorrect(uint256 expected, uint256 received); + /// @notice Thrown when required execution steps are missing - error SIR__StepsNotProvided(); + error SIR__StepsNotProvided(uint256 providedSteps); + /// @notice Thrown when a token address doesn't contain contract code - error SIR__TokenNotContract(); + error SIR__TokenNotContract(address token); + /// @notice Thrown when there are unspent funds after execution - error SIR__UnspentFunds(); + error SIR__UnspentFunds(uint256 amount); + /// @notice Thrown when zap returns incorrect value - error SIR__ZapIncorrectReturnValue(); + error SIR__ZapIncorrectReturnValue(bytes returnData); + /// @notice Thrown when zap doesn't return any value - error SIR__ZapNoReturnValue(); + error SIR__ZapNoReturnValue(address zap);packages/contracts-rfq/test/mocks/DefaultPoolMock.sol (2)
13-22: Add input validation to match production behaviorEven in mock contracts, it's important to validate input parameters to catch integration issues early. Consider adding basic validations like token index bounds checking and deadline validation.
function swap( uint8 tokenIndexFrom, uint8 tokenIndexTo, uint256 dx, uint256 minDy, uint256 deadline ) external returns (uint256 amountOut) -{} +{ + require(tokenIndexFrom < TOKENS, "Invalid input token"); + require(tokenIndexTo < TOKENS, "Invalid output token"); + require(tokenIndexFrom != tokenIndexTo, "Same token"); + require(deadline >= block.timestamp, "Expired"); +}
24-32: Add input validation to calculateSwapSimilar to the swap function, add input validation to maintain consistency with production behavior.
function calculateSwap( uint8 tokenIndexFrom, uint8 tokenIndexTo, uint256 dx ) external view returns (uint256 amountOut) -{} +{ + require(tokenIndexFrom < TOKENS, "Invalid input token"); + require(tokenIndexTo < TOKENS, "Invalid output token"); + require(tokenIndexFrom != tokenIndexTo, "Same token"); +}packages/contracts-rfq/test/zaps/TokenZapV1.GasBench.t.sol (2)
45-45: Consider making the parameter offset more maintainableThe hardcoded offset
4 + 32is fragile and could break if thevault.depositfunction signature changes. Consider extracting this as a constant with a clear name explaining its purpose, or better yet, implement a dynamic way to determine the offset.- return tokenZap.encodeZapData(address(vault), originalPayload, 4 + 32, address(0), address(0)); + uint256 constant AMOUNT_PARAMETER_OFFSET = 4 + 32; // function selector (4) + first parameter (32) + return tokenZap.encodeZapData(address(vault), originalPayload, AMOUNT_PARAMETER_OFFSET, address(0), address(0));
Line range hint
48-63: Add gas benchmarking assertionsDespite being a gas benchmark test file, there are no actual gas measurements or assertions. Consider adding gas snapshots or explicit gas checks using Forge's gas tracking features.
Add gas assertions to the test functions:
function test_deposit_erc20() public { bytes memory depositPayload = getVaultPayload(address(erc20), AMOUNT); bytes memory zapData = getZapData(depositPayload); // Transfer tokens to the zap contract first. erc20.transfer(address(tokenZap), AMOUNT); + uint256 gasBefore = gasleft(); tokenZap.zap(address(erc20), AMOUNT, zapData); + uint256 gasUsed = gasBefore - gasleft(); + assertLt(gasUsed, 150000, "Gas usage too high"); // Check that the vault registered the deposit. assertEq(vault.balanceOf(user, address(erc20)), AMOUNT); }packages/contracts-rfq/test/mocks/PoolMock.sol (2)
30-43: Consider adding overflow protection and events.While this is a mock contract, consider:
- Using SafeMath or checked arithmetic for amount calculations
- Emitting events for better testability and debugging
function swap(uint256 amountIn, address tokenIn) external returns (uint256 amountOut) { address tokenOut; if (tokenIn == token0) { tokenOut = token1; - amountOut = amountIn * ratioWei / 1e18; + amountOut = (amountIn * ratioWei) / 1e18; } else if (tokenIn == token1) { tokenOut = token0; - amountOut = amountIn * 1e18 / ratioWei; + amountOut = (amountIn * 1e18) / ratioWei; } else { revert PoolMock__TokenNotSupported(); } IERC20(tokenIn).safeTransferFrom(msg.sender, address(this), amountIn); IERC20(tokenOut).safeTransfer(msg.sender, amountOut); + emit Swap(msg.sender, tokenIn, tokenOut, amountIn, amountOut); } +event Swap( + address indexed user, + address indexed tokenIn, + address indexed tokenOut, + uint256 amountIn, + uint256 amountOut +);
7-8: Enhance documentation with usage examples.While the contract is marked as a mock, consider adding more detailed documentation including:
- Example usage in tests
- Expected behavior of ratio adjustments
- Limitations and assumptions
docs/bridge/blog-posts/RFQFlow.tsx (3)
14-21: Add ARIA attributes for decorative elements.The background rectangles are decorative and should be hidden from screen readers.
- <g fill="currentcolor" fillOpacity=".05"> + <g fill="currentcolor" fillOpacity=".05" aria-hidden="true">
55-66: Add internationalization support for text labels.Consider extracting text content for translation support.
+import { useTranslation } from 'react-i18next' + export const RFQFlow: FC = () => { + const { t } = useTranslation() return ( // ... - <text x="-33%" y="24"> - originChain - </text> + <text x="-33%" y="24"> + {t('bridge.flow.originChain')} + </text>
67-182: Extract colors to theme and add error states.Consider the following improvements:
- Move HSL colors to theme variables for consistency
- Add error states for failed transactions
+const THEME_COLORS = { + primary: 'hsl(211deg 67% 50%)', + success: 'hsl(164deg 37% 50%)', + error: 'hsl(0deg 67% 50%)', +} as const + export const RFQFlow: FC = () => { return ( // ... - fill="hsl(211deg 67% 50%)" - stroke="hsl(211deg 67% 50%)" + fill={THEME_COLORS.primary} + stroke={THEME_COLORS.primary}docs/bridge/blog-posts/2024-12-12-fastbridgev2-post.md (3)
4-4: Uncomment the authors metadata.The authors line is currently commented out, which might affect blog post attribution and metadata processing. Consider uncommenting this line:
-# authors: [synapse] +authors: [synapse]
58-68: Consider using BigNumber for amount fields.The
origin_amount_exactfield is typed as string, which could lead to precision issues when handling large numbers. Consider using a more precise type like BigNumber or documenting the expected string format (e.g., decimal string, hex string).Also, consider adding input validation requirements in the interface documentation:
- Valid chain IDs
- Non-zero amounts
- Valid token addresses
19-20: Fix formatting issues.
- Replace hard tabs with spaces in the figure element (lines 19-20)
- Add a comma in line 84: "...better pricing for end users, as relayers can..."
Also applies to: 84-84
🧰 Tools
🪛 Markdownlint (0.37.0)
19-19: Column: 1
Hard tabs(MD010, no-hard-tabs)
20-20: Column: 1
Hard tabs(MD010, no-hard-tabs)
packages/synapse-interface/components/_Transaction/_Transactions.tsx (1)
65-69: Consider extracting chain-specific status logicThe chain-specific status logic embedded in the render method could be moved to a separate utility function for better maintainability and reusability.
+const getTransactionStatus = (tx: _TransactionDetails) => { + if (tx.destinationChain.id === HYPERLIQUID.id) { + return 'completed'; + } + return tx.status; +}; // Then in the component: -status={ - tx.destinationChain.id === HYPERLIQUID.id - ? 'completed' - : tx.status -} +status={getTransactionStatus(tx)}packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
Line range hint
32-37: ChangequoteExclusivitySecondsto unsigned integerThe
quoteExclusivitySecondsinBridgeParamsV2is declared asint256, but a negative exclusivity period is invalid. Consider changing it touint256to prevent negative values.Apply this diff to fix the issue:
struct BridgeParamsV2 { address quoteRelayer; - int256 quoteExclusivitySeconds; + uint256 quoteExclusivitySeconds; bytes quoteId; uint256 zapNative; bytes zapData; }packages/synapse-interface/components/StateManagedBridge/HyperliquidDepositButton.tsx (3)
176-184: Remove unnecessary React FragmentThe fragment wrapping the
<div>is redundant since it contains only a single child element.Apply this diff to remove the redundant fragment:
- <> <div className="flex flex-col w-full"> <TransactionButton {...buttonProperties} disabled={isButtonDisabled} chainId={fromChainId} /> </div> - </>🧰 Tools
🪛 Biome (1.9.4)
[error] 176-184: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
116-118: Add user feedback on deposit failureIn the
catchblock ofhandleDeposit, the error is logged but the user is not notified. Consider adding user-facing error handling to inform the user if the deposit fails.
61-64: Simplify connection state managementThe
isConnectedstate duplicates the value provided byuseAccount. Remove the redundant state and useisConnecteddirectly fromuseAccount.Apply this diff to simplify the code:
- const [isConnected, setIsConnected] = useState(false) const { address, isConnected } = useAccount() ... - useAccountEffect({ - onDisconnect() { - setIsConnected(false) - }, - }) - useEffect(() => { - setIsConnected(isConnectedInit) - }, [isConnectedInit]) ... - if (!isConnected && hasValidInput) { + if (!isConnected && hasValidInput) { ... - (isConnected && !hasSufficientBalance) + (isConnected && !hasSufficientBalance)Also applies to: 123-132
packages/contracts-rfq/contracts/zaps/TokenZapV1.sol (2)
Line range hint
51-91: Consider adding reentrancy protection to thezapfunctionThe
zapfunction performs external calls and may forward tokens via_forwardToken, which could introduce reentrancy risks. Adding reentrancy protection, such as thenonReentrantmodifier from OpenZeppelin'sReentrancyGuard, would enhance the contract's security against potential reentrancy attacks.
Line range hint
122-136: Clarify the type ofamountPositionto prevent potential truncationIn
encodeZapData, theamountPositionparameter is accepted as auint256but cast touint16, which could lead to unexpected truncation ifamountPositionexceedsuint16maximum value. SinceamountPositionshould not exceedpayload.length, which is constrained byZapDataV1.AMOUNT_NOT_PRESENT, consider acceptingamountPositionasuint16directly to avoid unnecessary casting and potential errors.packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol (3)
218-238: Typographical error in function name_createHandleHativeStepsThe function
_createHandleHativeStepsappears to have a typo in its name. It should be_createHandleNativeStepsto reflect its purpose of handling native gas tokens. This correction improves code readability and prevents confusion.Apply this diff to correct the typo:
-function _createHandleHativeSteps( +function _createHandleNativeSteps(
65-72: Consistent naming: Update function calls to match corrected function nameAfter renaming the function to
_createHandleNativeSteps, ensure all calls to this function reflect the new name to maintain consistency and prevent runtime errors.Apply this diff to update the function calls:
-steps = _createHandleHativeSteps(tokenIn, tokenOut, amountIn, forwardTo); +steps = _createHandleNativeSteps(tokenIn, tokenOut, amountIn, forwardTo);
146-155: Avoid using exceptions for control flow when determining token countIn the loop beginning at line 146, the code uses a
try...catchblock to detect when there are no more tokens by catching an exception. Using exceptions for control flow can be less efficient and may obscure the logic.Consider refactoring to determine the number of tokens without relying on exceptions. If possible, utilize a function from the pool interface that returns the total number of tokens, enhancing performance and code clarity.
packages/contracts-rfq/test/router/SynapseIntentPreviewer.t.sol (1)
Line range hint
245-763: Consider refactoring repetitive code in test functionsSeveral test functions, such as
getSwapZapData,getAddLiquidityZapData, and similar, have similar patterns and structures. Refactoring common code into helper functions can reduce duplication and improve maintainability.packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol (1)
245-1379: Consider refactoring repetitive test cases to reduce duplicationMultiple test functions have similar structures, testing various scenarios with different parameters. Refactoring common setup and assertions into helper functions or parameterized tests can improve maintainability and reduce code duplication.
packages/contracts-rfq/contracts/interfaces/IAdminV2Errors.sol (1)
7-10: LGTM! Well-defined error declarations for prover management.The new error declarations are well-scoped and follow a consistent naming convention. They provide clear error handling for prover management and dispute penalty scenarios.
Consider documenting these errors with NatSpec comments to provide more context about when each error is thrown, especially for
DisputePenaltyTimeBelowMinwhere knowing the minimum time value would be helpful.packages/contracts-rfq/test/mocks/WETHMock.sol (1)
11-11: Add test-only warning in contract inheritance.Since this contract uses forge-std's CommonBase, it should be clearly marked as test-only to prevent accidental production usage.
-contract WETHMock is ERC20, CommonBase { +/// @dev This contract uses forge-std and should NEVER be deployed to production +contract WETHMock is ERC20, CommonBase {packages/contracts-rfq/contracts/interfaces/ISynapseIntentPreviewer.sol (1)
2-2: Consider upgrading Solidity versionThe contract uses ^0.8.4 while the latest version is 0.8.24. Consider upgrading to benefit from compiler optimizations and bug fixes.
-pragma solidity ^0.8.4; +pragma solidity ^0.8.24;packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol (1)
11-12: Consider using a more explicit coverage exclusion approachWhile the empty function works for coverage exclusion, consider using a more standard approach like pragma comments or configuration in the coverage tool.
packages/synapse-interface/constants/existingBridgeRoutes.ts (2)
52-62: Verify edge cases and add type safetyThe function modifies routes based on specific conditions but could benefit from:
- Type annotations for parameters and return value
- Validation for undefined/null routes
- Documentation of the function's purpose
-const addUSDCHyperLiquid = (routes) => { +const addUSDCHyperLiquid = (routes: BridgeRoutes): BridgeRoutes => { + if (!routes) return {} const usdcHyperliquid = `USDC-${HYPERLIQUID.id}` return _.mapValues(routes, (innerList, key) => { if (key === 'USDC-42161' || innerList.includes('USDC-42161')) { return [...innerList, usdcHyperliquid] } return innerList }) }
65-67: Consider memoizing the route generationThe route generation could be expensive for large bridge maps. Consider memoizing the result if the inputs don't change frequently.
+const memoizedConstructJSON = _.memoize(constructJSON) +const memoizedAddUSDCHyperLiquid = _.memoize(addUSDCHyperLiquid) export const EXISTING_BRIDGE_ROUTES: BridgeRoutes = addUSDCHyperLiquid( constructJSON(BRIDGE_MAP, PAUSED_TOKENS) )packages/contracts-rfq/deployments/optimism/SynapseIntentPreviewer.json (2)
22-87: Document parameter validation for previewIntent functionThe function has a complex interface with multiple address parameters and returns structured swap data. Consider adding NatSpec documentation to clarify:
- Valid ranges for
amountIn- Requirements for
swapQuoterandforwardToaddresses- Relationship between
tokenInandtokenOut
88-122: Consider adding parameters to custom errorsThe custom errors are well-named but could be more informative with parameters. For example:
SIP__PoolTokenMismatchcould include the mismatched tokensSIP__TokenNotNativecould include the token addresspackages/synapse-interface/components/StateManagedBridge/OutputContainer.tsx (1)
57-61: Document special case for Arbitrum to Hyperliquid transfersThe code uses
debouncedFromValueinstead of the calculatedshowValuespecifically for Arbitrum to Hyperliquid transfers. Please add a comment explaining why this route requires different handling.showValue={ + // Use input value directly for Arbitrum to Hyperliquid transfers as they maintain 1:1 ratio fromChainId === ARBITRUM.id && toChainId === HYPERLIQUID.id ? debouncedFromValue : showValue }packages/contracts-rfq/deployments/optimism/TokenZapV1.json (1)
94-122: Review the security implications of the zap functionThe
zapfunction is payable and handles token operations, making it security-critical. Key observations:
- Takes token address, amount, and arbitrary zapData as input
- Returns bytes4 (likely a function selector)
- Proper error handling is implemented through custom errors
Consider documenting:
- The expected function selector returns
- The format and validation rules for zapData
- The conditions under which the function reverts
packages/synapse-interface/slices/bridgeQuote/thunks.ts (1)
123-131: Improve error message for Hyperliquid-specific case.The current error message doesn't distinguish between regular routing failures and Hyperliquid-specific restrictions. Consider providing a more specific error message when
toChainId === HYPERLIQUID.id.if ( !( originQuery && maxAmountOut && destQuery && feeAmount && toChainId !== HYPERLIQUID.id ) ) { - const msg = `No route found for bridging ${debouncedFromValue} ${fromToken?.symbol} on ${CHAINS_BY_ID[fromChainId]?.name} to ${toToken?.symbol} on ${CHAINS_BY_ID[toChainId]?.name}` + const msg = toChainId === HYPERLIQUID.id + ? `Bridging to Hyperliquid is not supported in this way. Please use the Hyperliquid deposit flow.` + : `No route found for bridging ${debouncedFromValue} ${fromToken?.symbol} on ${CHAINS_BY_ID[fromChainId]?.name} to ${toToken?.symbol} on ${CHAINS_BY_ID[toChainId]?.name}` return rejectWithValue(msg) }packages/synapse-interface/components/StateManagedBridge/BridgeTransactionButton.tsx (1)
62-67: Simplify the minimum deposit check logicThe nested ternary operator can be simplified to a direct boolean expression.
- const hasHyperliquidMinDeposit = - toChainId === HYPERLIQUID.id - ? Number(debouncedFromValue) > HYPERLIQUID_MINIMUM_DEPOSIT - ? true - : false - : true + const hasHyperliquidMinDeposit = + toChainId !== HYPERLIQUID.id || + Number(debouncedFromValue) > HYPERLIQUID_MINIMUM_DEPOSIT🧰 Tools
🪛 Biome (1.9.4)
[error] 64-66: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
packages/synapse-interface/components/HyperliquidDepositInfo.tsx (2)
3-7: Consider adding TypeScript prop typesThe component implementation looks good, but would benefit from explicit TypeScript prop types for better type safety and documentation.
interface HyperliquidDepositInfoProps { fromChainId: number; isOnArbitrum: boolean; hasDepositedOnHyperliquid: boolean; }
78-181: Consider consolidating SVG componentsThe SVG components share similar structure but differ mainly in colors. Consider refactoring them into a single component with color props to reduce code duplication.
interface StepCircleProps { number?: number; strokeColor: string; fillColor: string; checkmark?: boolean; } const StepCircle = ({ number, strokeColor, fillColor, checkmark }: StepCircleProps) => { return ( <svg width="34" height="34" viewBox="0 0 34 34" fill="none" xmlns="http://www.w3.org/2000/svg"> <rect x="1" y="1" width="32" height="32" rx="16" stroke={strokeColor} strokeWidth="2"/> {checkmark ? ( <path d="..." fill={fillColor}/> // Checkmark path ) : ( <path d="..." fill={fillColor}/> // Number path )} </svg> ); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/synapse-interface/assets/chains/hyperliquid.svgis excluded by!**/*.svg
📒 Files selected for processing (79)
cspell.json(1 hunks)docs/bridge/CHANGELOG.md(1 hunks)docs/bridge/blog-posts/2024-12-12-fastbridgev2-post.md(1 hunks)docs/bridge/blog-posts/RFQFlow.tsx(1 hunks)docs/bridge/package.json(1 hunks)packages/contracts-rfq/CHANGELOG.md(1 hunks)packages/contracts-rfq/contracts/AdminV2.sol(5 hunks)packages/contracts-rfq/contracts/FastBridgeV2.sol(5 hunks)packages/contracts-rfq/contracts/interfaces/IAdminV2.sol(2 hunks)packages/contracts-rfq/contracts/interfaces/IAdminV2Errors.sol(1 hunks)packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol(1 hunks)packages/contracts-rfq/contracts/interfaces/ISynapseIntentPreviewer.sol(1 hunks)packages/contracts-rfq/contracts/interfaces/ISynapseIntentRouter.sol(1 hunks)packages/contracts-rfq/contracts/interfaces/ISynapseIntentRouterErrors.sol(1 hunks)packages/contracts-rfq/contracts/legacy/router/interfaces/IDefaultExtendedPool.sol(1 hunks)packages/contracts-rfq/contracts/legacy/router/interfaces/IDefaultPool.sol(1 hunks)packages/contracts-rfq/contracts/legacy/router/interfaces/IWETH9.sol(1 hunks)packages/contracts-rfq/contracts/libs/ZapDataV1.sol(4 hunks)packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol(1 hunks)packages/contracts-rfq/contracts/router/SynapseIntentRouter.sol(1 hunks)packages/contracts-rfq/contracts/zaps/TokenZapV1.sol(6 hunks)packages/contracts-rfq/deployments/arbitrum/SynapseIntentPreviewer.json(1 hunks)packages/contracts-rfq/deployments/arbitrum/SynapseIntentRouter.json(1 hunks)packages/contracts-rfq/deployments/arbitrum/TokenZapV1.json(1 hunks)packages/contracts-rfq/deployments/optimism/SynapseIntentPreviewer.json(1 hunks)packages/contracts-rfq/deployments/optimism/SynapseIntentRouter.json(1 hunks)packages/contracts-rfq/deployments/optimism/TokenZapV1.json(1 hunks)packages/contracts-rfq/foundry.toml(1 hunks)packages/contracts-rfq/package.json(1 hunks)packages/contracts-rfq/script/DeploySIR.s.sol(1 hunks)packages/contracts-rfq/test/FastBridgeV2.Management.t.sol(7 hunks)packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol(2 hunks)packages/contracts-rfq/test/FastBridgeV2.Src.RefundV1.t.sol(1 hunks)packages/contracts-rfq/test/FastBridgeV2.Src.t.sol(24 hunks)packages/contracts-rfq/test/FastBridgeV2.t.sol(2 hunks)packages/contracts-rfq/test/harnesses/BridgeTransactionV2Harness.sol(1 hunks)packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol(1 hunks)packages/contracts-rfq/test/integration/FastBridge.MulticallTarget.t.sol(1 hunks)packages/contracts-rfq/test/integration/FastBridgeV2.MulticallTarget.t.sol(1 hunks)packages/contracts-rfq/test/integration/TokenZapV1.t.sol(4 hunks)packages/contracts-rfq/test/libs/ZapDataV1.t.sol(6 hunks)packages/contracts-rfq/test/mocks/DefaultPoolMock.sol(1 hunks)packages/contracts-rfq/test/mocks/MockRevertingRecipient.sol(1 hunks)packages/contracts-rfq/test/mocks/PoolMock.sol(1 hunks)packages/contracts-rfq/test/mocks/SwapQuoterMock.sol(1 hunks)packages/contracts-rfq/test/mocks/VaultMock.sol(2 hunks)packages/contracts-rfq/test/mocks/WETHMock.sol(2 hunks)packages/contracts-rfq/test/router/SynapseIntentPreviewer.t.sol(1 hunks)packages/contracts-rfq/test/router/SynapseIntentRouter.BalanceChecks.t.sol(1 hunks)packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol(1 hunks)packages/contracts-rfq/test/zaps/TokenZapV1.GasBench.t.sol(1 hunks)packages/contracts-rfq/test/zaps/TokenZapV1.t.sol(11 hunks)packages/synapse-interface/CHANGELOG.md(1 hunks)packages/synapse-interface/components/HyperliquidDepositInfo.tsx(1 hunks)packages/synapse-interface/components/StateManagedBridge/BridgeQuoteResetTimer.tsx(2 hunks)packages/synapse-interface/components/StateManagedBridge/BridgeTransactionButton.tsx(4 hunks)packages/synapse-interface/components/StateManagedBridge/HyperliquidDepositButton.tsx(1 hunks)packages/synapse-interface/components/StateManagedBridge/OutputContainer.tsx(4 hunks)packages/synapse-interface/components/StateManagedBridge/hooks/useBridgeValidations.ts(2 hunks)packages/synapse-interface/components/_Transaction/_Transaction.tsx(2 hunks)packages/synapse-interface/components/_Transaction/_Transactions.tsx(2 hunks)packages/synapse-interface/components/layouts/LandingPageWrapper/index.tsx(1 hunks)packages/synapse-interface/constants/chains/master.tsx(2 hunks)packages/synapse-interface/constants/existingBridgeRoutes.ts(2 hunks)packages/synapse-interface/constants/index.ts(1 hunks)packages/synapse-interface/constants/urls/index.tsx(1 hunks)packages/synapse-interface/messages/ar.json(1 hunks)packages/synapse-interface/messages/en-US.json(1 hunks)packages/synapse-interface/messages/es.json(1 hunks)packages/synapse-interface/messages/fr.json(1 hunks)packages/synapse-interface/messages/jp.json(1 hunks)packages/synapse-interface/messages/tr.json(1 hunks)packages/synapse-interface/messages/zh-CN.json(1 hunks)packages/synapse-interface/package.json(1 hunks)packages/synapse-interface/pages/state-managed-bridge/index.tsx(12 hunks)packages/synapse-interface/slices/bridgeQuote/thunks.ts(2 hunks)packages/widget/CHANGELOG.md(1 hunks)packages/widget/package.json(1 hunks)packages/widget/src/constants/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (14)
- packages/synapse-interface/constants/index.ts
- packages/contracts-rfq/contracts/legacy/router/interfaces/IDefaultExtendedPool.sol
- packages/widget/package.json
- packages/contracts-rfq/package.json
- packages/contracts-rfq/contracts/legacy/router/interfaces/IWETH9.sol
- packages/contracts-rfq/test/mocks/MockRevertingRecipient.sol
- packages/contracts-rfq/contracts/legacy/router/interfaces/IDefaultPool.sol
- packages/synapse-interface/constants/urls/index.tsx
- packages/widget/CHANGELOG.md
- packages/synapse-interface/components/StateManagedBridge/BridgeQuoteResetTimer.tsx
- packages/widget/src/constants/index.ts
- packages/synapse-interface/package.json
- docs/bridge/package.json
- packages/contracts-rfq/deployments/arbitrum/SynapseIntentPreviewer.json
🧰 Additional context used
📓 Learnings (7)
packages/contracts-rfq/test/integration/FastBridge.MulticallTarget.t.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56
Timestamp: 2024-11-12T03:37:02.230Z
Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.
packages/contracts-rfq/test/FastBridgeV2.Src.RefundV1.t.sol (2)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-11-12T03:37:08.148Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56
Timestamp: 2024-11-12T03:37:02.230Z
Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.
packages/contracts-rfq/test/integration/FastBridgeV2.MulticallTarget.t.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56
Timestamp: 2024-11-12T03:37:02.230Z
Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.
packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol (2)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-11-12T03:37:08.148Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.t.sol:53-56
Timestamp: 2024-11-12T03:37:02.230Z
Learning: Modifying test utility contracts in the `test` directory is unnecessary and would only pollute the code.
packages/synapse-interface/pages/state-managed-bridge/index.tsx (1)
Learnt from: abtestingalpha
PR: synapsecns/sanguine#3461
File: packages/synapse-interface/pages/state-managed-bridge/index.tsx:438-438
Timestamp: 2024-12-13T18:55:49.060Z
Learning: In `packages/synapse-interface/pages/state-managed-bridge/index.tsx`, the `switchChain` function from `@wagmi/core` can be called with `wagmiConfig` and an object containing `{ chainId: ARBITRUM.id }` as arguments.
packages/contracts-rfq/contracts/FastBridgeV2.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-11-12T03:37:08.148Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-11-12T03:37:08.148Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
🪛 Biome (1.9.4)
packages/synapse-interface/components/StateManagedBridge/BridgeTransactionButton.tsx
[error] 64-66: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
packages/synapse-interface/components/StateManagedBridge/HyperliquidDepositButton.tsx
[error] 176-184: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🪛 LanguageTool
docs/bridge/blog-posts/2024-12-12-fastbridgev2-post.md
[uncategorized] ~84-~84: Possible missing comma found.
Context: ...ates directly to better pricing for end users as relayers can operate with tighter sp...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 Markdownlint (0.37.0)
docs/bridge/blog-posts/2024-12-12-fastbridgev2-post.md
19-19: Column: 1
Hard tabs
(MD010, no-hard-tabs)
20-20: Column: 1
Hard tabs
(MD010, no-hard-tabs)
docs/bridge/CHANGELOG.md
9-9: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time
(MD001, heading-increment)
packages/contracts-rfq/CHANGELOG.md
20-20: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time
(MD001, heading-increment)
packages/synapse-interface/CHANGELOG.md
9-9: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time
(MD001, heading-increment)
🔇 Additional comments (92)
packages/contracts-rfq/test/mocks/VaultMock.sol (1)
6-6: LGTM! Clear documentation and appropriate solhint directive.
The solhint directive is correctly placed and the contract documentation clearly indicates this is a mock for testing purposes.
packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol (1)
30-38: LGTM! Role management and configuration changes look good
The changes improve role management by using the dedicated addProver method and properly configure the dispute penalty time. The code maintains a clear structure with role assignments followed by configuration settings.
packages/contracts-rfq/CHANGELOG.md (1)
Line range hint 1-24: LGTM! The changelog follows conventional commits guidelines.
The recent changes are well-documented with:
- Clear version numbers and dates
- Proper categorization (Features/Bug Fixes)
- Referenced issue numbers and commit hashes
- Descriptive commit messages
🧰 Tools
🪛 Markdownlint (0.37.0)
20-20: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time
(MD001, heading-increment)
packages/contracts-rfq/test/harnesses/BridgeTransactionV2Harness.sol (1)
6-9: LGTM! The empty test function serves its purpose.
The empty function with its documentation clearly explains its purpose of excluding this test harness from coverage reports, which is a good practice for test utilities.
packages/contracts-rfq/test/mocks/DefaultPoolMock.sol (4)
1-8: LGTM: Contract setup is well-structured
The contract setup follows best practices with appropriate license, Solidity version, and interface implementation.
10-11: LGTM: Well-documented test function
The empty test function serves its purpose of excluding the contract from coverage reports, which is a common testing pattern.
34-46: LGTM: Well-structured storage view function
The function signature correctly matches the interface requirements and will be properly mocked during tests.
48-54: LGTM: Well-implemented token retrieval with proper validation
The function correctly validates token indices and provides deterministic addresses for testing. The error message is clear, and the implementation comment about vm.mockCall is helpful.
packages/contracts-rfq/test/zaps/TokenZapV1.GasBench.t.sol (1)
45-45: Add test cases for non-zero finalToken and forwardTo
The test only covers the case where both finalToken and forwardTo are address(0). Consider adding test cases with non-zero addresses to ensure the new parameters work correctly in all scenarios.
packages/contracts-rfq/foundry.toml (1)
10-10:
Security Risk: Extremely high gas limit could lead to vulnerabilities
The gas limit of 9223372036854775807 (2^63 - 1) is unusually high and could pose several risks:
- Potential for extremely expensive transactions that could drain user funds
- Vulnerability to DOS attacks through resource exhaustion
- Bypass of important gas-related safety checks
Most networks have block gas limits far below this value. For example, Ethereum mainnet's block gas limit is around 30M gas.
Consider setting a more reasonable gas limit that aligns with your target networks' constraints. For example:
-gas_limit = 9223372036854775807
+gas_limit = 30000000 # 30M gas, similar to Ethereum mainnetLet's verify the gas limits of the target networks specified in your RPC endpoints:
packages/contracts-rfq/test/mocks/PoolMock.sol (3)
1-9: LGTM! Good practices followed in contract setup.
The contract follows security best practices with a recent Solidity version and proper use of SafeERC20 for token operations.
16-16: LGTM! Proper use of custom errors.
The custom error follows good naming conventions and is gas efficient.
23-24: LGTM! Well-documented test function.
The empty function's purpose is clearly documented for coverage exclusion.
docs/bridge/blog-posts/RFQFlow.tsx (1)
9-13: LGTM!
The timer setup with a 2-second delay between animation cycles provides a good user experience.
packages/contracts-rfq/test/integration/FastBridgeV2.MulticallTarget.t.sol (3)
9-9: LGTM: Appropriate linter directive
The solhint disable comment is correctly placed and specifically targets the empty test function that follows.
11-12: LGTM: Well-documented coverage exclusion
The empty test function is appropriately documented and serves its purpose of excluding this utility contract from coverage reports.
14-17: Verify API changes across the codebase
The change from grantRole to addProver suggests an API update. Let's verify this change is consistent across all related files.
✅ Verification successful
Inconsistent API usage detected across FastBridge versions
The verification reveals mixed usage of both APIs:
FastBridgeV2files use the newaddProvermethod- Original
FastBridgefiles still usegrantRole(RELAYER_ROLE) - One file (
FastBridgeV2.Src.Base.t.sol) uses bothaddProverandgrantRole
Let's verify if this is an intentional coexistence of two contract versions:
API change is intentional with FastBridgeV2 upgrade
The verification reveals this is a valid change as part of a major contract upgrade from FastBridge to FastBridgeV2:
- FastBridge uses role-based access with
grantRole(RELAYER_ROLE) - FastBridgeV2 introduces a new prover system with
addProver - Both contracts coexist in the codebase with separate test suites
- The file under review correctly uses the new V2 API as it's a V2-specific test
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of grantRole in FastBridgeV2 related files
rg -A 2 "grantRole.*relayer" "packages/contracts-rfq/"
# Search for the new addProver method usage
rg -A 2 "addProver.*relayer" "packages/contracts-rfq/"
# Verify the FastBridgeV2 contract interface
ast-grep --pattern 'contract FastBridgeV2 {
$$$
function addProver($_) {
$$$
}
$$$
}'
Length of output: 2489
Script:
#!/bin/bash
# Check if FastBridge and FastBridgeV2 are separate contracts
fd "FastBridge" --type f --exec grep -l "contract FastBridge" {}
# Check inheritance relationship
ast-grep --pattern 'contract FastBridgeV2 is $_ {
$$$
}'
Length of output: 2112
Script:
#!/bin/bash
# Check the content of both contract files to understand the relationship
cat packages/contracts-rfq/contracts/FastBridge.sol packages/contracts-rfq/contracts/FastBridgeV2.sol
# Check if FastBridgeV2 is meant to replace FastBridge
fd -e sol -E "test/*" -E "legacy/*" --exec grep -l "FastBridge"
Length of output: 41826
docs/bridge/blog-posts/2024-12-12-fastbridgev2-post.md (1)
99-106: Review timestamp field size for future compatibility.
The proofBlockTimestamp uses uint40 which will overflow in year 2088 (2^40 seconds from Unix epoch). Consider if uint48 would be more appropriate for longer-term compatibility, while still maintaining the single storage slot.
packages/synapse-interface/components/_Transaction/_Transactions.tsx (2)
10-10: LGTM! Import is properly placed and utilized.
The HYPERLIQUID constant import is correctly positioned with other chain-related imports and is effectively used within the component.
65-69: Verify the automatic completion of Hyperliquid transactions
The code automatically marks all Hyperliquid transactions as 'completed', regardless of their actual status. This could potentially hide transaction failures from users.
Consider adding:
- Error handling for undefined chain IDs
- Actual status verification for Hyperliquid transactions
Let's verify if this is the intended behavior:
✅ Verification successful
Automatic completion of Hyperliquid transactions is intentional and correct
The code behavior is intentionally designed this way because:
- Hyperliquid transactions are handled through a direct USDC transfer to a dedicated deposit address (
0x2Df1c51E09aECF9cacB7bc98cB1742757f163dF7) - The codebase has special handling for Hyperliquid throughout:
- Uses a minimum deposit requirement (
HYPERLIQUID_MINIMUM_DEPOSIT) - Has a dedicated
HyperliquidDepositButtoncomponent - Skips explorer links and status tracking for Hyperliquid transactions
- Uses a minimum deposit requirement (
- The transaction flow is different from regular bridge transactions - it's a simple deposit rather than a cross-chain transfer that needs status tracking
The status being marked as 'completed' is correct as the transaction is considered final once the USDC transfer is confirmed on Arbitrum.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any error handling or status verification for Hyperliquid transactions
rg -A 5 "HYPERLIQUID.*status|status.*HYPERLIQUID"
# Check if there's any documentation explaining this behavior
fd -e md -e txt . | xargs rg -l "hyperliquid.*status|status.*hyperliquid" -i
# Look for related test cases
fd -e test.tsx -e spec.tsx -e test.ts -e spec.ts . | xargs rg -l "HYPERLIQUID.*status|status.*HYPERLIQUID"
Length of output: 374
Script:
#!/bin/bash
# Let's try a broader search for Hyperliquid-related transaction handling
rg -A 10 "HYPERLIQUID"
# Search for status-related code in the transaction component
ast-grep --pattern 'status = $_' packages/synapse-interface/components/_Transaction/_Transactions.tsx
# Look for any transaction status handling patterns
ast-grep --pattern 'tx.status'
# Check for any transaction processing logic for Hyperliquid
rg -A 5 "destinationChain.*HYPERLIQUID|HYPERLIQUID.*destinationChain"
Length of output: 42786
packages/contracts-rfq/contracts/interfaces/IAdminV2.sol (4)
6-7: Events are correctly defined and appropriately named
The new events for deploy block setting, dispute penalty updates, and prover management are well-defined and follow the established naming conventions.
Also applies to: 11-14
15-20: Prover management functions are properly declared
The addition of addProver and removeProver functions enhances the interface for prover management. The function signatures and documentation are appropriate.
25-34: Administrative functions are well-implemented
The setDeployBlock and setDisputePenaltyTime functions are correctly declared with clear documentation, allowing for proper administrative control.
43-58: Getter functions provide necessary prover information
The inclusion of getActiveProverID, getProverInfo, getProverInfoByID, and getProvers functions offers comprehensive access to prover details, enhancing functionality.
packages/contracts-rfq/contracts/interfaces/ISynapseIntentRouter.sol (1)
1-66: New interface ISynapseIntentRouter is well-defined
The interface introduces necessary structures and functions for handling intent completion with Zap steps. The StepParams struct and the functions completeIntentWithBalanceChecks and completeIntent are appropriately declared with clear documentation.
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (2)
18-19: Update to BridgeTxDetails struct is appropriate
The addition of proverID and adjustment of proofBlockTimestamp in BridgeTxDetails enhance the struct's functionality. The chosen data types are suitable for their intended purposes.
Line range hint 38-94: V2 structs and functions are well-defined
The introduction of BridgeTransactionV2 and the new V2 functions appropriately extend the interface to support enhanced bridging capabilities with additional parameters and improved functionality.
packages/contracts-rfq/contracts/libs/ZapDataV1.sol (1)
51-58: LGTM!
The encodeV1 function correctly incorporates the new parameters finalToken_ and forwardTo_, enhancing the functionality of the Zap data encoding.
packages/contracts-rfq/test/router/SynapseIntentRouter.BalanceChecks.t.sol (1)
7-258: Comprehensive balance check tests added
The new test cases provide thorough coverage for scenarios involving unspent funds and ensure that the SynapseIntentRouter behaves correctly under various conditions. This strengthens the reliability of the contract.
packages/contracts-rfq/contracts/AdminV2.sol (9)
21-30: ProverInfo struct is appropriately defined for prover management
The ProverInfo struct effectively encapsulates prover identification and activation information using uint16 for id and uint240 for activeFromTimestamp, accommodating a sufficient range for provers and timestamps.
60-64: Constants for dispute penalty time are well-established
The inclusion of MIN_DISPUTE_PENALTY_TIME and DEFAULT_DISPUTE_PENALTY_TIME constants provides flexibility and security in configuring dispute penalties, ensuring that the values remain within acceptable bounds.
74-79: Deploy block variable introduces necessary flexibility for certain chains
The addition of the deployBlock variable addresses the variability in block.number implementations across different chains like Arbitrum. Making it mutable allows for accurate tracking and interaction with off-chain indexers.
95-97: Constructor initializes new state variables correctly
The constructor appropriately initializes the deployBlock and disputePenaltyTime using the new setter functions, ensuring that the contract starts with the correct configuration.
99-116: Prover addition logic is sound and secure
The addProver function correctly manages adding new provers, including checks for existing active provers, capacity limits, and proper initialization of prover information. The use of events enhances transparency.
118-124: Prover removal process maintains integrity of the prover list
The removeProver function effectively deactivates provers while preserving their IDs, ensuring historical data remains consistent. The function includes necessary checks and state updates.
132-135: Protected setter for deploy block enhances administrative control
The setDeployBlock function allows administrators to update the deployBlock securely, with role-based access control preventing unauthorized changes.
137-140: Dispute penalty time configuration is properly managed
The setDisputePenaltyTime function provides governors with the ability to adjust the penalty time, with safeguards to prevent values below the minimum threshold.
166-185: Function getProvers() effectively retrieves active provers
The getProvers() function accurately compiles a list of active provers by iterating over the _allProvers array. The implementation is appropriate given the expected number of provers, and the function's view modifier.
packages/synapse-interface/pages/state-managed-bridge/index.tsx (1)
438-438: Verify correct usage of switchChain function with wagmiConfig
At line 438, switchChain is called with wagmiConfig and an object containing { chainId: ARBITRUM.id }. As per retrieved learnings, this is acceptable, but ensure that this usage aligns with the version of @wagmi/core in use. In newer versions, switchChain might be accessed through hooks or might not require wagmiConfig as a parameter.
packages/contracts-rfq/contracts/FastBridgeV2.sol (5)
58-58: Removal of 'deployBlock' variable and its initialization is acceptable
The deployBlock variable and its initialization in the constructor have been removed as it is no longer needed. This streamlines the contract by eliminating unused code.
118-125: Ensure correct application of prover penalty in dispute function
The addition of _applyDisputePenaltyTime(proverID) applies a penalty to the prover after a dispute. Verify that this function correctly updates the prover's status and that all edge cases, such as already removed provers, are properly handled.
352-354: Addition of active prover check in proveV2 enhances security
By retrieving the proverID using getActiveProverID(msg.sender) and ensuring the prover is active, the contract now prevents unauthorized provers from submitting proofs.
362-363: Storing proverID and updating proofBlockTimestamp improves tracking
The addition of storing proverID and updating proofBlockTimestamp with uint40 enhances traceability and aligns with the new timestamp handling.
480-487: Verify correctness of uint40 timestamp usage in _timeSince function
The use of uint40 for storing timestamps reduces gas costs but may introduce risks of overflow after the uint40 maximum value is reached. Ensure that _timeSince correctly handles timestamp rollover and that no underflow or overflow errors can occur.
packages/contracts-rfq/test/FastBridgeV2.Src.t.sol (3)
35-36: Addition of DisputePenaltyTimeApplied event enhances test logging
The new event DisputePenaltyTimeApplied improves observability in tests related to dispute handling.
97-102: New function expectDisputePenaltyTimeApplied improves test clarity
The addition of expectDisputePenaltyTimeApplied aids in asserting the emission of the DisputePenaltyTimeApplied event during tests, enhancing test readability and reliability.
299-301: Update checkStatusAndProofAfterProve function signature for prover ID verification
The modification to include expectedProverID in checkStatusAndProofAfterProve allows for more precise assertions in tests, improving test accuracy.
packages/contracts-rfq/test/mocks/SwapQuoterMock.sol (1)
1-20: LGTM! Well-structured mock implementation.
The mock contract follows good practices with proper interface implementation and coverage exclusion pattern.
packages/contracts-rfq/test/FastBridgeV2.Src.RefundV1.t.sol (1)
6-9: LGTM! Consistent test utility implementation.
The changes follow established patterns for test utilities with proper coverage exclusion.
packages/contracts-rfq/test/integration/FastBridge.MulticallTarget.t.sol (1)
10-11: LGTM! Coverage exclusion is appropriate.
The empty test function with documentation clearly indicates its purpose for coverage exclusion, which is a common and acceptable practice for test utility contracts.
packages/contracts-rfq/script/DeploySIR.s.sol (1)
17-19: Verify constructor arguments for deployed contracts.
The deployment calls use empty constructor arguments (""). This could lead to deployment failures if these contracts require initialization parameters.
✅ Verification successful
Empty constructor arguments are correct for these contracts
After reviewing the contract implementations, none of the contracts (SynapseIntentRouter, SynapseIntentPreviewer, and TokenZapV1) have explicit constructors that require initialization parameters. The contracts are designed to be stateless and don't need any initialization values during deployment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if contracts require constructor arguments
for contract in "SynapseIntentRouter" "SynapseIntentPreviewer" "TokenZapV1"; do
ast-grep --pattern $'contract $contract {
constructor($$$) {
$$$
}
}'
done
Length of output: 494
Script:
#!/bin/bash
# Search for contract definitions and their constructors
rg -U "contract (SynapseIntentRouter|SynapseIntentPreviewer|TokenZapV1).*?\{.*?constructor.*?\{.*?\}" --multiline
Length of output: 115
Script:
#!/bin/bash
# First, find the contract files
fd -e sol | xargs rg -l "contract (SynapseIntentRouter|SynapseIntentPreviewer|TokenZapV1)"
# Then search for contract definitions with their content
rg -A 10 "^[^/]*contract (SynapseIntentRouter|SynapseIntentPreviewer|TokenZapV1)"
Length of output: 10885
Script:
#!/bin/bash
# Let's check the actual contract files for constructors
cat packages/contracts-rfq/contracts/router/SynapseIntentRouter.sol packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
Length of output: 32088
packages/contracts-rfq/contracts/interfaces/ISynapseIntentPreviewer.sol (2)
7-18: Well-documented interface design
Excellent NatSpec documentation that clearly explains:
- Function behavior for edge cases (no-op scenarios)
- Parameter purposes and constraints
- Return value meanings
19-29: Clean interface design
The function signature is well-designed with:
- Appropriate view modifier for gas efficiency
- Clear parameter naming
- Proper return types
packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol (1)
36-42: Well-implemented new accessor functions
The new finalToken and forwardTo functions are correctly implemented as pure functions that delegate to the underlying library.
cspell.json (1)
40-40: LGTM! New words added correctly
The new words "fastbridge" and "hyperliquid" are correctly added in alphabetical order and align with the new Hyperliquid bridge feature being implemented.
Also applies to: 46-46
packages/contracts-rfq/deployments/optimism/SynapseIntentPreviewer.json (1)
1-7: Verify contract deployment on Optimism
The contract is deployed at 0x9519E8D136d0a89d7e10D1a66C97249E0135544B on Optimism. Let's verify the deployment.
✅ Verification successful
Contract deployment on Optimism is verified and successful
The verification results show:
- Contract is deployed and has bytecode at
0x9519E8D136d0a89d7e10D1a66C97249E0135544B - Transaction
0x928a7db8741fb992934302f73e076f7630075151384529b538cb133e797c4bacwas successful (status: 0x1) - Deployment was confirmed in block 129029951 (0x7b0d73f)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract deployment and transaction details
# Check if the contract address has code
curl -s -X POST https://optimism-mainnet.public.blastapi.io \
-H "Content-Type: application/json" \
-d '{
"jsonrpc":"2.0",
"method":"eth_getCode",
"params":["0x9519E8D136d0a89d7e10D1a66C97249E0135544B", "latest"],
"id":1
}'
# Verify transaction receipt
curl -s -X POST https://optimism-mainnet.public.blastapi.io \
-H "Content-Type: application/json" \
-d '{
"jsonrpc":"2.0",
"method":"eth_getTransactionReceipt",
"params":["0x928a7db8741fb992934302f73e076f7630075151384529b538cb133e797c4bac"],
"id":1
}'
Length of output: 16984
packages/synapse-interface/components/StateManagedBridge/OutputContainer.tsx (2)
19-19: LGTM! State management improvements
Good use of destructuring to access required state values and importing specific chain constants.
Also applies to: 30-30
48-51: Verify UX impact of hiding destination address for Hyperliquid
Please confirm that Hyperliquid transactions don't require a destination address. This is a significant UX change that affects user input requirements.
✅ Verification successful
Hiding destination address for Hyperliquid is correct and intentional
The codebase shows that Hyperliquid bridge transactions are handled differently by design:
- Transactions to Hyperliquid are always routed through Arbitrum using a fixed deposit address (
0x2Df1c51E09aECF9cacB7bc98cB1742757f163dF7) - The UI is specifically designed to hide the destination address input for Hyperliquid as users can only deposit to this predefined contract
- There's a minimum deposit requirement of 5 USDC for Hyperliquid transactions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related Hyperliquid address handling
# Search for Hyperliquid address handling in the codebase
rg -l "HYPERLIQUID.*address" --type ts --type tsx
# Check for any tests covering this scenario
rg -l "test.*Hyperliquid.*address" --type ts --type tsx
Length of output: 165
Script:
#!/bin/bash
# Search for Hyperliquid address handling patterns with correct file extensions
rg -l "HYPERLIQUID.*address"
# Look for any Hyperliquid-related transaction or bridge logic
rg -l "HYPERLIQUID.*transaction|HYPERLIQUID.*bridge"
# Search for destination address handling patterns
ast-grep --pattern 'DestinationAddressInput'
Length of output: 583
Script:
#!/bin/bash
# Check the implementation of DestinationAddressInput component
cat packages/synapse-interface/components/StateManagedBridge/DestinationAddressInput.tsx
# Look for Hyperliquid-specific configurations or constants
rg "HYPERLIQUID" -A 5 -B 5
# Search for bridge transaction handling logic
ast-grep --pattern 'function $FUNC($$$) {
$$$
HYPERLIQUID
$$$
}'
Length of output: 53209
packages/synapse-interface/components/StateManagedBridge/hooks/useBridgeValidations.ts (2)
11-11: LGTM: New chain constants imported
The addition of ARBITRUM and HYPERLIQUID constants is consistent with the bridge functionality enhancement.
70-70: Verify the chain mapping implementation
The code maps HYPERLIQUID.id to ARBITRUM.id in the bridge state, suggesting that HYPERLIQUID transactions are processed through Arbitrum. This is a critical path that requires verification.
Consider documenting this chain mapping behavior in the codebase to make it explicit for other developers.
✅ Verification successful
Based on the search results, I can now provide a definitive response about the HYPERLIQUID to ARBITRUM chain mapping implementation.
Chain mapping implementation verified
The code correctly maps HYPERLIQUID transactions to be processed through ARBITRUM. This is a deliberate and consistent implementation across the codebase:
- The mapping is consistently applied in multiple components:
- Bridge quote fetching:
toChainId === HYPERLIQUID.id ? ARBITRUM.id : toChainId - Transaction payload generation
- Analytics events
- Output amount display logic
- Bridge quote fetching:
The implementation is part of a specialized Hyperliquid deposit flow, with dedicated components like HyperliquidTransactionButton and HyperliquidDepositInfo handling the specific requirements of this route.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of HYPERLIQUID to ARBITRUM mapping
rg -l "HYPERLIQUID.*ARBITRUM" --type ts
# Search for any bridge-related configurations or constants
ast-grep --pattern 'const $_ = {
$$$
HYPERLIQUID: $_,
ARBITRUM: $_,
$$$
}'
Length of output: 277
Script:
#!/bin/bash
# Let's check the actual content of these files to understand the mapping implementation
rg -A 5 -B 5 "HYPERLIQUID.*ARBITRUM|ARBITRUM.*HYPERLIQUID" --type ts
# Look for chain configuration definitions
ast-grep --pattern 'export const HYPERLIQUID = $_'
ast-grep --pattern 'export const ARBITRUM = $_'
# Search for any bridge-related documentation
fd README.md | xargs rg -i "hyperliquid|arbitrum|bridge"
Length of output: 41161
packages/contracts-rfq/deployments/optimism/TokenZapV1.json (1)
2-7: Verify the contract deployment transaction
The contract is deployed at 0x6327797F149a75D506aFda46D5fCE6E74fC409D5 on Optimism. Ensure this deployment is tracked in deployment history.
packages/contracts-rfq/deployments/arbitrum/TokenZapV1.json (1)
2-7: Verify cross-chain deployment consistency
The contract is deployed at the same address on both Arbitrum and Optimism (0x6327797F149a75D506aFda46D5fCE6E74fC409D5), suggesting deterministic deployment. This is good for cross-chain consistency.
✅ Verification successful
Cross-chain deployment consistency confirmed
The TokenZapV1 contract is indeed deployed with identical:
- Contract address:
0x6327797F149a75D506aFda46D5fCE6E74fC409D5 - Constructor arguments:
0x - Contract ABI and error handling
This confirms deterministic deployment across both Arbitrum and Optimism chains, ensuring consistent behavior and interface for cross-chain operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent deployment across chains
fd -e json -x jq -r '. | select(.address == "0x6327797F149a75D506aFda46D5fCE6E74fC409D5") | input_filename' .
Length of output: 14664
Script:
#!/bin/bash
# Let's try a more specific approach to find and compare the TokenZapV1 deployments
echo "=== Arbitrum TokenZapV1 ==="
cat packages/contracts-rfq/deployments/arbitrum/TokenZapV1.json
echo -e "\n=== Optimism TokenZapV1 ==="
cat packages/contracts-rfq/deployments/optimism/TokenZapV1.json
Length of output: 8616
packages/contracts-rfq/deployments/optimism/SynapseIntentRouter.json (2)
133-209: LGTM! Comprehensive error handling implementation.
The contract implements thorough error handling covering various scenarios:
- Address and balance validations
- Token contract checks
- Execution flow validations
- Return value validations
2-2: Verify the contract address reuse across chains.
The same contract address (0x57203c65DeA2ded4EE4E303a9494bee04df030BF) is used on both Optimism and Arbitrum networks. While this could be intentional through deterministic deployment, it's worth verifying.
✅ Verification successful
Contract addresses are intentionally deterministic using CREATE2 deployment
The same contract address appearing on different chains is expected and intentional. Both SynapseIntentRouter and FastBridge are deployed using CREATE2 factory pattern for deterministic addresses across chains, as evidenced by:
- DeploySIR.sol using
deployCodeFunc: cbDeployCreate2 - DeployFastBridgeCREATE2.sol explicitly named and using the same CREATE2 deployment pattern
This is a common practice for cross-chain protocols to maintain consistent addresses across different networks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract deployment consistency across chains
# Check if the same contract address is used in other chain deployments
fd -e json -x grep -l "0x57203c65DeA2ded4EE4E303a9494bee04df030BF" {} \;
Length of output: 217
Script:
#!/bin/bash
# Check if these are deterministic deployments by examining the deployment scripts
# and configuration for create2 or similar deterministic deployment patterns
# Search for deployment scripts or configuration
fd "deploy|hardhat.config" -x cat {} \;
# Search for create2 or deterministic deployment patterns
rg -i "create2|deterministic|deploy.*same.*address" -A 5
Length of output: 127702
Script:
#!/bin/bash
# Search for deterministic deployment patterns in the deployment scripts
rg -i "create2|create3|deterministic|salt" -A 5 "script/DeployFastBridge|script/DeploySIR"
# Search for deployment configuration files that might contain salt or deterministic deployment info
fd -e json "FastBridge|SynapseIntentRouter" -x cat {} \;
Length of output: 67141
Script:
#!/bin/bash
# Search for deployment scripts and configuration to understand if the contract addresses are meant to be deterministic
rg -i "create2|create3|deterministic|salt" "script/.*Deploy.*"
# Look for deployment scripts specifically for SynapseIntentRouter and FastBridge
fd -e sol "Deploy(SIR|FastBridge)" -x cat {} \;
Length of output: 3615
packages/contracts-rfq/deployments/arbitrum/SynapseIntentRouter.json (1)
1-211: Identical to Optimism deployment.
The contract deployment matches the Optimism deployment in terms of ABI and functionality, with only deployment-specific details (transaction hash, block number) being different.
packages/synapse-interface/slices/bridgeQuote/thunks.ts (1)
13-13: Verify Hyperliquid integration impact.
The addition of HYPERLIQUID check affects the quote validation logic. Ensure this change is properly tested and documented.
packages/contracts-rfq/test/integration/TokenZapV1.t.sol (3)
14-14: LGTM! Common pattern for excluding abstract test contracts.
The empty test function is a well-known pattern to prevent abstract test contracts from appearing in coverage reports.
Also applies to: 46-47
90-92: LGTM! Consistent implementation of new ZapData fields.
The new fields finalToken and forwardTo are consistently added across all test cases, ensuring proper test coverage.
Also applies to: 101-103, 109-111, 117-119, 124-126
51-51: Verify consistent usage of the new role management system.
The change from grantRole to addProver reflects an update in the role management system.
✅ Verification successful
Role management system has been consistently updated across the codebase
The verification confirms that:
- No instances of the old
grantRolewithPROVER_ROLEwere found - The new
addProvermethod is consistently used across test files and contracts - The implementation is properly defined in
AdminV2.soland its interfaceIAdminV2.sol
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining uses of the old role management system
rg -A 2 "grantRole.*PROVER_ROLE"
# Verify consistent usage of the new system
rg -A 2 "addProver"
Length of output: 6592
packages/synapse-interface/components/_Transaction/_Transaction.tsx (2)
23-23: LGTM! Required import for chain-specific logic.
The HYPERLIQUID constant import supports the new conditional rendering logic.
189-197: LGTM! Appropriate chain-specific UX handling.
The conditional rendering correctly handles the special case for Hyperliquid chain while maintaining existing behavior for other chains.
packages/contracts-rfq/test/libs/ZapDataV1.t.sol (3)
21-22: LGTM! Consistent with ZapDataV1 library changes.
The function signature and implementation correctly handle the new ZapData fields.
Also applies to: 30-30
34-35: LGTM! Comprehensive test coverage for new fields.
The test properly validates the encoding and decoding of the new ZapData fields.
Also applies to: 53-53, 57-58, 62-64
67-90: LGTM! Consistent test coverage across all scenarios.
The test suite maintains comprehensive coverage with the new fields consistently handled across all test cases.
Also applies to: 95-95, 99-100, 114-115, 118-118, 122-123, 132-133, 146-147, 155-156, 166-167, 174-175
packages/synapse-interface/components/StateManagedBridge/BridgeTransactionButton.tsx (2)
90-91: LGTM! Button disable conditions are properly updated
The new validation checks for destination address and minimum deposit are correctly integrated into the existing conditions.
151-155: LGTM! Clear user feedback for minimum deposit requirement
The button property correctly displays the minimum deposit requirement when the condition is not met.
packages/synapse-interface/components/layouts/LandingPageWrapper/index.tsx (1)
62-65: LGTM! Verify banner display timing
The announcement banner update is properly implemented. Consider verifying that the banner displays correctly within the specified date range.
✅ Verification successful
Banner implementation verified - no conflicts found
The Hyperliquid announcement banner is properly implemented and there are no conflicting banners with overlapping IDs. The search revealed:
- The Hyperliquid banner is unique with ID "2024-12-11-hyperliquid"
- Other existing banners have different purposes and IDs:
- Maintenance banner (dynamic ID)
- Ecotone fork banner (ID: "03142024-ecotone-fork")
The banner's display timing (Dec 11, 2024 - Jan 10, 2025) doesn't overlap with other announcement periods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any other announcement banners that might conflict
rg -A 2 'bannerId.*hyperliquid' --type ts --type tsx
Length of output: 82
Script:
#!/bin/bash
# Search for any banner IDs containing 'hyperliquid' in TypeScript files
rg -A 2 'bannerId.*hyperliquid' --type typescript
# Also check for any other active banners in the codebase
rg -A 3 'bannerId.*=.*"' --type typescript
Length of output: 166
Script:
#!/bin/bash
# Search for any banner IDs in all files
rg -A 3 'bannerId.*='
# Also check for any Banner component usage
fd -e ts -e tsx -e js -e jsx . -x cat {} | rg -A 5 '<.*Banner'
Length of output: 3350
docs/bridge/CHANGELOG.md (1)
6-11: LGTM! Changelog entry follows the conventional format.
The version bump to 0.6.0 is appropriate for adding new features (Hyperliquid bridge & deposit support). The entry includes proper references to PR #3461 and the commit hash.
🧰 Tools
🪛 Markdownlint (0.37.0)
9-9: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time
(MD001, heading-increment)
packages/contracts-rfq/test/FastBridgeV2.t.sol (1)
12-12: Verify the integration of admin error handling.
The addition of IAdminV2Errors interface suggests new admin-related error handling capabilities. Let's verify the usage of these errors in the test cases.
Also applies to: 22-22
✅ Verification successful
Admin error handling is properly integrated and tested
The admin error handling is correctly integrated and thoroughly tested in the FastBridgeV2.Management.t.sol test file. The test suite includes specific test cases for all admin errors:
test_addProver_revertAlreadyActive: Tests ProverAlreadyActive errortest_addProver_revertTooManyProvers: Tests ProverCapacityExceeded errortest_removeProver_revertNotActive: Tests ProverNotActive errortest_setCancelDelay_revertBelowMin: Tests CancelDelayBelowMin errortest_setDisputePenaltyTime_revertBelowMin: Tests DisputePenaltyTimeBelowMin errortest_setProtocolFeeRate_revert_tooHigh: Tests FeeRateAboveMax error
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of admin error handling in test cases
# Search for admin error assertions in test files
rg -A 5 'expectRevert.*Admin' packages/contracts-rfq/test/
Length of output: 60
Script:
#!/bin/bash
# Let's first check if the IAdminV2Errors interface is actually used in the test file
rg -l "IAdminV2Errors" packages/contracts-rfq/test/FastBridgeV2.t.sol
# Then let's check the content of the test file to understand the context
cat packages/contracts-rfq/test/FastBridgeV2.t.sol
# Also check the interface definition
cat packages/contracts-rfq/contracts/interfaces/IAdminV2Errors.sol
Length of output: 11494
Script:
#!/bin/bash
# Let's check for test files that inherit from FastBridgeV2Test to see where admin errors might be used
fd -e sol . packages/contracts-rfq/test/ --exec rg -l "contract.*is.*FastBridgeV2Test"
# Then let's check for any usage of the admin error constants in these files
rg -l "CancelDelayBelowMin|FeeRateAboveMax|ProverAlreadyActive|ProverCapacityExceeded|ProverNotActive|DisputePenaltyTimeBelowMin" packages/contracts-rfq/test/
Length of output: 584
Script:
#!/bin/bash
# Let's specifically check the Management test file since it's likely to contain admin-related tests
cat packages/contracts-rfq/test/FastBridgeV2.Management.t.sol
# Also check if there are any admin-related functions in the main contract
rg -A 5 "function.*admin" packages/contracts-rfq/contracts/FastBridgeV2.sol
Length of output: 15085
packages/synapse-interface/messages/zh-CN.json (1)
21-22: LGTM! Translations are accurate and consistent.
The new deposit-related translations maintain consistency with existing entries and correctly preserve the {symbol} placeholder.
packages/synapse-interface/messages/jp.json (1)
21-22: LGTM! Translations are accurate and consistent.
The new Japanese translations for deposit functionality maintain proper grammar and formatting.
packages/synapse-interface/constants/chains/master.tsx (1)
14-14: LGTM! Image import is correctly placed.
The Hyperliquid image import follows the established pattern of chain image imports.
packages/synapse-interface/messages/ar.json (1)
21-22: LGTM! Arabic translations are accurate and maintain RTL formatting.
The new Arabic translations for deposit functionality are grammatically correct and maintain proper RTL (Right-to-Left) text direction.
packages/synapse-interface/messages/en-US.json (1)
21-22: LGTM! New deposit-related translations added.
The new entries follow the existing pattern for action strings and maintain consistency with other similar entries in the Bridge section.
packages/synapse-interface/messages/tr.json (1)
21-22: LGTM! Turkish translations are accurate and natural.
The translations maintain the correct placeholder usage and provide clear, contextually appropriate Turkish equivalents.
packages/synapse-interface/messages/es.json (1)
21-22: LGTM! Spanish translations are accurate and consistent.
The translations maintain proper Spanish grammar and are consistent with the existing terminology in the file.
packages/synapse-interface/messages/fr.json (1)
21-22: LGTM! French translations for deposit functionality.
The translations are concise and maintain consistency with the existing translations. The placeholder {symbol} is correctly preserved.
packages/contracts-rfq/test/zaps/TokenZapV1.t.sol (4)
57-63: Improved function naming for better clarity.
The functions have been renamed to be more descriptive:
getZapData→getZapDataDepositgetZapDataNoAmount→getZapDataDepositNoAmount
181-218: Well-structured helper functions for wrap/unwrap operations.
New helper functions added with clear naming and consistent parameter handling:
getZapDataUnwrapgetZapDataUnwrapAndForwardgetZapDataWrapgetZapDataWrapAndForward
221-444: Comprehensive test coverage for wrap/unwrap operations.
New test cases cover various scenarios:
- Basic unwrap and deposit
- Unwrap with forwarding to EOA/Contract
- Extra funds handling
- Error cases
Line range hint 467-680: Updated encoding tests with proper error handling.
The encoding tests have been updated to:
- Use new function names consistently
- Handle zero address cases
- Verify token and target validation
packages/synapse-interface/CHANGELOG.md (1)
6-24: LGTM! Well-formatted changelog entry.
The changelog entry follows the conventional commit format and properly documents the new Hyperliquid bridge & deposit support feature with appropriate version bump and links.
🧰 Tools
🪛 Markdownlint (0.37.0)
9-9: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time
(MD001, heading-increment)
| address public immutable token0; | ||
| address public immutable token1; | ||
|
|
||
| uint256 public ratioWei = 1e18; |
There was a problem hiding this comment.
Add zero address validation in constructor.
The immutable token addresses should be validated to prevent deployment with zero addresses.
constructor(address token0_, address token1_) {
+ if (token0_ == address(0) || token1_ == address(0)) revert("PoolMock: zero address");
token0 = token0_;
token1 = token1_;
}Also applies to: 18-21
| function setRatioWei(uint256 ratioWei_) external { | ||
| ratioWei = ratioWei_; | ||
| } |
There was a problem hiding this comment.
Add ratio validation to prevent division by zero.
The setRatioWei function should validate that the ratio is not zero to prevent potential division by zero in the swap function.
function setRatioWei(uint256 ratioWei_) external {
+ if (ratioWei_ == 0) revert("PoolMock: zero ratio");
ratioWei = ratioWei_;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function setRatioWei(uint256 ratioWei_) external { | |
| ratioWei = ratioWei_; | |
| } | |
| function setRatioWei(uint256 ratioWei_) external { | |
| if (ratioWei_ == 0) revert("PoolMock: zero ratio"); | |
| ratioWei = ratioWei_; | |
| } |
| export const RFQFlow = () => { | ||
| return ( | ||
| <svg | ||
| width="100%" | ||
| viewBox="-240 0 480 224" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| className="flowAnimation" | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes and TypeScript types.
Consider enhancing the SVG accessibility and type safety:
-export const RFQFlow = () => {
+import { FC } from 'react'
+
+export const RFQFlow: FC = () => {
return (
<svg
width="100%"
viewBox="-240 0 480 224"
xmlns="http://www.w3.org/2000/svg"
className="flowAnimation"
+ role="img"
+ aria-label="RFQ Flow Diagram"
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const RFQFlow = () => { | |
| return ( | |
| <svg | |
| width="100%" | |
| viewBox="-240 0 480 224" | |
| xmlns="http://www.w3.org/2000/svg" | |
| className="flowAnimation" | |
| > | |
| import { FC } from 'react' | |
| export const RFQFlow: FC = () => { | |
| return ( | |
| <svg | |
| width="100%" | |
| viewBox="-240 0 480 224" | |
| xmlns="http://www.w3.org/2000/svg" | |
| className="flowAnimation" | |
| role="img" | |
| aria-label="RFQ Flow Diagram" | |
| > |
| <line | ||
| x1="-50%" | ||
| y1="100%" | ||
| x2="-50%" | ||
| y2="100%" | ||
| stroke="currentcolor" | ||
| strokeWidth="4" | ||
| strokeOpacity=".25" | ||
| > | ||
| <set attributeName="x1" to="-50%" begin="rfqFlowTimer.begin" /> | ||
| <set attributeName="x2" to="-50%" begin="rfqFlowTimer.begin" /> | ||
| <animate | ||
| attributeName="x2" | ||
| values="-50%; 50%" | ||
| begin="rfqFlowSend.begin" | ||
| dur="1.85s" | ||
| calcMode="linear" | ||
| keyTimes="0; 1" | ||
| keySplines=".5 0 1 1" | ||
| fill="freeze" | ||
| /> | ||
| <animate | ||
| id="rfqFlowTimerOut" | ||
| attributeName="x1" | ||
| values="-50%; 50%" | ||
| begin="rfqFlowRepay.end + 1s" | ||
| dur=".75s" | ||
| calcMode="spline" | ||
| keyTimes="0; 1" | ||
| keySplines=".5 0 .2 1" | ||
| fill="freeze" | ||
| /> | ||
| </line> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add support for reduced motion preferences.
Consider respecting the user's motion preferences to improve accessibility.
+ <style>
+ @media (prefers-reduced-motion: reduce) {
+ .flowAnimation line {
+ animation-duration: 0.01s;
+ transition-duration: 0.01s;
+ }
+ }
+ </style>Committable suggestion skipped: line range outside the PR's diff.
| interface IZapRecipient { | ||
| function zap( | ||
| address token, | ||
| uint256 amount, | ||
| bytes memory zapData | ||
| ) external payable returns (bytes4); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document IZapRecipient interface requirements.
The interface lacks crucial documentation:
- Expected return value (bytes4)
- Format and structure of zapData
- Requirements for token validation
- Revert conditions
This documentation is essential for developers implementing the interface correctly.
| function _performZap(address zapRecipient, uint256 msgValue, bytes memory zapRecipientCallData) internal { | ||
| // Perform the low-level call to ZapRecipient, bubbling up any revert reason. | ||
| bytes memory returnData = | ||
| Address.functionCallWithValue({target: zapRecipient, data: zapRecipientCallData, value: msgValue}); | ||
|
|
||
| // Explicit revert if no return data at all. | ||
| if (returnData.length == 0) revert SIR__ZapNoReturnValue(); | ||
| // Check that exactly a single return value was returned. | ||
| if (returnData.length != 32) revert SIR__ZapIncorrectReturnValue(); | ||
| // Return value should be abi-encoded hook function selector. | ||
| if (bytes32(returnData) != bytes32(IZapRecipient.zap.selector)) { | ||
| revert SIR__ZapIncorrectReturnValue(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding reentrancy protection to prevent potential attacks
The function _performZap makes external calls using Address.functionCallWithValue, which could introduce reentrancy risks. Consider adding reentrancy protection, such as using the nonReentrant modifier from OpenZeppelin's ReentrancyGuard, to secure the contract against potential reentrancy attacks.
| for (uint256 i = 0; i < type(uint16).max; i++) { | ||
| addProver(admin, address(uint160(i))); | ||
| } | ||
| vm.expectRevert(ProverCapacityExceeded.selector); | ||
| addProver(admin, proverA); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize test test_addProver_revertTooManyProvers to reduce execution time
The test adds provers in a loop until it reaches uint16.max, which can significantly increase test execution time. Consider mocking the _allProvers array length or directly setting the proverID to simulate the capacity being exceeded, thus efficiently triggering the ProverCapacityExceeded revert without extensive iterations.
Apply this diff to optimize the test:
-function test_addProver_revertTooManyProvers() public {
- for (uint256 i = 0; i < type(uint16).max; i++) {
- addProver(admin, address(uint160(i)));
- }
+function test_addProver_revertTooManyProvers_mocked() public {
+ // Mock the _allProvers length to uint16.max
+ vm.store(
+ address(fastBridge),
+ keccak256(abi.encode(uint256(84), uint256(0))),
+ bytes32(uint256(type(uint16).max))
+ );
vm.expectRevert(ProverCapacityExceeded.selector);
addProver(admin, proverA);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (uint256 i = 0; i < type(uint16).max; i++) { | |
| addProver(admin, address(uint160(i))); | |
| } | |
| vm.expectRevert(ProverCapacityExceeded.selector); | |
| addProver(admin, proverA); | |
| function test_addProver_revertTooManyProvers_mocked() public { | |
| // Mock the _allProvers length to uint16.max | |
| vm.store( | |
| address(fastBridge), | |
| keccak256(abi.encode(uint256(84), uint256(0))), | |
| bytes32(uint256(type(uint16).max)) | |
| ); | |
| vm.expectRevert(ProverCapacityExceeded.selector); | |
| addProver(admin, proverA); | |
| } |
| function testDeploySIR() external {} | ||
|
|
||
| function run() external broadcastWithHooks { | ||
| // TODO: create2 salts |
There was a problem hiding this comment.
TODO comment needs implementation.
The create2 salts implementation is missing. This could affect deterministic deployment addresses which may be required for cross-chain verification.
Would you like help implementing the create2 salt generation logic?
| function mint(address to, uint256 amount) external { | ||
| uint256 newBalance = address(this).balance + amount; | ||
| vm.deal(address(this), newBalance); | ||
| _mint(to, amount); | ||
| } |
There was a problem hiding this comment.
Add access control and SafeMath for mint function.
The mint function has several concerns:
- No access control - anyone can mint tokens
- Potential overflow in balance calculation
- Uses vm.deal which only works in test environment
Consider this safer implementation:
- function mint(address to, uint256 amount) external {
- uint256 newBalance = address(this).balance + amount;
- vm.deal(address(this), newBalance);
- _mint(to, amount);
- }
+ function mint(address to, uint256 amount) external {
+ require(msg.sender == owner, "WETHMock: only owner");
+ uint256 newBalance = address(this).balance + amount;
+ require(newBalance >= address(this).balance, "WETHMock: balance overflow");
+ vm.deal(address(this), newBalance);
+ _mint(to, amount);
+ }Committable suggestion skipped: line range outside the PR's diff.
| export const HYPERLIQUID: Chain = { | ||
| priorityRank: 99, | ||
| id: 998, // this is Hyperliquid Testnet from their docs | ||
| chainSymbol: 'HYPERLIQUID', | ||
| name: 'Hyperliquid', | ||
| chainImg: hyperliquidImg, | ||
| layer: 2, | ||
| blockTime: 300, | ||
| rpcUrls: { | ||
| primary: | ||
| 'https://arb-mainnet.g.alchemy.com/v2/7kjdkqKTh1zQ1mRYGi4nJJbxbyJXHkef', | ||
| fallback: 'https://arb1.arbitrum.io/rpc', | ||
| }, | ||
| nativeCurrency: { | ||
| name: 'Ethereum', | ||
| symbol: 'ETH', | ||
| decimals: 18, | ||
| address: zeroAddress, | ||
| icon: ethImg, | ||
| }, | ||
| explorerUrl: 'https://arbiscan.io', | ||
| explorerName: 'Arbiscan', | ||
| explorerImg: arbitrumExplorerImg, | ||
| color: 'gray', |
There was a problem hiding this comment.
Critical: Hyperliquid chain configuration appears to be using Arbitrum's values
Several issues with the Hyperliquid chain configuration:
- Using testnet chain ID (998) in production configuration
- RPC URLs are copied from Arbitrum configuration
- Explorer URLs and image are copied from Arbitrum configuration
This could cause serious issues in production as transactions would be sent to incorrect networks.
Please verify and update the following:
- Correct chain ID for Hyperliquid mainnet
- Correct RPC URLs for Hyperliquid
- Correct explorer URLs and image for Hyperliquid
export const HYPERLIQUID: Chain = {
priorityRank: 99,
- id: 998, // this is Hyperliquid Testnet from their docs
+ id: <mainnet_chain_id>,
chainSymbol: 'HYPERLIQUID',
name: 'Hyperliquid',
chainImg: hyperliquidImg,
layer: 2,
blockTime: 300,
rpcUrls: {
primary:
- 'https://arb-mainnet.g.alchemy.com/v2/7kjdkqKTh1zQ1mRYGi4nJJbxbyJXHkef',
- fallback: 'https://arb1.arbitrum.io/rpc',
+ '<hyperliquid_primary_rpc_url>',
+ fallback: '<hyperliquid_fallback_rpc_url>',
},
nativeCurrency: {
name: 'Ethereum',
symbol: 'ETH',
decimals: 18,
address: zeroAddress,
icon: ethImg,
},
- explorerUrl: 'https://arbiscan.io',
- explorerName: 'Arbiscan',
- explorerImg: arbitrumExplorerImg,
+ explorerUrl: '<hyperliquid_explorer_url>',
+ explorerName: '<hyperliquid_explorer_name>',
+ explorerImg: hyperliquidImg,
color: 'gray',
isNew: true,
}Committable suggestion skipped: line range outside the PR's diff.
Bundle ReportChanges will decrease total bundle size by 3.03MB (-8.51%) ⬇️. This is within the configured threshold ✅ Detailed changes
ℹ️ *Bundle size includes cached data from a previous commit |
null
ac2875e: docs preview link
ac2875e: synapse-interface preview link
Summary by CodeRabbit
Release Notes
New Features
Enhancements
Bug Fixes
Version Updates
@synapsecns/bridge-docsand@synapsecns/synapse-interface.