Graph ADO (version 1.1.0) and Point ADO #576
Conversation
…eda-core into mdjakovic/graph-ado
WalkthroughThe pull request introduces extensive updates to the Andromeda data storage project, including new modules for managing maps and coordinates, enhancements to existing ADOs, and improvements to the permissioning system. It features the addition of various ADOs, enums, and queries, alongside a comprehensive smart contract implementation for the Andromeda Graph module. Testing utilities and mock implementations have been added to support robust testing. The changes also address multiple bugs and refine existing functionalities, contributing to a more efficient framework for data management. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 42
🧹 Outside diff range and nitpick comments (24)
contracts/data-storage/andromeda-point/.cargo/config (1)
1-4: LGTM! Consider adding optimization flags for WASM builds.The Cargo aliases are well-defined and will streamline common development tasks. They cover essential operations: building for WebAssembly, running unit tests, and generating the schema.
For the
wasmalias, consider adding optimization flags to ensure the smallest possible binary size, which is crucial for smart contracts. You could modify it as follows:-wasm = "build --release --target wasm32-unknown-unknown" +wasm = "build --release --target wasm32-unknown-unknown --no-default-features --features producer"This change removes default features and adds a hypothetical "producer" feature, which is a common pattern in smart contract projects to minimize the contract size.
contracts/data-storage/andromeda-graph/src/lib.rs (2)
1-2: LGTM! Consider adding module-level documentation.The public modules
contractandstatealign well with the PR objectives for introducing the Graph ADO. This modular structure promotes better organization and separation of concerns.Consider adding module-level documentation comments (//!) for these public modules to provide a brief overview of their purposes and contents. This would enhance the codebase's maintainability and make it easier for other developers to understand the project structure.
4-5: LGTM! Consider adding a brief comment explaining the conditional compilation.The conditional compilation for the
mockmodule is well-structured, ensuring it's only included for testing purposes and not in WebAssembly builds. This aligns with best practices for separating test code from production code.Consider adding a brief comment above the
#[cfg]attribute to explain why this module is conditionally compiled. For example:// Mock module for testing, excluded from WebAssembly builds #[cfg(all(not(target_arch = "wasm32"), feature = "testing"))] pub mod mock;This would provide quick context for other developers reviewing or working on the code.
contracts/data-storage/andromeda-graph/examples/schema.rs (2)
3-10: LGTM: Main function correctly generates the API schema.The
mainfunction effectively uses thewrite_api!macro to generate the API schema for the Andromeda data storage graph. It correctly specifies the message types for instantiation, querying, and execution.Consider removing the empty line (line 8) within the
write_api!macro call for better code consistency:fn main() { write_api! { instantiate: InstantiateMsg, query: QueryMsg, execute: ExecuteMsg, - } }
1-10: Overall assessment: Well-structured schema generation for Graph ADO.This file effectively sets up the schema generation for the Graph ADO, which is a key component introduced in this PR. It correctly imports the necessary types and uses the
write_api!macro to define the API schema. The implementation aligns well with CosmWasm conventions and the PR objectives.As the project grows, consider maintaining a consistent schema generation approach across all ADOs to ensure uniformity and ease of maintenance.
contracts/data-storage/andromeda-point/src/state.rs (2)
5-7: LGTM: Constants are well-defined and cover necessary state items.The constants
DATA,DATA_OWNER, andRESTRICTIONare appropriately defined usingcw_storage_plus::Itemfor efficient state management. They cover the essential aspects of the Point ADO: coordinate data, ownership, and access restrictions.For consistency, consider using all uppercase for the string identifiers:
-pub const DATA: Item<PointCoordinate> = Item::new("data"); -pub const DATA_OWNER: Item<Addr> = Item::new("data_owner"); -pub const RESTRICTION: Item<PointRestriction> = Item::new("restriction"); +pub const DATA: Item<PointCoordinate> = Item::new("DATA"); +pub const DATA_OWNER: Item<Addr> = Item::new("DATA_OWNER"); +pub const RESTRICTION: Item<PointRestriction> = Item::new("RESTRICTION");This aligns with the common convention of using uppercase for constant values and improves readability.
1-7: Overall implementation is solid and aligns with PR objectives.This
state.rsfile effectively sets up the core state management for the Point ADO, as described in the PR objectives. It defines storage for three-dimensional coordinates (DATA), ownership (DATA_OWNER), and access restrictions (RESTRICTION), which align perfectly with the Point ADO requirements.The use of
cw_storage_plus::Itemfor state management is a good practice in CosmWasm contract development, ensuring efficient and type-safe storage operations.As the project grows, consider creating a separate
types.rsfile forPointCoordinateandPointRestrictionif they're not already in one. This can improve modularity and make it easier to reuse these types across different parts of the project.contracts/data-storage/andromeda-graph/Cargo.toml (1)
18-23: LGTM! Features are well-defined, but consider adding more documentation.The features section is well-structured and provides useful options for development and testing. The "backtraces" and "testing" features are clear in their purpose.
However, the "library" feature's purpose might not be immediately clear to all developers. Consider adding a brief comment or documentation explaining when and why one might want to use this feature.
contracts/data-storage/andromeda-point/src/testing/mock.rs (4)
18-29: LGTM:proper_initializationfunction is well-implemented.The function correctly sets up the mock environment and instantiates the contract. It's a good practice to return both the mock dependencies and the message info for further testing.
Consider adding a brief comment explaining the purpose of this function at the beginning, e.g.:
/// Initializes the mock environment and instantiates the contract with the given PointRestriction. /// Returns the mock dependencies and message info for further testing. pub fn proper_initialization(restriction: PointRestriction) -> (MockDeps, MessageInfo) { // ... (existing implementation) }
31-37: LGTM:query_pointfunction is correctly implemented.The function properly queries the contract and handles both success and error cases.
Consider simplifying the error handling by using the
?operator instead of pattern matching:pub fn query_point(deps: Deps) -> Result<PointCoordinate, ContractError> { let res = query(deps, mock_env(), QueryMsg::GetPoint {})?; Ok(from_json(res)?) }This change would make the function more concise and idiomatic Rust.
39-62: LGTM:set_pointandset_point_with_fundsfunctions are well-implemented.Both functions correctly create and execute the
SetPointmessage, withset_point_with_fundsproviding the additional functionality of including funds.Consider using a single function with an optional
fundsparameter to reduce code duplication:pub fn set_point( deps: DepsMut<'_>, point: &PointCoordinate, sender: &str, funds: Option<Coin>, ) -> Result<Response, ContractError> { let msg = ExecuteMsg::SetPoint { point: point.clone(), }; let info = match funds { Some(coin) => mock_info(sender, &[coin]), None => mock_info(sender, &[]), }; execute(deps, mock_env(), info, msg) }This change would combine the functionality of both functions, making the code more maintainable.
1-68: Overall, the mock testing utilities are well-implemented and comprehensive.This file provides a solid foundation for testing the Andromeda data storage contract. The functions cover initialization, querying, setting, and deleting points, which should allow for thorough testing of the contract's functionality.
Consider adding the following to further enhance the testing capabilities:
- A function to query multiple points or all points stored in the contract.
- Functions to test edge cases, such as setting points with invalid coordinates or attempting operations with insufficient permissions.
- Helper functions to assert the state of the contract after various operations.
These additions would make the testing suite even more robust and comprehensive.
CHANGELOG.md (1)
19-19: LGTM! Consider adding a brief description of the Graph ADO.The addition of the Graph ADO is well-documented and consistent with the changelog format. It aligns with the PR objectives mentioned earlier.
To provide more context for future readers, consider adding a brief description of the Graph ADO's purpose. For example:
- Added Graph ADO [(#526)](https://github.com/andromedaprotocol/andromeda-core/pull/526) + Added Graph ADO for storing and managing X-Y coordinates [(#526)](https://github.com/andromedaprotocol/andromeda-core/pull/526)packages/std/src/testing/mock_querier.rs (2)
Line range hint
309-313: LGTM! Consider using a match arm for better readability.The addition of the new case for code ID 5 is correct and aligns with the introduction of the Point ADO. To improve code readability and maintainability, consider refactoring this part of the method to use a match expression instead of multiple if-else statements.
Here's a suggested refactor:
- ADODBQueryMsg::ADOType { code_id } => match code_id { - 5 => SystemResult::Ok(ContractResult::Ok(to_json_binary(&"point").unwrap())), - 3 => SystemResult::Ok(ContractResult::Ok(to_json_binary(&"app-contract").unwrap())), - 1 => SystemResult::Ok(ContractResult::Ok(to_json_binary(&"ADOType").unwrap())), - _ => SystemResult::Ok(ContractResult::Err("Invalid Code ID".to_string())), - }, + ADODBQueryMsg::ADOType { code_id } => { + let ado_type = match code_id { + 5 => "point", + 3 => "app-contract", + 1 => "ADOType", + _ => return SystemResult::Ok(ContractResult::Err("Invalid Code ID".to_string())), + }; + SystemResult::Ok(ContractResult::Ok(to_json_binary(&ado_type).unwrap())) + },This refactoring improves readability and reduces code duplication.
513-514: LGTM! Consider using a constant for consistency.The addition of the new case for key "5" is correct and consistent with the changes in the
handle_adodb_querymethod. To improve code consistency and maintainability, consider using a constant for the "point" ADO type.Here's a suggested improvement:
+const POINT_ADO_TYPE: &str = "point"; // In the handle_adodb_query method - 5 => SystemResult::Ok(ContractResult::Ok(to_json_binary(&"point").unwrap())), + 5 => SystemResult::Ok(ContractResult::Ok(to_json_binary(&POINT_ADO_TYPE).unwrap())), // In the handle_adodb_raw_query method - SystemResult::Ok(ContractResult::Ok(to_json_binary("point").unwrap())) + SystemResult::Ok(ContractResult::Ok(to_json_binary(POINT_ADO_TYPE).unwrap()))This change improves consistency and makes it easier to update the ADO type name if needed in the future.
contracts/data-storage/andromeda-point/src/query.rs (2)
16-16: Reconsider the Redundant Use ofis_operatorThe final return statement
Ok(is_operator || allowed)includesis_operatoreven though it's already considered in theallowedvariable for thePrivaterestriction case. This redundancy might not affect functionality but could be simplified for clarity.If
is_operatoris intended to be separately checked regardless of the restriction type, consider adding a comment to clarify this intention. Otherwise, you might simplify the return statement if appropriate.
24-29: Simplify the Conversion fromAddrtoAndrAddrIn the
get_data_ownerfunction, you're convertingownerfromAddrtoAndrAddrusingAndrAddr::from_string(owner). Sinceowneris already anAddr, you can simplify this conversion by usingAndrAddr::from(owner), which avoids unnecessary string allocation and improves efficiency.Apply this diff to make the change:
- owner: AndrAddr::from_string(owner), + owner: AndrAddr::from(owner),This change makes the code more concise and leverages the implemented
From<Addr>trait forAndrAddr.contracts/data-storage/andromeda-point/src/contract.rs (1)
42-42: Add a comment for clarity when saving restrictionFor improved readability, consider adding a comment above
RESTRICTION.save(...)to explain that the restriction setting is being saved to storage.packages/andromeda-data-storage/src/point.rs (1)
45-47: Fix grammatical error in error messagesThe error messages use "can not parse to f64", which should be "cannot parse to f64".
Apply this diff to correct the error messages:
- err: "x_coordinate: can not parse to f64".to_string(), + err: "x_coordinate: cannot parse to f64".to_string(),Make similar changes for the
y_coordinateandz_coordinateerror messages in both the implementation and test assertions.Also applies to: 62-64, 121-121, 134-134, 147-147
contracts/data-storage/andromeda-graph/src/contract.rs (5)
247-250: Simplify booleanmatchto anifstatement.Using
matchon a boolean value is unnecessary and can be simplified for readability. Replace thematchstatement with anifexpression:let timestamp = if is_timestamp_allowed { Some(ctx.env.block.time.nanos()) } else { None };
382-382: Consistency in passing dependencies toget_raw_address.In this line,
get_raw_addressis called with&deps:let user_addr = user.get_raw_address(&deps)?;For consistency with other parts of the code, consider using
&deps.as_ref():let user_addr = user.get_raw_address(&deps.as_ref())?;
332-332: Consistency in passing dependencies toget_raw_address.Similar to a previous comment, ensure consistent use of
&ctx.deps.as_ref()when callingget_raw_address:let user_addr = user.get_raw_address(&ctx.deps.as_ref())?;If
&ctx.depssuffices, ensure it's used consistently throughout the code.
247-250: Improve readability by simplifyingmatchon boolean toif-else.The
matchonis_timestamp_allowedcan be simplified:let timestamp = if is_timestamp_allowed { Some(ctx.env.block.time.nanos()) } else { None };This reduces complexity and aligns with common Rust practices.
176-185: Clarify error message for Z-axis validation.The error message in the
ensure!macro could be confusing:error: Some(if is_z_allowed { "Z-axis is allowed".to_string() } else { "Z-axis is not allowed".to_string() })Consider rephrasing to indicate the actual issue, such as "Z-coordinate provided but Z-axis is not allowed" or "Z-coordinate is required when Z-axis is allowed."
Example:
ensure!( z_coordinate.is_some() == is_z_allowed, ContractError::InvalidParameter { error: Some(if is_z_allowed { "Z-coordinate is required when Z-axis is allowed.".to_string() } else { "Z-coordinate should not be provided when Z-axis is not allowed.".to_string() }) } );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
- CHANGELOG.md (1 hunks)
- contracts/data-storage/andromeda-graph/.cargo/config (1 hunks)
- contracts/data-storage/andromeda-graph/Cargo.toml (1 hunks)
- contracts/data-storage/andromeda-graph/examples/schema.rs (1 hunks)
- contracts/data-storage/andromeda-graph/src/contract.rs (1 hunks)
- contracts/data-storage/andromeda-graph/src/lib.rs (1 hunks)
- contracts/data-storage/andromeda-graph/src/mock.rs (1 hunks)
- contracts/data-storage/andromeda-graph/src/state.rs (1 hunks)
- contracts/data-storage/andromeda-graph/src/testing/mock.rs (1 hunks)
- contracts/data-storage/andromeda-graph/src/testing/mock_querier.rs (1 hunks)
- contracts/data-storage/andromeda-graph/src/testing/mod.rs (1 hunks)
- contracts/data-storage/andromeda-graph/src/testing/tests.rs (1 hunks)
- contracts/data-storage/andromeda-point/.cargo/config (1 hunks)
- contracts/data-storage/andromeda-point/Cargo.toml (1 hunks)
- contracts/data-storage/andromeda-point/examples/schema.rs (1 hunks)
- contracts/data-storage/andromeda-point/src/contract.rs (1 hunks)
- contracts/data-storage/andromeda-point/src/execute.rs (1 hunks)
- contracts/data-storage/andromeda-point/src/lib.rs (1 hunks)
- contracts/data-storage/andromeda-point/src/mock.rs (1 hunks)
- contracts/data-storage/andromeda-point/src/query.rs (1 hunks)
- contracts/data-storage/andromeda-point/src/state.rs (1 hunks)
- contracts/data-storage/andromeda-point/src/testing/mock.rs (1 hunks)
- contracts/data-storage/andromeda-point/src/testing/mod.rs (1 hunks)
- contracts/data-storage/andromeda-point/src/testing/tests.rs (1 hunks)
- packages/andromeda-data-storage/src/graph.rs (1 hunks)
- packages/andromeda-data-storage/src/lib.rs (1 hunks)
- packages/andromeda-data-storage/src/point.rs (1 hunks)
- packages/std/src/testing/mock_querier.rs (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- contracts/data-storage/andromeda-point/src/testing/mod.rs
🧰 Additional context used
🔇 Additional comments (57)
contracts/data-storage/andromeda-graph/src/testing/mod.rs (1)
1-3: Excellent test structure implementation!The addition of these three modules (
mock,mock_querier, andtests) demonstrates a well-organized approach to testing the Andromeda graph data storage system. This structure facilitates:
- Isolation of components during testing through mock implementations.
- Controlled testing of query-related functionalities with a dedicated mock querier.
- Clear separation of actual tests from mock implementations.
This setup will contribute to more robust and maintainable tests, which is crucial for the long-term health of the project.
packages/andromeda-data-storage/src/lib.rs (1)
3-4: LGTM! New modules added as expected.The addition of
graphandpointmodules aligns with the PR objectives to introduce Graph ADO and Point ADO. These changes enhance the functionality of the Andromeda protocol by enabling the storage and management of spatial data.To ensure the new modules are properly implemented, let's verify their existence and basic structure:
Don't forget to update any relevant documentation (e.g., README.md, API docs) to reflect these new modules and their functionalities.
✅ Verification successful
Verified! The
graphandpointmodules are properly implemented and align with the PR objectives to introduce Graph ADO and Point ADO functionalities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and basic structure of new modules # Check if the graph.rs file exists echo "Checking graph.rs:" fd --type f "graph.rs" packages/andromeda-data-storage/src # Check if the point.rs file exists echo "Checking point.rs:" fd --type f "point.rs" packages/andromeda-data-storage/src # Display the contents of graph.rs echo "Contents of graph.rs:" cat packages/andromeda-data-storage/src/graph.rs # Display the contents of point.rs echo "Contents of point.rs:" cat packages/andromeda-data-storage/src/point.rsLength of output: 6337
contracts/data-storage/andromeda-graph/.cargo/config (1)
1-4: LGTM! These Cargo aliases will streamline development tasks.The introduction of these Cargo aliases is a great addition to the project. They provide convenient shortcuts for common development tasks:
wasm: Compiles the project for WebAssembly in release mode, which is essential for smart contract deployment.unit-test: Runs unit tests for the library, facilitating quick testing during development.schema: Generates the contract's message schema, which is a best practice in CosmWasm development.These aliases will improve developer productivity and maintain consistency in build and test processes across the team.
contracts/data-storage/andromeda-point/.cargo/config (1)
4-4: Verify the existence of the "schema" example.The
schemaalias assumes there's an example named "schema" in your project. Please ensure this example exists and is correctly implemented.contracts/data-storage/andromeda-graph/src/lib.rs (2)
7-8: LGTM! Well-structured test module.The conditional compilation for the
testingmodule is correctly set up. It ensures that test code is only included when running tests, which is a standard practice in Rust. The module being private (modinstead ofpub mod) correctly indicates that it's for internal testing only.This structure promotes better organization of test code and ensures it's not included in production builds, which is excellent for maintaining a clean and efficient codebase.
1-8: Overall, excellent structure and organization.The
lib.rsfile is well-structured and follows Rust best practices. It effectively sets up the main modules for the Graph ADO (contractandstate) while properly handling test and mock code through conditional compilation. This aligns perfectly with the PR objectives and promotes good code organization.The minor suggestions for documentation improvements will further enhance the code's readability and maintainability, but the current implementation is solid and ready for integration.
contracts/data-storage/andromeda-point/src/lib.rs (7)
1-1: LGTM: Public contract module declaration.The public declaration of the
contractmodule is appropriate. This module likely contains the main entry points for the smart contract, which need to be accessible from outside the crate.
2-2: LGTM: Private execute module declaration.The private declaration of the
executemodule is appropriate. This module likely contains internal implementation details for contract execution, which should be encapsulated within the crate.
3-4: LGTM: Conditional mock module for testing.The
mockmodule is appropriately declared as public and conditionally compiled. This setup ensures:
- The module is only available for testing purposes.
- It's not included in the final WASM binary, optimizing contract size.
- It can be used in integration tests or other crates during testing.
This approach aligns with best practices for separating test code from production code in Rust smart contracts.
5-5: LGTM: Private query module declaration.The private declaration of the
querymodule is appropriate. This module likely contains internal implementation details for contract queries, which should be encapsulated within the crate.
6-6: LGTM: Private state module declaration.The private declaration of the
statemodule is appropriate. This module likely contains internal state management logic, which should be encapsulated within the crate to prevent direct manipulation of contract state from outside.
8-9: LGTM: Conditional testing module for unit tests.The
testingmodule is appropriately declared as private and conditionally compiled only for test builds. This setup ensures:
- The module is only available during testing.
- It's not included in the final binary, optimizing contract size.
- It's used only within this crate for unit tests.
This approach aligns with best practices for organizing test code in Rust projects.
1-9: Well-structured module organization for the andromeda-point project.The overall structure of this file is well-organized and follows Rust best practices for smart contract development:
- Clear separation of concerns with distinct modules for contract, execute, query, and state.
- Appropriate use of public and private module declarations.
- Effective use of conditional compilation for testing-related modules (
mockandtesting).This structure provides several benefits:
- Improved code organization and readability.
- Clear boundaries between different components of the smart contract.
- Efficient separation of production and test code.
- Potential for better performance and smaller contract size in production builds.
Great job on setting up a solid foundation for the andromeda-point project!
contracts/data-storage/andromeda-graph/examples/schema.rs (1)
1-2: LGTM: Imports are correct and follow best practices.The imports are appropriate for the schema generation task. Using specific imports enhances code readability and maintainability.
contracts/data-storage/andromeda-point/examples/schema.rs (3)
1-2: LGTM: Imports are correct and follow best practices.The imports are appropriate for generating the schema. Good job on using specific imports rather than wildcard imports, which enhances code readability and maintainability.
3-10: LGTM: Schema generation is correctly implemented.The
mainfunction correctly uses thewrite_api!macro to generate the schema for the Andromeda Point ADO. It includes all necessary message types (InstantiateMsg, QueryMsg, ExecuteMsg) as required for a complete CosmWasm contract interface.
1-10: Overall assessment: Well-structured and purpose-driven schema generation.This file effectively sets up the schema generation for the Andromeda Point ADO. It follows CosmWasm best practices and provides a clear, concise implementation. The code is well-organized and serves its intended purpose without any unnecessary complexity.
contracts/data-storage/andromeda-point/src/state.rs (1)
1-3: LGTM: Imports are appropriate and well-structured.The imports cover all necessary types for the state management of the Andromeda Point module. They include custom types from the project (
PointCoordinate,PointRestriction), standard CosmWasm types (Addr), and storage management utilities (Item).contracts/data-storage/andromeda-graph/src/state.rs (2)
1-4: LGTM: Imports are appropriate and well-organized.The imports are correctly chosen for the functionality being implemented. They include necessary types from the
andromeda_data_storagecrate for both graph and point modules, theAddrtype fromcosmwasm_stdfor blockchain addresses, and storage primitives fromcw_storage_plus.
6-9: LGTM: Well-designed constants for graph data storage.The constants are appropriately defined using
cw_storage_plusprimitives:
MAP_INFO: Stores a singleMapInfoitem, suitable for map metadata.TOTAL_POINTS_NUMBER: Keeps track of the total number of points as a singleu128value.USER_COORDINATE: MapsAddrtoPointCoordinate, efficiently storing user-specific coordinates.These definitions align well with the PR objectives of introducing Graph ADO for storing X-Y coordinates within specified map boundaries.
contracts/data-storage/andromeda-graph/Cargo.toml (4)
1-13: LGTM! Package metadata is well-defined.The package metadata is appropriately set up for the new andromeda-graph project. The exclusion of build artifacts from publication is a good practice.
Note: The minimum Rust version is set to 1.75.0, which is quite recent. Ensure that this aligns with your project's compatibility requirements and development environment.
15-16: LGTM! Library configuration is correct.The library is correctly configured to build both a cdylib (for WebAssembly) and an rlib (for Rust library usage). This setup allows for maximum flexibility in how the library can be used.
25-33: LGTM! Dependencies are well-defined and use workspace-level version management.The dependencies section includes all necessary libraries for a CosmWasm-based project in the Andromeda ecosystem. The use of workspace-level version management (
workspace = true) for all dependencies is an excellent practice, ensuring consistency across the project and simplifying version updates.
35-37: LGTM! Conditional dependencies are correctly configured.The conditional dependencies for non-wasm32 targets are well-defined. Including cw-multi-test and andromeda-testing as optional dependencies for non-WebAssembly builds is a good practice. This setup allows these testing dependencies to be included only when necessary, likely controlled by the "testing" feature defined earlier.
contracts/data-storage/andromeda-point/Cargo.toml (6)
1-6: LGTM: Package metadata is well-defined.The package metadata is appropriately set up for a new package. The Rust edition (2021) and version (1.75.0) are up-to-date, and the version number (1.0.0) follows semantic versioning conventions.
8-12: LGTM: Appropriate files excluded from package.The
excludesection correctly omits build artifacts ("contract.wasm" and "hash.txt") from the package. This is a good practice to keep the package size small and avoid including non-source files. The comment provides helpful context for why these files are excluded.
16-17: LGTM: Appropriate library configuration for a CosmWasm contract.The
[lib]section correctly specifies both "cdylib" and "rlib" crate types. This configuration is standard for CosmWasm contracts, allowing the crate to be compiled to WebAssembly (cdylib) and used as a dependency by other crates (rlib).
19-24: LGTM: Well-defined features with clear purposes.The
[features]section is well-structured with clear purposes for each feature:
- "backtraces" for improved debugging
- "library" for disabling exports when needed
- "testing" for including test-specific dependencies
The comments provide helpful context for the "backtraces" and "library" features, enhancing maintainability.
26-39: LGTM: Dependencies are well-structured and appropriate.The dependencies are well-organized and suitable for a CosmWasm contract:
- Main dependencies use workspace versions, ensuring consistency across the project.
- Includes standard CosmWasm libraries and Andromeda-specific crates.
- Target-specific dependencies for non-wasm32 architectures are correctly configured for testing purposes.
The use of workspace versions and the separation of test dependencies demonstrate good practices in dependency management.
1-39: Excellent Cargo.toml configuration for the andromeda-point package.This Cargo.toml file is well-structured and follows best practices for CosmWasm contract development. It includes:
- Appropriate package metadata
- Correct exclusion of build artifacts
- Standard library configuration for WebAssembly compilation
- Well-defined features for various use cases
- Properly organized dependencies using workspace versions
The file demonstrates good organization and attention to detail, which will contribute to the maintainability and usability of the andromeda-point package.
contracts/data-storage/andromeda-point/src/testing/mock.rs (2)
1-16: LGTM: Imports and type alias are well-structured.The imports cover all necessary modules for mock testing, and the
MockDepstype alias is correctly defined.
64-68: LGTM:delete_pointfunction is correctly implemented.The function properly creates and executes the
DeletePointmessage, following the same pattern as the other execution functions in this file.CHANGELOG.md (2)
Line range hint
1-85: Excellent changelog structure and content.The changelog is well-organized, comprehensive, and follows best practices:
- Clear sections for Added, Changed, Fixed, and Removed items
- Consistent formatting with pull request links
- Covers a wide range of changes, aligning with the PR objectives and AI-generated summary
This structure effectively communicates the project's evolution and will be valuable for users and developers.
Line range hint
1-85: Verify the status of the Point ADO mentioned in the PR objectives.The PR objectives mentioned introducing both Graph ADO and Point ADO, but the changelog only includes the Graph ADO. Could you please clarify:
- Is the Point ADO part of this PR?
- If yes, should it be added to the changelog?
- If no, is it planned for a future PR?
This verification will ensure that the changelog accurately reflects all significant changes in this PR.
packages/std/src/testing/mock_querier.rs (1)
Line range hint
1-554: Overall, the changes look good and align with the PR objectives.The additions to both
handle_adodb_queryandhandle_adodb_raw_querymethods successfully implement support for the new Point ADO. The changes are consistent with the existing patterns in the file and enhance the mock querier's capabilities for testing purposes.Consider implementing the suggested refactoring and consistency improvements to further enhance the code quality. These changes will make the code more readable, maintainable, and consistent across the file.
contracts/data-storage/andromeda-point/src/query.rs (2)
1-4: Imports are Appropriate and NecessaryThe imported modules and crates are appropriate for the functionality provided in this file and are necessary for the code to compile and run correctly.
19-22:get_pointFunction is Correct and EffectiveThe
get_pointfunction correctly retrieves thePointCoordinatefrom storage and properly handles any errors that may occur during the loading process.packages/andromeda-data-storage/src/graph.rs (1)
61-61: Verify if users can have multiple coordinatesThe
GetUserCoordinatequery returns a singleCoordinateInfo, suggesting each user can have only one coordinate. If users are allowed to have multiple coordinates, consider returning a list of coordinates. This ensures the query response accurately reflects the data model.Run the following script to check for multiple coordinate storage:
✅ Verification successful
Verification Successful: Users Cannot Have Multiple Coordinates
After reviewing the storage methods, it is confirmed that users can only have one coordinate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if users can store multiple coordinates # Test: Search for usage of user coordinate storage methods # Expect: Identify methods that store multiple coordinates per user rg --type rust -A 5 $'StoreUserCoordinate' | rg 'Vec<Coordinate>' || echo "No multiple coordinates found"Length of output: 133
contracts/data-storage/andromeda-point/src/contract.rs (5)
22-44:instantiatefunction is correctly implementedThe
instantiatefunction properly initializes the contract usingADOContractand saves therestrictionto storage. Parameters are correctly handled, and error propagation is appropriate.
46-60:executefunction handles message routing appropriatelyThe
executefunction correctly creates anExecuteContextand routes messages. It delegatesExecuteMsg::AMPReceivetoADOContract::execute_amp_receiveand handles other messages withhandle_execute.
62-69:queryfunction correctly routes queriesThe
queryfunction properly handlesGetPointandGetDataOwnerqueries and delegates other queries toADOContract::query.
71-74:migratefunction implementation is appropriateThe
migratefunction correctly delegates toADOContract::migrate, ensuring proper contract migration with the updated name and version.
55-59: Verify allExecuteMsgvariants are handledPlease ensure that all variants of
ExecuteMsgare properly handled in your execution logic. Missing handlers for certain variants could lead to unhandled messages and potential errors.You can run the following script to verify that all
ExecuteMsgvariants are accounted for:contracts/data-storage/andromeda-point/src/mock.rs (1)
1-111: Well-structured mock implementation for testingThe
MockPointstruct and its associated methods provide a comprehensive mock setup for testing the Andromeda Point contract. The use of helper functions and themock_ado!macro enhances modularity and code clarity.packages/andromeda-data-storage/src/point.rs (1)
87-151: Good test coverage forvalidatemethodThe unit tests thoroughly cover both valid and invalid cases for the
validatemethod, ensuring its correctness.contracts/data-storage/andromeda-point/src/execute.rs (1)
128-150: Verify the correctness of tax amount calculation and refund logicEnsure that the calculation of
tax_amount,refund, andafter_tax_paymentcorrectly handles all possible scenarios, including when the sent funds are less than the tax amount. Incorrect handling might result in arithmetic errors or unintended behavior.Run the following script to check for potential arithmetic errors:
Review any warnings related to integer arithmetic to ensure safe operations.
✅ Verification successful
Tax amount calculation and refund logic are correctly handled, with no unchecked arithmetic operations found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for unchecked arithmetic operations that might panic. # Test: Use cargo clippy to identify potential issues. Expect: No warnings about unchecked arithmetic. cargo clippy -- -A clippy::integer_arithmeticLength of output: 120
Script:
#!/bin/bash # Description: Search for unchecked arithmetic operations that might panic in execute.rs # Using ast-grep to find arithmetic operations not using checked methods ast-grep --lang rust --pattern '$_ * $_' --file contracts/data-storage/andromeda-point/src/execute.rs ast-grep --lang rust --pattern '$_ + $_' --file contracts/data-storage/andromeda-point/src/execute.rs ast-grep --lang rust --pattern '$_ - $_' --file contracts/data-storage/andromeda-point/src/execute.rs ast-grep --lang rust --pattern '$_ / $_' --file contracts/data-storage/andromeda-point/src/execute.rsLength of output: 1191
Script:
#!/bin/bash # Description: Search for unchecked arithmetic operations that might panic in execute.rs # Using ast-grep to find arithmetic operations not using checked methods ast-grep --lang rust --pattern '$_ * $_' contracts/data-storage/andromeda-point/src/execute.rs ast-grep --lang rust --pattern '$_ + $_' contracts/data-storage/andromeda-point/src/execute.rs ast-grep --lang rust --pattern '$_ - $_' contracts/data-storage/andromeda-point/src/execute.rs ast-grep --lang rust --pattern '$_ / $_' contracts/data-storage/andromeda-point/src/execute.rsLength of output: 387
contracts/data-storage/andromeda-point/src/testing/tests.rs (10)
1-18: Approved: Import statements are correctThe import statements correctly include all necessary modules and dependencies.
20-23: Approved:test_instantiationfunction is properly implementedThe test for contract instantiation verifies that the contract initializes correctly with a specified point restriction.
25-43: Approved:test_set_and_update_pointfunction correctly tests point setting and updatingThe test function properly sets an initial point, verifies it, updates it, and verifies the update.
45-139: Approved:test_set_point_with_taxfunction accurately tests tax handlingThe test cases cover scenarios with exact payment, insufficient funds, and overpayment, ensuring proper handling of taxes and refunds.
141-145: Approved:TestHandlePointCoordinatestruct is appropriately definedThe struct correctly represents test cases for invalid point coordinates.
147-195: Approved:test_set_point_invalidfunction effectively tests invalid coordinatesThe test function validates that invalid point coordinates result in the expected parsing errors.
197-204: Approved:test_delete_pointfunction correctly tests point deletionThe test ensures that points can be deleted and that querying a deleted point results in an error.
206-229: Approved:test_restriction_privatefunction properly tests private restriction behaviorThe test verifies that only the owner can set or delete points when the restriction is private.
231-256: Approved:test_restriction_publicfunction correctly tests public restriction behaviorThe test confirms that any user can set or delete points when the restriction is public.
303-327: Approved:test_query_data_ownerfunction accurately tests data owner queryingThe test verifies that the contract correctly identifies the data owner and handles unauthorized deletion attempts.
contracts/data-storage/andromeda-graph/src/testing/tests.rs (1)
354-375: Verify that updating the map clears stored pointsWhen
update_mapis called, all previously stored coordinates are cleared, andmax_point_numberis reset to zero. Verify that this is the intended behavior, as it may result in data loss if not handled properly.Run the following script to check if
update_mapimplementation intentionally clears stored points:This script searches for calls to
POINTS.clearor resettingMAX_POINT_NUMBERwithin theupdate_mapfunction.
|
|
||
| let is_z_allowed = z_length.is_some(); | ||
|
|
||
| let x_coordinate = ((coordinate.x_coordinate * 10_f64.powf(map_decimal as f64)) as i64) as f64 |
There was a problem hiding this comment.
Any risk of underflow causing a panic here?
There was a problem hiding this comment.
Resolved like this:
let scale = 10_f64.powf(map_decimal as f64);
let x_coordinate = (coordinate.x_coordinate * scale)
.min(i64::MAX as f64) // Ensuring it doesn't exceed i64 bounds
.max(i64::MIN as f64); // Ensuring it doesn't underflow
let x_coordinate = (x_coordinate as i64) as f64 / scale;
let y_coordinate = (coordinate.y_coordinate * scale)
.min(i64::MAX as f64) // Ensuring it doesn't exceed i64 bounds
.max(i64::MIN as f64); // Ensuring it doesn't underflow
let y_coordinate = (y_coordinate as i64) as f64 / scale;
let z_coordinate = coordinate.z_coordinate.map(|z| {
let z_scaled = (z * scale)
.min(i64::MAX as f64) // Clamp the value to prevent overflow
.max(i64::MIN as f64); // Clamp the value to prevent underflow
(z_scaled as i64) as f64 / scale
});
| pub fn execute_update_map( | ||
| ctx: ExecuteContext, | ||
| map_info: MapInfo, | ||
| action: String, |
There was a problem hiding this comment.
I'm not sure this parameter is necessary. The action is static so can be hardcoded in the attributes.
There was a problem hiding this comment.
Removed this parameter.
| ctx: ExecuteContext, | ||
| coordinate: Coordinate, | ||
| is_timestamp_allowed: bool, | ||
| action: String, |
There was a problem hiding this comment.
Removed this parameter.
| pub fn execute_store_user_coordinate( | ||
| ctx: ExecuteContext, | ||
| user_location_paths: Vec<AndrAddr>, | ||
| action: String, |
There was a problem hiding this comment.
Removed this parameter.
| pub fn execute_delete_user_coordinate( | ||
| ctx: ExecuteContext, | ||
| user: AndrAddr, | ||
| action: String, |
There was a problem hiding this comment.
Removed this parameter.
| let mut res: Vec<(CoordinateInfo, StoredDate)> = Vec::new(); | ||
|
|
||
| for point in 1..=max_point_number { | ||
| let coordinate = MAP_POINT_INFO.load(storage, &point)?; |
There was a problem hiding this comment.
This can error if a point is removed. Maybe use may_load and only append Option values that have a Some value.
There was a problem hiding this comment.
This is also unpaginated so has a potential DOS vector if too many points are added.
There was a problem hiding this comment.
Using a paginated range value may actually be a better approach.
There was a problem hiding this comment.
Resolved like this:
pub fn get_all_points(
storage: &dyn Storage,
start: Option<u128>,
limit: Option<u32>,
) -> Result<GetAllPointsResponse, ContractError> {
let max_point_number = TOTAL_POINTS_NUMBER.load(storage)?;
// Set default values for pagination
let start_point = start.unwrap_or(1); // Start from 1 if no start provided
let limit = limit.unwrap_or(100); // Default limit to 100 points
let mut res: Vec<(CoordinateInfo, StoredDate)> = Vec::new();
// Iterate with pagination
for point in start_point..=max_point_number {
if res.len() >= limit as usize {
break; // Stop when limit is reached
}
// Use `may_load` to handle cases where the point may not exist
if let Some(coordinate) = MAP_POINT_INFO.may_load(storage, &point)? {
res.push(coordinate);
}
}
Ok(GetAllPointsResponse { points: res })
}| match res { | ||
| Ok(res) => Ok(from_json(res)?), | ||
| Err(err) => Err(err), | ||
| } |
There was a problem hiding this comment.
This is a nit but I think is a nice change
| #[cw_serde] | ||
| pub struct PointCoordinate { | ||
| pub x_coordinate: String, | ||
| pub y_coordinate: String, | ||
| pub z_coordinate: Option<String>, | ||
| } |
There was a problem hiding this comment.
Is there a reason we don't use f64 here?
…0/andromeda-core into mdjakovic/graph-ado
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
packages/andromeda-data-storage/src/graph.rs (1)
11-23: LGTM: MapInfo and MapSize structures are well-definedThe
MapInfoandMapSizestructures provide necessary information for map configuration. The use ofOption<u64>forz_widthallows for both 2D and 3D maps, which is a good design choice.Consider adding documentation comments to explain the purpose of each field, especially
allow_negativeandmap_decimal, to improve code readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- contracts/data-storage/andromeda-graph/src/contract.rs (1 hunks)
- contracts/data-storage/andromeda-graph/src/testing/mock.rs (1 hunks)
- contracts/data-storage/andromeda-graph/src/testing/tests.rs (1 hunks)
- packages/andromeda-data-storage/src/graph.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- contracts/data-storage/andromeda-graph/src/contract.rs
- contracts/data-storage/andromeda-graph/src/testing/tests.rs
🧰 Additional context used
🔇 Additional comments (7)
packages/andromeda-data-storage/src/graph.rs (3)
1-9: LGTM: Imports and InstantiateMsg structure look goodThe imports are appropriate for the functionality being implemented, and the
InstantiateMsgstructure is well-defined, following the expected pattern for CosmWasm contract instantiation.
50-80: LGTM: QueryMsg and response structures are well-definedThe
QueryMsgenum and associated response structures (GetMapInfoResponse,GetMaxPointNumberResponse, andGetAllPointsResponse) are well-defined and cover the necessary query operations for the Graph ADO. The use of#[returns(...)]attributes is correct and helpful for schema generation.
1-92: Overall assessment: Well-structured Graph ADO with room for improvementsThe Graph ADO implementation in this file is well-structured and covers essential functionalities for managing map-related data. The use of appropriate CosmWasm and Andromeda standard libraries demonstrates good adherence to best practices.
Key points from the review:
- Imports and basic structures are well-defined.
- Suggested improvements for naming consistency in
CoordinateandExecuteMsg.- Recommendation to align data types between
CoordinateandCoordinateInfo.- Suggestion to use
Timestamptype for better time representation.- Consider adding documentation comments to improve code readability.
Implementing these suggestions will enhance the consistency, usability, and maintainability of the Graph ADO. Great work overall!
contracts/data-storage/andromeda-graph/src/testing/mock.rs (4)
10-10:⚠️ Potential issueReplace
from_jsonwithfrom_binaryin importsThe
from_jsonfunction is not provided bycosmwasm_std; instead, you should usefrom_binaryto deserializeBinaryresponses from queries.Apply this diff to correct the import:
use cosmwasm_std::{ - from_json, + from_binary, testing::{mock_env, mock_info, MockApi, MockStorage}, Deps, DepsMut, MessageInfo, OwnedDeps, Response, };⛔ Skipped due to learnings
Learnt from: crnbarr93 PR: andromedaprotocol/andromeda-core#576 File: contracts/data-storage/andromeda-graph/src/testing/mock_querier.rs:93-110 Timestamp: 2024-10-11T10:52:10.088Z Learning: In the Rust file `contracts/data-storage/andromeda-graph/src/testing/mock_querier.rs`, panics caused by `unwrap()` are acceptable because the code is only used for testing.
107-113:⚠️ Potential issueUse
from_binaryinstead offrom_jsoninquery_user_coordinateIn the
query_user_coordinatefunction, replacefrom_jsonwithfrom_binaryto correctly deserialize the query response.Apply this diff to correct the function:
pub fn query_user_coordinate(deps: Deps, user: AndrAddr) -> Result<CoordinateInfo, ContractError> { let res = query(deps, mock_env(), QueryMsg::GetUserCoordinate { user }); match res { - Ok(res) => Ok(from_json(res)?), + Ok(res) => Ok(from_binary(&res)?), Err(err) => Err(err), } }Likely invalid or redundant comment.
87-93:⚠️ Potential issueUse
from_binaryinstead offrom_jsoninquery_max_point_numberIn the
query_max_point_numberfunction, replacefrom_jsonwithfrom_binaryto correctly deserialize the query response.Apply this diff to correct the function:
pub fn query_max_point_number(deps: Deps) -> Result<GetMaxPointNumberResponse, ContractError> { let res = query(deps, mock_env(), QueryMsg::GetMaxPointNumber {}); match res { - Ok(res) => Ok(from_json(res)?), + Ok(res) => Ok(from_binary(&res)?), Err(err) => Err(err), } }Likely invalid or redundant comment.
79-85:⚠️ Potential issueUse
from_binaryinstead offrom_jsoninquery_map_infoIn the
query_map_infofunction, replacefrom_jsonwithfrom_binaryto correctly deserialize the query response.Apply this diff to fix the deserialization:
pub fn query_map_info(deps: Deps) -> Result<GetMapInfoResponse, ContractError> { let res = query(deps, mock_env(), QueryMsg::GetMapInfo {}); match res { - Ok(res) => Ok(from_json(res)?), + Ok(res) => Ok(from_binary(&res)?), Err(err) => Err(err), } }Likely invalid or redundant comment.
1. Graph ADO
Motivation
The purpose of this ADO is to store X-Y coordinates into specific map.
The map bounds or limits to where points can be stored. And also it contains decimals and allow/disallow negative numbers.
At version 1.1.0
store_user_coordinatemethod is added.This function references the user coordinate from Point ADO.
Implementation
2. Point ADO
Motivation
The purpose of this ADO is to store x, y, z coordinate.
Implementation
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests