Skip to content

test common::subgraph #1742

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+68 −35
Open

test common::subgraph #1742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e6e8c3
test common::subgraph
0xgleb May 6, 2025
bbaa8e6
add suggested changes
0xgleb May 8, 2025
0a3b6c2
Merge branch 'main' into test-common-subgraph
0xgleb May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions crates/cli/src/commands/order/detail.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ impl Execute for CliOrderDetailArgs {
async fn execute(&self) -> Result<()> {
let subgraph_args: SubgraphArgs = self.subgraph_args.clone().into();
let order = subgraph_args
.to_subgraph_client()
.await?
.to_subgraph_client()?
.order_detail(self.order_id.clone().into())
.await?;
let order_extended: OrderDetailExtended = order.try_into()?;
Expand Down
6 changes: 2 additions & 4 deletions crates/cli/src/commands/order/list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ impl Execute for CliOrderListArgs {

if self.pagination_args.csv {
let csv_text = subgraph_args
.to_subgraph_client()
.await?
.to_subgraph_client()?
.orders_list_all()
.await?
.into_iter()
Expand All @@ -43,8 +42,7 @@ impl Execute for CliOrderListArgs {
} else {
let table = build_table(
subgraph_args
.to_subgraph_client()
.await?
.to_subgraph_client()?
.orders_list(
self.filter_args.clone().into(),
self.pagination_args.clone().into(),
Expand Down
3 changes: 1 addition & 2 deletions crates/cli/src/commands/order/remove.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ impl Execute for CliOrderRemoveArgs {
async fn execute(&self) -> Result<()> {
let subgraph_args: SubgraphArgs = self.subgraph_args.clone().into();
let order = subgraph_args
.to_subgraph_client()
.await?
.to_subgraph_client()?
.order_detail(self.order_id.clone().into())
.await?;
let remove_order_args: RemoveOrderArgs = order.into();
Expand Down
3 changes: 1 addition & 2 deletions crates/cli/src/commands/trade/detail.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ impl Execute for CliOrderTradeDetailArgs {
async fn execute(&self) -> Result<()> {
let subgraph_args: SubgraphArgs = self.subgraph_args.clone().into();
let order_take = subgraph_args
.to_subgraph_client()
.await?
.to_subgraph_client()?
.order_trade_detail(self.id.clone().into())
.await?;
info!("{:#?}", order_take);
Expand Down
6 changes: 2 additions & 4 deletions crates/cli/src/commands/trade/list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ impl Execute for CliOrderTradesListArgs {

if self.pagination_args.csv {
let csv_text = subgraph_args
.to_subgraph_client()
.await?
.to_subgraph_client()?
.order_trades_list_all(self.order_id.clone().into(), None, None)
.await?
.into_iter()
Expand All @@ -43,8 +42,7 @@ impl Execute for CliOrderTradesListArgs {
} else {
let table = build_table(
subgraph_args
.to_subgraph_client()
.await?
.to_subgraph_client()?
.order_trades_list(
self.order_id.clone().into(),
self.pagination_args.clone().into(),
Expand Down
3 changes: 1 addition & 2 deletions crates/cli/src/commands/vault/detail.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ impl Execute for CliVaultDetailArgs {
async fn execute(&self) -> Result<()> {
let subgraph_args: SubgraphArgs = self.subgraph_args.clone().into();
let vault = subgraph_args
.to_subgraph_client()
.await?
.to_subgraph_client()?
.vault_detail(self.vault_id.clone().into())
.await?;
info!("{:#?}", vault);
Expand Down
6 changes: 2 additions & 4 deletions crates/cli/src/commands/vault/list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@ impl Execute for CliVaultListArgs {

if self.pagination_args.csv {
let vaults = subgraph_args
.to_subgraph_client()
.await?
.to_subgraph_client()?
.vaults_list_all()
.await?;
let vaults_flattened: Vec<TokenVaultFlattened> = vaults
Expand All @@ -46,8 +45,7 @@ impl Execute for CliVaultListArgs {
let pagination_args: SgPaginationArgs = self.pagination_args.clone().into();
let filter_args = self.filter_args.clone().into();
let vaults = subgraph_args
.to_subgraph_client()
.await?
.to_subgraph_client()?
.vaults_list(filter_args, pagination_args)
.await?;
let vaults_flattened: Vec<TokenVaultFlattened> = vaults
Expand Down
6 changes: 2 additions & 4 deletions crates/cli/src/commands/vault/list_balance_changes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ impl Execute for CliVaultBalanceChangesList {

if self.pagination_args.csv {
let csv_text = subgraph_args
.to_subgraph_client()
.await?
.to_subgraph_client()?
.vault_balance_changes_list_all(self.vault_id.clone().into())
.await?
.into_iter()
Expand All @@ -43,8 +42,7 @@ impl Execute for CliVaultBalanceChangesList {
} else {
let table = build_table(
subgraph_args
.to_subgraph_client()
.await?
.to_subgraph_client()?
.vault_balance_changes_list(
self.vault_id.clone().into(),
self.pagination_args.clone().into(),
Expand Down
48 changes: 47 additions & 1 deletion crates/common/src/subgraph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,53 @@ pub struct SubgraphArgs {
}

impl SubgraphArgs {
pub async fn to_subgraph_client(&self) -> Result<OrderbookSubgraphClient, ParseError> {
pub fn to_subgraph_client(&self) -> Result<OrderbookSubgraphClient, ParseError> {
Ok(OrderbookSubgraphClient::new(Url::parse(self.url.as_str())?))
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_to_subgraph_client_ok() {
let url = "https://api.thegraph.com/subgraphs/name/org1/sg1";
let subgraph_args = SubgraphArgs {
url: url.to_string(),
};
let subgraph_client = subgraph_args.to_subgraph_client().unwrap();
assert_eq!(subgraph_client.url().as_str(), url);

let url = "https://api.thegraph.com/subgraphs/name/org1/sg1?version=latest&format=json";
let subgraph_args = SubgraphArgs {
url: url.to_string(),
};
let subgraph_client = subgraph_args.to_subgraph_client().unwrap();
assert_eq!(subgraph_client.url().as_str(), url);
}

#[test]
fn test_to_subgraph_client_err() {
let url = "api.thegraph.com/subgraphs/name/org1/sg1".to_string();
let subgraph_args = SubgraphArgs { url };
let err = subgraph_args.to_subgraph_client().unwrap_err();
assert_eq!(err, ParseError::RelativeUrlWithoutBase);

let url = "https:///".to_string();
let subgraph_args = SubgraphArgs { url };
let err = subgraph_args.to_subgraph_client().unwrap_err();
assert_eq!(err, ParseError::EmptyHost);

let url = "".to_string();
let subgraph_args = SubgraphArgs { url };
let err = subgraph_args.to_subgraph_client().unwrap_err();
// Empty URL results in RelativeUrlWithoutBase error
assert_eq!(err, ParseError::RelativeUrlWithoutBase);

let url = ":".to_string();
let subgraph_args = SubgraphArgs { url };
let err = subgraph_args.to_subgraph_client().unwrap_err();
assert_eq!(err, ParseError::RelativeUrlWithoutBase);
}
Comment on lines +37 to +59
Copy link
Contributor

@coderabbitai coderabbitai bot May 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider using test parameterization for error cases

While the current implementation is good, consider refactoring the error test cases using a parameterized approach to reduce repetition in the test code.

#[test]
fn test_to_subgraph_client_err() {
    let test_cases = vec![
        ("api.thegraph.com/subgraphs/name/org1/sg1", ParseError::RelativeUrlWithoutBase),
        ("https:///", ParseError::EmptyHost),
        ("", ParseError::RelativeUrlWithoutBase), // Empty URL results in RelativeUrlWithoutBase error
        (":", ParseError::RelativeUrlWithoutBase),
    ];

    for (url, expected_error) in test_cases {
        let subgraph_args = SubgraphArgs { url: url.to_string() };
        let err = subgraph_args.to_subgraph_client().unwrap_err();
        assert_eq!(err, expected_error, "Failed for URL: {}", url);
    }
}

This approach makes it easier to add more test cases in the future and reduces code duplication.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the test is unlikely to be changed a lot so this isn't necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

@0xgleb Got it! If the test is unlikely to change much, keeping the current implementation is perfectly fine. The existing tests are already well-structured and cover the necessary scenarios effectively.

(^‿^)

}
5 changes: 5 additions & 0 deletions crates/subgraph/src/orderbook_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ impl From<OrderbookSubgraphClientError> for JsValue {
}
}

#[derive(Debug)]
pub struct OrderbookSubgraphClient {
url: Url,
}
Expand All @@ -72,6 +73,10 @@ impl OrderbookSubgraphClient {
Self { url }
}

pub fn url(&self) -> &Url {
&self.url
}

/// Fetch single order
pub async fn order_detail(&self, id: Id) -> Result<SgOrder, OrderbookSubgraphClientError> {
let data = self
Expand Down
5 changes: 1 addition & 4 deletions tauri-app/src-tauri/src/commands/order.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ pub async fn orders_list_write_csv(
subgraph_args: SubgraphArgs,
) -> CommandResult<()> {
let orders = subgraph_args
.to_subgraph_client()
.await?
.to_subgraph_client()?
.orders_list_all()
.await?;
let orders_flattened: Vec<OrderFlattened> = orders
Expand Down Expand Up @@ -62,7 +61,6 @@ pub async fn order_remove(
) -> CommandResult<()> {
let order = subgraph_args
.to_subgraph_client()
.await
.map_err(|e| {
toast_error(app_handle.clone(), String::from("Subgraph URL is invalid"));
e
Expand Down Expand Up @@ -115,7 +113,6 @@ pub async fn order_remove_calldata(
) -> CommandResult<Bytes> {
let order = subgraph_args
.to_subgraph_client()
.await
.map_err(|e| {
toast_error(app_handle.clone(), String::from("Subgraph URL is invalid"));
e
Expand Down
3 changes: 1 addition & 2 deletions tauri-app/src-tauri/src/commands/order_take.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ pub async fn order_trades_list_write_csv(
end_timestamp: Option<u64>,
) -> CommandResult<()> {
let order_takes = subgraph_args
.to_subgraph_client()
.await?
.to_subgraph_client()?
.order_trades_list_all(order_id.clone().into(), start_timestamp, end_timestamp)
.await?;
let order_takes_flattened: Vec<OrderTakeFlattened> = order_takes
Expand Down
6 changes: 2 additions & 4 deletions tauri-app/src-tauri/src/commands/vault.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ pub async fn vaults_list_write_csv(
subgraph_args: SubgraphArgs,
) -> CommandResult<()> {
let vaults = subgraph_args
.to_subgraph_client()
.await?
.to_subgraph_client()?
.vaults_list_all()
.await?;
let vaults_flattened: Vec<TokenVaultFlattened> =
Expand All @@ -42,8 +41,7 @@ pub async fn vault_balance_changes_list_write_csv(
subgraph_args: SubgraphArgs,
) -> CommandResult<()> {
let data = subgraph_args
.to_subgraph_client()
.await?
.to_subgraph_client()?
.vault_balance_changes_list_all(id.into())
.await?;
let data_flattened: Vec<VaultBalanceChangeFlattened> =
Expand Down