-
Notifications
You must be signed in to change notification settings - Fork 2
feat(v1): add SafeERC20 support for USDT compatibility #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
Add OpenZeppelin's SafeERC20 to handle non-standard ERC20 tokens like USDT that don't return a boolean from transfer/transferFrom. Changes to EtomicSwap.sol: - Add SafeERC20 import and `using SafeERC20 for IERC20` - Replace transfer/transferFrom with safeTransfer/safeTransferFrom - Remove `payable` from erc20Payment() (function doesn't use msg.value) - Add tokenAddress != address(0) validation in erc20Payment() - Bump Solidity version to 0.8.33 (latest stable) Changes to hardhat.config.js: - Update compiler to 0.8.33 - Enable optimizer with 10000 runs Co-Authored-By: Claude Opus 4.5 <[email protected]>
Remove ERC721/ERC1155/ERC165 code from V1 contract since NFT swaps use separate V2 contracts. This results in a smaller, cleaner contract focused only on ETH and ERC20 atomic swaps. Changes: - Remove NFT imports and interface implementations - Remove receiverSpendErc721, receiverSpendErc1155 functions - Remove senderRefundErc721, senderRefundErc1155 functions - Remove onERC721Received, onERC1155Received, onERC1155BatchReceived - Remove supportsInterface and isContract helper - Keep SafeERC20 for USDT compatibility - Bump package version to 1.1.0 Bytecode reduced from ~13KB to ~5KB. Co-Authored-By: Claude Opus 4.5 <[email protected]>
52cd5e5 to
d2974f1
Compare
- Update @openzeppelin/contracts from ^5.0.0 to ^5.4.0 - Move test deps (chai, chai-as-promised, ripemd160) to devDependencies - Remove NFT tests from V1 test suite (NFT code removed from contract) - Update ethers version in test error message - Regenerate yarn.lock Co-Authored-By: Claude Opus 4.5 <[email protected]>
mariocynicys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Only have those comments.
Q: we will re-deploy v1 contract for ETH for the sake of getting USDT-ERC20 working, but only for ETH? no?
since other chains already have a functioning USDT-EVM20 coin?
hardhat.config.js
Outdated
| enabled: true, | ||
| runs: 10000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: what is runs: 10000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The runs parameter tells the optimizer how many times each opcode will execute over the contract's lifetime - it's a trade-off between deployment cost and execution cost.
10000 is a common choice for frequently-used contracts. Higher values have diminishing returns since most inlining decisions are already made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P.S. I favoured execution cost (what the users pay) vs the deployment cost which is what we will pay to deploy the contract.
| contract EtomicSwap { | ||
| using SafeERC20 for IERC20; | ||
|
|
||
| contract EtomicSwap is ERC165, IERC1155Receiver, IERC721Receiver { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we better keep erc165?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ERC165 is for interface detection - lets other contracts query "do you support interface X?".
We had it because the old contract implemented IERC721Receiver/IERC1155Receiver for NFT support. Standard ERC20 doesn't require ERC165 (it predates it), and ERC721/ERC1155 are the ones that mandate it for their receiver callbacks.
With NFTs removed, there's no interface to advertise. Our contract uses safeTransferFrom to pull tokens (not receive via callback), so no receiver interface is needed.
No need to re-deploy the others as there are no changes in KDF but tests only, so same code works for both contracts old and new. For consistency we can re-deploy others later but it's not a priority. You will see for yourself once I open the KDF PR. |
- Update swap_contract_bytes and ABI for SafeERC20 V1 contract (compiled with OpenZeppelin 5.4.0) - Add USDT contract bytecode and ABI for testing - Add docker tests for USDT swaps (taker spend, maker refund) - Add docker tests for ERC20 decimal handling (6, 8, 18 decimals) - Update test helpers for USDT deployment and funding The SafeERC20 changes enable USDT and other non-standard ERC20 tokens that don't return bool from transfer/transferFrom. Related: GLEECBTC/etomic-swap#11 Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
KDF PR opened: GLEECBTC/komodo-defi-framework#2711 |
Add Hardhat Ignition module with CREATE2 strategy via CreateX factory for deterministic cross-chain deployment. Configure 17 mainnets, 5 testnets, and Etherscan V2 verification. Deploy and verify EtomicSwap V1 (SafeERC20) to Ethereum mainnet and Sepolia at 0x61EEC68Cf64d1b31e41EA713356De2563fB6D3F1. Co-Authored-By: Claude Opus 4.5 <[email protected]>
mariocynicys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Summary
erc20Payment()payable modifier bug (function doesn't use msg.value)Fixes GLEECBTC/komodo-defi-framework#408
Deployed Address (all chains)
Deployed via CreateX CREATE2 factory — same address on every EVM chain.
Changes
EtomicSwap.sol
using SafeERC20 for IERC20require(token.transferFrom(...))withsafeTransferFromrequire(token.transfer(...))withsafeTransferpayablemodifier fromerc20Payment()(prevents ETH being stuck)require(tokenAddress != address(0))checkDeployment Infrastructure
ignition/modules/EtomicSwapV1.js) with CREATE2 strategyhardhat.config.js.env.exampletemplateDEPLOYMENTS.mdwith deployment record and runbook for remaining chainsDependencies
Tests
Why SafeERC20?
USDT's
transfer/transferFromdon't return a boolean. The previous code usedrequire(token.transferFrom(...))which fails because Solidity tries to decode a non-existent return value. SafeERC20 handles this correctly.Testing
Future TODOs (for follow-up PRs)
.transfer()deprecation (uses 2300 gas limit) to support smart contract wallets (Gnosis Safe, etc.)Related