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(torii/graphql): use Connection abstraction to return data for ercBalance and ercTransfer query #2527

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions crates/torii/graphql/src/constants.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ pub const EVENT_MESSAGE_TABLE: &str = "event_messages";
pub const MODEL_TABLE: &str = "models";
pub const TRANSACTION_TABLE: &str = "transactions";
pub const METADATA_TABLE: &str = "metadata";
pub const ERC_BALANCE_TABLE: &str = "balances";
pub const ERC_TRANSFER_TABLE: &str = "erc_transfers";

pub const ID_COLUMN: &str = "id";
pub const EVENT_ID_COLUMN: &str = "event_id";
Expand Down
201 changes: 176 additions & 25 deletions crates/torii/graphql/src/object/erc/erc_balance.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,27 @@
use async_graphql::connection::PageInfo;
use async_graphql::dynamic::{Field, FieldFuture, InputValue, TypeRef};
use async_graphql::{Name, Value};
use convert_case::{Case, Casing};
use serde::Deserialize;
use sqlx::{FromRow, Pool, Sqlite, SqliteConnection};
use sqlx::sqlite::SqliteRow;
use sqlx::{FromRow, Pool, Row, Sqlite, SqliteConnection};
use starknet_crypto::Felt;
use torii_core::sql::utils::felt_to_sql_string;
use tracing::warn;

use crate::constants::{ERC_BALANCE_NAME, ERC_BALANCE_TYPE_NAME};
use super::handle_cursor;
use crate::constants::{
DEFAULT_LIMIT, ERC_BALANCE_NAME, ERC_BALANCE_TABLE, ERC_BALANCE_TYPE_NAME, ID_COLUMN,
};
use crate::mapping::ERC_BALANCE_TYPE_MAPPING;
use crate::object::connection::page_info::PageInfoObject;
use crate::object::connection::{
connection_arguments, cursor, parse_connection_arguments, ConnectionArguments,
};
use crate::object::{BasicObject, ResolvableObject};
use crate::query::data::count_rows;
use crate::query::filter::{Comparator, Filter, FilterValue};
use crate::query::order::{CursorDirection, Direction};
use crate::types::{TypeMapping, ValueMapping};
use crate::utils::extract;

Expand Down Expand Up @@ -38,41 +50,172 @@
TypeRef::named_nn(TypeRef::STRING),
);

let field = Field::new(self.name().0, TypeRef::named_list(self.type_name()), move |ctx| {
FieldFuture::new(async move {
let mut conn = ctx.data::<Pool<Sqlite>>()?.acquire().await?;
let address = extract::<Felt>(
ctx.args.as_index_map(),
&account_address.to_case(Case::Camel),
)?;
let mut field = Field::new(
self.name().0,
TypeRef::named(format!("{}Connection", self.type_name())),
move |ctx| {
FieldFuture::new(async move {
let mut conn = ctx.data::<Pool<Sqlite>>()?.acquire().await?;
let connection = parse_connection_arguments(&ctx)?;
let address = extract::<Felt>(
ctx.args.as_index_map(),
&account_address.to_case(Case::Camel),
)?;

Check warning on line 63 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L57-L63

Added lines #L57 - L63 were not covered by tests

let erc_balances = fetch_erc_balances(&mut conn, address).await?;
let filter = vec![Filter {
field: "account_address".to_string(),
comparator: Comparator::Eq,
value: FilterValue::String(felt_to_sql_string(&address)),
}];

Check warning on line 69 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L65-L69

Added lines #L65 - L69 were not covered by tests

Ok(Some(Value::List(erc_balances)))
})
})
let total_count =
count_rows(&mut conn, ERC_BALANCE_TABLE, &None, &Some(filter)).await?;

Check warning on line 72 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L71-L72

Added lines #L71 - L72 were not covered by tests

let (data, page_info) =
fetch_erc_balances(&mut conn, address, &connection, total_count).await?;

Check warning on line 75 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L74-L75

Added lines #L74 - L75 were not covered by tests

let results = erc_balance_connection_output(&data, total_count, page_info)?;

Check warning on line 77 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L77

Added line #L77 was not covered by tests

Ok(Some(Value::Object(results)))
})

Check warning on line 80 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L79-L80

Added lines #L79 - L80 were not covered by tests
},
)
.argument(argument);

field = connection_arguments(field);
vec![field]
}
}

async fn fetch_erc_balances(
conn: &mut SqliteConnection,
address: Felt,
) -> sqlx::Result<Vec<Value>> {
let query = "SELECT t.contract_address, t.name, t.symbol, t.decimals, b.balance, b.token_id, \
c.contract_type
FROM balances b
connection: &ConnectionArguments,
total_count: i64,
) -> sqlx::Result<(Vec<SqliteRow>, PageInfo)> {
let table_name = ERC_BALANCE_TABLE;
let id_column = format!("b.{}", ID_COLUMN);

let mut query = format!(
"SELECT b.id, t.contract_address, t.name, t.symbol, t.decimals, b.balance, b.token_id, \
c.contract_type
FROM {table_name} b

Check warning on line 102 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L93-L102

Added lines #L93 - L102 were not covered by tests
JOIN tokens t ON b.token_id = t.id
JOIN contracts c ON t.contract_address = c.contract_address
WHERE b.account_address = ?";
JOIN contracts c ON t.contract_address = c.contract_address"
);
let mut conditions = vec!["b.account_address = ?".to_string()];
Comment on lines +99 to +106
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure Safe SQL Query Construction to Prevent Injection Attacks

Ohayo, sensei! Constructing SQL queries using format! and string concatenation can introduce risks if any variables are user-controlled. Even though table_name and id_column are likely constants, it's a best practice to use parameterized queries or the query builders provided by sqlx to enhance security and prevent SQL injection attacks.

Consider modifying the query construction to use parameter binding instead of string interpolation. Here's an example:

let query = "
    SELECT b.id, t.contract_address, t.name, t.symbol, t.decimals, b.balance, b.token_id, c.contract_type
    FROM erc_balances b
    JOIN tokens t ON b.token_id = t.id
    JOIN contracts c ON t.contract_address = c.contract_address
    WHERE b.account_address = ?";

let mut stmt = sqlx::query(&query).bind(felt_to_sql_string(&address));


let mut cursor_param = &connection.after;
if let Some(after_cursor) = &connection.after {
conditions.push(handle_cursor(after_cursor, CursorDirection::After, ID_COLUMN)?);
}

Check warning on line 111 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L104-L111

Added lines #L104 - L111 were not covered by tests

if let Some(before_cursor) = &connection.before {
cursor_param = &connection.before;
conditions.push(handle_cursor(before_cursor, CursorDirection::Before, ID_COLUMN)?);
}

Check warning on line 116 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L113-L116

Added lines #L113 - L116 were not covered by tests

if !conditions.is_empty() {
query.push_str(&format!(" WHERE {}", conditions.join(" AND ")));
}

Check warning on line 120 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L118-L120

Added lines #L118 - L120 were not covered by tests

let is_cursor_based = connection.first.or(connection.last).is_some() || cursor_param.is_some();

Check warning on line 122 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L122

Added line #L122 was not covered by tests

let data_limit =
connection.first.or(connection.last).or(connection.limit).unwrap_or(DEFAULT_LIMIT);
let limit = if is_cursor_based {
match &cursor_param {
Some(_) => data_limit + 2,
None => data_limit + 1, // prev page does not exist

Check warning on line 129 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L124-L129

Added lines #L124 - L129 were not covered by tests
}
} else {
data_limit

Check warning on line 132 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L132

Added line #L132 was not covered by tests
};
Comment on lines +122 to +133
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify limit calculation logic.

Ohayo, sensei! The limit calculation logic could be simplified using a more declarative approach.

Consider this refactoring:

-let data_limit =
-    connection.first.or(connection.last).or(connection.limit).unwrap_or(DEFAULT_LIMIT);
-let limit = if is_cursor_based {
-    match &cursor_param {
-        Some(_) => data_limit + 2,
-        None => data_limit + 1, // prev page does not exist
-    }
-} else {
-    data_limit
-};
+let data_limit = connection.first
+    .or(connection.last)
+    .or(connection.limit)
+    .unwrap_or(DEFAULT_LIMIT);
+let limit = data_limit + match (is_cursor_based, cursor_param) {
+    (true, Some(_)) => 2,
+    (true, None) => 1,
+    (false, _) => 0,
+};
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let is_cursor_based = connection.first.or(connection.last).is_some() || cursor_param.is_some();
let data_limit =
connection.first.or(connection.last).or(connection.limit).unwrap_or(DEFAULT_LIMIT);
let limit = if is_cursor_based {
match &cursor_param {
Some(_) => data_limit + 2,
None => data_limit + 1, // prev page does not exist
}
} else {
data_limit
};
let is_cursor_based = connection.first.or(connection.last).is_some() || cursor_param.is_some();
let data_limit = connection.first
.or(connection.last)
.or(connection.limit)
.unwrap_or(DEFAULT_LIMIT);
let limit = data_limit + match (is_cursor_based, cursor_param) {
(true, Some(_)) => 2,
(true, None) => 1,
(false, _) => 0,
};


let rows = sqlx::query(query).bind(felt_to_sql_string(&address)).fetch_all(conn).await?;
let order_direction = match (connection.first, connection.last) {
(Some(_), _) => Direction::Desc,
(_, Some(_)) => Direction::Asc,
_ => Direction::Desc,

Check warning on line 138 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L135-L138

Added lines #L135 - L138 were not covered by tests
};
Comment on lines +135 to +139
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Ohayo, sensei! After reviewing the implementation, it appears that order_direction is determined solely by first and last parameters without considering before and after. This may lead to inconsistencies with Relay's Cursor Connections Specification.

  • Recommendation: Modify the order_direction logic to account for before and after parameters to ensure proper alignment with pagination behavior.
🔗 Analysis chain

Verify Pagination Direction Logic for Correctness

Ohayo, sensei! The determination of order_direction based on connection.first and connection.last might not align with expected pagination behaviors. Ensure that the ordering corresponds correctly to the first, last, before, and after parameters to comply with Relay's Cursor Connections Specification.

Here's a script to help verify the pagination logic:


Ohayo, sensei! To ensure the order_direction aligns correctly with all pagination parameters, let's examine how connection.before and connection.after are handled.


Ohayo, sensei! Let's ensure that order_direction is correctly influenced by both before and after pagination parameters to fully comply with Relay's specification.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that order_direction is correctly set based on pagination arguments.

# Test: Search for order_direction usage and related logic.
rg 'let order_direction.*' -A 10 crates/torii/graphql/src/object/erc/erc_balance.rs

Length of output: 481


Script:

#!/bin/bash
# Description: Verify handling of 'before' and 'after' pagination arguments.

rg 'connection\.(before|after)' crates/torii/graphql/src/object/erc/erc_balance.rs

Length of output: 279


Script:

#!/bin/bash
# Description: Check if 'order_direction' is modified or influenced by 'before' and 'after' parameters.

rg 'order_direction.*' crates/torii/graphql/src/object/erc/erc_balance.rs -A 10

Length of output: 703


let mut erc_balances = Vec::new();
query.push_str(&format!(" ORDER BY {id_column} {} LIMIT {limit}", order_direction.as_ref()));

Check warning on line 141 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L141

Added line #L141 was not covered by tests

if let Some(offset) = connection.offset {
query.push_str(&format!(" OFFSET {}", offset));
}

Check warning on line 145 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L143-L145

Added lines #L143 - L145 were not covered by tests

let mut data = sqlx::query(&query).bind(felt_to_sql_string(&address)).fetch_all(conn).await?;
let mut page_info = PageInfo {
has_previous_page: false,
has_next_page: false,
start_cursor: None,
end_cursor: None,
};

if data.is_empty() {
Ok((data, page_info))
} else if is_cursor_based {
match cursor_param {
Some(cursor_query) => {
let first_cursor = cursor::encode(
&data[0].try_get::<String, &str>(&id_column)?,
&data[0].try_get_unchecked::<String, &str>(&id_column)?,

Check warning on line 162 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L147-L162

Added lines #L147 - L162 were not covered by tests
);

if &first_cursor == cursor_query && data.len() != 1 {
data.remove(0);
page_info.has_previous_page = true;
} else {
data.pop();
}

Check warning on line 170 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L165-L170

Added lines #L165 - L170 were not covered by tests

if data.len() as u64 == limit - 1 {
page_info.has_next_page = true;
data.pop();
}

Check warning on line 175 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L172-L175

Added lines #L172 - L175 were not covered by tests
}
None => {
if data.len() as u64 == limit {
page_info.has_next_page = true;
data.pop();
}

Check warning on line 181 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L178-L181

Added lines #L178 - L181 were not covered by tests
}
}
Comment on lines +155 to +183
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Extract cursor-based pagination logic into a helper function.

Ohayo, sensei! The cursor-based pagination logic is complex and could benefit from being extracted into a dedicated helper function.

Consider creating a helper function:

fn handle_cursor_based_pagination(
    data: &mut Vec<SqliteRow>,
    cursor_param: &Option<String>,
    limit: u64,
    page_info: &mut PageInfo,
) -> sqlx::Result<()> {
    match cursor_param {
        Some(cursor_query) => {
            let first_cursor = cursor::encode(
                &data[0].try_get::<String, &str>(&id_column)?,
                &data[0].try_get_unchecked::<String, &str>(&id_column)?,
            );

            if &first_cursor == cursor_query && data.len() != 1 {
                data.remove(0);
                page_info.has_previous_page = true;
            } else {
                data.pop();
            }

            if data.len() as u64 == limit - 1 {
                page_info.has_next_page = true;
                data.pop();
            }
        }
        None => {
            if data.len() as u64 == limit {
                page_info.has_next_page = true;
                data.pop();
            }
        }
    }
    Ok(())
}


if !data.is_empty() {
page_info.start_cursor = Some(cursor::encode(
&data[0].try_get::<String, &str>(ID_COLUMN)?,
&data[0].try_get_unchecked::<String, &str>(ID_COLUMN)?,

Check warning on line 188 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L185-L188

Added lines #L185 - L188 were not covered by tests
));
page_info.end_cursor = Some(cursor::encode(
&data[data.len() - 1].try_get::<String, &str>(ID_COLUMN)?,
&data[data.len() - 1].try_get_unchecked::<String, &str>(ID_COLUMN)?,

Check warning on line 192 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L190-L192

Added lines #L190 - L192 were not covered by tests
));
}

Check warning on line 194 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L194

Added line #L194 was not covered by tests

Comment on lines +185 to +195
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify Cursor Encoding Logic for Clarity

Ohayo, sensei! The cursor encoding logic is repeated and could be simplified for better readability.

Consider extracting the cursor encoding into a helper function:

fn encode_cursor(row: &SqliteRow) -> sqlx::Result<String> {
    Ok(cursor::encode(
        &row.try_get::<String, &str>(ID_COLUMN)?,
        &row.try_get_unchecked::<String, &str>(ID_COLUMN)?,
    ))
}

Then update the code:

page_info.start_cursor = Some(encode_cursor(&data[0])?);
page_info.end_cursor = Some(encode_cursor(&data[data.len() - 1])?);

Ok((data, page_info))

Check warning on line 196 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L196

Added line #L196 was not covered by tests
} else {
let offset = connection.offset.unwrap_or(0);
if 1 < offset && offset < total_count as u64 {
page_info.has_previous_page = true;
}
if limit + offset < total_count as u64 {
page_info.has_next_page = true;
}

Check warning on line 204 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L198-L204

Added lines #L198 - L204 were not covered by tests

Ok((data, page_info))

Check warning on line 206 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L206

Added line #L206 was not covered by tests
}
}

Check warning on line 208 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L208

Added line #L208 was not covered by tests
lambda-0x marked this conversation as resolved.
Show resolved Hide resolved

for row in rows {
let row = BalanceQueryResultRaw::from_row(&row)?;
fn erc_balance_connection_output(
data: &[SqliteRow],
total_count: i64,
page_info: PageInfo,
) -> sqlx::Result<ValueMapping> {
let mut edges = Vec::new();
for row in data {
let row = BalanceQueryResultRaw::from_row(row)?;
let cursor = cursor::encode(&row.id, &row.id);

Check warning on line 218 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L210-L218

Added lines #L210 - L218 were not covered by tests

let balance_value = match row.contract_type.to_lowercase().as_str() {
"erc20" => {
Expand Down Expand Up @@ -116,10 +259,17 @@
}
};

erc_balances.push(balance_value);
edges.push(Value::Object(ValueMapping::from([
(Name::new("node"), balance_value),
(Name::new("cursor"), Value::String(cursor)),
])));

Check warning on line 265 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L262-L265

Added lines #L262 - L265 were not covered by tests
}

Ok(erc_balances)
Ok(ValueMapping::from([
(Name::new("totalCount"), Value::from(total_count)),
(Name::new("edges"), Value::List(edges)),
(Name::new("pageInfo"), PageInfoObject::value(page_info)),
]))

Check warning on line 272 in crates/torii/graphql/src/object/erc/erc_balance.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/object/erc/erc_balance.rs#L268-L272

Added lines #L268 - L272 were not covered by tests
}

// TODO: This would be required when subscriptions are needed
Expand All @@ -133,6 +283,7 @@
#[derive(FromRow, Deserialize, Debug, Clone)]
#[serde(rename_all = "camelCase")]
struct BalanceQueryResultRaw {
pub id: String,
pub contract_address: String,
pub name: String,
pub symbol: String,
Expand Down
Loading
Loading