Merge swap tutorials in one #517
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request involves the deletion of the Changes
Possibly related PRs
Suggested reviewers
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/pages/developers/tutorials/swap-any.mdx(0 hunks)src/pages/developers/tutorials/swap.mdx(9 hunks)
💤 Files with no reviewable changes (1)
- src/pages/developers/tutorials/swap-any.mdx
🧰 Additional context used
🪛 LanguageTool
src/pages/developers/tutorials/swap.mdx
[uncategorized] ~349-~349: Loose punctuation mark.
Context: ... of information: - address target: The ZRC-20 address of the target token ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~350-~350: Loose punctuation mark.
Context: ...get token on ZetaChain. - bytes to: The recipient's address on the destinat...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~353-~353: Loose punctuation mark.
Context: ...ns like Bitcoin. - bool withdraw: Indicates whether to withdraw the swapp...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (3)
src/pages/developers/tutorials/swap.mdx (3)
83-88: LGTM: Well-implemented upgradeable pattern
The contract correctly implements the OpenZeppelin upgradeable pattern with proper initialization and ownership controls.
Also applies to: 114-124
Line range hint 264-336: LGTM: Robust withdrawal and revert handling
The implementation provides comprehensive error handling and recovery mechanisms for failed withdrawals.
346-420: LGTM: High-quality documentation
The documentation is comprehensive, well-structured, and provides clear examples for both testnet and localnet deployments.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~349-~349: Loose punctuation mark.
Context: ... of information: - address target: The ZRC-20 address of the target token ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~350-~350: Loose punctuation mark.
Context: ...get token on ZetaChain. - bytes to: The recipient's address on the destinat...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~353-~353: Loose punctuation mark.
Context: ...ns like Bitcoin. - bool withdraw: Indicates whether to withdraw the swapp...
(UNLIKELY_OPENING_PUNCTUATION)
| uint256 out = SwapHelperLib.swapExactTokensForTokens( | ||
| uniswapRouter, | ||
| inputToken, | ||
| swapAmount, | ||
| targetToken, | ||
| 0 | ||
| ); | ||
| return (outputAmount, gasZRC20, gasFee); | ||
| return (out, gasZRC20, gasFee); |
There was a problem hiding this comment.
Remove hardcoded minimum output amount
Using 0 as the minimum output amount is unsafe and could lead to significant slippage or sandwich attacks.
Add a minOutputAmount parameter:
uint256 out = SwapHelperLib.swapExactTokensForTokens(
uniswapRouter,
inputToken,
swapAmount,
targetToken,
- 0
+ minOutputAmount
);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/pages/developers/frontend/universalkit.mdx (2)
Line range hint
1-14: Enhance the warning message for production readinessThe current alert about testnet usage is good, but could be more comprehensive for production-grade documentation.
Consider expanding the alert message to include:
-<Alert> The component library is in under active development and currently is recommended for testnet use only.</Alert> +<Alert type="warning"> + ⚠️ Production Notice: + This component library is under active development and is currently recommended for testnet use only. + Breaking changes may occur without notice. + For production deployments, please await stable release announcements. +</Alert>🧰 Tools
🪛 LanguageTool
[uncategorized] ~142-~142: Possible missing comma found.
Context: ...ns on connected chains For cross-chain swaps the component depends on a universal sw...(AI_HYDRA_LEO_MISSING_COMMA)
Line range hint
156-182: Enhance code examples for production readinessThe current example uses a hardcoded contract address without proper context or error handling.
Consider updating the example to be more production-ready:
const contract = "0xb459F14260D1dc6484CE56EB0826be317171e91F"; // universal swap contract + +// In production, consider using environment variables +const UNIVERSAL_SWAP_CONTRACT = process.env.NEXT_PUBLIC_UNIVERSAL_SWAP_CONTRACT; +if (!UNIVERSAL_SWAP_CONTRACT) { + throw new Error("Universal swap contract address not configured"); +} const Page = () => { return ( <div className="m-4"> <div className="flex justify-end gap-2 mb-10"> <ConnectBitcoin /> <ConnectButton label="Connect EVM" showBalance={false} /> </div> <div className="flex justify-center"> <div className="w-[400px]"> - <Swap contract={contract} /> + <Swap contract={UNIVERSAL_SWAP_CONTRACT} /> </div> </div> </div> ); };🧰 Tools
🪛 LanguageTool
[uncategorized] ~142-~142: Possible missing comma found.
Context: ...ns on connected chains For cross-chain swaps the component depends on a universal sw...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/pages/developers/frontend/universalkit.mdx(1 hunks)src/pages/developers/tutorials/_meta.json(0 hunks)
💤 Files with no reviewable changes (1)
- src/pages/developers/tutorials/_meta.json
| @@ -140,7 +140,7 @@ The Swap component provides the following functionality: | |||
| - Transfer native gas and ERC-20 tokens on connected chains | |||
|
|
|||
| For cross-chain swaps the component depends on a universal swap contract. | |||
| Complete [the swap tutorial](/developers/tutorials/swap-any) and deploy your own | |||
| Complete [the swap tutorial](/developers/tutorials/swap) and deploy your own | |||
There was a problem hiding this comment.
💡 Codebase verification
Tutorial link update requires sitemap correction
The link update in the documentation files is consistent, but the sitemap needs to be updated:
- The old path
/developers/tutorials/swap-any/should be removed frompublic/sitemap-0.xml - All documentation files correctly reference the new path
/developers/tutorials/swap/ - The old tutorial file has been properly removed from the codebase
🔗 Analysis chain
Verify tutorial link consistency across documentation
The tutorial link has been updated to reflect the merged tutorials.
Let's verify all references to the old tutorial have been updated:
Update sitemap and verify remaining tutorial references
The verification reveals inconsistencies in the documentation:
- The old path
/developers/tutorials/swap-any/still exists inpublic/sitemap-0.xml - The new path
/developers/tutorials/swap/is correctly referenced in:- src/pages/developers/services/goldsky.mdx
- src/pages/developers/frontend/universalkit.mdx
- src/pages/developers/frontend/bitcoin.mdx
Let's verify if there are any other potential references we should check:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old tutorial path
rg -i "swap-any"
# Search for consistency in swap tutorial references
rg -i "tutorials/swap[^a-z]"
Length of output: 1005
Script:
#!/bin/bash
# Search for any content files that might contain tutorial references
fd -e md -e mdx | xargs rg -l -i "swap.*tutorial"
# Check if the old tutorial file still exists
fd "swap-any.mdx?"
Length of output: 203
Co-authored-by: Hernan Clich <[email protected]>
Co-authored-by: Hernan Clich <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/pages/developers/tutorials/swap.mdx (2)
94-98: Enhance error messages for better debuggingThe current error messages are too generic, making it difficult to identify the specific cause of failures.
Consider making the error messages more descriptive:
- error InvalidAddress(); - error Unauthorized(); - error ApprovalFailed(); - error TransferFailed(); + error InvalidAddress(address addr); + error Unauthorized(address sender, address expected); + error ApprovalFailed(address token, address spender, uint256 amount); + error TransferFailed(address token, address from, address to, uint256 amount);
349-354: Fix markdown formatting inconsistenciesThere are some formatting inconsistencies in the documentation.
Apply these changes:
- Remove extra spaces before bullet points
- Add consistent spacing after bullet points
- Ensure uniform indentation in the list items
🧰 Tools
🪛 LanguageTool
[uncategorized] ~349-~349: Loose punctuation mark.
Context: ... of information: -address target: The ZRC-20 address of the target token ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~350-~350: Loose punctuation mark.
Context: ...get token on ZetaChain. -bytes to: The recipient's address on the destinat...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~353-~353: Loose punctuation mark.
Context: ...ns like Bitcoin. -bool withdraw: Indicates whether to withdraw the swapp...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/pages/developers/tutorials/swap.mdx(9 hunks)
🧰 Additional context used
🪛 LanguageTool
src/pages/developers/tutorials/swap.mdx
[uncategorized] ~349-~349: Loose punctuation mark.
Context: ... of information: - address target: The ZRC-20 address of the target token ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~350-~350: Loose punctuation mark.
Context: ...get token on ZetaChain. - bytes to: The recipient's address on the destinat...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~353-~353: Loose punctuation mark.
Context: ...ns like Bitcoin. - bool withdraw: Indicates whether to withdraw the swapp...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~400-~400: Possible missing comma found.
Context: ...kens are approved, either combined or separately depending on whether they are the same....
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (1)
src/pages/developers/tutorials/swap.mdx (1)
255-262:
Add slippage protection to prevent sandwich attacks
The swap is performed without specifying a minimum output amount, which could lead to significant slippage or sandwich attacks.
This issue was previously identified. The fix involves adding a minOutputAmount parameter to protect users from unfavorable trades.
| function _authorizeUpgrade( | ||
| address newImplementation | ||
| ) internal override onlyOwner {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance upgrade implementation with additional safeguards
The current upgrade implementation lacks important safety measures and transparency features.
Consider implementing these improvements:
+ uint256 public constant VERSION = 1;
+ event ContractUpgraded(address indexed implementation, uint256 indexed version);
function _authorizeUpgrade(
address newImplementation
- ) internal override onlyOwner {}
+ ) internal override onlyOwner {
+ if (newImplementation == address(0)) revert InvalidAddress();
+ emit ContractUpgraded(newImplementation, VERSION + 1);
+ }📝 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 _authorizeUpgrade( | |
| address newImplementation | |
| ) internal override onlyOwner {} | |
| uint256 public constant VERSION = 1; | |
| event ContractUpgraded(address indexed implementation, uint256 indexed version); | |
| function _authorizeUpgrade( | |
| address newImplementation | |
| ) internal override onlyOwner { | |
| if (newImplementation == address(0)) revert InvalidAddress(); | |
| emit ContractUpgraded(newImplementation, VERSION + 1); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/pages/developers/tutorials/swap.mdx(9 hunks)
🧰 Additional context used
🪛 LanguageTool
src/pages/developers/tutorials/swap.mdx
[uncategorized] ~345-~345: Loose punctuation mark.
Context: ... of information: - address target: The ZRC-20 address of the target token ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~346-~346: Loose punctuation mark.
Context: ...get token on ZetaChain. - bytes to: The recipient's address on the destinat...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~349-~349: Loose punctuation mark.
Context: ...ns like Bitcoin. - bool withdraw: Indicates whether to withdraw the swapp...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~396-~396: Possible missing comma found.
Context: ...kens are approved, either combined or separately depending on whether they are the same....
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (3)
src/pages/developers/tutorials/swap.mdx (3)
255-262: Remove hardcoded minimum output amount
Using 0 as the minimum output amount is unsafe and could lead to significant slippage or sandwich attacks.
334-336: Enhance upgrade implementation with additional safeguards
The current upgrade implementation lacks important safety measures and transparency features.
187-228:
Add slippage and deadline protection
The swap function should include slippage protection and a deadline parameter to prevent sandwich attacks and stale transactions.
function swap(
address inputToken,
uint256 amount,
address targetToken,
bytes memory recipient,
- bool withdrawFlag
+ bool withdrawFlag,
+ uint256 minOutputAmount,
+ uint256 deadline
) public {
+ if (block.timestamp > deadline) revert DeadlineExpired();
bool success = IZRC20(inputToken).transferFrom(
msg.sender,
address(this),
amount
);
if (!success) {
revert TransferFailed();
}
(uint256 out, address gasZRC20, uint256 gasFee) = handleGasAndSwap(
inputToken,
amount,
- targetToken
+ targetToken,
+ minOutputAmount
);Likely invalid or redundant comment.
| function initialize( | ||
| address payable gatewayAddress, | ||
| address uniswapRouterAddress, | ||
| uint256 gasLimitAmount | ||
| ) { | ||
| uint256 gasLimitAmount, | ||
| address owner | ||
| ) public initializer { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for gasLimitAmount parameter
The initialize function should validate that gasLimitAmount is within reasonable bounds to prevent potential issues with cross-chain operations.
function initialize(
address payable gatewayAddress,
address uniswapRouterAddress,
uint256 gasLimitAmount,
address owner
) public initializer {
if (gatewayAddress == address(0) || uniswapRouterAddress == address(0))
revert InvalidAddress();
+ if (gasLimitAmount == 0 || gasLimitAmount > 1_000_000)
+ revert InvalidGasLimit();
__UUPSUpgradeable_init();
__Ownable_init(owner);📝 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 initialize( | |
| address payable gatewayAddress, | |
| address uniswapRouterAddress, | |
| uint256 gasLimitAmount | |
| ) { | |
| uint256 gasLimitAmount, | |
| address owner | |
| ) public initializer { | |
| function initialize( | |
| address payable gatewayAddress, | |
| address uniswapRouterAddress, | |
| uint256 gasLimitAmount, | |
| address owner | |
| ) public initializer { | |
| if (gatewayAddress == address(0) || uniswapRouterAddress == address(0)) | |
| revert InvalidAddress(); | |
| if (gasLimitAmount == 0 || gasLimitAmount > 1_000_000) | |
| revert InvalidGasLimit(); | |
| __UUPSUpgradeable_init(); | |
| __Ownable_init(owner); |
| if (context.chainID == BITCOIN_TESTNET || context.chainID == BITCOIN) { | ||
| params.target = BytesHelperLib.bytesToAddress(message, 0); | ||
| params.to = abi.encodePacked( | ||
| BytesHelperLib.bytesToAddress(message, 20) | ||
| ); | ||
| if (message.length >= 41) { | ||
| params.withdraw = BytesHelperLib.bytesToBool(message, 40); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add length validation for Bitcoin messages
The Bitcoin message handling should validate the total message length before accessing specific byte ranges to prevent potential out-of-bounds access.
if (context.chainID == BITCOIN_TESTNET || context.chainID == BITCOIN) {
+ if (message.length < 40) revert InvalidMessageLength();
params.target = BytesHelperLib.bytesToAddress(message, 0);
params.to = abi.encodePacked(
BytesHelperLib.bytesToAddress(message, 20)
);
if (message.length >= 41) {
params.withdraw = BytesHelperLib.bytesToBool(message, 40);
}📝 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.
| if (context.chainID == BITCOIN_TESTNET || context.chainID == BITCOIN) { | |
| params.target = BytesHelperLib.bytesToAddress(message, 0); | |
| params.to = abi.encodePacked( | |
| BytesHelperLib.bytesToAddress(message, 20) | |
| ); | |
| if (message.length >= 41) { | |
| params.withdraw = BytesHelperLib.bytesToBool(message, 40); | |
| } | |
| if (context.chainID == BITCOIN_TESTNET || context.chainID == BITCOIN) { | |
| if (message.length < 40) revert InvalidMessageLength(); | |
| params.target = BytesHelperLib.bytesToAddress(message, 0); | |
| params.to = abi.encodePacked( | |
| BytesHelperLib.bytesToAddress(message, 20) | |
| ); | |
| if (message.length >= 41) { | |
| params.withdraw = BytesHelperLib.bytesToBool(message, 40); | |
| } |
We have two Swap tutorials. One is meant to be slightly more compact, the other one more feature-rich (for example, being able to swap to ZETA on ZetaChain and swap directly from ZetaChain). The problem is, both contracts have grown fairly large and the LOC difference between them is no longer significant. I've decided to merge these contracts into one so that developers don't have to choose which tutorial to follow.
I've also modified the code to be upgradeable and added a companion contract.
Related: zeta-chain/example-contracts#223
Summary by CodeRabbit
New Features
Swapcontract tutorial with a clearer step-by-step process for creating cross-chain swaps.Alertcomponent in the UniversalKit documentation to indicate active development status.Bug Fixes
Documentation
Chores
SwapToAnyTokenand its associated metadata entry.