[IBC / Release 1] feat: IBC Registry #566
Conversation
WalkthroughThis pull request introduces multiple enhancements to the Andromeda project, primarily focusing on the implementation of the IBC Registry ADO. Key changes include the addition of a new contract for managing IBC denomination information, updates to error handling structures, and the introduction of integration tests to ensure the functionality of the new features. The modifications also encompass configuration improvements for easier project management and testing capabilities. Changes
Possibly related PRs
Suggested labels
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (4)
Files not reviewed due to no reviewable changes (1)
Files skipped from review as they are similar to previous changes (3)
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: 0
Outside diff range and nitpick comments (2)
contracts/os/andromeda-ibc-registry/src/testing/tests.rs (1)
11-23: Great start with the instantiation test!The
proper_initializationtest function provides good coverage for the basic instantiation flow of the contract. It correctly sets up the mock environment, constructs theInstantiateMsg, and validates that theinstantiatefunction returns no messages on successful initialization.To further improve the test coverage, consider adding more test cases to handle edge cases and error scenarios, such as:
- Instantiating with an invalid
kernel_addressorservice_address.- Instantiating with a non-empty
ownerfield.- Checking the state of the contract after instantiation to ensure the parameters are set correctly.
These additional tests will help ensure the robustness of the instantiation logic and catch potential issues early in the development process.
tests-integration/tests/ibc_registry.rs (1)
123-144: The authorization test looks good, but consider enabling the owner test.The test for the unauthorized user is implemented correctly. It attempts to execute the store method with an unauthorized user wallet and verifies that an
Unauthorizederror is returned.However, the test for the owner's authorization is currently commented out. It's important to test both the positive and negative cases for authorization.
Consider enabling the owner authorization test by uncommenting the code:
// Owner doesn't error let err: ContractError = ibc_registry .execute_execute_store_denom_info(&mut router, owner.clone(), None, vec![ibc_denom_info]) .unwrap_err() .downcast() .unwrap(); assert_eq!(err, ContractError::Unauthorized {});This test will ensure that the owner can successfully execute the store method without encountering an
Unauthorizederror.
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 (17)
- contracts/data-storage/andromeda-primitive/src/testing/tests.rs (1 hunks)
- contracts/os/andromeda-ibc-registry/.cargo/config (1 hunks)
- contracts/os/andromeda-ibc-registry/Cargo.toml (1 hunks)
- contracts/os/andromeda-ibc-registry/examples/schema.rs (1 hunks)
- contracts/os/andromeda-ibc-registry/src/contract.rs (1 hunks)
- contracts/os/andromeda-ibc-registry/src/lib.rs (1 hunks)
- contracts/os/andromeda-ibc-registry/src/mock.rs (1 hunks)
- contracts/os/andromeda-ibc-registry/src/state.rs (1 hunks)
- contracts/os/andromeda-ibc-registry/src/testing/mod.rs (1 hunks)
- contracts/os/andromeda-ibc-registry/src/testing/tests.rs (1 hunks)
- packages/andromeda-data-storage/src/primitive.rs (2 hunks)
- packages/std/src/error.rs (3 hunks)
- packages/std/src/os/ibc_registry.rs (1 hunks)
- packages/std/src/os/mod.rs (1 hunks)
- tests-integration/Cargo.toml (1 hunks)
- tests-integration/tests/ibc_registry.rs (1 hunks)
- tests-integration/tests/mod.rs (1 hunks)
Files skipped from review due to trivial changes (1)
- contracts/os/andromeda-ibc-registry/src/testing/mod.rs
Additional comments not posted (49)
packages/std/src/os/mod.rs (1)
4-4: Approve the addition of theibc_registrymodule.The new public module follows the existing naming convention and does not appear to impact the other modules in the file.
However, to better understand the purpose and scope of this module, please provide more information about its functionality and how it relates to inter-blockchain communication (IBC). This will help in conducting a more thorough review and identifying any potential issues or dependencies.
contracts/os/andromeda-ibc-registry/.cargo/config (3)
2-2: LGTM!The
wasmalias is a useful shorthand for building the project for the WebAssembly target with optimizations. This will help streamline the development workflow.
3-3: LGTM!The
unit-testalias provides a convenient way to run unit tests for the library components. This will facilitate a more efficient testing workflow.
4-4: LGTM!The
schemaalias makes it easier to execute the specific example related to schema generation or manipulation without needing to remember the full command.contracts/os/andromeda-ibc-registry/src/state.rs (3)
1-1: LGTM!The import statement is necessary and correct.
2-2: LGTM!The import statement is necessary and correct.
4-4: LGTM!The
REGISTRYconstant is defined correctly using theMaptype fromcw_storage_plus. The chosen namespace "registry" is appropriate for the purpose of this map.contracts/os/andromeda-ibc-registry/src/lib.rs (4)
1-1: LGTM!The
contractmodule is declared correctly as a public module, which is appropriate for encapsulating the core contract logic.
2-2: LGTM!The
statemodule is declared correctly as a public module, which is appropriate for encapsulating the state management logic of the contract.
3-4: LGTM!The
testingmodule is declared correctly as a public module with conditional compilation based on thetestconfiguration. This is a good practice for encapsulating testing utilities and ensuring they are only included when running tests.
6-7: LGTM!The
mockmodule is declared correctly as a public module with conditional compilation based on the target architecture and thetestingfeature. This is a good practice for encapsulating mocking utilities and ensuring they are only included when running tests in a non-wasm32environment.contracts/os/andromeda-ibc-registry/examples/schema.rs (1)
1-10: LGTM!The
schema.rsfile correctly defines the API schema for the Andromeda IBC Registry module. It follows the expected structure and uses thewrite_api!macro appropriately to generate the necessary API endpoints for instantiation, querying, and execution.The imports from the
andromeda_stdlibrary are correct and the message types are used as expected in thewrite_api!macro.This file will facilitate interaction with the IBC Registry by clearly defining how to instantiate the contract, query its state, and execute transactions.
tests-integration/tests/mod.rs (1)
10-12: LGTM!The addition of the
ibc_registrytest module is a valuable enhancement to the testing framework. It demonstrates a commitment to ensuring the correctness and reliability of the IBC registry functionality.contracts/os/andromeda-ibc-registry/Cargo.toml (5)
1-11: LGTM!The
[package]section correctly defines the package's metadata, including the name, version, edition, and Rust version. The exclusion list is also properly configured to exclude thecontract.wasmandhash.txtfiles, which are considered artifacts not meant for source code publication.
15-16: LGTM!The
[lib]section correctly specifies the crate types ascdylibandrlib, indicating that the crate can be compiled as both a dynamic library and a static library. This configuration is suitable for a library crate that can be used in both dynamic and static linking contexts.
18-23: LGTM!The
[features]section correctly defines optional features for the package, includingbacktraces,library, andtesting. These features provide flexibility in configuring the package for different use cases and testing scenarios.
- The
backtracesfeature enables backtraces for more explicit tests.- The
libraryfeature disables all instantiate/execute/query exports.- The
testingfeature includes dependencies oncw-multi-testandandromeda-testing, which are essential for running tests in a multi-contract environment.
25-34: LGTM!The
[dependencies]section correctly lists the package's dependencies, including several workspace dependencies that are crucial for building CosmWasm smart contracts and leveraging the Andromeda ecosystem.
- The workspace dependencies include
cosmwasm-std,cosmwasm-schema,cw-storage-plus,cw-utils, andcw20, which are essential for building CosmWasm smart contracts.- The
andromeda-stddependency includes a specific feature for rates, indicating a reliance on Andromeda's rate functionality.- The
andromeda-data-storagedependency is also included, suggesting integration with Andromeda's data storage capabilities.
37-39: LGTM!The
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]section correctly lists optional dependencies for non-WASM targets, includingcw-multi-testandandromeda-testing. These dependencies enhance the testing capabilities of the package, particularly for running tests in a multi-contract environment.contracts/os/andromeda-ibc-registry/src/mock.rs (6)
1-14: LGTM!The conditional compilation directive and imports are appropriate for the mock implementation.
15-16: LGTM!The
MockIbcRegistrystruct andmock_adomacro usage are appropriate for the mock implementation.
18-72: LGTM!The
MockIbcRegistryimplementation provides a clean and well-structured set of methods for instantiating, executing, and querying the mock contract. The usage ofandromeda_testingandcosmwasm_stdlibraries is appropriate.
74-77: LGTM!The
mock_andromeda_ibc_registryfunction correctly creates and returns a boxedContractWrapperas a mock contract.
79-89: LGTM!The
mock_ibc_registry_instantiate_msgfunction correctly creates and returns anInstantiateMsgstruct with the provided parameters.
91-93: LGTM!The
mock_execute_store_denom_info_msgfunction correctly creates and returns anExecuteMsg::StoreDenomInfovariant with the providedibc_denom_info.packages/std/src/os/ibc_registry.rs (6)
10-15: LGTM!The
InstantiateMsgstruct is well-defined with appropriate fields and attributes.
16-20: LGTM!The
DenomInfostruct is well-defined with appropriate fields and attributes.
21-25: LGTM!The
IBCDenomInfostruct is well-defined with appropriate fields and attributes.
27-36: LGTM!The
ExecuteMsgenum is well-defined with appropriate variants and attributes.
38-58: LGTM!The
verify_denomfunction correctly validates thedenomparameter and returns appropriate errors if the checks fail.
82-119: LGTM!The
test_validate_denomfunction thoroughly tests theverify_denomfunction with various input scenarios and asserts the expected behavior.tests-integration/Cargo.toml (1)
104-106: LGTM!The addition of the
andromeda-ibc-registrydependency with the "testing" feature is a positive change that aligns with the PR objective of introducing the IBC registry contract. This will enable the integration tests to cover scenarios involving the IBC registry functionality, enhancing the overall testing capabilities of the project.tests-integration/tests/ibc_registry.rs (4)
1-13: LGTM!The imports are correct and necessary for the integration test.
14-57: LGTM!The mock environment setup is comprehensive and follows the necessary steps to initialize the app, create wallets, deploy contracts, and instantiate the IBC registry app component. The code is well-structured and easy to understand.
59-86: LGTM!The test for storing and querying denom info is implemented correctly. It creates an
IBCDenomInfoinstance, executes the store method, and verifies the stored denom info using the query method. The assertions ensure that the stored denom info matches the expected values.
88-121: LGTM!The test for storing an additional denom info and querying all denom info is implemented correctly. It creates another
IBCDenomInfoinstance, executes the store method, and verifies all stored denom info using the query method. The assertions ensure that the retrieved denom info matches the expected values, including both previously stored denom info instances.contracts/os/andromeda-ibc-registry/src/contract.rs (7)
27-63: LGTM!The
instantiatefunction correctly initializes the contract by:
- Using the
ADOContractto handle the instantiation process.- Saving the service address and granting it the
STORE_DENOM_INFOpermission.- Returning a response with the service address as an attribute.
65-79: LGTM!The
executefunction correctly handles incoming messages by:
- Creating an
ExecuteContextfrom the provided parameters.- Matching on the message type and calling the appropriate handler function.
- For
AMPReceivemessages, calling theexecute_amp_receivefunction of theADOContract.- For other messages, calling the
handle_executefunction.
81-103: LGTM!The
handle_executefunction correctly processes various execution messages by:
- Extracting the action from the message.
- Calling the
call_actionfunction to handle the message and get anaction_response.- Matching on the message type and calling the appropriate handler function.
- For
StoreDenomInfomessages, calling theexecute_store_denom_infofunction.- For other messages, calling the
executefunction of theADOContract.- Returning a response with the
action_responseadded to it.
105-143: LGTM!The
execute_store_denom_infofunction correctly handles theStoreDenomInfoaction by:
- Verifying that the sender has the required permissions to execute the action.
- Ensuring that the
ibc_denom_infovector is not empty.- Iterating over the
ibc_denom_infovector and:
- Verifying the denomination using the
verify_denomfunction.- Checking for duplicates using a
HashSet.- Storing the denomination information securely using the
REGISTRYmap.- Returning a response with the action and sender as attributes.
145-153: LGTM!The
queryfunction correctly handles querying the contract state by:
- Matching on the query message type.
- For
DenomInfomessages, calling theget_denom_infofunction.- For
AllDenomInfomessages, calling theget_all_denom_infofunction.- Encoding the result of the query function using
encode_binary.
155-161: LGTM!The
get_denom_infofunction correctly retrieves information for a specific denomination by:
- Loading the denomination information from the
REGISTRYmap using the provideddenomkey.- Returning the denomination information wrapped in a
DenomInfoResponse.
163-189: LGTM!The
get_all_denom_infofunction correctly retrieves all denominations with pagination support by:
- Converting the
start_afterparameter into aBoundfor pagination.- Setting the limit to the provided value or defaulting to 100 if none is provided.
- Querying the
REGISTRYmap with pagination using therangemethod.- Collecting the results into a vector of
DenomInfo.- Returning the vector of
DenomInfowrapped in anAllDenomInfoResponse.packages/andromeda-data-storage/src/primitive.rs (2)
65-68: LGTM!The change improves the clarity and consistency of the error handling by explicitly setting the
msgfield toNonein theContractError::InvalidDenomvariant. This change does not introduce any logical or functional issues.
305-305: Test case updated correctly.The change in the test case correctly updates the expected error to include the
msg: Nonefield. This maintains consistency with the change made in thevalidatemethod and ensures that the test case correctly verifies the error handling behavior.contracts/data-storage/andromeda-primitive/src/testing/tests.rs (1)
245-245: Approved: The change enhances error handling in the test suite.The modification introduces a
msgfield to theContractError::InvalidDenomvariant, which is explicitly set toNone. This change may improve the test suite's ability to handle and report errors related to invalid denominations by allowing for more detailed error messages or logging in the future.While the change does not directly affect the contract's core functionality, it lays the groundwork for better error handling and reporting within the test suite.
packages/std/src/error.rs (4)
55-56: LGTM!The new
EmptyDenomerror variant is a valid addition to handle scenarios where a denomination is empty or missing.
58-59: LGTM!The new
NoDenomInfoProvidederror variant is a valid addition to handle scenarios where the required denomination information is not provided.
444-445: LGTM!The new
DuplicateDenomserror variant is a valid addition to handle scenarios where duplicate denominations are encountered. The associateddenomfield allows specifying the problematic denomination for better error reporting.
464-464: LGTM!The modification to the
InvalidDenomerror variant to include an optionalmsgfield is a valid change. It allows providing additional context about the invalid denomination while maintaining backward compatibility.
1.0.0
I think this is fine |
| let service_address = andr.get_wallet("service_address"); | ||
| let user1 = andr.get_wallet("user1"); | ||
|
|
||
| let app_code_id = andr.get_code_id(&mut router, "app-contract"); |
There was a problem hiding this comment.
The plan is to have this be an OS module under the key ibc-registry, we should add it as such for more realistic tests.
… all denoms to lowercase
Motivation
Part of [Q4 | RC1] IBC / Release 1
This OS contract,
ibc-registry, stores a mapping ofStringtoDenomInfowith the ability to query the mapping.Implementation
service_addressis set by the user during Instantiation; It's the only address (alongside the contract owner) that can callStoreDenomInfo.The structs are as follows:
There's only one
Mapin state:The two queries are:
Testing
Unit test that tests
validate_denom.Integration test that goes through instantiation, execution, and querying the results. Also tests authorization.
Version Changes
Should this be set as 2.0.0 or 1.0.0?
Notes
Should we remove
owner's ability to callStoreDenomInfo?Future work
Needs a
READMEfileSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Testing Enhancements
Documentation