atomicDEX-API icon indicating copy to clipboard operation
atomicDEX-API copied to clipboard

Published 20 hours ago • verified publisher icon KomodoPlatform

feat(nft-swap): standalone nft maker swap contract and sepolia testenet

Open laruh opened this issue 10 months ago • 3 comments

related to issue #900

Etomic swap implementation approach was changed to Multi Standalone Etomic Swap contracts (see https://github.com/KomodoPlatform/etomic-swap/pull/7#issuecomment-2074382943)

At the first stage we are going to support EtomicSwapMakerNftV2 and EtomicSwapTakerV2 contracts in NFT Swap Komodefi feature, where Maker is NFT owner and Taker swaps ETH/ETH20.

  • The part related to standalone Maker Nft swap contract is r2r
  • Additionally, this commit https://github.com/KomodoPlatform/komodo-defi-framework/pull/2100/commits/ebac6e6b86bba28738499a986280153fdcb62616 contains additional checks for malicious token_uri links like this

https://docs.moralis.io/web3-data-api/evm/reference/get-wallet-nfts?address=0xf622a6C52C94b500542E2AE6bcAD24C53Bc5b6a2&chain=polygon&format=decimal&token_addresses=[]&media_items=false image https://en.wikipedia.org/wiki/Zip_bomb

  • In the same commit https://github.com/KomodoPlatform/komodo-defi-framework/pull/2100/commits/ebac6e6b86bba28738499a986280153fdcb62616 clear_all param in clear_nft_db RPC now is optional. false is default.
curl --url "http://127.0.0.1:7783" --data '{"userpass":"'$USERPASS'","method":"clear_nft_db","mmrpc":"2.0","params":{"chains":["POLYGON","BSC"]}}'
  • added komodefi-proxy support for nft feature. actually it can be used by any other EVM based feature if it needs to send Http Get requests (ps currently porxy supports only JsonRpc Method calls for EVM). PS: but please note that some todos are left on proxy source code, they shouldn't affect komodefi API.

Req/Res examples for enable eth with tokens RPC

If we want to add support of other block chain types, we will need to make SignOps more generic, right now it focuses on eth types https://github.com/KomodoPlatform/komodo-defi-proxy/blob/80d92fbe1714c5052628b80d90c5a994640e3762/src/security/sign.rs#L5

use ethereum_types::{Address, H256};
use ethkey::{sign, verify_address, Secret, Signature};

we will implement this as needed.

laruh avatar Apr 24 '24 07:04 laruh

notes: on this commit https://github.com/KomodoPlatform/komodo-defi-framework/pull/2100/commits/495e7a9a059caec93ab009b36fbc5f9621582327 both send and spend erc721/erc1155 tests (use sepolia testnet) passed Screenshot 2024-05-06 at 22 40 48

starting from this commit https://github.com/KomodoPlatform/komodo-defi-framework/pull/2100/commits/e917b0b50544fa35310bc869c3defe7ee4f3ee1b komodefi app is able to send http get reqs to proxy layer. https://github.com/KomodoPlatform/komodo-defi-proxy/pull/22

laruh avatar May 06 '24 15:05 laruh

Seems like wait_until: now_sec() + 70 is not enough for ERC-721 transactions. I will increase the wait time for confirmations.

 ---- docker_tests::eth_docker_tests::send_and_spend_erc721_maker_payment stdout ----
All pending transactions have been confirmed.
09 06:31:49, eth_docker_tests:899] Maker sent ERC721 NFT Payment tx hash a2b9563048ab4ba9b6276f72fac881abe7be9693e51233de5623f327e82612c2

09 06:31:49, eth_docker_tests:903] Maker ERC721 NFT Payment transaction
SignedTransaction { transaction: UnverifiedTransaction { unsigned: Transaction { nonce: 37, gas_price: 1762128017, gas: 150000, action: Call(0xbac1c9f2087f39caaa4e93412c6412809186870e), value: 0, data: [184, 141, 79, 222, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 233, 243, 170, 235, 90, 144, 242, 48, 164, 206, 120, 190, 115, 123, 182, 214, 120, 249, 186, 172, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 158, 184, 140, 213, 134, 5, 216, 251, 155, 20, 101, 45, 97, 82, 114, 127, 126, 149, 251, 77, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 128, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 192, 80, 62, 16, 235, 243, 242, 233, 54, 104, 38, 197, 179, 98, 233, 108, 162, 43, 76, 178, 18, 131, 192, 139, 176, 207, 10, 1, 22, 166, 131, 4, 88, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 22, 226, 129, 169, 242, 231, 88, 18, 105, 161, 62, 81, 106, 167, 157, 106, 74, 28, 217, 128, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 186, 193, 201, 242, 8, 127, 57, 202, 170, 78, 147, 65, 44, 100, 18, 128, 145, 134, 135, 14, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 114, 205, 110, 132, 34, 196, 7, 251, 109, 9, 134, 144, 241, 19, 11, 125, 237, 126, 194, 247, 245, 225, 211, 11, 217, 213, 33, 240, 21, 54, 55, 147, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 102, 60, 113, 182] }, v: 22310258, r: 42419588080056669813463425996730670363686721372639109638565201115831479704616, s: 28850038770609371634543925984120968814264788795854789419694462717329751747165, hash: 0xa2b9563048ab4ba9b6276f72fac881abe7be9693e51233de5623f327e82612c2 }, sender: 0xe9f3aaeb5a90f230a4ce78be737bb6d678f9baac, public: Some(0x50bf94757a2d47462ecdb31020e14e5774015dfe144d78479383fcaf642a697c177c93bfff7293aa42118924c521d794e1104244df729935825e07b6f8429565) }
· 2024-05-09 06:31:49 +0000 [NFT_ETH] Waiting for confirmations… Confirmed.
· 2024-05-09 06:32:38 +0000 [NFT_ETH] Waiting for confirmations… Confirmed.
Transaction sent: SignedTransaction { transaction: UnverifiedTransaction { unsigned: Transaction { nonce: 63, gas_price: 2037141813, gas: 150000, action: Call(0xbac1c9f2087f39caaa4e93412c6412809186870e), value: 0, data: [66, 132, 46, 14, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 22, 226, 129, 169, 242, 231, 88, 18, 105, 161, 62, 81, 106, 167, 157, 106, 74, 28, 217, 128, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 233, 243, 170, 235, 90, 144, 242, 48, 164, 206, 120, 190, 115, 123, 182, 214, 120, 249, 186, 172, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1] }, v: 22310258, r: 56687763884389397417130400852405368812392203385583777532393960486988863832868, s: 22904028151235900081215252054745845209626830383396477128908044089325015673617, hash: 0xc5afbe4789ac9d79310f866b0e157a14c7b6d18c7f92af76664c37e65d043def }, sender: 0x16e281a9f2e7581269a13e516aa79d6a4a1cd980, public: Some(0x467212fad2a960aa28941eba4ffb78e1092064d40bf25ba78721fa0d60511e5d8348eb6fc7a03800c94ef6e1a7154f3a056f9e43b11f0711c89fc159b6966305) }
· 2024-05-09 06:33:56 +0000 [NFT_ETH] Waiting for confirmations… Timed out.
thread 'docker_tests::eth_docker_tests::send_and_spend_erc721_maker_payment' panicked at 'called `Result::unwrap()` on an `Err` value: "eth:4942] Timeout: eth:4942] Waited too long until 1715236435 for payment tx: c5afbe4789ac9d79310f866b0e157a14c7b6d18c7f92af76664c37e65d043def, for coin:NFT_ETH, to be confirmed!"', mm2src/mm2_main/tests/docker_tests/eth_docker_tests.rs:969:67

According to sepolia tx was successful https://sepolia.etherscan.io/tx/0xa2b9563048ab4ba9b6276f72fac881abe7be9693e51233de5623f327e82612c2 and Taker sent erc721 token back to Maker

UPD: yep, that works image

laruh avatar May 09 '24 07:05 laruh

@mariocynicys

Q: Why are we replacing geth node tests with sepolia?

At the beginning, we were using this one etomic smart contract version for NFT swaps, which supported both maker-NFT and taker-coin logic.

Later, we transitioned to a new Multi Standalone Smart Contract approach. The multi-standalone contracts strategy aims to call a specific smart contract for each swap participant (each contract contains only Maker or Taker swap methods) with a specific asset type (maker coin method / maker NFT method).

However, both NFT tests started failing after we began using the new maker NFT swap contract instead of the old one, despite the fact that the actual change was only the removal of the taker's methods from the smart contract. The maker NFT functions remained the same. I encountered the Waiting for confirmations… Failed. error, and the duration set for waiting for transaction confirmations did not make a difference.

However, both nft tests started to fail after using new maker nft swap contract instead of old one, despite the fact that actual change was in deleting taker's methods from smart contract. maker nft functions stayed the same. Had Waiting for confirmations… Failed. error and it doesnt matter how much time I set for waiting for transactions confirmation

In addition to increasing the sleep duration, I spent time on Geth node configuration changes and minor smart contract adjustments, but these did not resolve the issue. Once I deployed th enew maker NFT swap contract on the Sepolia test net, everything worked fine.

Therefore, I suppose the problem was in the test environment.

Also, does these sepolia test spend on fees? do they need refilling?

Yes, both maker and taker pay fees for transfers. Here are sepolia POW facets https://www.ethereum-ecosystem.com/faucets/ethereum-sepolia https://sepolia-faucet.pk910.de/

I suggest not ignoring tests even if they fail in CI, when the branch is merged to dev and other developers trigger CI with new commits. When necessary, we can mine some Sepolia ETH for the maker and taker addresses and periodically check to ensure that the tests are passing.

@shamardy is the above ^ ok for you?

laruh avatar May 10 '24 10:05 laruh

wip, as need some changes https://github.com/KomodoPlatform/komodo-defi-proxy/pull/22#issuecomment-2106158228

laruh avatar May 12 '24 08:05 laruh

PS: if someone will have more notes, I would like to fix them in this PR https://github.com/KomodoPlatform/komodo-defi-framework/pull/2129 to avoid conflicts

laruh avatar Jun 17 '24 07:06 laruh

@laruh I guess I will have to review, approve and then merge this PR #2129 first and also this PR KomodoPlatform/komodo-defi-proxy#22 before merging this. Is that correct?

@shamardy I think this #2100 PR can be merged in dev first, before #2129 if I dont have review notes which require me to refactor the whole architecture. Then I would prefer to do such big changes in #2129 (to avoid conflicts and messed up logic). In this case, yes we have to merge #2129 to #2100 and then #2100 to dev. If #2100 approved with non-blocker changes like your note, then I can do the changes right in this branch and merge it to #2129.

as for proxy PRs. I have two:

  • https://github.com/KomodoPlatform/komodo-defi-proxy/pull/22 this branch uses this feature actually
  • and this, related to Quicknode refactoring https://github.com/KomodoPlatform/komodo-defi-proxy/pull/23 #2129 PR supports Quicknode changes in Proxy layer. You can check it in PRs description.

To sum up: So if #2100 is approved without breaking changes for #2129 branch, then we can merge it to dev, and I will change merge branch to dev in #2129.

Proxies PRs: #2100 uses this feature https://github.com/KomodoPlatform/komodo-defi-proxy/pull/22 #2129 additionally started to use this pr quicknode refactoring https://github.com/KomodoPlatform/komodo-defi-proxy/pull/23

If #2100 will be fully approved, then it means that reviewers agreed with https://github.com/KomodoPlatform/komodo-defi-proxy/pull/22 logic, same for #2129 and https://github.com/KomodoPlatform/komodo-defi-proxy/pull/23

Proxy prs can get more non braking notes after komodefi being approved, but if reviewers decide to change the format of request to Proxy layer, then we have to update komodefi nft opened prs, or open new if previous were merged

laruh avatar Jun 22 '24 07:06 laruh

reqs examples how to enable nft, with platform coin and separately

curl --url "http://127.0.0.1:7783" --data '{
  "userpass": "'$USERPASS'",
  "method": "enable_eth_with_tokens",
  "mmrpc": "2.0",
  "params": {
    "ticker": "MATIC",
    "mm2": 1,
    "swap_contract_address": "0x9130b257D37A52E52F21054c4DA3450c72f595CE",
    "maker_swap_v2_contract": "0x9130b257D37A52E52F21054c4DA3450c72f595CE",
    "taker_swap_v2_contract": "0x9130b257D37A52E52F21054c4DA3450c72f595CE",
    "fallback_swap_contract": "0x9130b257D37A52E52F21054c4DA3450c72f595CE",
    "nodes": [
      {
        "url": "https://polygon-rpc.com"
      }
    ],
    "erc20_tokens_requests": [],
    "nft_req": {
      "provider":{
        "type": "Moralis",
        "info": {
          "url":"http://localhost:6150/nft-test",
          "proxy_auth":true
        }
      }
    }
  }
}'

curl --url "http://127.0.0.1:7783" --data '{
  "userpass": "'$USERPASS'",
  "method": "enable_nft",
  "mmrpc": "2.0",
  "params": {
    "ticker": "NFT_MATIC",
    "activation_params": {
      "provider":{
        "type": "Moralis",
        "info": {
          "url":"http://localhost:6150/nft-test",
          "proxy_auth":true
        }
      }
    }
  }
}'

laruh avatar Jun 24 '24 04:06 laruh

@laruh please fix merge conflicts so that I can give this one final look then approve and merge. I guess NFT will still work if we don't deploy komodefi proxy latest code, right? Also, since docs and coin config PRs are open, I can merge this PR without waiting for QA, unless some testing using new proxy code or for new enable method is needed first c.c. @KomodoPlatform/qa

shamardy avatar Jul 18 '24 03:07 shamardy

@shamardy thanks for pinging, will fix conflicts asap. i suppose they occurred after eth gas limit PR merge this night.

I guess NFT will still work if we don't deploy komodefi proxy latest code, right?

NFT feature should work in both cases: if latest komo proxy code deployed or not deployed. as currently https://moralis-proxy.komodo.earth endpoint doesnt have proxy limits, this url can be used with proxy_auth: false (this field even can be skipped in RPC as we use serde default annotation) Screenshot 2024-07-18 at 10 51 53

I would say its more about not merging this the second nft pr https://github.com/KomodoPlatform/komodo-defi-framework/pull/2129 to dev, as it contains Quicknode http request changes: Screenshot 2024-07-18 at 10 59 51

without the up to date proxy version quicknode gui_auth:true wont work

I can merge this PR without waiting for QA, unless some testing using new proxy code or for new enable method is needed first

Yes, I suppose this standalone nft pr could be merged to dev now, while the second nft pr with refund methods, v2 activation and Quicknode changes should be merged after @KomodoPlatform/qa team testing and docs, coins pr approve.

laruh avatar Jul 18 '24 04:07 laruh