Conversation
…general generate_payload_from_req func; doc comms; add url crate
…ize, and for general impl path in req uri have to represent only inbound_route
|
Here is the example of how to send request from komodo-defi using proxy layer for Nft feature. You should add enable_eth_with_tokens request examplecurl --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
} |
…_http_get, have general validation_middleware func which handle all PayloadData types
|
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
@onur-ozkan its r2r. |
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 I wanted to know if adding the UPD: "outbound_route" in configs for nft feature should be not moralis original base url, but komodo moralis proxy |
…rent stage only quicknode feature supports both websocket and http.
|
@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. |
shamardy
left a comment
There was a problem hiding this comment.
LGTM! Only one non-blocker comment from my side.
src/net/http.rs
Outdated
| // 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(); |
There was a problem hiding this comment.
Is this comment still needed? I see that you already use path()
There was a problem hiding this comment.
oh, yes, should remove it, as it was supposed to be an important note for reviewers on pr's early stages.
fixed it 226db21
…ft-support-refactor-quicknode
…node refactor quicknode: move SignedMessage to Header
onur-ozkan
left a comment
There was a problem hiding this comment.
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
| /// Value | ||
| pub(crate) const APPLICATION_JSON: &str = "application/json"; | ||
| /// Header | ||
| pub(crate) const X_FORWARDED_FOR: &str = "x-forwarded-for"; |
There was a problem hiding this comment.
| /// 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"; |
src/net/http.rs
Outdated
| 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(), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The problem is this will not work with the configuration other than
/somethinginbound.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_orwill be triggered andinbound_routebecome 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/specialand/nftin the configuration, both will returntrueforstarts_withon a request path likedemo.com/nft/special/1/2/3. In such cases, if multiple inbound routes returntrueforstarts_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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| Quicknode { | ||
| payload: QuicknodePayload, | ||
| signed_message: SignedMessage, | ||
| }, | ||
| /// Moralis feature requires only Signed Message in X-Auth-Payload header | ||
| Moralis(SignedMessage), |
There was a problem hiding this comment.
Since #23 this is same for quicknode too if I am not mistaken.
There was a problem hiding this comment.
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
| /// 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>( |
There was a problem hiding this comment.
Header is not part of the body, so function name and its documentation doesn't seem to be connected.
There was a problem hiding this comment.
changed namings for both parse functions e81cfb9
src/proxy/moralis.rs
Outdated
| // Remove the first path segment | ||
| let mut path_segments: Vec<&str> = req_uri | ||
| .path() | ||
| .split('/') | ||
| .filter(|s| !s.is_empty()) | ||
| .collect(); |
There was a problem hiding this comment.
I suppose this is the same reason why we skip the first segment during inbound route parsing.
There was a problem hiding this comment.
Yes, in modify_request_uri we should remove inbound_route from req uri and change base url
src/proxy/moralis.rs
Outdated
| /// 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( |
There was a problem hiding this comment.
Is there any particular reason for making this function async ?
There was a problem hiding this comment.
you are right, should should be sync e81cfb9
I did changes on komodefi side 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. |
|
last nightly lint started to fail. will check it UPD: fixed there 765f584 |
…y_route_by_uri_inbound` test, update modify_request_uri logic
src/ctx.rs
Outdated
| let remaining_path: String = path_segments.iter().skip(matched_segments).fold( | ||
| String::new(), | ||
| |mut acc, segment| { |
There was a problem hiding this comment.
You know the total size already - Can you consider preallocating String size here?
src/ctx.rs
Outdated
| 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(); |
There was a problem hiding this comment.
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);
}There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
onur-ozkan
left a comment
There was a problem hiding this comment.
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 (':
In the PR the Proxy layer handles the request method type according to the
"proxy_type"field specified in the application configuration.ProxyTypewas provided. It enumerates different proxy types supported by the application, focusing on separating feature logic.PayloadDatawas provided. It is supposed to handle different payload types related to request to proxy layer.docker-compose.ymlis configured for development and local test.TODOs
UPD:
validation_middleware_moralisworks in particular for multiple requests to NFT moralis endpoints. If rate exceed, you will getNOT_ACCEPTABLEerror and have to wait some time before sending more requests"rate_limiter"field to theProxyRoutestructure. This will allow us to provide unique rate limits for specific features if necessary. IfNoneproxy will just use the default"rate_limiter"params which you can find in Readme in current configuration file exampleRequest example
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
Run containers in the background (detached mode)
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
-for--followoption ensures that new log entries are continuously displayed as they are produced, while the-tor--timestampsoption adds timestamps to each log entry.Stop containers
Closes #21