feat(nft-swap): nft swap protocol v2 POC #2084

laruh · 2024-03-10T08:10:01Z

Related to #900

To deploy smart contract you need to have it's abi json and bytes. They can be compiled from smart contract solidity code.

nft_swap_contract_abi.json and NFT_SWAP_CONTRACT_BYTES are compiled from EtomicSwapNft.sol contract from this PR

Solidity code of test ERC721 and ERC1155 tokens from which abi jsons and bytes were compiled:

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.20;
import "@openzeppelin/contracts/token/ERC721/ERC721.sol";

contract TestERC721 is ERC721 {
    constructor(string memory name, string memory symbol)
        ERC721(name, symbol)
    {}

    function mint(address to, uint256 tokenId) external {
        _mint(to, tokenId);
    }
}

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.20;
import "@openzeppelin/contracts/token/ERC1155/ERC1155.sol";

contract TestERC1155 is ERC1155 {
    constructor(string memory uri) ERC1155(uri) {}

    function mint(
        address to,
        uint256 tokenId,
        uint256 amount,
        bytes memory data
    ) external {
        _mint(to, tokenId, amount, data);
    }
}

You can get abi json & bytes easily using https://remix.ethereum.org/ web IDE (PS dont forget to format code with shift+alt+F pls)

Some small note:
There was a moment that I wanted to validate event and topics of onERC1155Received and onERC721Received functions. But if payment was confirmed and its state is MakerPaymentSent in validation function, there is no need to check on...Received events.
Still would like to share a nice article about logs, topics and data objects in ETH receipt.
https://medium.com/mycrypto/understanding-event-logs-on-the-ethereum-blockchain-f4ae7ba50378

… and test nft abi. Make NFT type uppercase in CoinProtocol

…5_TEST_TOKEN_BYTES from formatted code

…data for Erc1155 transfer

…ransferFrom func

shamardy

Great start! We need to start paying more attention to the design choices early on before we commit to a suboptimal one.

mm2src/coins/eth.rs

shamardy · 2024-03-13T14:11:57Z

mm2src/coins/eth.rs

+        self.rlp_append(&mut stream);
+        // there is minimal but risk that stream.out() may panic
+        debug_assert!(stream.is_finished(), "RlpStream must be finished before calling out");
+        Vec::from(stream.out())


Maybe use an empty vec if stream is not finished, this way we will avoid panicking and the preimage will be validated anyway and swap will fail if there is any problems in serialization.
P.S. Panicking here can be an attack vector if we are not careful since the preimage is received from the other side through a p2p message and can be made to crash the other side's node, right now it's won't be a problem probably since we use parse_preimage first but it can be a problem if we are not careful with code changes.

Maybe use an empty vec if stream is not finished

Oh, exactly! yep we can do if stream.is_finished() check

right now it's won't be a problem probably since we use parse_preimage first but it can be a problem if we are not careful with code changes.

I see, we can refactor trait ToBytes impl and return Result<Vec, Error>. Well, it will trigger other code parts, so in some other traits we also have to do the refactoring and return Result type. For now I suggest to leave stream.is_finished() check,

impl ToBytes for SignedEthTx { fn to_bytes(&self) -> Vec<u8> { let mut stream = RlpStream::new(); self.rlp_append(&mut stream); // Handle potential panicking. if stream.is_finished() { Vec::from(stream.out()) } else { // TODO: Consider returning Result<Vec<u8>, Error> in future refactoring for better error handling. warn!("RlpStream was not finished; returning an empty Vec as a fail-safe."); vec![] } } }

and in next pr(s) do the refactor and return Result where we will have to. Or at least try to refactor and see, do we actually have problems with it or not. I think its better to propagate errors instead of being careful (I mean we all are humans and there is a risk of miss something).

UPD: added stream.is_finished() check for now

I think it's better to propagate errors instead of being careful (I mean we all are humans and there is a risk of miss something).

I agree, I have the same problem with my_addr function in EVM HD wallet PR

komodo-defi-framework/mm2src/coins/utxo.rs

Lines 1056 to 1066 in d085af1

async fn my_addr(&self) -> Self::Address {

match &self.as_ref().derivation_method {

DerivationMethod::SingleAddress(addr) => addr.clone(),

// Todo: Expect should not fail but we need to handle it properly

DerivationMethod::HDWallet(hd_wallet) => hd_wallet

.get_enabled_address()

.await

.expect("Getting enabled address should not fail!")

.address(),

}

}

To modify it I need to follow the same design choices @artemii235 did in the state machine swaps implementation, he would rather save a value to be retrieved later instead of calling the same function in multiple places and doing error handling in multiple places for the same function which makes the code harder to understand, and for such critical parts we need the code to be as clear as possible.

shamardy · 2024-03-13T14:31:57Z

mm2src/coins/lp_coins.rs

+/// Defines associated types specific to each coin (Pubkey, Address, etc.)
+pub trait NftAssocTypes {
+    type TokenContractAddr: Send + Sync + fmt::Display;
+    type TokenContractAddrParseError: fmt::Debug + Send + fmt::Display;
+    type TokenId: ToBytes + Send + Sync;
+    type TokenIdParseError: fmt::Debug + Send + fmt::Display;
+    type Chain: ToBytes + Send + Sync;
+    type ChainParseError: fmt::Debug + Send + fmt::Display;
+    type ContractType: ToBytes + Send + Sync;
+    type ContractTypeParseError: fmt::Debug + Send + fmt::Display;
+
+    fn parse_token_contract_address(
+        &self,
+        token_contract_addr: &[u8],
+    ) -> Result<Self::TokenContractAddr, Self::TokenContractAddrParseError>;
+
+    fn parse_token_id(&self, token_id: &[u8]) -> Result<Self::TokenId, Self::TokenIdParseError>;
+
+    fn parse_chain(&self, chain: &[u8]) -> Result<Self::Chain, Self::ChainParseError>;
+
+    fn parse_contract_type(&self, contract_type: &[u8]) -> Result<Self::ContractType, Self::ContractTypeParseError>;
+}


Will we actually need those in swaps code? I am not sure yet, parsing data is used for p2p data and from storage data so maybe we will need for the from storage data but for p2p data I think the TokenContractAddr/TokenId/Chain/ContractType will be known for both sides before starting the swap. We can leave them for now but we have to be careful with adding coin specific logic inside swap state machine logic which we did in legacy swap and want to avoid in v2 code to make it cleaner.

I see two options of nft swap implementation:

Have optional nft fields in current args structures, for example in SendMakerPaymentArgs. So in maker state changes we will need to check do we have Some in nft related fields or None. So in each maker state we will have to handle two inner implementations: for coin and for NFT. We may have the same problem as we had during trying to integrate nft activation into existing coin related code, which will lead to decision of logic separation.

Create separate NFT related structures for args (eg SendNftMakerPaymentArgs) and NFT Maker states for MakerSwapStateMachine. This way we wont need to check optional nft fields in args as all of them will be required. Also we wont need to handle the coin and nft behaviors in current on_changed functions for coin maker states, as we will need to implement NFT related states. I believe we will still have same struct MakerSwapStateMachine.

I think the TokenContractAddr/TokenId/Chain/ContractType will be known for both sides before starting the swap

I wasnt sure about it so just decided to prepare the functionality. As we already use parse_... functions in maker_swap_v2.rs and taker_swap_v2.rs I decided that we may have nft related fields in args structures or negotiation data, so we need this.

The another big approach of nft implementation is providing in args structure(doesnt matter for now in NFT args or in optional nft field in current ags structures, its about having or not having NftAssocTypes) the string of concatenated "token_address,token_id".
This way Maker coin should be NFT global "coin" from the beginning I think, from its "nfts_infos" field we can get NftInfo value using "token_address,token_id" key. You know, this approach sounds not bad. This way we wont need NftAssocTypes. Need to refactor NFT activation and enable global NFT using nft json from coin config file.

Create separate NFT related structures for args (eg SendNftMakerPaymentArgs) and NFT Maker states for MakerSwapStateMachine

NFT states for swap state machines is a nice idea if it can be done cleanly :)

I wasn't sure about it so just decided to prepare the functionality.

We can leave it in this PR and re-assess when implementing NFT functionalities in the swaps state machine.

shamardy · 2024-03-13T14:34:22Z

mm2src/coins/lp_coins.rs

@@ -1502,6 +1544,31 @@ pub struct SendMakerPaymentArgs<'a, Coin: CoinAssocTypes + ?Sized> {
    pub swap_unique_data: &'a [u8],
 }

+pub struct SendNftMakerPaymentArgs<'a, Coin: CoinAssocTypes + NftAssocTypes + ?Sized> {


Again, we shouldn't have maker payment args specific for NFTs. It will create a lot of conditional code in swaps code, you can leave it for now for the P.O.C. purpose but we need to have a plan on how we will integrate NFT swaps logic in current swap state machine.

Yep, its just one of two ways of two nft implementations from this #2084 (comment) description

but we need to have a plan on how we will integrate NFT swaps logic in current swap state machine.

Oke, then we can think about approach with concatenated string of "token_address,token_id" parameter in Maker Payment args. If we want to reutulize the existing code, then we can add is_nft:bool param and optional nft_key:String.
If is_nft:true, nft_key must be Some, and we try to get NftInfo value from nfts_infos in global NFT. Well this means we will have to handle nft behavior and coin in same States.

If is_nft:true, nft_key must be Some, and we try to get NftInfo value from nfts_infos in global NFT. Well this means we will have to handle nft behavior and coin in same States.

Answered here #2084 (comment) , we will re-assess in next PRs I guess.

shamardy · 2024-03-13T14:35:57Z

mm2src/coins/lp_coins.rs

@@ -1575,6 +1642,38 @@ pub trait MakerCoinSwapOpsV2: CoinAssocTypes + Send + Sync + 'static {
    async fn spend_maker_payment_v2(&self, args: SpendMakerPaymentArgs<'_, Self>) -> Result<Self::Tx, TransactionErr>;
 }

+#[async_trait]
+pub trait MakerNftSwapOpsV2: CoinAssocTypes + NftAssocTypes + Send + Sync + 'static {


Same comment, are you planning to make a different maker_swap_v2.rs and taker_swap_v2.rs files for NFTs?

No, I wanted only to have NFT related states in maker_swap_v2.rs and taker_swap_v2.rs for MakerSwapStateMachine and TakerSwapStateMachine.
Well yeah for nft states we will need to require NFT related trait impl here impl<MakerCoin: MmCoin + MakerCoinSwapOpsV2, TakerCoin: MmCoin + TakerCoinSwapOpsV2> State

But we will discuss the nft swap approach of course.

…actAddr

shamardy

Next review iteration! Probably the last one :)

mm2src/coins/nft/trading_proto_v2/mod.rs

mm2src/coins/lp_coins.rs

mm2src/coins/nft/trading_proto_v2/mod.rs

…ContractValidation in validate_payment_args

…_v2_data

…index_bytes, erc721_transfer_with_data standalone non public

shamardy

🔥

shamardy · 2024-04-08T19:39:20Z

@borngraced if you don't have anymore comments and all your review notes are resolved please approve this PR.

borngraced

Great work!
here's my last review iteration

mm2src/coins/eth.rs

mm2src/coins/eth/nft_swap_v2/structs.rs

borngraced

one more note please :)

borngraced · 2024-04-08T22:12:11Z

mm2src/coins/lp_coins.rs

+pub trait NftAssocTypes {
+    type ContractAddress: Send + Sync + fmt::Display;
+    type ContractAddrParseError: fmt::Debug + Send + fmt::Display;
+    type TokenId: ToBytes + Send + Sync;
+    type TokenIdParseError: fmt::Debug + Send + fmt::Display;
+    type ContractType: ToBytes + Send + Sync;
+    type ContractTypeParseError: fmt::Debug + Send + fmt::Display;
+


why can't we have just one error type that covers all error types? I mean a single, more informative error enum that encapsulates these different kinds of errors
e.g

type Error: fmt::Debug + Send + fmt::Display;

Also, this trait name is better as NftParser or something

why can't we have just one error type that covers all error types?

I followed the approach from CoinAssocTypes trait

komodo-defi-framework/mm2src/coins/utxo.rs

Lines 1051 to 1061 in 76af668

impl<T: UtxoCommonOps> CoinAssocTypes for T {

type Address = Address;

type AddressParseError = MmError<AddrFromStrError>;

type Pubkey = Public;

type PubkeyParseError = MmError<keys::Error>;

type Tx = UtxoTx;

type TxParseError = MmError<serialization::Error>;

type Preimage = UtxoTx;

type PreimageParseError = MmError<serialization::Error>;

type Sig = Signature;

type SigParseError = MmError<secp256k1::Error>;

I suppose Artem decided to keep error types separately, as for UTXO he had errors from crates and usually our own enum errors already implement From for such types.

However, I agree that we can leave one error type for nft assoc trait, because usually we have to create our own structures/enum for this feature, so we cant avoid From trait implementation.

added one err here 95411dc also renamed eth error as its related to eth coin.

Also, this trait name is better as NftParser or something

I would like to disagree, CoinAssocTypes and NftAssocTypes traits are created to parse specific types associated with Coin or with NFT instances. we dont get the whole Coin or Nft.

I would like to disagree, CoinAssocTypes and NftAssocTypes traits are created to parse specific types associated with Coin or with NFT instances. we dont get the whole Coin or Nft.

doesn't mean it's a good name imo.. they can be rename to ParseCoinAssocTypes, ParseNftAssocTypes. currently, no one will ever guess what the trait is suppose to do just by looking at the name..I mean it doesn't tell any meaning

borngraced

LGTM 🚀 Thank you very much for the great work!

* dev: feat(nft-swap): nft swap protocol v2 POC (KomodoPlatform#2084) fix(zcoin): syncing and activation improvements (KomodoPlatform#2089) feat(crypto): mnemonic generation/encryption/decryption/storage (KomodoPlatform#2014) fix(eth): error handling in RPCs (KomodoPlatform#2090)

laruh added 7 commits March 5, 2024 19:03

functionality to deploy and mint NFTs, add nft_swap_contract_abi json…

3386666

… and test nft abi. Make NFT type uppercase in CoinProtocol

Merge remote-tracking branch 'origin/dev' into nft-swaps-poc

47b626a

encode nft contracts contracture arguments

1f6b2f7

encode dex_fee_address for nft_swap_data

388d2e9

use geth_account as dex_fee_address

ccd0645

impl CoinAssocTypes for EthCoin

141c9f0

impl NftAssocTypes. move nft tests to new nft_swap_proto_v2_tests.rs

56303c6

laruh added the in progress Changes will be made from the author label Mar 10, 2024

laruh force-pushed the nft-swaps-poc branch from a474fec to 032f134 Compare March 10, 2024 08:18

laruh changed the title ~~feat(nft-swap): NFT Swap Protocol V2 POC~~ feat(nft-swap): nft swap protocol v2 POC Mar 10, 2024

use parse_pubkey

c9ee4de

laruh force-pushed the nft-swaps-poc branch from 032f134 to c9ee4de Compare March 10, 2024 08:31

laruh added 11 commits March 10, 2024 20:41

remove await and use block_on

35b6dbc

use Public from keys crate as we call derive_htlc_pubkey func

816b295

call try_to_address in parse_token_contract_address, recompile ERC115…

10943d3

…5_TEST_TOKEN_BYTES from formatted code

Merge remote-tracking branch 'origin/dev' into nft-swaps-poc

42d27b0

add some values and checks in send_nft_maker_payment_v2_impl func

f937bb7

rename coin in nft_dev_conf

8484e0e

WIP send_nft_maker_payment_v2_impl: added some validations, prepared …

53ad64c

…data for Erc1155 transfer

polish send_nft_maker_payment_v2_impl func

66fcead

Merge remote-tracking branch 'origin/dev' into nft-swaps-poc

38f7020

call send_nft_maker_payment_v2 in nft test

c42799e

use &nft_swap_contract().to_bytes()

5343192

laruh force-pushed the nft-swaps-poc branch from 7b0c2be to 5343192 Compare March 13, 2024 10:33

laruh added 4 commits March 13, 2024 18:22

send_and_spend_erc1155_maker_payment and logs

3bde774

fill global nft wallet with ETH and search the right version of safeT…

854482e

…ransferFrom func

increase gas usage for erc721 mint, use different token ids

6f1ba46

add note about function method

2c6fb5a

laruh force-pushed the nft-swaps-poc branch from bc6a086 to 2c6fb5a Compare March 13, 2024 14:00

shamardy reviewed Mar 13, 2024

View reviewed changes

laruh added 2 commits April 5, 2024 17:08

fix: provide field types &'a Coin::ContractType, &'a Coin::TokenContr…

15c8a31

…actAddr

fix: avoid nesting code

977cf40

laruh mentioned this pull request Apr 5, 2024

ERC721/ERC1155 support adding for EVM protocols #900

Open

laruh force-pushed the nft-swaps-poc branch from b7c2006 to 977cf40 Compare April 5, 2024 11:43

fix: remove unneeded trait constraint CoinAssocTypes

2d02280

shamardy reviewed Apr 5, 2024

View reviewed changes

laruh added 6 commits April 5, 2024 21:19

fix: use args:SendNftMakerPaymentArgs instead, use generic and trait …

8b5476b

…ContractValidation in validate_payment_args

fix: provide swap_contract_address: &'a Coin::ContractAddress

5d99c34

fix: reuse args: &SpendNftMakerPaymentArgs in prepare_spend_nft_maker…

f7c8863

…_v2_data

fix fmt

5fcdab1

fix: self.my_addr() for Token::Address

defb7bb

fix linter in tests

b596d1d

laruh removed the request for review from r2st April 5, 2024 15:38

laruh added 3 commits April 7, 2024 13:27

fix: simplify validate_payment_args func, make it standalone non public

71db8d7

fix: make validate_from_to_and_maker_status, get_decoded_tx_data_and_…

71f4df5

…index_bytes, erc721_transfer_with_data standalone non public

fix: provide nft_swap_v2 module in eth

8e6183c

shamardy previously approved these changes Apr 8, 2024

View reviewed changes

borngraced requested changes Apr 8, 2024

View reviewed changes

mm2src/coins/eth.rs Outdated Show resolved Hide resolved

mm2src/coins/eth.rs Show resolved Hide resolved

mm2src/coins/eth.rs Outdated Show resolved Hide resolved

mm2src/coins/eth.rs Show resolved Hide resolved

mm2src/coins/eth/nft_swap_v2/structs.rs Outdated Show resolved Hide resolved

fix review notes

76af668

laruh dismissed shamardy’s stale review via 76af668 April 8, 2024 21:40

borngraced reviewed Apr 8, 2024

View reviewed changes

laruh added 2 commits April 9, 2024 14:26

fix: provide one err type in NftAssocTypes trait, rename errors

95411dc

fix: rename assoc types traits

014552d

borngraced approved these changes Apr 9, 2024

View reviewed changes

Merge remote-tracking branch 'origin/dev' into nft-swaps-poc

e74a69a

shamardy merged commit 6d45cfe into dev Apr 9, 2024
24 of 30 checks passed

shamardy deleted the nft-swaps-poc branch April 9, 2024 14:06

shamardy mentioned this pull request Aug 1, 2024

chore(release): v2.1.0-beta #2043

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(nft-swap): nft swap protocol v2 POC #2084

feat(nft-swap): nft swap protocol v2 POC #2084

laruh commented Mar 10, 2024 •

edited

Loading

shamardy left a comment

shamardy Mar 13, 2024

laruh Mar 14, 2024 •

edited

Loading

shamardy Mar 14, 2024

shamardy Mar 13, 2024

laruh Mar 14, 2024 •

edited

Loading

shamardy Mar 14, 2024

shamardy Mar 14, 2024

shamardy Mar 13, 2024

laruh Mar 14, 2024

laruh Mar 14, 2024 •

edited

Loading

shamardy Mar 14, 2024

shamardy Mar 13, 2024

laruh Mar 14, 2024

shamardy left a comment

shamardy left a comment

shamardy commented Apr 8, 2024

borngraced left a comment

borngraced left a comment

borngraced Apr 8, 2024 •

edited

Loading

laruh Apr 9, 2024

borngraced Apr 9, 2024 •

edited

Loading

borngraced left a comment

	async fn my_addr(&self) -> Self::Address {
	match &self.as_ref().derivation_method {
	DerivationMethod::SingleAddress(addr) => addr.clone(),
	// Todo: Expect should not fail but we need to handle it properly
	DerivationMethod::HDWallet(hd_wallet) => hd_wallet
	.get_enabled_address()
	.await
	.expect("Getting enabled address should not fail!")
	.address(),
	}
	}

	impl<T: UtxoCommonOps> CoinAssocTypes for T {
	type Address = Address;
	type AddressParseError = MmError<AddrFromStrError>;
	type Pubkey = Public;
	type PubkeyParseError = MmError<keys::Error>;
	type Tx = UtxoTx;
	type TxParseError = MmError<serialization::Error>;
	type Preimage = UtxoTx;
	type PreimageParseError = MmError<serialization::Error>;
	type Sig = Signature;
	type SigParseError = MmError<secp256k1::Error>;

feat(nft-swap): nft swap protocol v2 POC #2084

feat(nft-swap): nft swap protocol v2 POC #2084

Conversation

laruh commented Mar 10, 2024 • edited Loading

shamardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laruh Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laruh Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laruh Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

shamardy commented Apr 8, 2024

borngraced left a comment

Choose a reason for hiding this comment

borngraced left a comment

Choose a reason for hiding this comment

borngraced Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borngraced Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

borngraced left a comment

Choose a reason for hiding this comment

laruh commented Mar 10, 2024 •

edited

Loading

laruh Mar 14, 2024 •

edited

Loading

laruh Mar 14, 2024 •

edited

Loading

laruh Mar 14, 2024 •

edited

Loading

borngraced Apr 8, 2024 •

edited

Loading

borngraced Apr 9, 2024 •

edited

Loading