feat(nft-swap): nft swap PoC and multi standalone swap contracts v2

[laruh]

No description provided.

[shamardy]

Thanks for the PR! Mid sprint review!

[shamardy]

Order of requires is changed from ETH/ERC20 contract, shouldn't

require(
            makerPayments[params.id].state == MakerPaymentState.Uninitialized,
            "Maker ERC1155 payment must be Uninitialized"
        );

be first?

[laruh]

You are right! done

[r2st]

Can Erc721 and Erc1155 functions be combined ? That will reduce the code size.
"amount" can be zero for Erc721 and greater than zero for Erc1155

[laruh]

Can Erc721 and Erc1155 functions be combined ? That will reduce the code size. "amount" can be zero for Erc721 and greater than zero for Erc1155

Hmmm good idea. Im just thinking about compromise between the deploy cost (on our side) and gas cost for each function (on taker/maker sides).
while smaller code size reduces deploy cost, if we combine functions we shouldnt rely only on amount field in Nft functions, additional checks are needed.
Except check if amount == 0 then use IERC721 token = IERC721(tokenAddress); we should do supportsInterface check to be sure that token contract supports IERC721 interface (as an example). As it is external call, it will increase gas usage for transaction.
We have goal to optimize and minimize gas fee (eg this PR in komodefi).

The explicit separation of erc721 and erc1155 functions offers clear distinction and type safety, reducing the chance of incorrect token transfer.

But I will do gas cost checks for combined function versions, so we can decide.

[laruh]

Can Erc721 and Erc1155 functions be combined ? That will reduce the code size. "amount" can be zero for Erc721 and greater than zero for Erc1155

added combined spend NFT call here dffa69e

Spend combined ERC721 Gas used: 77669
Spend combined ERC1155 Gas used: 73308

Spend separate ERC721 Gas used: 74022
Spend separate ERC1155 Gas used: 70289

gas usage increased by ~3000. this gas cost is kinda significant.

I would like to stick with separate function implementation. Because even if we combine NftMakerPayment functions, it is unlikely that we will have enough available EtomicSwapNft size (eip-170) to add NftTakerPayment + ETH/ERC20 Maker Payment functions into it.
During implementation I decided that probably we will have 3 nft swap contract types:

• Taker Coin <-> Maker Nft
• Taker Nft <-> Maker Coin
• Taker Nft <-> Maker Nft (our bright future)

By above ^ I mean that we need to deploy contract once, while users do swaps constantly.

But Im curious about @shamardy opinion. What do you think?

[shamardy]

But Im curious about @shamardy opinion. What do you think?

Agree that gas increase is significant, having 4 contracts is fine (4 because there is also the ERC20-ERC20 or Taker Coin <-> Maker Coin normal contract done by Artem). Deployment cost will increase for sure but I don't see a way around it. But I think the below:

Taker Coin <-> Maker Nft
Taker Nft <-> Maker Coin
Taker Nft <-> Maker Nft (our bright future)

is not entirely right, basically a contract is for locking and spending or refunding if we do it as above it we will duplicate code across contracts, we should have contracts for:
1 - Taker Coin: send, spend and refund
2 - Maker Coin: send, spend and refund
3 - Taker Nft: send, spend and refund
4 - Maker Nft: send, spend and refund

so for instance for erc20-erc20 swap, the maker will use one contract and the taker will use another, the taker will extract the secret from one contract and use this secret in the other one. BTW, did you consider having 2 contracts, one for maker for any coin/token/NFT and another for taker for the same, maybe this will not pass the contract size since we will not duplicate some of the structs.

P.S. Maybe we should take a look at The Diamond Standard (EIP-2535) ref:
https://www.quicknode.com/guides/ethereum-development/smart-contracts/the-diamond-standard-eip-2535-explained-part-1
https://eips.ethereum.org/EIPS/eip-2535
We need to make sure this works for all EVM chains. I will share more links in DM.

[laruh]

we should have contracts for:
1 - Taker Coin: send, spend and refund
2 - Maker Coin: send, spend and refund
3 - Taker Nft: send, spend and refund
4 - Maker Nft: send, spend and refund
so for instance for erc20-erc20 swap, the maker will use one contract and the taker will use another, the taker will extract the secret from one contract and use this secret in the other one.

ohh, exactly!!! we could just call different contracts for each side. This should work!

BTW, did you consider having 2 contracts, one for maker for any coin/token/NFT and another for taker for the same

nope, I wanted to group swap contracts by assets types:

• one contract implements: Taker swaps coin - Maker swaps coin
• another contract implements: Taker swaps coin - Maker swaps NFT
  etc

The approach above ^ avoids calling different swap addresses. I thought that it will be bad if two etomic swap contracts will be involved in swap process.

But now I see that there is actually no problem with involving two contracts.
In your approach you group the swap contracts by participants and their assets:

• one contact is for: all Taker coin methods
• another contact is for: all Maker coin methods
  etc

thanks to such grouping in komodefi swap impl we easily can understand by maker/taker asset type which etomic contract we should call in maker and taker operations. Of course it also avoids code duplication.

P.S. Maybe we should take a look at The Diamond Standard (EIP-2535)

Very interesting pattern.
As I see it uses other contracts(Facets) for delegated calls. By brief look I see that delegated calls are not completely the same as external calls, but they do have some gas overhead. Well there are some suggestions how to optimize gas cost.

Also I think that while diamond standard reduces the total number of contracts, the initial deployment still could be gas-intensive due to the setup of multiple facets. Well maybe slightly cheaper, than 4 contracts deployment.

For me the actual main benefit from Diamond usage is the high degree of flexibility for upgrades and changes to the etomic swap contract logic.

The open question: do we really need this upgrade opportunity? We usually dont do a lot of changes in swap contracts.
Your etomic contracts functionality grouping makes code easier to maintain, avoids code duplication and doesnt need external/delegated calls.
I mean, our main task of creating a contract for atomic swaps already has an advantage. This is a clear separation of functionality (we have Maker side and Taker side). That is, one smart contract takes on the responsibilities of the Maker, the other - the Taker.

This is my the initial answer for Diamond Standard. However I would like to study this contracts creation pattern more.

[shamardy]

For me the actual main benefit from Diamond usage is the high degree of flexibility for upgrades and changes to the etomic swap contract logic.

This is also a drawback of the pattern, as a contract issuer/deployer can change the code of a contract after it has been used by users. This modification removes the trustless feature of a smart contract. The upgradeability features of diamonds are offered so that during development, alpha, or beta stages, you can still change the code easily. However, once the contract is production/stable release ready, upgradeability should be disabled. This is why diamonds provide a feature to make a contract immutable at a later time.

The open question: do we really need this upgrade opportunity? We usually dont do a lot of changes in swap contracts.
Your etomic contracts functionality grouping makes code easier to maintain, avoids code duplication and doesnt need external/delegated calls.

This is what I am leaning towards too, but I thought we should consider any possibilities before setting anything in stone. Making contracts simpler is also good for security audits and checks, a benefit of the etomic swap contract is that it's simple and doesn't have a lot of layers or complex logic that can hide vulnerabilities.

P.S. As agreed on DM, we will not do any more reviews for this PR until we decide on the approach to follow during the next sprint.

[laruh]

Staring from d2de9a5 commit (Create maker taker contracts, test script for MakerV2 TakerV2) the implementation approach was changed to Multi Standalone Etomic Swap contracts

The two strategies were proposed previously (see #7 (comment)):

1. ERC-2535 Diamond standard
2. Multi Standalone contracts

[shamardy]

@laruh I guess we can now remove contracts/EtomicSwapV2.sol and contracts/EtomicSwapNft.sol

[shamardy]

LGTM! Only one question!

[laruh]

@laruh I guess we can now remove contracts/EtomicSwapV2.sol and contracts/EtomicSwapNft.sol

Yep, we dont need them for prod, but I decided to keep old contracts for dev/test processes, eg to quickly compile contracts for sepolia/geth tests if necessary