feat sum #65
feat sum #65
Conversation
WalkthroughThe changes introduce a new summation capability to the codebase by adding a Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Engine as ExecutionEngine
participant Get as GetExpression
Caller->>Engine: run(Expression::Sum)
Engine->>Engine: run_sum_expr(SumExpression)
Engine->>Engine: run_get_expr(SumExpression.query)
Engine-->>Caller: ExpressionResult
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
crates/core/src/common/types.rs (1)
1-103: Implement parsing for SumExpression.The
SumExpressionlacks parsing implementation unlikeGetExpression.Add TryFrom implementation for parsing:
impl TryFrom<Pairs<'_, Rule>> for SumExpression { type Error = GetExpressionError; fn try_from(pairs: Pairs<'_, Rule>) -> Result<Self, Self::Error> { let get_expr = GetExpression::try_from(pairs)?; Ok(Self::new(get_expr)?) } }crates/core/src/interpreter/backend/execution_engine.rs (1)
77-432: Add tests for Sum functionality.The new
Sumfunctionality lacks test coverage.Add test cases for:
- Summing account balances
- Summing block gas used
- Error cases for non-numeric fields
- Error cases for invalid queries
Example test:
#[tokio::test] async fn test_sum_account_balances() { let execution_engine = ExecutionEngine::new(); let expressions = vec![Expression::Sum(SumExpression::new( GetExpression { entity: Entity::Account( Account::new( Some(vec![ NameOrAddress::Address(address!("0x1")), NameOrAddress::Address(address!("0x2")), ]), None, vec![AccountField::Balance], ) ), chains: vec![ChainOrRpc::Chain(Chain::Ethereum)], dump: None, } ).unwrap())]; let result = execution_engine.run(expressions).await.unwrap(); assert_eq!(result[0].result, ExpressionResult::Sum(/* expected sum */)); }
🧹 Nitpick comments (2)
crates/core/src/common/types.rs (2)
13-14: LGTM! Consider adding documentation.The
Sumvariant is correctly added to theExpressionenum.Add documentation to explain the purpose and usage of the
Sumvariant:#[derive(Debug, PartialEq)] pub enum Expression { Get(GetExpression), + /// Represents a summation expression that aggregates values from a query Sum(SumExpression), }
17-20: Add documentation and consider making fields private.The struct definition looks good, but could benefit from documentation and encapsulation.
+/// Represents a summation expression that aggregates values from a query #[derive(Debug, PartialEq)] pub struct SumExpression { - pub query: GetExpression, + query: GetExpression, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/core/src/common/types.rs(1 hunks)crates/core/src/interpreter/backend/execution_engine.rs(3 hunks)
🔇 Additional comments (1)
crates/core/src/interpreter/backend/execution_engine.rs (1)
39-42: LGTM! Consider adding error handling.The match arm correctly handles the
Sumvariant.Consider adding error handling for cases where the query result cannot be summed.
| impl SumExpression { | ||
| fn new(query: GetExpression) -> Self { | ||
| Self { | ||
| query | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Make constructor public and add input validation.
The constructor should be public to allow creating instances from other modules.
impl SumExpression {
- fn new(query: GetExpression) -> Self {
+ pub fn new(query: GetExpression) -> Result<Self, GetExpressionError> {
+ // Validate that the query returns numeric fields that can be summed
Self {
query
}
}
+
+ pub fn query(&self) -> &GetExpression {
+ &self.query
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| async fn run_sum_expr( | ||
| &self, | ||
| expr: &SumExpression, | ||
| ) -> Result<ExpressionResult> { | ||
| let query_result = self.run_get_expr(&expr.query).await?; | ||
|
|
||
| Ok(query_result) | ||
| } |
There was a problem hiding this comment.
Implement summation logic.
The method currently just returns the query result without performing any summation.
async fn run_sum_expr(
&self,
expr: &SumExpression,
) -> Result<ExpressionResult> {
let query_result = self.run_get_expr(&expr.query).await?;
-
- Ok(query_result)
+ match query_result {
+ ExpressionResult::Account(accounts) => {
+ // Sum numeric fields from accounts
+ }
+ ExpressionResult::Block(blocks) => {
+ // Sum numeric fields from blocks
+ }
+ // Handle other variants
+ _ => Err(anyhow::anyhow!("Cannot sum non-numeric fields")),
+ }
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/cli/src/repl.rs (1)
244-249: Consider using a unique color for sum results.Currently, sum results use the same green color as account results. Consider using a different color (e.g., magenta) to help users distinguish between different result types.
ExpressionResult::Sum(query_res) => { let table = to_table(query_res)?; table.to_string().split("\n").for_each(|line| { - queue!(stdout(), MoveToNextLine(1), Print(line.green())).unwrap(); + queue!(stdout(), MoveToNextLine(1), Print(line.magenta())).unwrap(); }); }crates/core/src/common/query_result.rs (1)
33-44: Add documentation for the SumQueryRes struct.Consider adding documentation to explain the purpose and usage of the struct, including:
- What the
sumfield represents- When it might be
None- Example usage
Apply this diff to add documentation:
#[serde_with::skip_serializing_none] #[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Clone)] +/// Represents the result of a sum query operation. +/// +/// # Fields +/// * `sum` - The calculated sum value. `None` if no valid sum could be computed. +/// +/// # Example +/// ``` +/// let result = SumQueryRes { +/// sum: Some(U256::from(100)), +/// }; +/// ``` pub struct SumQueryRes { pub sum: Option<U256>, }crates/core/src/interpreter/backend/execution_engine.rs (1)
99-160: Enhance test coverage with more realistic test cases.The current test cases have a few limitations:
- They use hardcoded transaction hashes which might become invalid.
- They expect a sum of 0, which might not effectively test the summation logic.
Consider:
- Using test fixtures or mock data instead of hardcoded values.
- Adding test cases with non-zero expected sums.
- Adding test cases for error conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/cli/src/main.rs(1 hunks)crates/cli/src/repl.rs(1 hunks)crates/core/src/common/query_result.rs(1 hunks)crates/core/src/common/serializer.rs(2 hunks)crates/core/src/interpreter/backend/execution_engine.rs(4 hunks)crates/core/src/interpreter/backend/mod.rs(1 hunks)crates/core/src/interpreter/backend/resolve_sum_query.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/core/src/interpreter/backend/mod.rs
🔇 Additional comments (6)
crates/cli/src/main.rs (1)
61-63: LGTM!The implementation correctly handles the new
Sumvariant, following the same pattern as other result types.crates/core/src/common/serializer.rs (1)
28-29: Add test coverage for Sum serialization.The serialization implementation for the
Sumvariant lacks test coverage. Please add tests for both CSV and Parquet serialization of sum results.Example test structure:
#[test] fn test_serialize_sum_csv() { let res = vec![ SumQueryRes { sum: Some(U256::from_str("100").unwrap()), }, ]; let content = serialize_csv(&res).unwrap(); assert_eq!(content, "sum\n100\n"); } #[test] fn test_serialize_sum_parquet() { let res = SumQueryRes { sum: Some(U256::from_str("100").unwrap()), }; let result = ExpressionResult::Sum(vec![res]); let content = serialize_parquet(&result).unwrap(); assert!(!content.is_empty()); }Also applies to: 63-64
crates/core/src/common/query_result.rs (1)
28-30: LGTM!The
Sumvariant is correctly added to theExpressionResultenum with proper serde annotation.crates/core/src/interpreter/backend/execution_engine.rs (3)
6-6: LGTM!The imports are correctly organized and necessary for the new sum functionality.
Also applies to: 9-9
40-43: LGTM!The match arm for
Expression::Sumfollows the established pattern and correctly integrates with the existing code.
68-75: LGTM!The implementation now correctly delegates to
resolve_sum_queryfor handling the summation logic.
| //let mut result = Vec::new(); | ||
|
|
||
| // for chain in chains { | ||
| // let provider = Arc::new(ProviderBuilder::new().on_http(chain.rpc_url()?)); | ||
|
|
||
| // // TODO: Handle filter | ||
| // // TODO: Remove unwrap | ||
| // for account_id in account.ids().unwrap() { | ||
| // let fields = account.fields().clone(); | ||
| // let provider = provider.clone(); | ||
|
|
||
| // let account_future = async move { | ||
| // match account_id { | ||
| // NameOrAddress::Address(address) => { | ||
| // get_account(address, fields, &provider, chain).await | ||
| // } | ||
| // NameOrAddress::Name(name) => { | ||
| // let address = to_address(name).await?; | ||
| // get_account(&address, fields, &provider, chain).await | ||
| // } | ||
| // } | ||
| // }; | ||
|
|
||
| // all_account_futures.push(account_future); | ||
| // } | ||
| // } | ||
|
|
||
| // let account_res = try_join_all(all_account_futures).await?; | ||
| let res = ExpressionResult::Sum(vec![SumQueryRes::default()]); | ||
| Ok(res) | ||
| } |
There was a problem hiding this comment.
Implement the commented-out code.
The function currently returns a default result without implementing the actual summation logic. The commented-out code should be properly implemented to handle multiple chains and account IDs concurrently.
Would you like me to help implement the complete solution for handling multiple chains and account IDs concurrently?
| pub fn resolve_sum_query( | ||
| exp: &ExpressionResult, | ||
| ) -> Result<ExpressionResult>{ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and improve documentation.
The function lacks input validation and return type documentation.
/// Resolve the query to get accounts after receiving an account entity expression
/// Iterate through entity_ids and map them to a futures list. Execute all futures concurrently and collect the results.
+///
+/// # Arguments
+/// * `exp` - The expression result to process
+///
+/// # Returns
+/// * `Result<ExpressionResult>` - The sum query result or an error
pub fn resolve_sum_query(
exp: &ExpressionResult,
) -> Result<ExpressionResult>{
+ // Validate input
+ match exp {
+ ExpressionResult::Account(_) => (),
+ _ => return Err(anyhow::anyhow!("Expected Account expression result")),
+ }📝 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.
| pub fn resolve_sum_query( | |
| exp: &ExpressionResult, | |
| ) -> Result<ExpressionResult>{ | |
| /// Resolve the query to get accounts after receiving an account entity expression | |
| /// Iterate through entity_ids and map them to a futures list. Execute all futures concurrently and collect the results. | |
| /// | |
| /// # Arguments | |
| /// * `exp` - The expression result to process | |
| /// | |
| /// # Returns | |
| /// * `Result<ExpressionResult>` - The sum query result or an error | |
| pub fn resolve_sum_query( | |
| exp: &ExpressionResult, | |
| ) -> Result<ExpressionResult> { | |
| // Validate input | |
| match exp { | |
| ExpressionResult::Account(_) => (), | |
| _ => return Err(anyhow::anyhow!("Expected Account expression result")), | |
| } | |
| // TODO: Implement the actual sum query resolution logic | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/core/src/interpreter/backend/resolve_sum_query.rs (1)
25-28:⚠️ Potential issueImplement the summation logic.
The function currently returns a default result without implementing the actual summation logic.
Would you like me to help implement the complete solution for handling multiple chains and account IDs concurrently?
crates/core/src/interpreter/backend/execution_engine.rs (1)
71-75:⚠️ Potential issueImplement the summation logic.
The method currently just returns the query result without performing any summation.
Apply this diff to implement the summation logic:
async fn run_sum_expr( &self, expr: &SumExpression, ) -> Result<ExpressionResult> { let query_result = self.run_get_expr(&expr.query).await?; - let result = resolve_sum_query(&query_result)?; - Ok(result) + match query_result { + ExpressionResult::Account(accounts) => { + // Sum numeric fields from accounts + resolve_sum_query(&query_result) + } + ExpressionResult::Block(blocks) => { + // Sum numeric fields from blocks + resolve_sum_query(&query_result) + } + // Handle other variants + _ => Err(anyhow::anyhow!("Cannot sum non-numeric fields")), + } }
🧹 Nitpick comments (1)
crates/core/src/interpreter/backend/execution_engine.rs (1)
102-131: Improve test cases with realistic values.The test cases use hardcoded expected values of 0 for the sum. Consider using realistic values that demonstrate the actual summation functionality.
Apply this diff to improve the test cases:
let expected = vec![SumQueryRes { - sum: Some(U256::from(0)), + sum: Some(U256::from(1000000000000000000_u128)), // 1 ETH }];Also applies to: 134-167
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/core/src/common/logs.rs(1 hunks)crates/core/src/common/query_result.rs(1 hunks)crates/core/src/common/types.rs(1 hunks)crates/core/src/interpreter/backend/execution_engine.rs(14 hunks)crates/core/src/interpreter/backend/mod.rs(1 hunks)crates/core/src/interpreter/backend/resolve_sum_query.rs(1 hunks)crates/macros/src/lib.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/macros/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/core/src/interpreter/backend/mod.rs
- crates/core/src/common/query_result.rs
- crates/core/src/common/types.rs
🔇 Additional comments (2)
crates/core/src/common/logs.rs (1)
145-146: Verify the address checksum implementation.The change in the
Address::to_checksumcall might affect address validation. Let's verify if this change aligns with the latest version of thealloylibrary.✅ Verification successful
🌐 Web query:
What is the correct usage of Address::to_checksum in the latest version of the alloy library?💡 Result:
The
Address::to_checksummethod in alloy-primitives v0.1.0+ is used to generate EIP-55/EIP-1191 compliant checksummed Ethereum addresses. Here's the correct usage:use alloy_primitives::{address, Address}; // Create an address from raw bytes or using the address! macro let addr = address!("0xd8da6bf26964af9d7eed9e03e53415d37aa96045"); // Basic EIP-55 checksum (no chain ID) let checksum = addr.to_checksum(None); assert_eq!(checksum, "0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045"); // EIP-1191 checksum with chain ID (e.g., Ethereum Mainnet) let chain_specific = addr.to_checksum(Some(1)); assert_eq!(chain_specific, "0xD8Da6bf26964Af9d7EEd9e03e53415d37AA96045");Key considerations:
- Chain ID Handling: Pass
Some(chain_id)for EIP-1191 chain-specific checksums[1][4]- Validation First: Use
Address::parse_checksummedto verify addresses before processing[1][4]- Formatting Options:
Displaytrait usesto_checksum(None)[1]Debugtrait shows raw lowercase hex[1]- Use
format!("{:#}")for compressed checksum (0xabcd...1234)[1]For address validation:
let parsed = Address::parse_checksummed("0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045", None)?;The implementation handles both EIP-55 (basic checksum) and EIP-1191 (chain-aware) formats automatically based on whether a chain ID is provided[1][2][4]. Always prefer checksummed addresses when displaying to users to prevent errors[3][8].
Citations:
- 1: https://docs.rs/alloy-primitives/latest/alloy_primitives/struct.Address.html
- 2: https://docs.ethers.org/v5/api/utils/address/
- 3: https://coincodex.com/article/2078/ethereum-address-checksum-explained/
- 4: https://lib.rs/crates/alloy-primitives
- 5: https://learnmeabitcoin.com/technical/keys/checksum/
- 6: foundry-rs/foundry#6772
- 7: https://docs.rs/alloy-primitives/latest/alloy_primitives/
- 8: ethers-io/ethers.js#3497
- 9: ethereum/EIPs#55
- 10: https://www.paradigm.xyz/2023/06/alloy
Address Checksum Implementation Verified
The usage of
Address::to_checksum(&Address::from_str(s)?, None)is correct. According to the latest alloy-primitives documentation, callingto_checksumwithNoneis the right way to generate an EIP-55 checksummed address when no specific chain ID is required.
- The method signature and usage in your code match the documented example.
- If chain-specific formatting (EIP-1191) is needed, the method should be called with
Some(chain_id)instead.crates/core/src/interpreter/backend/execution_engine.rs (1)
38-41: LGTM!The changes to the
runmethod correctly handle the newSumvariant by delegating to therun_sum_exprmethod.
| #[derive(Debug, Serialize, Deserialize, thiserror::Error)] | ||
| pub enum AccountResolverErrors { | ||
| #[error("Mismatch between Entity and EntityId, {0} can't be resolved as a account id")] | ||
| MismatchEntityAndEntityId(String), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Utilize the error enum for error handling.
The AccountResolverErrors enum is defined but not used in the function. Consider using it for proper error handling when resolving sum queries.
Apply this diff to add error handling:
pub fn resolve_sum_query(exp: &ExpressionResult) -> Result<ExpressionResult> {
+ match exp {
+ ExpressionResult::Account(_) => {
+ // Handle account summation
+ let res = ExpressionResult::Sum(vec![SumQueryRes::default()]);
+ Ok(res)
+ }
+ _ => Err(anyhow::anyhow!(AccountResolverErrors::MismatchEntityAndEntityId(
+ "Cannot sum non-account entities".to_string(),
+ ))),
+ }
- let res = ExpressionResult::Sum(vec![SumQueryRes::default()]);
- Ok(res)
}📝 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.
| #[derive(Debug, Serialize, Deserialize, thiserror::Error)] | |
| pub enum AccountResolverErrors { | |
| #[error("Mismatch between Entity and EntityId, {0} can't be resolved as a account id")] | |
| MismatchEntityAndEntityId(String), | |
| } | |
| pub fn resolve_sum_query(exp: &ExpressionResult) -> Result<ExpressionResult> { | |
| match exp { | |
| ExpressionResult::Account(_) => { | |
| // Handle account summation | |
| let res = ExpressionResult::Sum(vec![SumQueryRes::default()]); | |
| Ok(res) | |
| } | |
| _ => Err(anyhow::anyhow!(AccountResolverErrors::MismatchEntityAndEntityId( | |
| "Cannot sum non-account entities".to_string(), | |
| ))), | |
| } | |
| } |
1 participant
Summary
The SumExpression is designed to SUM the entities
Related Issue
#61
Technical details
Topics for Discussion
PR Checklist
Screenshots (if applicable)
Summary by CodeRabbit