Extend create allocation API #3031

scx1332 · 2024-01-30T17:52:15Z

resolves: #3023

nieznanysprawiciel · 2024-01-31T10:54:15Z

core/payment/src/api/allocations.rs

+                if let Err(err) = validate_driver(network, driver) {
+                    let err_msg = format!("Validate driver failed (1): {err}");
+                    log::error!("{}", err_msg);
+                    return response::bad_request(&err_msg);


You could simplify this. I would do:

Suggested change

if let Err(err) = validate_driver(network, driver) {

let err_msg = format!("Validate driver failed (1): {err}");

log::error!("{}", err_msg);

return response::bad_request(&err_msg);

if let Err(e) = validate_driver(network, driver).map_err(|e| format!("Validate driver failed (1): {err}")).log_err()

return response::bad_request(&e);

We have LogErr trait in yagna repository

nieznanysprawiciel · 2024-01-31T11:03:24Z

core/payment/src/api/allocations.rs

+    // payment_platform is of the form driver-network-token
+    // eg. erc20-rinkeby-tglm
+    let [driver, network, token]: [&str; 3] = match payment_platform
+        .split('-')
+        .collect::<Vec<_>>()
+        .try_into()
+    {
+        Ok(arr) => arr,
+        Err(_e) => {
+            let err_msg = format!("paymentPlatform must be of the form driver-network-token instead of {payment_platform}");
+            log::error!("{}", err_msg);
+            return response::bad_request(&err_msg);
        }
+    };
+
+    if let Err(err) = validate_network(network) {
+        let err_msg = format!("Validate network failed (2): {err}");
+        log::error!("{}", err_msg);
+        return response::bad_request(&err_msg);
+    }
+    if let Err(err) = validate_driver(network, driver) {
+        let err_msg = format!("Validate driver failed (2): {err}");
+        log::error!("{}", err_msg);
+        return response::bad_request(&err_msg);
+    }
+    if let Err(err) = validate_token(network, driver, token) {
+        let err_msg = format!("Validate token failed (2): {err}");
+        log::error!("{}", err_msg);
+        return response::bad_request(&err_msg);
+    }


From api you get these values already split. Why do you first build this string to dispatch it later?
You could avoid all these duplicated checks here

nieznanysprawiciel · 2024-01-31T11:06:44Z

core/payment/src/api/allocations.rs

+fn validate_network(network: &str) -> Result<(), String> {
+    match network {
+        "mainnet" => Ok(()),
+        "rinkeby" => Err("Rinkeby is no longer supported".to_string()),
+        "goerli" => Ok(()),
+        "holesky" => Ok(()),
+        "polygon" => Ok(()),
+        "mumbai" => Ok(()),
+        _ => Err(format!("Invalid network name: {network}")),
+    }
+}


i think you are duplicating a lot of code here. We have enum for network already, which could be just deserialized.

nieznanysprawiciel · 2024-01-31T11:06:59Z

core/payment/src/api/allocations.rs

+    }
+}
+
+fn validate_driver(_network: &str, driver: &str) -> Result<(), String> {


The same here. We have enum for driver

nieznanysprawiciel · 2024-01-31T11:10:47Z

core/payment/src/api/allocations.rs

+                    } else {
+                        log::debug!("Network not specified and token not specified, using default holesky");
+                        "holesky"
+                    }


Driver has get_default_network method. Probably you should use it, otherwise defaults will diverge.

nieznanysprawiciel · 2024-01-31T11:11:56Z

core/payment/src/api/allocations.rs

+        Some(PaymentPlatformEnum::PaymentPlatformName(name)) => {
+            log::debug!("Using old API payment platform name as pure str: {}", name);
+            name.clone()
+        }
+        Some(PaymentPlatformEnum::PaymentPlatform(p)) => {
+            if p.driver.is_none() && p.network.is_none() && p.token.is_none() {
+                log::debug!("Empty paymentPlatform object, using {DEFAULT_PAYMENT_PLATFORM}");
+                DEFAULT_PAYMENT_PLATFORM.to_string()
+            } else if p.token.is_some() && p.network.is_none() && p.driver.is_none() {
+                let token = p.token.as_ref().unwrap();
+                if token == "glm" {
+                    log::debug!("Selected network {DEFAULT_PAYMENT_PLATFORM_FOR_GLM} (default for glm token)");
+                    DEFAULT_PAYMENT_PLATFORM_FOR_GLM.to_string()
+                } else if token == "tglm" {
+                    log::debug!("Selected network {DEFAULT_PAYMENT_PLATFORM_FOR_TGLM} (default for tglm token)");
+                    DEFAULT_PAYMENT_PLATFORM_FOR_TGLM.to_string()
+                } else {
+                    let err_msg =
+                        format!("Only glm or tglm token values are accepted vs {token} provided");
+                    return response::bad_request(&err_msg);
+                }
+            } else {
+                let network = p.network.as_deref().unwrap_or_else(|| {
+                    if let Some(token) = p.token.as_ref() {
+                        if token == "glm" {
+                            log::debug!("Network not specified, using default polygon, because token set to glm");
+                            "polygon"
+                        } else {
+                            log::debug!("Network not specified, using default holesky");
+                            "holesky"
+                        }
+                    } else {
+                        log::debug!("Network not specified and token not specified, using default holesky");
+                        "holesky"
+                    }
+                });
+                if let Err(err) = validate_network(network) {
+                    let err_msg = format!("Validate network failed (1): {err}");
+                    log::error!("{}", err_msg);
+                    return response::bad_request(&err_msg);
+                }
+                let driver = p.driver.as_deref().unwrap_or_else(|| {
+                    log::debug!("Driver not specified, using default erc20");
+                    "erc20"
+                });
+                if let Err(err) = validate_driver(network, driver) {
+                    let err_msg = format!("Validate driver failed (1): {err}");
+                    log::error!("{}", err_msg);
+                    return response::bad_request(&err_msg);
+                }
+                if let Some(token) = p.token.as_ref() {
+                    if let Err(err) = validate_token(network, driver, token) {
+                        let err_msg = format!("Validate token failed (1): {err}");
+                        log::error!("{}", err_msg);
+                        return response::bad_request(&err_msg);
+                    }
+                    log::debug!("Selected network {driver}-{network}-{token}");
+                    format!("{}-{}-{}", driver, network, token)
+                } else {
+                    let default_token = match get_default_token(driver, network) {
+                        Ok(token) => token,
+                        Err(err) => {
+                            let err_msg = format!("Get default token failed (1): {err}");
+                            log::error!("{}", err_msg);
+                            return response::bad_request(&err_msg);
+                        }
+                    };
+                    log::debug!(
+                        "Selected network with default token {driver}-{network}-{default_token}"
+                    );
+                    format!("{}-{}-{}", driver, network, default_token)
+                }
+            }
+        }
+        None => {
+            log::debug!("No paymentPlatform entry found, using {DEFAULT_PAYMENT_PLATFORM}");
+            DEFAULT_PAYMENT_PLATFORM.to_string()
+        }
    };


This implementation is so complicated, that you should hide it in separate function

….com/golemfactory/yagna into scx1332/extend_create_allocation_api

Extend create allocation API

26304dc

scx1332 requested a review from a team as a code owner January 30, 2024 17:52

scx1332 added 9 commits January 30, 2024 19:20

f

ff46516

Merge branch 'master' into scx1332/extend_create_allocation_api

44804c7

f

d17cb5d

f

3c1afc1

f

12fc9ce

f

bbbe577

f

bb2eebf

f

99af695

f

442d0df

nieznanysprawiciel self-requested a review January 31, 2024 10:24

Merge branch 'master' into scx1332/extend_create_allocation_api

689626e

nieznanysprawiciel requested changes Jan 31, 2024

View reviewed changes

scx1332 added 6 commits January 31, 2024 18:07

Refactor using enums not hardcoded strings

7a8ec19

Merge branch 'scx1332/extend_create_allocation_api' of https://github…

f7c320f

….com/golemfactory/yagna into scx1332/extend_create_allocation_api

Refactor constants

1540a8e

Merge branch 'master' into scx1332/extend_create_allocation_api

9e8632a

refactor

8e25b45

Merge branch 'scx1332/extend_create_allocation_api' of https://github…

06cf63b

….com/golemfactory/yagna into scx1332/extend_create_allocation_api

scx1332 requested a review from nieznanysprawiciel January 31, 2024 21:34

scx1332 added 7 commits February 1, 2024 13:00

bad request and log

871d017

fx

6e21434

f

2123a4c

fmt

278e9a1

f

679a8c1

f

ccc0376

f

5a276c0

scx1332 merged commit 2631a77 into master Feb 2, 2024
15 checks passed

scx1332 deleted the scx1332/extend_create_allocation_api branch February 2, 2024 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend create allocation API #3031

Extend create allocation API #3031

scx1332 commented Jan 30, 2024 •

edited by nieznanysprawiciel

Loading

nieznanysprawiciel Jan 31, 2024

nieznanysprawiciel Jan 31, 2024

nieznanysprawiciel Jan 31, 2024

nieznanysprawiciel Jan 31, 2024

nieznanysprawiciel Jan 31, 2024

nieznanysprawiciel Jan 31, 2024

Extend create allocation API #3031

Extend create allocation API #3031

Conversation

scx1332 commented Jan 30, 2024 • edited by nieznanysprawiciel Loading

nieznanysprawiciel Jan 31, 2024

Choose a reason for hiding this comment

nieznanysprawiciel Jan 31, 2024

Choose a reason for hiding this comment

nieznanysprawiciel Jan 31, 2024

Choose a reason for hiding this comment

nieznanysprawiciel Jan 31, 2024

Choose a reason for hiding this comment

nieznanysprawiciel Jan 31, 2024

Choose a reason for hiding this comment

nieznanysprawiciel Jan 31, 2024

Choose a reason for hiding this comment

scx1332 commented Jan 30, 2024 •

edited by nieznanysprawiciel

Loading