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

Http Get support for NFT feature #22

Merged
merged 41 commits into from
Jul 8, 2024
Merged

Http Get support for NFT feature #22

merged 41 commits into from
Jul 8, 2024

Conversation

laruh
Copy link
Member

@laruh laruh commented May 5, 2024

In the PR the Proxy layer handles the request method type according to the "proxy_type" field specified in the application configuration.

  • enum ProxyType was provided. It enumerates different proxy types supported by the application, focusing on separating feature logic.
#[serde(rename_all = "snake_case")]  
pub(crate) enum ProxyType {  
    Quicknode,  
    Moralis,  
}
  • enum PayloadData was provided. It is supposed to handle different payload types related to request to proxy layer.
  • docker-compose.yml is configured for development and local test.

TODOs

  • validation_middleware for Moralis reqs
  • test ^ with multiple requests properly (this is more a note to the reviewers, that middleware still needs to be tested):
    UPD: validation_middleware_moralis works in particular for multiple requests to NFT moralis endpoints. If rate exceed, you will get NOT_ACCEPTABLE error and have to wait some time before sending more requests
  • add an optional "rate_limiter" field to the ProxyRoute structure. This will allow us to provide unique rate limits for specific features if necessary. If None proxy will just use the default "rate_limiter" params which you can find in Readme in current configuration file example
  • update readme

Request example

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",
    "swap_v2_contracts":{
	    "maker_swap_v2_contract": "0x9130b257D37A52E52F21054c4DA3450c72f595CE",
	    "taker_swap_v2_contract": "0x9130b257D37A52E52F21054c4DA3450c72f595CE",
	    "nft_maker_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
        }
      }
    }
  }
}'

PS: if you want to test feature locally, you can run docker containers with docker compose commands. DM me about nft url for app config.

Build image

docker compose build

Run containers in the background (detached mode)

docker compose up -d

Open a new terminal window or tab and execute this command to follow the logs of all services defined in a Docker Compose file. The -f or --follow option ensures that new log entries are continuously displayed as they are produced, while the -t or --timestamps option adds timestamps to each log entry.

docker compose logs -f -t

Stop containers

docker compose down

Closes #21

@laruh laruh self-assigned this May 5, 2024
@laruh
Copy link
Member Author

laruh commented May 7, 2024

Here is the example of how to send request from komodo-defi using proxy layer for Nft feature. You should add /nft path to proxy base url.

enable_eth_with_tokens request example
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"
        }
      }
    }
  }
}'
enable_eth_with_tokens response example
{
  "mmrpc": "2.0",
  "result": {
    "current_block": 56669925,
    "eth_addresses_infos": {
      "0xf622…b6a2": {
        "derivation_method": {
          "type": "Iguana"
        },
        "pubkey": "0412....b81",
        "balances": {
          "spendable": "1.89373072555993582",
          "unspendable": "0"
        }
      }
    },
    "erc20_addresses_infos": {
      "0xf622…b6a2": {
        "derivation_method": {
          "type": "Iguana"
        },
        "pubkey": "0412....b81",
        "balances": {}
      }
    },
    "nfts_infos": {
      "0x48c75fbf0452fa8ff2928ddf46b0fe7629cca2ff,5": {
        "token_address": "0x48c75fbf0452fa8ff2928ddf46b0fe7629cca2ff",
        "token_id": "5",
        "chain": "POLYGON",
        "contract_type": "ERC1155",
        "amount": "1"
      },
      "0xc06a6ab4c2b0ed14ed5692adf324a9a463259c43,0": {
        "token_address": "0xc06a6ab4c2b0ed14ed5692adf324a9a463259c43",
        "token_id": "0",
        "chain": "POLYGON",
        "contract_type": "ERC1155",
        "amount": "1"
      },
      "0x22d5f9b75c524fec1d6619787e582644cd4d7422,201": {
        "token_address": "0x22d5f9b75c524fec1d6619787e582644cd4d7422",
        "token_id": "201",
        "chain": "POLYGON",
        "contract_type": "ERC1155",
        "amount": "3000000000000000000"
      }
    }
  },
  "id": null
}

laruh added 2 commits May 8, 2024 14:29
…_http_get, have general validation_middleware func which handle all PayloadData types
@shamardy shamardy self-requested a review May 9, 2024 13:04
@laruh
Copy link
Member Author

laruh commented May 12, 2024

UPD: faced some issues in wasm target. Need to change nft reqs to regular POST format in komodefi repo and do some changes here.

@laruh laruh changed the title HTTP GET request support [wip] NFT feature support May 12, 2024
@laruh laruh changed the title [wip] NFT feature support Http Get req support May 12, 2024
@onur-ozkan
Copy link
Member

UPD: faced some issues in wasm target. Need to change nft reqs to regular POST format in komodefi repo and do some changes here.

Feel free to ping me once it's ready so I can make a review here.

…BLE error if rate exceed for HTTP GET request
@laruh
Copy link
Member Author

laruh commented May 13, 2024

Feel free to ping me once it's ready so I can make a review here.

@onur-ozkan its r2r.
made some changes to properly handle requests to the proxy by modifying incoming requests with specific payload data to use GET method.

@laruh
Copy link
Member Author

laruh commented May 23, 2024

Update: Just read the comment here #22 (comment) and it seems that this is what @onur-ozkan requested

Yes, thats what I wanted to clarify. The examples provided were related to quicknode requests, for which we dont need to catch and remove path segments. That was confusing.

I was uncertain whether handling two different logics (for one req type we can take uri.path() without checks, for another we have to get first path segment) within the general code, where we need to find ProxyRoute, is a good approach since it adds additional checks and manipulations with the URI.

I wanted to know if adding the inbound_route and then removing it is what we want (in the context of nft feature implementation)

UPD: "outbound_route" in configs for nft feature should be not moralis original base url, but komodo moralis proxy

@laruh
Copy link
Member Author

laruh commented May 24, 2024

@shamardy @onur-ozkan I suggest doing a Quicknode refactoring in HTTP headers as a separate issue with new pull requests (in the Defi framework and proxy repositories). This approach will make development and review easier. The pull request in the KomodoDefi Framework already contains a lot of code changes related to other features and should be merged first.

Copy link

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

LGTM! Only one non-blocker comment from my side.

src/net/http.rs Outdated
Comment on lines 198 to 201
// If we leave this code line `let proxy_route = match cfg.get_proxy_route_by_inbound(req.uri().to_string()) {`
// inbound_route cant be "/test", as it's not uri. I suppose inbound actually should be a Path.
// Two options: in `req.uri().to_string()` path() is missing or "/test" in test is wrong and the whole url should be.
let proxy_route = cfg.get_proxy_route_by_inbound("/test").unwrap();

Choose a reason for hiding this comment

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

Is this comment still needed? I see that you already use path()

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, yes, should remove it, as it was supposed to be an important note for reviewers on pr's early stages.
fixed it 226db21

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.

Do we have an automated deployment anywhere for this repository? @smk762 This PR contains lots of breaking changes, if we pull the latest changes to the live APIs, we should implement the necessary changes on kdf side before merging this.

src/net/http.rs Outdated
Comment on lines 14 to 17
/// Value
pub(crate) const APPLICATION_JSON: &str = "application/json";
/// Header
pub(crate) const X_FORWARDED_FOR: &str = "x-forwarded-for";
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
/// Value
pub(crate) const APPLICATION_JSON: &str = "application/json";
/// Header
pub(crate) const X_FORWARDED_FOR: &str = "x-forwarded-for";
/// Header value for `hyper::header::CONTENT_TYPE`
pub(crate) const APPLICATION_JSON: &str = "application/json";
/// Header key for `hyper::header::CONTENT_TYPE`
pub(crate) const X_FORWARDED_FOR: &str = "x-forwarded-for";

Copy link
Member Author

Choose a reason for hiding this comment

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

updated docs e81cfb9

src/net/http.rs Outdated
Comment on lines 96 to 98
let inbound_route = match req.method() {
// should take the second element as path string starts with a delimiter
&Method::GET => req.uri().path().split('/').nth(1).unwrap_or("").to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain why?

Copy link
Member Author

Choose a reason for hiding this comment

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

to extract inbound_route in GET method case

https://komodoproxy.com/nft-feature/api/v2.2/0x1f9090aaE28b8a3dCeaDf281B0F12828e676c326/nft/transfers?chain=eth&format=decimal&order=DESC

targets https://moralis.io/api/v2.2/0x1f9090aaE28b8a3dCeaDf281B0F12828e676c326/nft/transfers?chain=eth&format=decimal&order=DESC

In POST request it is easy to extract inbound_route, as its the whole path.
For GET we decided to include inbound_route in Url, not in header or body.
Which means that we will need to get inbound_route from Url and change Url on the step when we send req to target.

Copy link
Member

Choose a reason for hiding this comment

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

The problem is this will not work with the configuration other than /something inbound.

Let's say we have this configuration:

inbound_route: String::from_str("/").unwrap(),
outbound_route: "http://localhost:8000".to_string(),

or:

inbound_route: String::from_str("/something/else").unwrap(),
outbound_route: "http://localhost:8000".to_string(),

in this case, unwrap_or will be triggered and inbound_route become an empty string.

What about doing this: Iterate over the inbound routes from the configuration and check if the request path starts with any of them. Once you find the matching inbound route, extract that part from the request path and add it to the outbound route.

There is an edge case in this logic that needs to be handled; if you have inbound routes like /nft/special and /nft in the configuration, both will return true for starts_with on a request path like demo.com/nft/special/1/2/3. In such cases, if multiple inbound routes return true for starts_with, we should always pick the longest one.

Copy link
Member Author

@laruh laruh Jun 27, 2024

Choose a reason for hiding this comment

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

The problem is this will not work with the configuration other than /something inbound.

Let's say we have this configuration:

inbound_route: String::from_str("/").unwrap(),
outbound_route: "http://localhost:8000".to_string(),

or:

inbound_route: String::from_str("/something/else").unwrap(),
outbound_route: "http://localhost:8000".to_string(),

in this case, unwrap_or will be triggered and inbound_route become an empty string.

What about doing this: Iterate over the inbound routes from the configuration and check if the request path starts with any of them. Once you find the matching inbound route, extract that part from the request path and add it to the outbound route.

There is an edge case in this logic that needs to be handled; if you have inbound routes like /nft/special and /nft in the configuration, both will return true for starts_with on a request path like demo.com/nft/special/1/2/3. In such cases, if multiple inbound routes return true for starts_with, we should always pick the longest one.

The easiest way was just include inbound string in Header, then you can have any inbound_route what you want. So you dont have to do check iterations and dont do complex re creation of Url, just change base url

Copy link
Member Author

@laruh laruh Jun 27, 2024

Choose a reason for hiding this comment

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

I still can do some changes on kdf side and move proxy inbound route to header

But if you prefer to have /path1/path2/etc rout right in Path, I will do the inbound routs search to find the exact match of /path1/path2/etc inbound in Path. But then I suggest to do the req url change in get_proxy_route_by_inbound function and remove matching from Url, so we wont need to search the matching again to remove it in modify_request_uri function.

Copy link
Member

@onur-ozkan onur-ozkan Jun 27, 2024

Choose a reason for hiding this comment

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

I still can do some changes on kdf side and move proxy inbound route to header

That brings the old discussion where we talk about "putting inbound route to the request". For ref, I suggest to look how other layer7 proxy servers work (e.g., nginx, traefik, etc).

But if you prefer to have /path1/path2/etc rout right in Path, I will do the inbound routs search to find the exact match of /path1/path2/etc inbound in Path.

In practice, we can not have every possible route registered in the configuration, unfortunately.

Copy link
Member Author

@laruh laruh Jun 27, 2024

Choose a reason for hiding this comment

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

In practice, we can not have every possible route registered in the configuration, unfortunately.

Didnt get it, after finding the list of matched routs (by first segment), I suggest to go trough each path element until we exclude all matched elements.
If there are two routs in config: /nft, /nft/service1/, and request Url matches only the first variant
https://komodoproxy.com/nft/api/v2.2/0x1f9090aaE28b8a3dCeaDf281B0F12828e676c326/nft/transfers?chain=eth&format=decimal&order=DESC
We should stop at service1 as match for this element is not found

Copy link
Member Author

Choose a reason for hiding this comment

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

Done here 6422433

src/proxy/mod.rs Outdated
Comment on lines 33 to 38
Quicknode {
payload: QuicknodePayload,
signed_message: SignedMessage,
},
/// Moralis feature requires only Signed Message in X-Auth-Payload header
Moralis(SignedMessage),
Copy link
Member

Choose a reason for hiding this comment

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

Since #23 this is same for quicknode too if I am not mistaken.

Copy link
Member Author

Choose a reason for hiding this comment

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

It means that Moralis feature in Proxy layer needs only sig message from us, while Quicknode needs sig message + payload. I will add doc comm for Quicknode and update Moralis doc to avoid confusion

src/proxy/mod.rs Outdated
Comment on lines 132 to 137
/// Parses the request body and the `X-Auth-Payload` header into a payload and signed message.
///
/// This function extracts the `X-Auth-Payload` header from the request, parses it into a `SignedMessage`,
/// and then reads and deserializes the request body into a specified type `T`.
/// If the body is empty or the header is missing, an error is returned.
async fn parse_body_payload<T>(
Copy link
Member

Choose a reason for hiding this comment

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

Header is not part of the body, so function name and its documentation doesn't seem to be connected.

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 namings for both parse functions e81cfb9

Comment on lines 101 to 106
// Remove the first path segment
let mut path_segments: Vec<&str> = req_uri
.path()
.split('/')
.filter(|s| !s.is_empty())
.collect();
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this is the same reason why we skip the first segment during inbound route parsing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, in modify_request_uri we should remove inbound_route from req uri and change base url

/// Modifies the URI of an HTTP request by replacing its base URI with the outbound URI specified in `ProxyRoute`,
/// while incorporating the path and query parameters from the original request URI. Additionally, this function
/// removes the first path segment from the original URI.
async fn modify_request_uri(
Copy link
Member

Choose a reason for hiding this comment

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

Is there any particular reason for making this function async ?

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right, should should be sync e81cfb9

@laruh
Copy link
Member Author

laruh commented Jun 27, 2024

This PR contains lots of breaking changes, if we pull the latest changes to the live APIs, we should implement the necessary changes on kdf side before merging this

I did changes on komodefi side
KomodoPlatform/komodo-defi-framework#2100 - contains moralis changes
KomodoPlatform/komodo-defi-framework#2129 - quicknode changes

of course we should test #2129 before merging it in dev (I mean 2129 already contains 2100 changes, so we can test both moralis and quicknode updates with it) using some test proxy service.

@laruh
Copy link
Member Author

laruh commented Jun 27, 2024

https://github.com/KomodoPlatform/komodo-defi-proxy/actions/runs/9691846400/job/26744135818?pr=22#step:4:331

last nightly lint started to fail. will check it

UPD: fixed there 765f584
it is related to issue rust-lang/rust#123748

…y_route_by_uri_inbound` test, update modify_request_uri logic
src/ctx.rs Outdated
Comment on lines 128 to 130
let remaining_path: String = path_segments.iter().skip(matched_segments).fold(
String::new(),
|mut acc, segment| {
Copy link
Member

Choose a reason for hiding this comment

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

You know the total size already - Can you consider preallocating String size here?

src/ctx.rs Outdated
Comment on lines 105 to 116
for r in &self.proxy_routes {
let route_segments: Vec<&str> = r
.inbound_route
.split('/')
.filter(|s| !s.is_empty())
.collect();
// Count the number of segments that match between the route and the path
let matched_segments = route_segments
.iter()
.zip(&path_segments)
.take_while(|(route_seg, path_seg)| route_seg == path_seg)
.count();
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker but depending on the request and route configuration, this can cause lots of iterations (from parent to sub iterations).

I did something like this, which should be more sufficient:

fn find_route_from_url(incoming_url: &Url, routes: &[Route]) -> Option<Route> {
    let path = incoming_url.path();
    let mut matched_route: Option<&Route> = None;

    for route in routes {
        if path.starts_with(&route.inbound) {
            if let Some(r) = matched_route {
                if r.inbound.len() > route.inbound.len() {
                    continue;
                }
            }
            matched_route = Some(route);
        }
    }

    matched_route.cloned()
}

fn inbound_url_to_outbound_url(inbound_url: Url, route: &Route) -> Url {
    let inbound_url_str = inbound_url.to_string();
    let path = inbound_url_str
        .split_once(inbound_url.host_str().unwrap())
        .expect("inbound url must contain a host")
        .1;

    let path = path
        .strip_prefix(&route.inbound)
        .expect("Route doesn't match with the given inbound url.");

    let url = format!(
        "{}/{}",
        route.outbound.strip_suffix('/').unwrap_or(&route.outbound),
        path.strip_prefix('/').unwrap_or(&path)
    );

    Url::parse(&url).expect("valid url")
}

the test function:

#[test]
fn find_best_match() {
    fn test_helper(inbound_url: Url, routes: &[Route]) -> String {
        let route = find_route_from_url(&inbound_url, routes).unwrap();
        let result = inbound_url_to_outbound_url(inbound_url, &route);
        result.to_string()
    }

    let routes = vec![
        Route {
            inbound: "/".into(),
            outbound: "https://k.com".into(),
        },
        Route {
            inbound: "/demo".into(),
            outbound: "https://kmd.com".into(),
        },
        Route {
            inbound: "/demo/demo".into(),
            outbound: "https://komodo.com".into(),
        },
        Route {
            inbound: "/demo/demo/demo".into(),
            outbound: "https://kdf.com/komodo".into(),
        },
    ];

    let inbound = Url::parse("https://rproxy-app.service/hello/there").unwrap();
    let expected_outbound_url = "https://k.com/hello/there";
    let actual_outbound_url = test_helper(inbound, &routes);
    assert_eq!(actual_outbound_url, expected_outbound_url);

    let inbound = Url::parse("https://rproxy-app.service/demo/some/stuff/").unwrap();
    let expected_outbound_url = "https://kmd.com/some/stuff/";
    let actual_outbound_url = test_helper(inbound, &routes);
    assert_eq!(actual_outbound_url, expected_outbound_url);

    let inbound = Url::parse("https://rproxy-app.service/demo/demo/okay?q=123").unwrap();
    let expected_outbound_url = "https://komodo.com/okay?q=123";
    let actual_outbound_url = test_helper(inbound, &routes);
    assert_eq!(actual_outbound_url, expected_outbound_url);

    let inbound = Url::parse("https://rproxy-app.service/demo/demo/demo/1/2/3").unwrap();
    let expected_outbound_url = "https://kdf.com/komodo/1/2/3";
    let actual_outbound_url = test_helper(inbound, &routes);
    assert_eq!(actual_outbound_url, expected_outbound_url);
}

Copy link
Member Author

@laruh laruh Jul 2, 2024

Choose a reason for hiding this comment

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

I did something like this, which should be more sufficient

in your example you use Url type from url crate. Do you suggest to add this dependency? Currently we use only Uri

upd: I suppose I can do it with Uri

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it should be possible with uri crate too (if not feel free to add Url 2.2.2 as it's already in our dependency stack).

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in this commit e3d81fa

moved delete inbound from Uri logic back to modify uri function, so get route just gets proxy route, modify function modifies Uri

…, move delete inbound from Uri to modify_request_uri
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.

It feels quite risky but I assume we don't do any automatic deployments for this repository. So let's ship this and hope nothing breaks.

I wish we had already resolved #16 (':

@onur-ozkan onur-ozkan merged commit 547b493 into main Jul 8, 2024
24 checks passed
@onur-ozkan onur-ozkan deleted the nft-support branch July 8, 2024 10:25
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.

generic proxy implementation to support diff req and feture types
3 participants