Skip to content
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

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

Open
wants to merge 42 commits into
base: dev
Choose a base branch
from

Conversation

laruh
Copy link
Member

@laruh laruh commented Apr 24, 2024

related to issue #900

Etomic swap implementation approach was changed to Multi Standalone Etomic Swap contracts (see KomodoPlatform/etomic-swap#7 (comment))

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 ebac6e6 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 ebac6e6 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"]}}'

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 laruh added the in progress Changes will be made from the author label Apr 24, 2024
@laruh laruh self-assigned this Apr 24, 2024
@laruh laruh changed the title feat(nft-swap): standalone nft maker swap contract and sepolia testenet support feat(nft-swap): standalone nft maker swap contract and sepolia testenet Apr 24, 2024
@laruh laruh force-pushed the standalone-nft-maker-swap-contract-sepolia-test branch from 1fbe447 to fe498fa Compare April 26, 2024 14:07
@laruh laruh force-pushed the standalone-nft-maker-swap-contract-sepolia-test branch 2 times, most recently from 9dc1ea1 to 942d139 Compare April 29, 2024 08:12
@laruh laruh force-pushed the standalone-nft-maker-swap-contract-sepolia-test branch from 942d139 to 998708b Compare April 29, 2024 08:19
@laruh laruh force-pushed the standalone-nft-maker-swap-contract-sepolia-test branch from ee9c2c2 to 03860e1 Compare May 10, 2024 08:39
@laruh
Copy link
Member Author

laruh commented May 10, 2024

@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
Copy link
Member Author

laruh commented May 12, 2024

wip, as need some changes KomodoPlatform/komodo-defi-proxy#22 (comment)

@laruh laruh added under review and removed in progress Changes will be made from the author labels May 13, 2024
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! First review iteration:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of placing non-rust text files within the core layers seems not really good thing to do imho. I believe it would be better to move and maintain these in a separate repository and then include that repository as a submodule in this one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be add this as dependency? this will simplify the build process and CI configuration, as cargo will handle fetching the dependency itself

Copy link
Member Author

@laruh laruh May 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reread the suggestion and my response. I believe that moving the ABI JSONs out of the KomodoDeFi project would only complicate things.

  • keeping the ABI json files within the project makes the setup straightforward. There are no additional repositories or submodules to manage.
  • the ABI jsons are not changed independently from our core ETH logic. ETH functionality directly depends on these ABI files, so having them in the same repository ensures they are always in sync with the code.
  • adding ABI jsons as a submodule introduces an extra step in the CI/CD pipeline, which can complicate the build process.
  • Eth ABI json files are used only in our repository, so there is no strong need to move the ABIs to a separate repo. Moving them would only complicate the setup workflow and maintenance, as we would have to support different versions of the ABI json repo.

also @shamardy what do you think?

Comment on lines 79 to 82
/// Gui-auth specific data-type that needed in order to perform gui-auth calls.
/// Represents a signed message used for authenticating and validating requests processed by the proxy.
#[derive(Clone, Serialize)]
pub struct GuiAuthValidation {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Gui-auth specific data-type that needed in order to perform gui-auth calls.
/// Represents a signed message used for authenticating and validating requests processed by the proxy.
#[derive(Clone, Serialize)]
pub struct GuiAuthValidation {
/// komodo-defi-proxy specific data types including values that are being validated (e.g., verifying signed messages and request expirations) by the komodo-defi-proxy server.
#[derive(Clone, Serialize)]
pub struct KomodoDefiProxyPayload {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed the names of different types here 36eaf38

I renamed GuiAuthValidation to KomodefiProxyAuthValidation because the payload types were previously named AuthPayload (now QuicknodePayload) and HttpGetPayload (now MoralisPayload).

new payload names are the same as here
KomodoPlatform/komodo-defi-proxy@1c60030

Comment on lines 106 to 112
let mut attempts = 0;
loop {
if attempts >= 10 {
println!("Failed to connect to Geth node after several attempts.");
break;
}
match block_on(GETH_WEB3.eth().block_number()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the server responds very slowly, this can increase the amount of time we spend on tests significantly. We should add a timeout (~6 seconds for example) on the GETH_WEB3.eth().block_number() call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm here we decided to move from 5 attempts and 5s duration time to not to wait too long :D
#2100 (comment)
I suggest to decrease attempts to 5 and set 3s sleep duration. This should be enough.

This functionality was added when I had nft swap tests on Geth node. I thought that node may have issues with syncing so I decided to wait for the connection to be established to definitely eliminate this problem. But as further tests showed that the issue wasnt in Geth connection. Well nft swaps have problems in geth but node receives all requests.

actually, if you think that wait_for_geth_node_ready is just holding the process and increasing tests time we can remove this function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a timeout should be enough and simple, like this one:

match client.perform(HealthRequest).timeout(Duration::from_secs(15)).await {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added timeout
a2c47f8

@@ -72,6 +73,7 @@ pub fn docker_tests_runner(tests: &[&TestDescAndFn]) {
utxo_ops.wait_ready(4);
utxo_ops1.wait_ready(4);

wait_for_geth_node_ready();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function doesn't do anything other than holding the process for a while. Any reason to not to terminate the process if the target node isn't ready?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to not to terminate the process if the target node isn't ready?

Well, if the node isnt ready the calls in the init_geth_node() function will panic and terminate the process.
This was the idea to wait and give the init_geth_node calls a chance not to panic if the node is not ready right away.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, what I am asking is wait_for_geth_node_ready doesn't return any value, so we don't know if the node is ready or not. I was expecting to panic here if the target node isn't ready.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added panic if node isnt ready after several attempts a2c47f8

@@ -126,7 +126,7 @@ coins_activation = { path = "../coins_activation", features = ["for-tests"] }
mm2_test_helpers = { path = "../mm2_test_helpers" }
mocktopus = "0.8.0"
testcontainers = "0.15.0"
web3 = { git = "https://github.com/KomodoPlatform/rust-web3", tag = "v0.19.0", default-features = false, features = ["http"] }
web3 = { git = "https://github.com/KomodoPlatform/rust-web3", tag = "v0.19.0", default-features = false, features = ["http-tls"] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one of these https://github.com/KomodoPlatform/rust-web3/blob/01de1d732e61c920cfb2fb1533db7d7110c8a457/Cargo.toml#L78-L79 features adds less dependencies to the dependency stack. Please correct me if I am wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep rustls-tls is a bit smaller, http-native-tls doesnt change lock file, seems like its default-tls in reqwest.
added here 4478ccb

mm2src/coins/lp_coins.rs Outdated Show resolved Hide resolved
@laruh laruh force-pushed the standalone-nft-maker-swap-contract-sepolia-test branch from 5771333 to 350abd6 Compare May 14, 2024 12:11
Comment on lines 491 to 501
let validation_generator = ProxyAuthValidationGenerator {
coin_ticker: req.chain.to_nft_ticker().to_string(),
secret,
address: my_address,
};
let signed_message = EthCoin::generate_proxy_auth_signed_validation(validation_generator).map_err(|e| {
UpdateNftError::Internal(format!(
"KomodefiProxyAuthValidation signed message generation failed. Error: {:?}",
e
))
})?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if user decide to add his/her own nft endpoint? For example, on ETH we do this only if gui_auth param is true in the activation request.

Copy link
Member Author

@laruh laruh May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm yeah, right now id user decides to use komodefi they will have to setup komodo-proxy project.

will add proxy_auth/gui_auth param then in requests (I didnt change existing gui_auth field to not to break GUI eth reqs in prod). If false the direct Http GET request will be sent to Url.

UPD: ok Im thinking about this comment #2100 (comment) (proxy_auth bool VS headers)

Copy link
Member Author

@laruh laruh May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UPD2: answered in the message below #2100 (comment)

Comment on lines 661 to 667
// Create a new URL instance from uri_without_cursor and modify its query to include the cursor if present
let mut uri = uri_without_cursor.clone();
if !cursor.is_empty() {
uri.set_query(Some(&cursor));
}
let payload = moralis_payload_str(uri, wrapper.signed_message.clone())?;
let response = send_post_request_to_uri(wrapper.orig_url.as_str(), payload).await?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not change the way we send requests, otherwise this will not work when using endpoints without proxy. I guess you had to do this because you can't send auth payload in the get requests? Did you consider moving this payload to request headers instead of putting it into request body on the proxy side?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise this will not work when using endpoints without proxy

Hmm even if we decide to send a GET request with the signed message in the header to the proxy layer, we will still need to include a proxy_auth parameter (set to true or false) in the NFT request to correctly build the request on the Komodefi side.

I dont think its a good idea to always include the authentication message in the header and allow users to send GET requests directly to some third-party API. I mean, I dont want to provide this user's information in the header or body to anyone if it's not necessary, thats why we should add proxy_auth param and manage this logic.

Therefore, I suggest adding a proxy_auth boolean in the following places, and if it is false, we can send the HTTP GET request directly to the endpoint:

  • In the NFT provider enum:
#[derive(Clone, Deserialize)]
#[serde(tag = "type", content = "info")]
pub enum NftProviderEnum {
    Moralis { url: Url },
}
  • In NFT wallet RPCs

Anyway, I think both approaches—header vs. body—will require refactoring in both the Komodefi and proxy repositories. Additionally, since the proxy layer supports requests based on feature logic, the proxy will always need to parse and modify incoming requests according to the feature behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm even if we decide to send a GET request with the signed message in the header to the proxy layer, we will still need to include a proxy_auth parameter (set to true or false) in the NFT request to correctly build the request on the Komodefi side.

Don't we have an actiation process for NFTs so we can pass proxy_auth only once? Even if we don't have, you can simply add query param to url (something like xyz.com/some/endpoint?proxy_auth=true) or even better, add a header PROXY_AUTH=1 or PROXY_AUTH=true.

I dont think its a good idea to always include the authentication message in the header and allow users to send GET requests directly to some third-party API.

I didn't understand this.

I mean, I dont want to provide this user's information in the header or body to anyone if it's not necessary

That information doesn't access to the target proxy. Right now, we remove this payload from the body and also drop the certain headers from request before we sending it to proxy.

Copy link
Member Author

@laruh laruh May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have an actiation process for NFTs so we can pass proxy_auth only once?

No, also I think its better to allow user to decide does RPC need to use proxy or not. Actually the same idea is for Urls fields we ask them in every rpc.

Even if we don't have, you can simply add query param to url (something like xyz.com/some/endpoint?proxy_auth=true) or even better, add a header PROXY_AUTH=1 or PROXY_AUTH=true.

In KomodoDefi RPC if we want to support both functionality: proxy and direct requests to 3rd party API, user needs to highlight do they want to use proxy or not.

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",
    "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
        }
      }
    }
  }
}'

dont think its a good idea to always include the authentication message in the header and allow users to send GET requests directly to some third-party API.

I didn't understand this.

I mean, I dont want to provide this user's information in the header or body to anyone if it's not necessary

That information doesn't access to the target proxy. Right now, we remove this payload from the body and also drop the certain headers from request before we sending it to proxy.

I was talking about variant if we dont ask user to provide "proxy_auth" param in Komodefi RPC. It means that we dont know when to send request to proxy and when directly to 3rd party API.
KomoDefi RPC needs to have "proxy_auth" param. If its true then we build special request for Komodo Proxy layer, if false, then we send general http get request which we were doing all the time previously. Means we dont need to send req to proxy app to send it to 3rd party API.

When we are talking about nft feature, we use KomodoDefi-Proxy project to be able to send requests to komodo-moralis-proxy. If "proxy_auth" false, then user desires to send request not to komodo-moralis-proxy but directly to other 3rd party API.

I dont actually see the advantage of moving auth payload to headers. This way we have to check headers for one feature in proxy layer code and check body for other feature.
Proxy layer understands how to manage feature request by proxy_type: &ProxyType, so we dont need to over complicate the logic. If we send auth payload message for all features in body, we just need to parse payload from Body using one function
https://github.com/KomodoPlatform/komodo-defi-proxy/blob/d9922eeb44c694c160e2084d8ce0499bd86fde85/src/proxy/mod.rs#L40-L57

/// Asynchronously generates and organizes payload data from an HTTP request based on the specified proxy type.
/// This function ensures that requests are properly formatted to the correct service,
/// returning a tuple with the modified request and the structured payload.
pub(crate) async fn generate_payload_from_req(
    req: Request<Body>,
    proxy_type: &ProxyType,
) -> GenericResult<(Request<Body>, PayloadData)> {
    match proxy_type {
        ProxyType::Quicknode => {
            let (req, payload) = parse_payload::<QuicknodePayload>(req, false).await?;
            Ok((req, PayloadData::Quicknode(payload)))
        }
        ProxyType::Moralis => {
            let (req, payload) = parse_payload::<MoralisPayload>(req, true).await?;
            Ok((req, PayloadData::Moralis(payload)))
        }
    }
}

well if we have new EVM feature with more complex logic for Proxy layer, which cant use parse_payload, then we'll implement new parse function for it.

Copy link
Member Author

@laruh laruh May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if proxy_auth is false, then it suppose that user wants to send request directly to some other API

curl --url "http://127.0.0.1:7783" --data '{"userpass":"'$USERPASS'","method":"update_nft","mmrpc":"2.0","params":{"chains":["POLYGON"],"url":"http://other.example.com","url_antispam": "https://nft.antispam.dragonhound.info", "proxy_auth":false}}'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes it more complicated to maintain both KDF and KDP projects

If "proxy_auth" false -> use fn send_request_to_uri if "proxy_auth" true -> create payload str and use fn send_post_request_to_uri. proxy should handle the moralis feature with proxy_type and payload type

  1. When proxy_auth is true, KDF acts like it's using different service and changes the request method.
  2. KDP is taking the POST request and converting it into GET request.

These costs are pure complexities without any benefit. So why doing that?

Copy link
Member Author

@laruh laruh May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we had long discussion about default urls for nft and covered urls for gas provider a bit, but you can check one of the last comments #2049 (comment) from the thread

Copy link
Member Author

@laruh laruh May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes it more complicated to maintain both KDF and KDP projects

If "proxy_auth" false -> use fn send_request_to_uri if "proxy_auth" true -> create payload str and use fn send_post_request_to_uri. proxy should handle the moralis feature with proxy_type and payload type

1. When proxy_auth is true, KDF acts like it's using different service and changes the request method.

2. KDP is taking the POST request and converting it into GET request.

These costs are pure complexities without any benefit. So why doing that?

  1. When proxy_auth is true, KDF acts like it's using different service

Well yes :D this actually means that request is going to go through the middleware service (from my point of view of course), which is KDP: KDF -> KDP->komodo.moralis.proxy->Mortalis API.

If false, then send request directly to users API (this is the old way we were sending requests before this PR):
KDF -> 3rd party user's API -> Moralis API.
If proxy_auth false, user have to insert moralis priv key themself in 3rd party API, as KDF dont do it.

  1. KDP is taking the POST request and converting it into GET request.

yep, as Moralis feature requires GET reqs.

Ok lets find the compromise, as we have common goal to make a great project, right?

Lets send GET request with payload in header from KDF to KDP if "proxy_auth":true. On KDF side I suppose its better to reuse this function adding optional Header:

pub async fn send_request_to_uri(uri: &str) -> MmResult<Json, GetInfoFromUriError>

If "proxy_auth":false then send GET request using send_request_to_uri directly to url without additional header.

And the above should be for each req where we ask "url" field

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented new parse function in KDP KomodoPlatform/komodo-defi-proxy@e8f296d for features which handle auth signed message in Header

on KDF side added this functionality support for activation process 7882e6e

Will provide send_request_to_uri support for NFT Wallet RPCs (get nft list/transfers, update) tomorrow

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

provided proxy auth and 3rd party api support d03b6d1

this ff92a20 reduces some code duplication

@laruh laruh added in progress Changes will be made from the author and removed under review labels May 17, 2024
@laruh laruh added under review and removed in progress Changes will be made from the author labels May 17, 2024
laruh added 3 commits May 17, 2024 17:09
…e-nft-maker-swap-contract-sepolia-test

# Conflicts:
#	mm2src/coins/eth.rs
#	mm2src/coins/eth/nft_swap_v2/mod.rs
#	mm2src/coins/eth/v2_activation.rs
#	mm2src/mm2_main/Cargo.toml
#	mm2src/mm2_main/tests/docker_tests/eth_docker_tests.rs
…hcore_transaction::Action in eth_docker_tests.rs separately
@laruh laruh force-pushed the standalone-nft-maker-swap-contract-sepolia-test branch from b359ea7 to 9c3ff07 Compare May 21, 2024 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants