First System ADO - Current Block creating endpoints anywhere in the VFS #561
Conversation
WalkthroughThe pull request introduces enhancements to the Andromeda project, focusing on the addition of new modules and functionalities for managing system ADO paths and current block management. Key changes include updates to Changes
Possibly related PRs
Suggested labels
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 (
|
mdjakovic0920
requested review from
crnbarr93,
SlayerAnsh and
joemonem
and removed request for
SlayerAnsh
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
contracts/os/andromeda-vfs/src/state.rs (1)
205-233: LGTM, but consider adding a test case.The
resolve_system_pathfunction is well-implemented and correctly resolves system paths, handling symlinks appropriately.Consider adding a test case for this function to ensure its correctness and handle edge cases. You can create a new test function in the
testmodule at the end of the file.
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 (18)
- Cargo.toml (2 hunks)
- contracts/os/andromeda-vfs/src/contract.rs (2 hunks)
- contracts/os/andromeda-vfs/src/execute.rs (2 hunks)
- contracts/os/andromeda-vfs/src/query.rs (2 hunks)
- contracts/os/andromeda-vfs/src/state.rs (8 hunks)
- contracts/os/andromeda-vfs/src/testing/tests.rs (4 hunks)
- contracts/systems/andromeda-system-current-block/.cargo/config (1 hunks)
- contracts/systems/andromeda-system-current-block/Cargo.toml (1 hunks)
- contracts/systems/andromeda-system-current-block/examples/schema.rs (1 hunks)
- contracts/systems/andromeda-system-current-block/src/contract.rs (1 hunks)
- contracts/systems/andromeda-system-current-block/src/lib.rs (1 hunks)
- contracts/systems/andromeda-system-current-block/src/mock.rs (1 hunks)
- contracts/systems/andromeda-system-current-block/src/testing/mod.rs (1 hunks)
- contracts/systems/andromeda-system-current-block/src/testing/tests.rs (1 hunks)
- packages/andromeda-systems/Cargo.toml (1 hunks)
- packages/andromeda-systems/src/current_block.rs (1 hunks)
- packages/andromeda-systems/src/lib.rs (1 hunks)
- packages/std/src/os/vfs.rs (6 hunks)
Files skipped from review due to trivial changes (2)
- contracts/systems/andromeda-system-current-block/.cargo/config
- contracts/systems/andromeda-system-current-block/Cargo.toml
Additional comments not posted (35)
contracts/systems/andromeda-system-current-block/src/testing/mod.rs (1)
1-1: LGTM!The code changes are approved.
packages/andromeda-systems/src/lib.rs (1)
1-1: LGTM!The code changes are approved.
contracts/systems/andromeda-system-current-block/src/lib.rs (1)
1-6: LGTM!The code changes are approved.
contracts/systems/andromeda-system-current-block/examples/schema.rs (1)
1-10: LGTM!The code follows the standard pattern for generating the contract schema using
cosmwasm_schema::write_api!. TheInstantiateMsg,QueryMsg, andExecuteMsgare imported from theandromeda_systems::current_blockmodule.packages/andromeda-systems/src/current_block.rs (1)
1-21: LGTM!The code defines the message types for the current block contract:
InstantiateMsgwithnameandrootfields, using theandr_instantiatemacro.ExecuteMsgenum without any variants, using theandr_execmacro.QueryMsgenum with a singleGetCurrentBlockHeightvariant that returns aString, using theandr_querymacro andQueryResponsesderive macro.The code follows the standard patterns and looks good.
packages/andromeda-systems/Cargo.toml (1)
1-24: LGTM!The manifest file for the
andromeda-systemspackage looks good:
- The package metadata is properly specified with the name, version, edition, Rust version, description, and license.
- The
backtracesfeature is defined, which enables backtraces fromcosmwasm-std.- The library is set to generate both
cdylibandrlibcrate types.- The dependencies are properly specified, including
cosmwasm-std,cosmwasm-schema,serde,cw-utils,cw721,cw721-base, andandromeda-std.The file follows the standard structure and looks good.
contracts/systems/andromeda-system-current-block/src/mock.rs (3)
8-11: LGTM!The code changes are approved.
13-25: LGTM!The code changes are approved.
27-29: LGTM!The code changes are approved.
contracts/systems/andromeda-system-current-block/src/testing/tests.rs (2)
11-25: LGTM!The code changes are approved.
28-48: LGTM!The code changes are approved.
Cargo.toml (2)
12-12: LGTM!The code change is approved.
41-41: LGTM!The code change is approved.
contracts/os/andromeda-vfs/src/query.rs (2)
25-33: LGTM!The code changes are approved.
39-41: LGTM!The code changes are approved.
contracts/systems/andromeda-system-current-block/src/contract.rs (2)
22-56: LGTM!The code changes are approved.
101-106: LGTM!The code changes are approved.
contracts/os/andromeda-vfs/src/contract.rs (2)
78-80: LGTM!The code changes are approved.
113-118: LGTM!The code changes are approved.
Also applies to: 120-120
contracts/os/andromeda-vfs/src/execute.rs (1)
142-172: LGTM!The
add_system_ado_pathfunction is well-implemented with the following key points:
- It validates the sender is a system ADO contract.
- It ensures the
nameandrootare valid component names.- It checks for existing paths and prevents unauthorized overriding, enhancing security.
The code changes are approved.
packages/std/src/os/vfs.rs (4)
9-10: LGTM!The update to the
PATH_REGEXconstant to include a new segment for user-defined paths under the/homedirectory is a reasonable addition. It expands the flexibility of path validation and supports more diverse paths within the system.The code change is approved.
173-178: LGTM!The addition of the
AddSystemAdoPathvariant to theExecuteMsgenum is a good way to facilitate the registration of system ADOs by a registered admin. Thenameandrootfields are appropriately validated against theCOMPONENT_NAME_REGEXto ensure they conform to the expected format.The code change is approved.
208-217: LGTM!The introduction of the
SubSystemBoundstruct is a good way to manage subsystem boundaries more effectively. Encapsulating therootandnamefields within the struct provides a clean and organized approach. The conversion implementation to transform the struct into a tuple of strings adds flexibility in how it can be used.The code change is approved.
231-237: LGTM!The addition of the
SubSystemvariant to theQueryMsgenum is a valuable enhancement to the querying capabilities of the system. It enables more granular control over subsystem data retrieval by allowing optional bounds and limits. This provides flexibility in querying specific ranges of subsystems based on the requirements.The code change is approved.
contracts/os/andromeda-vfs/src/testing/tests.rs (3)
616-642: LGTM!The
test_add_system_ado_pathfunction is a good addition to the test suite. It covers the basic functionality of adding a system ADO path by executing a message and asserting the resolved address matches the sender. This ensures that the path resolution logic is functioning correctly for system ADOs.The code change is approved.
795-846: LGTM!The
test_get_subsystemsfunction is a valuable addition to the test suite. It covers the functionality of retrieving subsystems by adding two system ADO paths and querying them. The test constructs expected results and asserts that the query returns the correct subsystems, ensuring that the contract can handle multiple system ADO paths and return them accurately when queried.The code change is approved.
928-985: LGTM!The
test_get_system_pathsfunction is a good addition to the test suite. It covers the functionality of retrieving system paths associated with a specific sender. The test adds two system ADO paths with different roots and checks that the paths can be resolved correctly, ensuring that the contract maintains the integrity of the path resolution process for system ADO paths with different roots.The code change is approved.
contracts/os/andromeda-vfs/src/state.rs (8)
49-55: LGTM!The
SystemAdoPathInfostruct is well-defined and captures the necessary information for system paths.
57-65: LGTM!The
SystemAdoPathIndicesstruct is well-defined and provides efficient indexing for querying system paths by address and root.
67-74: LGTM!The
IndexListtrait implementation forSystemAdoPathIndicesis correct and follows the expected pattern.
76-91: LGTM!The
system_ado_pathsfunction is well-implemented and sets up theIndexedMapcorrectly for storing and querying system paths.
317-342: LGTM!The
get_subsystemfunction is well-implemented and correctly retrieves the subsystem paths based on the specified criteria.
370-386: LGTM!The
get_system_pathsfunction is well-implemented and correctly retrieves and formats the system paths for a given address.
407-424: LGTM!The
add_system_ado_path_namefunction is well-implemented and correctly adds a new system path entry to the storage.
731-759: LGTM!The
test_resolve_system_pathtest case is well-written and covers the basic functionality of resolving a system path.
| add_system_ado_path_name(deps.storage, root, name, info.sender)?; | ||
| } | ||
| Some(path) => { | ||
| ensure!(path.address == info.sender, ContractError::Unauthorized {}) |
There was a problem hiding this comment.
What is being done after this verification? Seems like we're checking authority, then doing nothing.
There was a problem hiding this comment.
If the system ado is already existed, the existing equal to Some(_).
In this case, it just needs path.address == info.sender, then we don't have to do anything.
It does not need to add the same path.
What do you think of this?
There was a problem hiding this comment.
We could return an error stating that all that information already exists. Also we should add relevant attributes to the response
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
24-24: Consider adding more context to the changelog entry.While the current entry is correct, it might be helpful for users to understand the purpose of this new System ADO. Consider expanding the entry to briefly explain its functionality, like this:
- Added System ADO - current block [(#526)](https://github.com/andromedaprotocol/andromeda-core/pull/526) + Added System ADO - current block for tracking and querying the current block height [(#526)](https://github.com/andromedaprotocol/andromeda-core/pull/526)This additional context aligns with the PR objectives and provides more clarity about the new feature.
📜 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 (3)
- CHANGELOG.md (1 hunks)
- Cargo.toml (2 hunks)
- DEADJOE (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- DEADJOE
🔇 Additional comments not posted (3)
Cargo.toml (2)
41-41: LGTM! Verify the andromeda-systems package.The addition of the
andromeda-systemspackage as a workspace dependency is consistent with the PR objectives. The version "1.0.0" aligns with the initial implementation mentioned in the PR summary.To ensure the new package is properly set up, run the following script:
#!/bin/bash # Description: Verify the existence and version of the andromeda-systems package # Test 1: Check if the package directory exists if [ -d "packages/andromeda-systems" ]; then echo "packages/andromeda-systems directory exists." else echo "Error: packages/andromeda-systems directory does not exist." exit 1 fi # Test 2: Check for Cargo.toml in the package directory if [ -f "packages/andromeda-systems/Cargo.toml" ]; then echo "Cargo.toml found in packages/andromeda-systems." else echo "Error: Cargo.toml not found in packages/andromeda-systems." exit 1 fi # Test 3: Verify the package version version=$(grep '^version =' packages/andromeda-systems/Cargo.toml | awk -F '"' '{print $2}') if [ "$version" = "1.0.0" ]; then echo "andromeda-systems package version is correct: 1.0.0" else echo "Warning: andromeda-systems package version is $version, expected 1.0.0" fi
12-12: LGTM! Verify the contents of the new systems directory.The addition of
"contracts/systems/*"to the workspace members aligns with the PR objectives of introducing system ADOs. This change allows the inclusion of system-related contracts in the workspace.To ensure the new directory is properly set up, run the following script:
CHANGELOG.md (1)
24-24: LGTM! The changelog entry is well-formatted and consistent.The new changelog entry for the System ADO - current block is:
- Correctly formatted, matching the style of other entries.
- Consistent with the PR objectives, which mention the introduction of a "Current Block ADO".
- Appropriately placed within the "Added" section of the changelog.
Good job on maintaining clear and consistent documentation!
Wouldn't the user still need to construct a Query message to be sent to the Current Block ADO, and handle its response? Which I find more technically challenging that simply getting the current block height from |
Yeah, you are right. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/std/src/testing/mock_querier.rs (1)
183-186: LGTM: ContractInfo query handling updated for system contract.The addition of
MOCK_SYSTEM_CONTRACTwithcode_id4 is consistent with the existing pattern and aligns with the introduction of the new system contract constant.Consider using a match statement instead of multiple if-else conditions for better readability:
- if contract_addr == MOCK_WALLET { - return SystemResult::Ok(ContractResult::Err( - "Not a valid contract".to_string(), - )); - } - let mut resp = ContractInfoResponse::default(); - resp.code_id = match contract_addr.as_str() { - MOCK_SYSTEM_CONTRACT => 4, - MOCK_APP_CONTRACT => 3, - INVALID_CONTRACT => 2, - _ => 1, - }; + match contract_addr.as_str() { + MOCK_WALLET => return SystemResult::Ok(ContractResult::Err("Not a valid contract".to_string())), + MOCK_SYSTEM_CONTRACT => ContractInfoResponse { code_id: 4, ..Default::default() }, + MOCK_APP_CONTRACT => ContractInfoResponse { code_id: 3, ..Default::default() }, + INVALID_CONTRACT => ContractInfoResponse { code_id: 2, ..Default::default() }, + _ => ContractInfoResponse { code_id: 1, ..Default::default() }, + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- contracts/os/andromeda-vfs/src/execute.rs (2 hunks)
- contracts/os/andromeda-vfs/src/testing/tests.rs (6 hunks)
- packages/std/src/testing/mock_querier.rs (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/os/andromeda-vfs/src/execute.rs
- contracts/os/andromeda-vfs/src/testing/tests.rs
🔇 Additional comments not posted (3)
packages/std/src/testing/mock_querier.rs (3)
31-32: LGTM: New mock system contract constant added.The addition of
MOCK_SYSTEM_CONTRACTis consistent with other mock contract constants and follows the established naming convention.
488-492: LGTM: ADODB raw query handling updated for system contract.The addition of the case for key "4" returning "system-contract" as the ADO type is consistent with the existing pattern and corresponds to the new
MOCK_SYSTEM_CONTRACTwithcode_id4.
Line range hint
1-559: Summary: Mock querier updated to support system contract.The changes in this file consistently introduce support for a new system contract in the mock querier:
- A new constant
MOCK_SYSTEM_CONTRACTis added.- ContractInfo query handling is updated to include the system contract.
- ADODB raw query handling is extended to support the system contract ADO type.
These modifications are well-integrated and follow the existing patterns in the file. The changes appear to be complete and consistent for supporting the new system contract in testing scenarios.
Motivation
This ADO aims to create an endpoint anywhere in the VFS at instantiation.
We need a bunch of VFS endpoints for all users to be able to reference.
So this is the first implementation of that.
This is not "user deployable ADO". It will be managed by the root or admin(DAO) user.
Of course, we can get current block height from
Envdirectly, but we need users, non developers to be able to reference those as objects.Implementation
1. Update VFS
Implemented some methods to add the path of system ADOs like
/chain/validators,/etc/aos_version.ExecuteMsg::AddSystemAdoPath
QueryMsg::Subsystem, QueryMsg::SystemPaths
2. Current Block ADO
Testing
Added unit test cases for change.
Version Changes
Version is set as 1.0.0
Future work
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation